golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#2598): pass shell: true to npm spawnSync on Windows (#2600)

Browse Source 
* fix(#2598): pass shell: true to npm spawnSync on Windows

Since Node's CVE-2024-27980 fix (>= 18.20.2 / >= 20.12.2 / >= 21.7.3),
spawnSync refuses to launch .cmd/.bat files on Windows without
`shell: true`. installSdkIfNeeded picks npmCmd='npm.cmd' on win32 and
then calls spawnSync five times — every one returns
{ status: null, error: EINVAL } before npm ever runs. The installer
checks `status !== 0`, trips the failure path, and emits a bare
"Failed to `npm install` in sdk/." with zero diagnostic output because
`stdio: 'inherit'` never had a child to stream.

Every fresh install on Windows has failed at the SDK build step on any
supported Node version for the life of the post-CVE bin/install.js.

Introduce a local `spawnNpm(args, opts)` helper inside
installSdkIfNeeded that injects `shell: process.platform === 'win32'`
when the caller doesn't override it. Route all five npm invocations
through it: `npm install`, `npm run build`, `npm install -g .`, and
both `npm config get prefix` calls.

Adds a static regression test that parses installSdkIfNeeded and
asserts no bare `spawnSync(npmCmd, ...)` remains, a shell-aware
wrapper exists, and at least five invocations go through it.

Closes #2598

* fix(#2598): surface spawnSync diagnostics in SDK install fatal paths

Thread result.error / result.signal / result.status into emitSdkFatal for
the three npm failure branches (install, run build, install -g .) via a
formatSpawnFailure helper. The root cause of #2598 went silent precisely
because `{ status: null, error: EINVAL }` was reduced to a generic
"Failed to `npm install` in sdk/." with no diagnostic — stdio: 'inherit'
had no child process to stream and result.error was swallowed. Any future
regression in the same area (EINVAL, ENOENT, signal termination) now
prints its real cause in the red fatal banner.

Also strengthen the regression test so it cannot pass with only four
real npm call sites: the previous `spawnSync(npmCmd, ..., shell)` regex
double-counted the spawnNpm helper's own body when a helper existed.
Separate arrow-form vs function-form helper detection and exclude the
wrapper body from explicitShellNpm so the `>= 5` assertion reflects real
invocations only. Add a new test that asserts all three fatal branches
now reference formatSpawnFailure / result.error / signal / status.

Addresses CodeRabbit review comments on PR #2600:
- r3126987409 (bin/install.js): surface underlying spawnSync failure
- r3126987419 (test): explicitShellNpm overcounts by one via helper def
This commit is contained in:
Tom Boucher
2026-04-22 21:23:44 -04:00
committed by GitHub
parent 1a694fcac3
commit c90081176d
2 changed files with 178 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
bin/install.js
View File

@@ -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'))

138
tests/bug-2598-windows-npm-spawn-shell.test.cjs Normal file
View File

@@ -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.`,
);
});
});