From 0977d0a475bb852ba125db70b0cf2a6968b2a714 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 20 Sep 2026 05:08:22 -0400 Subject: [PATCH] =?UTF-8?q?fix(#4797):=20run-with-timeout's=20.cmd=20media?= =?UTF-8?q?tion=20folds=20onto=20projectSpawnInvocation=20=E2=80=94=20spac?= =?UTF-8?q?es=20in=20the=20shim=20path=20no=20longer=20break=20it=20(#4892?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .changeset/proud-seals-sing.md | 5 ++++ gsd-core/bin/gsd-tools.cjs | 33 +++++++++++++++++----- tests/run-with-timeout.test.cjs | 49 ++++++++++++++++++++++++++++++++- 3 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 .changeset/proud-seals-sing.md diff --git a/.changeset/proud-seals-sing.md b/.changeset/proud-seals-sing.md new file mode 100644 index 000000000..3d4d9195f --- /dev/null +++ b/.changeset/proud-seals-sing.md @@ -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) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 15d151cdf..d2f8ea5b9 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -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 + // ...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)); diff --git a/tests/run-with-timeout.test.cjs b/tests/run-with-timeout.test.cjs index b1cc9ae37..3c97e849a 100644 --- a/tests/run-with-timeout.test.cjs +++ b/tests/run-with-timeout.test.cjs @@ -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 ...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); + } + }); +});