diff --git a/bin/install.js b/bin/install.js index 1cc51b66e..517f10428 100755 --- a/bin/install.js +++ b/bin/install.js @@ -6786,22 +6786,54 @@ function installSdkIfNeeded() { console.log(`\n ${cyan}Building GSD SDK from source (${sdkDir})…${reset}`); const npmCmd = process.platform === 'win32' ? 'npm.cmd' : 'npm'; + // Windows: Node.js refuses to spawn .cmd/.bat files without `shell: true` + // after CVE-2024-27980 (fixed in Node ≥ 18.20.2 / ≥ 20.12.2 / ≥ 21.7.3). + // Without shell, spawnSync returns { status: null, error: EINVAL } and + // every `status !== 0` check trips — producing a silent build failure + // with no underlying diagnostic because stdio: 'inherit' never gets a + // child to stream (#2598). + const needsShell = process.platform === 'win32'; + const spawnNpm = (args, opts = {}) => + spawnSync(npmCmd, args, { ...opts, shell: opts.shell ?? needsShell }); + + // Format the underlying spawnSync failure so EINVAL / ENOENT / signal exits + // surface in the fatal banner instead of being swallowed. The #2598 silent + // failure happened precisely because `{ status: null, error: EINVAL }` was + // reduced to a generic "Failed to npm install" with no diagnostic — the real + // cause (CVE-2024-27980 on Windows) was invisible in the output. + const formatSpawnFailure = (result) => { + if (!result) return ''; + if (result.error) return ` (${result.error.code || result.error.name || 'spawn error'}: ${result.error.message})`; + if (result.signal) return ` (signal: ${result.signal})`; + if (typeof result.status === 'number') return ` (exit status: ${result.status})`; + return ''; + }; + // 1. Install sdk build-time dependencies (tsc, etc.) - const installResult = spawnSync(npmCmd, ['install'], { cwd: sdkDir, stdio: 'inherit' }); + const installResult = spawnNpm(['install'], { cwd: sdkDir, stdio: 'inherit' }); if (installResult.status !== 0) { - emitSdkFatal('Failed to `npm install` in sdk/.', { globalBin: null, exitCode: 1 }); + emitSdkFatal( + `Failed to \`npm install\` in sdk/.${formatSpawnFailure(installResult)}`, + { globalBin: null, exitCode: 1 }, + ); } // 2. Compile TypeScript → sdk/dist/ - const buildResult = spawnSync(npmCmd, ['run', 'build'], { cwd: sdkDir, stdio: 'inherit' }); + const buildResult = spawnNpm(['run', 'build'], { cwd: sdkDir, stdio: 'inherit' }); if (buildResult.status !== 0) { - emitSdkFatal('Failed to `npm run build` in sdk/.', { globalBin: null, exitCode: 1 }); + emitSdkFatal( + `Failed to \`npm run build\` in sdk/.${formatSpawnFailure(buildResult)}`, + { globalBin: null, exitCode: 1 }, + ); } // 3. Install the built package globally so `gsd-sdk` lands on PATH. - const globalResult = spawnSync(npmCmd, ['install', '-g', '.'], { cwd: sdkDir, stdio: 'inherit' }); + const globalResult = spawnNpm(['install', '-g', '.'], { cwd: sdkDir, stdio: 'inherit' }); if (globalResult.status !== 0) { - emitSdkFatal('Failed to `npm install -g .` from sdk/.', { globalBin: null, exitCode: 1 }); + emitSdkFatal( + `Failed to \`npm install -g .\` from sdk/.${formatSpawnFailure(globalResult)}`, + { globalBin: null, exitCode: 1 }, + ); } // 3a. Explicitly chmod dist/cli.js to 0o755 in the global install location. @@ -6811,7 +6843,7 @@ function installSdkIfNeeded() { // a non-executable file and `command -v gsd-sdk` fails on every first install // (root cause of #2453). Mirrors the pattern used for hook files in this installer. try { - const prefixRes = spawnSync(npmCmd, ['config', 'get', 'prefix'], { encoding: 'utf-8' }); + const prefixRes = spawnNpm(['config', 'get', 'prefix'], { encoding: 'utf-8' }); if (prefixRes.status === 0) { const npmPrefix = (prefixRes.stdout || '').trim(); const sdkPkg = JSON.parse(fs.readFileSync(path.join(sdkDir, 'package.json'), 'utf-8')); @@ -6835,7 +6867,7 @@ function installSdkIfNeeded() { } // Off-PATH: resolve npm global bin dir for actionable remediation. - const prefixResult = spawnSync(npmCmd, ['config', 'get', 'prefix'], { encoding: 'utf-8' }); + const prefixResult = spawnNpm(['config', 'get', 'prefix'], { encoding: 'utf-8' }); const prefix = prefixResult.status === 0 ? (prefixResult.stdout || '').trim() : null; const globalBin = prefix ? (process.platform === 'win32' ? prefix : path.join(prefix, 'bin')) diff --git a/tests/bug-2598-windows-npm-spawn-shell.test.cjs b/tests/bug-2598-windows-npm-spawn-shell.test.cjs new file mode 100644 index 000000000..5d1bcbf9a --- /dev/null +++ b/tests/bug-2598-windows-npm-spawn-shell.test.cjs @@ -0,0 +1,138 @@ +/** + * Regression test for bug #2598 + * + * On Windows, Node.js refuses to spawn `.cmd`/`.bat` files via spawnSync + * unless `shell: true` is passed (post CVE-2024-27980, fixed in + * Node >= 18.20.2 / >= 20.12.2 / >= 21.7.3 and every version since). + * + * installSdkIfNeeded() picks `npmCmd = 'npm.cmd'` on win32 and then shells + * out five times: `npm install`, `npm run build`, `npm install -g .`, and + * two `npm config get prefix` calls. Without `shell: true`, every single + * one of those returns `{ status: null, error: EINVAL }` before npm ever + * launches. The installer checks `status !== 0` (null !== 0 is true) and + * trips its failure path — producing a silent SDK build failure with zero + * diagnostic output because `stdio: 'inherit'` never got a child to stream. + * + * Fix: every spawnSync that targets `npmCmd` inside installSdkIfNeeded + * must pass `shell: true` on Windows. The structural invariant verified + * here is that no bare `spawnSync(npmCmd, ...)` remains inside the + * function — all npm invocations go through a helper that injects the + * shell option on win32 (or have it explicitly on the call site). + */ + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js'); + +function installSdkBody() { + const src = fs.readFileSync(INSTALL_SRC, 'utf-8'); + const fnStart = src.indexOf('function installSdkIfNeeded()'); + assert.ok(fnStart !== -1, 'installSdkIfNeeded function must exist in install.js'); + const fnEnd = src.indexOf('\nfunction ', fnStart + 1); + return fnEnd !== -1 ? src.slice(fnStart, fnEnd) : src.slice(fnStart); +} + +describe('bug #2598: Windows npm.cmd spawnSync must pass shell: true', () => { + test('install.js source exists', () => { + assert.ok(fs.existsSync(INSTALL_SRC), 'bin/install.js must exist'); + }); + + test('installSdkIfNeeded does not call spawnSync(npmCmd, ...) directly', () => { + const body = installSdkBody(); + // A bare `spawnSync(npmCmd,` call will fail with EINVAL on Windows + // because npm.cmd requires `shell: true`. Every npm invocation must + // go through a wrapper that injects the shell option on win32. + // + // Exclude the implementation of the wrapper itself (its `spawnSync(npmCmd, args, ...)` + // line is the ONE legitimate place that spawns npmCmd directly — and it forwards + // a shell option from `opts`). + const allCalls = body.match(/spawnSync\s*\(\s*npmCmd[^)]*\)/g) || []; + const bareCalls = allCalls.filter(c => !/\bshell\s*:/.test(c)); + assert.equal( + bareCalls.length, + 0, + 'installSdkIfNeeded must not call spawnSync(npmCmd, ...) directly. ' + + 'On Windows, npm.cmd spawns fail with EINVAL (CVE-2024-27980) unless ' + + 'shell: true is passed. Route all npm invocations through a helper ' + + 'that injects `shell: process.platform === "win32"`.' + ); + }); + + test('installSdkIfNeeded defines a shell-aware npm wrapper', () => { + const body = installSdkBody(); + // The canonical implementation introduces `spawnNpm` (or equivalent) + // that injects `shell: true` on win32. Accept either a helper + // function or `shell: true` / `shell: needsShell` / `shell: win32` + // appearing alongside an npm spawn. + const hasHelper = /\bspawnNpm\b/.test(body); + const hasShellOption = /shell\s*:\s*(true|needsShell|process\.platform\s*===\s*['"]win32['"])/.test(body); + assert.ok( + hasHelper || hasShellOption, + 'installSdkIfNeeded must introduce a helper or pass shell:true/win32-aware ' + + 'shell option for npm spawnSync calls. Found neither.' + ); + }); + + test('installSdkIfNeeded calls the npm wrapper at least five times', () => { + const body = installSdkBody(); + // Five documented npm invocations: install, run build, install -g ., + // and two config get prefix calls. If any are still bare spawnSync + // calls targeting npmCmd, the first assertion above already fails. + // `\bspawnNpm\s*\(` matches real call sites only — a `const spawnNpm = (…)` + // declaration does not match because `=` sits between name and `(`. A + // `function spawnNpm(…)` declaration WOULD match, so subtract one for that + // form. `explicitShellNpm` previously double-counted the wrapper's own + // `spawnSync(npmCmd, args, { …, shell })` — when a helper exists, its body + // is the only legitimate raw spawn and must be excluded. + const wrapperCallMatches = (body.match(/\bspawnNpm\s*\(/g) || []).length; + const hasArrowHelper = /\b(?:const|let|var)\s+spawnNpm\s*=/.test(body); + const hasFunctionHelper = /\bfunction\s+spawnNpm\s*\(/.test(body); + const hasHelper = hasArrowHelper || hasFunctionHelper; + const wrapperCalls = hasFunctionHelper + ? Math.max(0, wrapperCallMatches - 1) + : wrapperCallMatches; + const explicitShellNpm = hasHelper + ? 0 + : (body.match(/spawnSync\s*\(\s*npmCmd[^)]*\bshell\s*:/g) || []).length; + const total = wrapperCalls + explicitShellNpm; + assert.ok( + total >= 5, + `installSdkIfNeeded should route at least 5 npm invocations through ` + + `a shell-aware wrapper (install, run build, install -g ., and two ` + + `config get prefix calls). Found ${total}.` + ); + }); + + test('installSdkIfNeeded surfaces underlying spawnSync failure in fatal branches', () => { + // Root cause of #2598 was invisible because `{ status: null, error: EINVAL }` + // was reduced to a generic "Failed to `npm install` in sdk/." with no + // diagnostic — stdio: 'inherit' had no child to stream and result.error was + // dropped. Each of the three `emitSdkFatal` calls inside the install/build/ + // global-install failure paths must now thread spawn diagnostics (error, + // signal, or numeric status) into the reason string so future regressions + // print their real cause instead of failing silently. + const body = installSdkBody(); + const hasFormatter = /formatSpawnFailure\s*\(/.test(body); + const fatalCalls = body.match(/emitSdkFatal\s*\([^)]*`[^`]*`[^)]*\)/gs) || []; + const fatalWithDiagnostic = fatalCalls.filter( + (c) => /formatSpawnFailure|\.error|\.signal|\.status/.test(c), + ); + assert.ok( + hasFormatter, + 'installSdkIfNeeded must define a spawn-failure formatter so fatal ' + + 'npm failures surface result.error / result.signal / result.status ' + + 'instead of swallowing them (root cause of #2598 being invisible).', + ); + assert.ok( + fatalWithDiagnostic.length >= 3, + `At least 3 emitSdkFatal calls (install, build, install -g .) must ` + + `include spawn diagnostics. Found ${fatalWithDiagnostic.length} that ` + + `reference formatSpawnFailure or result.error/signal/status.`, + ); + }); +});