From 79ed181ec0621a655f76730029627a0f5284ddab Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Jul 2026 23:14:17 -0400 Subject: [PATCH] fix(#2667): run-with-timeout mediates .cmd/.bat spawns on Windows (CVE-2024-27980); fallow pre-pass names failure kind (#2897) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2667): mediate .cmd/.bat/.exe spawns on Windows; split fallow pre-pass failure diagnostic run-with-timeout spawned .cmd/.bat/.exe commands without shell:true on Windows, tripping Node's CVE-2024-27980 EINVAL (April 2024 security hardening). The fallow structural pre-pass then no-op'd silently — a hard execution failure read the same as 'optional dependency absent'. (A) gsd-core/bin/gsd-tools.cjs runWithTimeout: gate shell:true on (win32 && command ends in .cmd/.bat/.exe). Narrow by design — never fires for the 7 `bash -c` callers (command is `bash`, no such suffix), so the recorded no-shell-for-argv-array security contract (DEFECT.UNBOUNDED-SUBPROCESS) is preserved; cmdArgs stays an array. POSIX untouched. (B) code-review.md fallow pre-pass: name the failure KIND (timeout / spawn failure / crash / not-found) so a Windows .cmd spawn failure is not mistaken for an absent binary. Regression test in tests/run-with-timeout.test.cjs gated to win32 (.cmd/.bat/.exe shims run with exit 0 + non-empty stdout; pre-fix EINVAL → exit 125/empty). POSIX negative-space test guards the unchanged bash -c callers. * chore(#2667): changeset fragment * chore(#2667): backfill changeset PR 2897 + correct body (cmd.exe array, not shell:true) * fix(#2667): exclude .exe from the win32 spawn-mediation gate; ack code-review.md growth CI caught two failures on the first push: 1. windows-24: 'exits 124 when the wall-clock budget is exceeded' regressed. The gate matched .exe, so the HANG command (node.exe -e 'setTimeout(...)') was wrapped in 'cmd.exe /c node.exe ...' — the wrapped child escaped the timeout cap's process-group reap (exit 124 never fired; hit the 30s harness backstop) AND cmd.exe risked mis-parsing the -e script arg. .exe is INTENTIONALLY excluded now: real PE executables spawn fine directly; only .cmd/.bat are the CVE-2024-27980 EINVAL cases. The .exe test becomes a negative-space test (node.exe spawned directly, exit 0). 2. ubuntu-22: emitted-attribution — code-review.md grew 1177 bytes from the #2667 fallow pre-pass failure-KIND case statement; acknowledge it. --------- Co-authored-by: Test --- .changeset/gallant-koalas-forage.md | 5 +++ gsd-core/bin/gsd-tools.cjs | 29 ++++++++++++++- gsd-core/workflows/code-review.md | 19 ++++++++-- tests/emitted-drift-ack.json | 2 +- tests/run-with-timeout.test.cjs | 55 +++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 .changeset/gallant-koalas-forage.md diff --git a/.changeset/gallant-koalas-forage.md b/.changeset/gallant-koalas-forage.md new file mode 100644 index 000000000..58591693f --- /dev/null +++ b/.changeset/gallant-koalas-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2897 +--- +**Fallow structural pre-pass no longer silently no-ops on Windows** — `run-with-timeout` now mediates `.cmd`/`.bat`/`.exe` spawns via an explicit `cmd.exe /c` argv array (Node's CVE-2024-27980 hardening requires a shell for these on Windows), and the fallow pre-pass names the failure kind so a Windows spawn failure is not mistaken for an absent binary. The existing `bash -c` callers and POSIX behavior are unchanged. (#2667) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index cd6b66fb1..baa31cedb 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -3033,6 +3033,29 @@ function runWithTimeout(argv) { const detached = !isWin && secs > 0; const spawnFailureCode = (err) => (err && err.code === 'ENOENT' ? 127 : err && err.code === 'EACCES' ? 126 : 125); + // #2667: on Windows, a `.cmd`/`.bat`/`.exe` command cannot be spawned directly + // — Node's CVE-2024-27980 hardening (April 2024, all active lines incl. 22.x) + // throws EINVAL when child_process.spawn is given a `.cmd`/`.bat` without a + // shell, so e.g. `run-with-timeout 120 -- node_modules/.bin/fallow.cmd` silently + // produced empty stdout + exit 125 and the fallow pre-pass no-op'd. + // + // We do NOT use `shell: true` for this: with `shell:true`, Node space-joins the + // unescaped cmdArgs into a cmd.exe command string (DEP0190) — that would re-open + // a shell-injection surface and violate the recorded no-shell-for-argv-array + // contract (DEFECT.UNBOUNDED-SUBPROCESS, CONTEXT.md:772). Instead we spawn + // `cmd.exe /c ` with an explicit argv ARRAY, which is what Node's + // own exec does internally and keeps every arg a discrete, un-interpolated + // token. The gate is NARROW: it fires ONLY for the Windows shim extensions, + // never for the `bash -c` callers (command is `bash`, no such suffix), so the 7 + // bash callers keep their array-only argv on every platform. POSIX untouched. + // NOTE: .exe is INTENTIONALLY excluded — real PE executables (node.exe, etc.) + // spawn fine directly and mediating them through cmd.exe /c breaks the timeout + // 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. + 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; // 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); @@ -3043,7 +3066,11 @@ function runWithTimeout(argv) { return new Promise((resolve) => { let child; try { - child = spawn(cmd, cmdArgs, { stdio: 'inherit', detached }); + // #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 }); } catch (err) { process.stderr.write(`run-with-timeout: ${cmd}: ${err && err.message ? err.message : 'failed to start'}\n`); resolve(spawnFailureCode(err)); diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 27f72edbb..71515ec07 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -464,7 +464,22 @@ FALLOW_OK=$(FALLOW_TMP=\"${FALLOW_JSON_PATH}.tmp\" node -e \" if [ \"$FALLOW_OK\" != \"1\" ]; then FALLOW_STDERR_SUMMARY=$(head -5 \"$FALLOW_STDERR_TMP\") rm -f \"${FALLOW_JSON_PATH}.tmp\" \"$FALLOW_STDERR_TMP\" - echo \"WARNING: fallow structural pre-pass failed (exit ${FALLOW_EXIT}): ${FALLOW_STDERR_SUMMARY}\" + # #2667: distinguish a hard EXECUTION failure (the binary was found at step 1 + # but would not run) from the binary-missing path (step 2). Exit 124 = timeout, + # 2 = usage error, 125 = spawn failure (e.g. Windows EINVAL on a .cmd shim — + # CVE-2024-27980, now mediated by run-with-timeout), 126/127 = not executable / + # not found. A non-zero exit here with a resolved binary means fallow is + # installed but did not produce a report — surface that loudly so a Windows + # user does not mistake it for "fallow absent". + case \"$FALLOW_EXIT\" in + 124) FALLOW_FAIL_KIND=\"timed out\" ;; + 2) FALLOW_FAIL_KIND=\"usage error\" ;; + 125) FALLOW_FAIL_KIND=\"spawn failure (the binary was found but did not start — e.g. a Windows .cmd shim; run-with-timeout mediates this)\" ;; + 126) FALLOW_FAIL_KIND=\"not executable\" ;; + 127) FALLOW_FAIL_KIND=\"not found\" ;; + *) FALLOW_FAIL_KIND=\"crashed\" ;; + esac + echo \"WARNING: fallow structural pre-pass failed (${FALLOW_FAIL_KIND}, exit ${FALLOW_EXIT}): ${FALLOW_STDERR_SUMMARY}\" FALLOW_JSON_PATH=\"\" else mv \"${FALLOW_JSON_PATH}.tmp\" \"$FALLOW_JSON_PATH\" @@ -472,7 +487,7 @@ else fi ``` -On any failure of the structural pre-pass (binary missing, timeout, empty output, or unparseable JSON), the workflow continues with no `` injection; the reviewer agent receives a normal review request. +On any failure of the structural pre-pass (binary missing at step 2, or an execution failure here — timeout, spawn failure, crash, empty output, or unparseable JSON), the workflow continues with no `` injection; the reviewer agent receives a normal review request. The WARNING above names the failure KIND so a hard execution failure (e.g. a Windows `.cmd` spawn failure) is not mistaken for an absent optional dependency. 4) Optional MCP bridge path (runtime-dependent): - If `FALLOW_MCP=true`, set reviewer input mode to MCP-backed structural findings. diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json index fa2f84cde..ede87875f 100644 --- a/tests/emitted-drift-ack.json +++ b/tests/emitted-drift-ack.json @@ -14,7 +14,7 @@ "reason": "#2800 — same derived-flag-loop change as autonomous.md. next.md needed no relocation (its launcher preamble already precedes the loop), so the growth here is only the loop plus the comment recording that --all and --text stay literal because they are convergence controls, not reviewer lanes." }, "code-review.md": { - "reason": "#2666: the Tier-2 SUMMARY extractor predicate is relaxed to accept root-level and extensionless build files (Dockerfile/Makefile/etc.), and the Tier-3 git-diff fallback is converted from an eq-zero gate into an intersect-and-warn that cross-checks the SUMMARY scope against `git diff --name-only` with exact whole-line matching (grep -Fxq) and warns about + adds any changed files the extractor missed. Growth is the relaxed predicate + the portable cross-check branch + their explanatory comments." + "reason": "#2667: the fallow structural pre-pass failure WARNING now names the failure KIND (timeout / spawn failure / crash / not-found) via a case statement, so a Windows .cmd spawn failure (CVE-2024-27980) is not mistaken for an absent binary. Growth is the case statement + the explanatory prose." } } } diff --git a/tests/run-with-timeout.test.cjs b/tests/run-with-timeout.test.cjs index 0cc820493..64bb151f5 100644 --- a/tests/run-with-timeout.test.cjs +++ b/tests/run-with-timeout.test.cjs @@ -199,6 +199,61 @@ describe('#2351 run-with-timeout — kill semantics (POSIX process groups)', () }); }); +describe('#2667 run-with-timeout — Windows .cmd/.bat/.exe spawn mediation (CVE-2024-27980)', () => { + // Node's CVE-2024-27980 hardening throws EINVAL when child_process.spawn is + // given a .cmd/.bat without a shell. run-with-timeout now mediates .cmd/.bat + // on win32 via an explicit `cmd.exe /d /s /c ...args` argv ARRAY (not + // shell:true — that space-joins unescaped args per DEP0190), while leaving + // every `bash`/argv-array caller unchanged (the recorded no-shell-for-argv- + // array contract). .exe is INTENTIONALLY excluded — real PEs spawn fine + // directly and mediating them breaks the timeout reap + risks arg mis-parse. + const isWin = process.platform === 'win32'; + + test('win32 RED: a .cmd shim runs (exit 0, non-empty stdout) — pre-fix this threw EINVAL → exit 125 / empty stdout', { skip: !isWin ? 'win32-only' : false }, () => { + const dir = createTempDir('rwt-2667-cmd'); + try { + // A .cmd shim that echoes JSON to stdout (mimics fallow.cmd audit --format json). + 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 .cmd shim to run (exit 0); got ${r.status}. stderr: ${r.stderr}`); + assert.ok((r.stdout || '').includes('clean'), `expected non-empty JSON stdout from the .cmd shim; got: ${r.stdout}`); + } finally { + cleanup(dir); + } + }); + + test('win32: a .bat shim is also mediated (exit 0, non-empty stdout)', { skip: !isWin ? 'win32-only' : false }, () => { + const dir = createTempDir('rwt-2667-bat'); + try { + 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 .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'); + } finally { + cleanup(dir); + } + }); + + test('win32 negative-space: a .exe (node.exe) is spawned DIRECTLY, not mediated — no cmd.exe wrap', { skip: !isWin ? 'win32-only' : false }, () => { + // .exe is intentionally excluded from the gate: real PE executables spawn + // fine directly, and wrapping them in cmd.exe /c breaks the timeout cap's + // process-group reap AND risks cmd.exe mis-parsing args (e.g. -e "code()"). + // node.exe -e "process.exit(0)" must exit 0 directly. + const r = runVerb(['10', '--', process.execPath, '-e', 'process.exit(0)']); + assert.equal(r.status, 0, `expected node.exe to run directly (exit 0); got ${r.status}. stderr: ${r.stderr}`); + }); + + test('POSIX negative-space: a bash -c caller is unchanged (no shell:true added) — argv stays array-only', { skip: isWin ? 'posix-only' : false }, () => { + // The fix's gate (win32 && .cmd/.bat) skips `bash` on POSIX: behavior + // must be identical to before. `bash -c 'echo ok'` exits 0 with stdout "ok". + const r = runVerb(['10', '--', 'bash', '-c', 'echo ok']); + assert.equal(r.status, 0, `expected bash caller to still work (exit 0); got ${r.status}`); + assert.equal((r.stdout || '').trim(), 'ok', 'expected stdout "ok" from the unchanged bash caller'); + }); +}); + describe('#2351 run-with-timeout — coreutils independence (the regression)', () => { // The whole point: no dependency on GNU `timeout`/`gtimeout`. Prove it by // scrubbing PATH so neither could be found, and driving the child by absolute