fix(#4797): run-with-timeout's .cmd mediation folds onto projectSpawnInvocation — spaces in the shim path no longer break it (#4892)
* test(#4797): failing-first on Windows — a .cmd/.bat shim under a spaced path must run * fix(#4797): run-with-timeout's .cmd mediation folds onto projectSpawnInvocation — spaces in the shim path no longer break it The private /d /s /c argv-array copy quoted the shim path (any space in the path) and cmd.exe /s stripped the first-and-last quote of the whole /c string, so the pre-space fragment became the program name: exit 1, empty stdout, on every repo whose path contains a space. The declared seam wraps the whole command line in one extra quote pair with windowsVerbatimArguments — the reporter verified the shape against the repro on Windows. * chore(#4797): backfill changeset PR number (4892) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/proud-seals-sing.md
Normal file
5
.changeset/proud-seals-sing.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4892
|
||||
---
|
||||
**run-with-timeout works from project paths containing spaces on Windows** — the .cmd/.bat mediation quoted the shim path and cmd.exe /s stripped the wrong quotes, so every mediated call under a spaced project path failed with exit 1 and empty stdout (the /gsd-code-review fallow pre-pass included); the mediation now uses the same projectSpawnInvocation seam as every other Windows spawn site. (#4797)
|
||||
@@ -4602,9 +4602,26 @@ function runWithTimeout(argv) {
|
||||
// cap's process-group kill (the wrapped child escapes reap → exit 124 never
|
||||
// fires) and risks cmd.exe mis-parsing an arg like `-e "setTimeout(()=>{})"`.
|
||||
// Only .cmd/.bat are the CVE-2024-27980 EINVAL cases that require mediation.
|
||||
// #4797: the mediation is NOT hand-rolled here — the private `/d /s /c <cmd>
|
||||
// ...args` copy broke on any shim path containing a space (Node quotes the
|
||||
// argv token; `/s` strips the FIRST and LAST quote of the /c string, so
|
||||
// cmd.exe took the pre-space fragment as the program). projectSpawnInvocation
|
||||
// (the declared single owner, #3411/#3617) wraps the WHOLE command line in
|
||||
// one extra quote pair with windowsVerbatimArguments — the shape that
|
||||
// survives spaces. One behavior delta, accepted: the seam DECLINES mediation
|
||||
// when the target or an arg carries CR/LF (the old block mediated anyway) —
|
||||
// the unmediated spawn of a .cmd then fails EINVAL, loud, at the catch below.
|
||||
const winShim = isWin && /\.(cmd|bat)$/i.test(path.basename(cmd));
|
||||
const spawnCmd = winShim ? (process.env.ComSpec || 'cmd.exe') : cmd;
|
||||
const spawnArgs = winShim ? ['/d', '/s', '/c', cmd, ...cmdArgs] : cmdArgs;
|
||||
let spawnCmd = cmd;
|
||||
let spawnArgs = cmdArgs;
|
||||
let spawnOpts;
|
||||
if (winShim) {
|
||||
const { projectSpawnInvocation } = require('./lib/shell-command-projection.cjs');
|
||||
const inv = projectSpawnInvocation(cmd, cmdArgs);
|
||||
spawnCmd = inv.command;
|
||||
spawnArgs = inv.args;
|
||||
if (inv.windowsVerbatimArguments) spawnOpts = { windowsVerbatimArguments: true };
|
||||
}
|
||||
// Node's setTimeout delay is a 32-bit signed ms int; a larger value silently
|
||||
// clamps to 1ms → a spurious immediate timeout. Cap the budget (~24.8 days).
|
||||
const timerMs = Math.min(Math.round(secs * 1000), 2 ** 31 - 1);
|
||||
@@ -4615,11 +4632,13 @@ function runWithTimeout(argv) {
|
||||
return new Promise((resolve) => {
|
||||
let child;
|
||||
try {
|
||||
// #2667: on win32 `.cmd`/`.bat`/`.exe`, spawn cmd.exe with an explicit argv
|
||||
// array (spawnCmd/spawnArgs) rather than the shim directly — preserves the
|
||||
// array-only, no-shell-string argv contract. `detached` is always false on
|
||||
// win32, so it never co-occurs with the cmd.exe mediation.
|
||||
child = spawn(spawnCmd, spawnArgs, { stdio: 'inherit', detached });
|
||||
// #2667: on win32 `.cmd`/`.bat` shims, spawn cmd.exe with an explicit
|
||||
// argv ARRAY rather than the shim directly — preserves the array-only,
|
||||
// no-shell-string argv contract. #4797: the exact argv shape (quote
|
||||
// wrapping, verbatim arguments) is projected by projectSpawnInvocation —
|
||||
// see the block above. `detached` is always false on win32, so it never
|
||||
// co-occurs with the cmd.exe mediation.
|
||||
child = spawn(spawnCmd, spawnArgs, { stdio: 'inherit', detached, ...spawnOpts });
|
||||
} catch (err) {
|
||||
process.stderr.write(`run-with-timeout: ${cmd}: ${err && err.message ? err.message : 'failed to start'}\n`);
|
||||
resolve(spawnFailureCode(err));
|
||||
|
||||
@@ -334,7 +334,7 @@ describe('#2667 run-with-timeout — Windows .cmd/.bat/.exe spawn mediation (CVE
|
||||
fs.writeFileSync(shim, '@echo {"verdict":"clean"}\r\n', 'utf8');
|
||||
const r = runVerb(['10', '--', shim]);
|
||||
assert.equal(r.status, 0, `expected the .bat shim to run (exit 0); got ${r.status}. stderr: ${r.stderr}`);
|
||||
assert.ok((r.stdout || '').length > 0, 'expected non-empty stdout from the .bat shim');
|
||||
assert.ok((r.stdout || '').includes('clean'), `expected JSON stdout from the .bat shim; got: ${r.stdout}`);
|
||||
} finally {
|
||||
cleanup(dir);
|
||||
}
|
||||
@@ -447,3 +447,50 @@ describe('#2351 parity guard — no hardcoded timeout in workflow/agent/referenc
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #4797: the mediation must survive spaces in the shim path ──────────────
|
||||
|
||||
describe('#4797 run-with-timeout — .cmd mediation with spaces in the shim path', () => {
|
||||
// The #2667 mediation's private copy passed `/d /s /c <cmd> ...args` with the
|
||||
// shim path as a discrete argv token: Node quotes the whole argv (a space in
|
||||
// the path forces quotes) and `/s` strips the FIRST and LAST quote of the /c
|
||||
// string — so cmd.exe took the pre-space fragment as the program name and
|
||||
// every mediated call under a spaced path failed with exit 1 and empty
|
||||
// stdout. The projectSpawnInvocation seam wraps the WHOLE command line in
|
||||
// one extra quote pair with windowsVerbatimArguments, which is the shape
|
||||
// that survives. The issue's own Windows repro is the pre-fix RED evidence
|
||||
// (exit 1, cmd.exe "not recognized", 0-byte stdout); the linux bench cannot
|
||||
// run cmd.exe, so these tests are win32-gated like their #2667 siblings.
|
||||
const isWin = process.platform === 'win32';
|
||||
const SPACED = 'rwt 4797 dir with spaces';
|
||||
|
||||
test('win32: a .cmd shim under a spaced path runs (the fallow pre-pass shape)', { skip: !isWin ? 'win32-only' : false }, () => {
|
||||
const base = createTempDir('rwt-4797-base');
|
||||
const dir = path.join(base, SPACED);
|
||||
try {
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
const shim = path.join(dir, 'fake.cmd');
|
||||
fs.writeFileSync(shim, '@echo {"verdict":"clean"}\r\n', 'utf8');
|
||||
const r = runVerb(['10', '--', shim]);
|
||||
assert.equal(r.status, 0, `expected the spaced-path .cmd shim to run (exit 0); got ${r.status}. stderr: ${r.stderr}`);
|
||||
assert.ok((r.stdout || '').includes('clean'), `expected JSON stdout; got: ${r.stdout}`);
|
||||
} finally {
|
||||
cleanup(base);
|
||||
}
|
||||
});
|
||||
|
||||
test('win32: a .bat shim under a spaced path runs too', { skip: !isWin ? 'win32-only' : false }, () => {
|
||||
const base = createTempDir('rwt-4797-bat-base');
|
||||
const dir = path.join(base, SPACED);
|
||||
try {
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
const shim = path.join(dir, 'fake.bat');
|
||||
fs.writeFileSync(shim, '@echo {"verdict":"clean"}\r\n', 'utf8');
|
||||
const r = runVerb(['10', '--', shim]);
|
||||
assert.equal(r.status, 0, `expected the spaced-path .bat shim to run (exit 0); got ${r.status}. stderr: ${r.stderr}`);
|
||||
assert.ok((r.stdout || '').includes('clean'), `expected JSON stdout from the .bat shim; got: ${r.stdout}`);
|
||||
} finally {
|
||||
cleanup(base);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user