diff --git a/CHANGELOG.md b/CHANGELOG.md index f52885a7..d1a7d548 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Windows: the Claude Code executable is now resolved from `where.exe` instead of assuming the npm shim. `execFileSync` does not apply PATHEXT, and the previous hardcoded `claude.cmd` did not exist for native-installer users who have `claude.exe` - those calls raised `ENOENT`, the error was swallowed, and the CLI reported success while installing nothing. A directly launchable `claude.exe` is preferred over a batch shim regardless of PATH order. - Windows: an npm-global `claude.cmd` shim is launched through `cmd.exe` rather than handed to `execFileSync`, which fails with `EINVAL` - Node's `src` has disallowed direct `.bat`/`.cmd` spawning since the CVE-2024-27980 fix in 18.20.2 / 20.12.2 / 21.7.3. Arguments are rejected unless they are free of whitespace and shell metacharacters, so the extra hop cannot reintroduce the injection surface #388 closed. - `agentsys install` no longer reports `[OK] Installed ... successfully` when Claude Code rejected a plugin. It names the plugins that failed, with the errno when the shim could not be spawned at all, and how to retry. The same now applies when the `claude` CLI is not on PATH at all (`~/.claude` alone was enough to mark the platform as installed) and when a dependency id would be rejected. Such a plugin is also no longer recorded against `claude` in `installed.json` - `agentsys list` and `agentsys remove` read those platforms back - though a registration recorded by an earlier successful install is preserved, since a failed re-install is not evidence the first one never landed. The process now exits non-zero, matching the `--tool` path so `agentsys install x && ...` stops. +- Windows: `agentsys-dev test`, `agentsys-dev bump`, `runBenchmark`, and `runProfiling` all handed a `.cmd` shim straight to `spawnSync`/`execFileSync`, which fails with `EINVAL` for the same reason the Claude shim did - `resolveExecutableForPlatform` turns `npm` into `npm.cmd` and `node_modules/.bin/vitest` into `vitest.cmd`, so every benchmark, profile, and dev test run died on Windows. These now route through `cmd.exe` via a shared `planShimSpawn` in `lib/utils/command-parser.js`, which `bin/cli.js` uses as well so the two mechanisms cannot drift apart. Unlike the Claude call sites, these carry user-written commands, so arguments are quoted for `cmd.exe` rather than rejected for containing whitespace or metacharacters; a literal `"`, `%`, CR or LF is still refused, in the executable as well as the arguments, since none survives `cmd.exe` intact - `bench\ncalc` would otherwise reach the command line, which `bin/cli.js` is shielded from only by its allowlist. The rewrite is win32-only: a repo-local `build.cmd` on Linux is spawnable as it stands, and routing it through a `cmd.exe` that is not there would only turn a working command into `ENOENT`. `agentsys-dev test` also prints the error it used to swallow, since a refused argument now reaches the user from there. +- Windows: a custom source naming an npm-shipped CLI (`npx`, `pnpm`, `yarn`) was always probed as unavailable - `execFileSync` applies no PATHEXT, so the bare name raised `ENOENT`, and the `.cmd` it needs cannot be spawned directly either. `probeCLI` resolves the shim and routes it the same way. - `lib/utils/command-parser.js` used a raw null byte where `'\0'` was intended. Git and grep classified the file as binary, so changes to it could not be reviewed as a diff. Also normalized to LF, the only CRLF-encoded source file in the repo. ## [6.0.1] - 2026-07-22 diff --git a/__tests__/bump-version.test.js b/__tests__/bump-version.test.js index 4977fd24..b9f4a8fd 100644 --- a/__tests__/bump-version.test.js +++ b/__tests__/bump-version.test.js @@ -81,6 +81,44 @@ describe('bump-version', () => { expect(consoleErrorSpy).not.toHaveBeenCalled(); }); + test('spawns npm directly off Windows', () => { + const { execFileSync } = require('child_process'); + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); + + try { + expect(main(['3.7.3'])).toBe(0); + } finally { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + } + + expect(execFileSync).toHaveBeenCalledWith( + 'npm', + ['version', '3.7.3', '--no-git-tag-version'], + expect.not.objectContaining({ windowsVerbatimArguments: true }) + ); + }); + + test('routes the npm.cmd shim through cmd.exe on Windows', () => { + // A direct .cmd spawn fails with EINVAL since the CVE-2024-27980 fix, so + // `agentsys-dev bump` needs the same hop as the rest of the CLI. + const { execFileSync } = require('child_process'); + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + + try { + expect(main(['3.7.3'])).toBe(0); + } finally { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + } + + expect(execFileSync).toHaveBeenCalledWith( + 'cmd.exe', + ['/d', '/s', '/c', '""npm.cmd" "version" "3.7.3" "--no-git-tag-version""'], + expect.objectContaining({ windowsVerbatimArguments: true }) + ); + }); + test('rejects invalid version format - missing patch', () => { const code = main(['3.7']); expect(code).toBe(1); diff --git a/__tests__/cli-args.test.js b/__tests__/cli-args.test.js index af78c5fe..5002b7bf 100644 --- a/__tests__/cli-args.test.js +++ b/__tests__/cli-args.test.js @@ -229,34 +229,45 @@ describe('claudeSpawnPlan', () => { const args = ['plugin', 'install', 'agentsys-core@agentsys']; test('spawns a posix or native executable directly', () => { - expect(claudeSpawnPlan('claude', args)).toEqual({ file: 'claude', args }); - expect(claudeSpawnPlan('C:\\bin\\claude.exe', args)).toEqual({ file: 'C:\\bin\\claude.exe', args }); + expect(claudeSpawnPlan('claude', args)).toEqual({ file: 'claude', args, verbatim: false }); + expect(claudeSpawnPlan('C:\\bin\\claude.exe', args)).toEqual({ file: 'C:\\bin\\claude.exe', args, verbatim: false }); }); test('routes a batch shim through cmd.exe, which execFileSync cannot spawn', () => { // Node's src disallows direct .bat/.cmd spawning since the CVE-2024-27980 // fix, so a shim handed to execFileSync fails with EINVAL. const shim = 'C:\\npm\\claude.cmd'; - expect(claudeSpawnPlan(shim, args)).toEqual({ + expect(claudeSpawnPlan(shim, args, undefined, 'win32')).toEqual({ file: 'cmd.exe', - args: ['/d', '/s', '/c', '""C:\\npm\\claude.cmd" plugin install agentsys-core@agentsys"'], + args: ['/d', '/s', '/c', '""C:\\npm\\claude.cmd" "plugin" "install" "agentsys-core@agentsys""'], verbatim: true }); - expect(claudeSpawnPlan('C:\\npm\\claude.bat', args).file).toBe('cmd.exe'); + expect(claudeSpawnPlan('C:\\npm\\claude.bat', args, undefined, 'win32').file).toBe('cmd.exe'); + }); + + test('spawns a .cmd directly off Windows, where cmd.exe does not exist', () => { + // The extension alone is not evidence a file needs a shell: routing a + // repo-local build.cmd through a missing cmd.exe would only cause ENOENT. + expect(claudeSpawnPlan('./claude.cmd', args, undefined, 'linux')) + .toEqual({ file: './claude.cmd', args, verbatim: false }); }); test('honours COMSPEC when routing through a shell', () => { - expect(claudeSpawnPlan('claude.cmd', args, 'C:\\Windows\\system32\\cmd.exe').file) + expect(claudeSpawnPlan('claude.cmd', args, 'C:\\Windows\\system32\\cmd.exe', 'win32').file) .toBe('C:\\Windows\\system32\\cmd.exe'); }); test('refuses arguments cmd.exe would reparse', () => { // cmd.exe re-splits its command line, so an unquoted metacharacter would be // a command injection - the exact hazard behind CVE-2024-27980. - expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'x&calc'])).toThrow(/Refusing to pass/); - expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a|b'])).toThrow(/Refusing to pass/); - expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a b'])).toThrow(/Refusing to pass/); - expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a"b'])).toThrow(/Refusing to pass/); + const reject = arg => () => claudeSpawnPlan('claude.cmd', ['plugin', 'install', arg], undefined, 'win32'); + expect(reject('x&calc')).toThrow(/Refusing to pass/); + expect(reject('a|b')).toThrow(/Refusing to pass/); + expect(reject('a b')).toThrow(/Refusing to pass/); + expect(reject('a"b')).toThrow(/Refusing to pass/); + // CR and LF are refused wherever they appear, trailing included. + expect(reject('agentsys-core@agentsys\n')).toThrow(/Refusing to pass/); + expect(reject('a\n&calc')).toThrow(/Refusing to pass/); }); test('passes the same arguments through unchecked when no shell is involved', () => { diff --git a/__tests__/command-parser.test.js b/__tests__/command-parser.test.js index 71fdb922..2ec58b79 100644 --- a/__tests__/command-parser.test.js +++ b/__tests__/command-parser.test.js @@ -1,6 +1,6 @@ const fs = require('fs'); const path = require('path'); -const { parseCommand, resolveExecutableForPlatform } = require('../lib/utils/command-parser'); +const { parseCommand, resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../lib/utils/command-parser'); describe('command parser', () => { test('parses simple command into executable and args', () => { @@ -70,6 +70,101 @@ describe('resolveExecutableForPlatform', () => { }); }); +describe('planShimSpawn', () => { + // The .bat/.cmd rewrite is win32-only, so every shim case names the platform + // rather than depending on the host running the suite. + const plan = (executable, args, options = {}) => + planShimSpawn(executable, args, { platform: 'win32', ...options }); + + test('leaves a directly spawnable executable alone', () => { + const args = ['run', 'bench']; + expect(plan('npm', args)).toEqual({ file: 'npm', args, verbatim: false }); + expect(plan('node.exe', args)).toEqual({ file: 'node.exe', args, verbatim: false }); + expect(plan('/usr/bin/node', args)).toEqual({ file: '/usr/bin/node', args, verbatim: false }); + }); + + test('leaves a .cmd alone off Windows, where cmd.exe does not exist', () => { + // A repo-local build.cmd on Linux spawns as it stands; rewriting it to a + // cmd.exe that is not there would turn a working command into ENOENT. + const args = ['run', 'bench']; + for (const platform of ['linux', 'darwin']) { + expect(planShimSpawn('./build.cmd', args, { platform })) + .toEqual({ file: './build.cmd', args, verbatim: false }); + } + }); + + test('routes a batch shim through cmd.exe, which spawn cannot launch', () => { + // Node's src disallows direct .bat/.cmd spawning since the CVE-2024-27980 + // fix, so spawnSync/execFileSync fail with EINVAL on a shim. + expect(plan('npm.cmd', ['run', 'bench'])).toEqual({ + file: 'cmd.exe', + args: ['/d', '/s', '/c', '""npm.cmd" "run" "bench""'], + verbatim: true + }); + expect(plan('yarn.bat', []).args).toEqual(['/d', '/s', '/c', '""yarn.bat""']); + }); + + test('quotes arguments so cmd.exe cannot reinterpret them', () => { + // Inside double quotes cmd.exe leaves these alone, so a benchmark command + // carrying them runs instead of being split into extra commands. + const [, , , payload] = plan('npm.cmd', ['run', 'a && calc', 'x|y', 'a>b']).args; + expect(payload).toBe('""npm.cmd" "run" "a && calc" "x|y" "a>b""'); + }); + + test('preserves an empty argument', () => { + const [, , , payload] = plan('npm.cmd', ['run', '']).args; + expect(payload).toBe('""npm.cmd" "run" """'); + }); + + test('doubles trailing backslashes so the closing quote survives', () => { + // "C:\dir\" would read as an escaped quote when the child parses argv back. + const [, , , payload] = plan('npm.cmd', ['--cwd', 'C:\\dir\\']).args; + expect(payload).toBe('""npm.cmd" "--cwd" "C:\\dir\\\\""'); + }); + + test('doubles a long run of backslashes without backtracking', () => { + const [, , , payload] = plan('npm.cmd', ['\\'.repeat(5000)]).args; + expect(payload).toBe(`""npm.cmd" "${'\\'.repeat(10000)}""`); + }); + + test('refuses arguments cmd.exe cannot carry faithfully', () => { + // % is expanded even inside quotes, and a literal " ends the quoting. + expect(() => plan('npm.cmd', ['run', '%PATH%'])).toThrow(/not representable/); + expect(() => plan('npm.cmd', ['run', 'say "hi"'])).toThrow(/not representable/); + // A newline ends the command line, so what follows is dropped or run alone. + expect(() => plan('npm.cmd', ['run', 'bench\ncalc'])).toThrow(/not representable/); + expect(() => plan('npm.cmd', ['run', 'bench\r'])).toThrow(/not representable/); + expect(() => plan('npm.cmd', ['run', 'a\0b'])).toThrow(/null byte/); + expect(() => plan('npm.cmd', ['run', 42])).toThrow(/must be a string/); + }); + + test('refuses a shim path cmd.exe would rewrite before resolving it', () => { + // The executable lands on the same command line as the arguments, and + // cmd.exe expands %TEMP% there too - the path it opens is then not the one + // the caller named. + expect(() => plan('C:\\build%TEMP%\\.bin\\vitest.cmd', ['run'])) + .toThrow(/executable path.*not representable/s); + }); + + test('leaves those arguments alone when no shell is involved', () => { + // Nothing reparses an execFileSync argv, so the restriction is shim-only. + expect(plan('npm', ['run', '%PATH%', 'say "hi"']).args) + .toEqual(['run', '%PATH%', 'say "hi"']); + }); + + test('honours an explicit comspec', () => { + expect(plan('npm.cmd', [], { comspec: 'C:\\Windows\\system32\\cmd.exe' }).file) + .toBe('C:\\Windows\\system32\\cmd.exe'); + }); + + test('shimSpawnOptions adds windowsVerbatimArguments only for a shim', () => { + expect(shimSpawnOptions(plan('npm.cmd', ['run']), { cwd: '/tmp' })) + .toEqual({ cwd: '/tmp', windowsVerbatimArguments: true }); + expect(shimSpawnOptions(plan('npm', ['run']), { cwd: '/tmp' })) + .toEqual({ cwd: '/tmp' }); + }); +}); + describe('command-parser source hygiene', () => { test('rejects a null byte in an argument', () => { expect(() => parseCommand('node --eval a\0b')).toThrow(/null byte/); diff --git a/__tests__/dev-cli.test.js b/__tests__/dev-cli.test.js index 426772b6..c5787329 100644 --- a/__tests__/dev-cli.test.js +++ b/__tests__/dev-cli.test.js @@ -245,10 +245,17 @@ describe('dev-cli module', () => { test('test command resolves npm executable without shell interpolation', () => { expect(cliSource).toContain('resolveExecutableForPlatform'); - expect(cliSource).toContain('spawnSync(npmExecutable'); expect(cliSource).toContain('shell: false'); }); + test('test command routes the npm.cmd shim through a spawn plan', () => { + // npm resolves to npm.cmd on Windows, and a direct spawn of it fails with + // EINVAL since the CVE-2024-27980 fix, so the plan must not be bypassed. + expect(cliSource).toContain('planShimSpawn(npmExecutable'); + expect(cliSource).toContain('spawnSync(plan.file, plan.args, shimSpawnOptions(plan'); + expect(cliSource).not.toContain('spawnSync(npmExecutable'); + }); + test('exports parseArgs, COMMANDS, VALIDATE_SUBCOMMANDS, NEW_SUBCOMMANDS, route', () => { expect(cliSource).toContain('module.exports'); expect(typeof parseArgs).toBe('function'); diff --git a/__tests__/perf-benchmark-runner.test.js b/__tests__/perf-benchmark-runner.test.js index 03fba72a..ccad2fa9 100644 --- a/__tests__/perf-benchmark-runner.test.js +++ b/__tests__/perf-benchmark-runner.test.js @@ -109,6 +109,45 @@ describe('runBenchmark', () => { })); }); + it('runs a batch shim through cmd.exe instead of spawning it directly', () => { + // Node fails with EINVAL on a direct .cmd spawn since the CVE-2024-27980 + // fix, so an npm-based benchmark command needs the cmd.exe hop. The hop is + // win32-only, so the platform is faked rather than skipping off Windows. + execFileSync.mockImplementation(() => 'output'); + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + + try { + runBenchmark('npm.cmd run bench', { allowShort: true }); + } finally { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + } + + expect(execFileSync).toHaveBeenCalledWith( + 'cmd.exe', + ['/d', '/s', '/c', '""npm.cmd" "run" "bench""'], + expect.objectContaining({ windowsVerbatimArguments: true }) + ); + }); + + it('spawns a .cmd directly off Windows, where cmd.exe does not exist', () => { + execFileSync.mockImplementation(() => 'output'); + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); + + try { + runBenchmark('./build.cmd bench', { allowShort: true }); + } finally { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + } + + expect(execFileSync).toHaveBeenCalledWith( + './build.cmd', + ['bench'], + expect.not.objectContaining({ windowsVerbatimArguments: true }) + ); + }); + it('sets PERF_RUN_DURATION env variable by default', () => { execFileSync.mockImplementation(() => 'output'); diff --git a/__tests__/perf-profiling-runner.test.js b/__tests__/perf-profiling-runner.test.js index 198ea313..e4e33cb5 100644 --- a/__tests__/perf-profiling-runner.test.js +++ b/__tests__/perf-profiling-runner.test.js @@ -19,6 +19,41 @@ describe('perf profiling runner', () => { }); }); +it('runs a batch shim profiler command through cmd.exe', () => { + // A direct .cmd spawn fails with EINVAL since the CVE-2024-27980 fix. + jest.resetModules(); + const execFileSync = jest.fn(); + jest.doMock('child_process', () => ({ execFileSync })); + jest.doMock('../lib/perf/profilers', () => ({ + selectProfiler: () => ({ + id: 'fake', + buildCommand: () => 'npx.cmd clinic doctor', + parseOutput: () => ({ tool: 'fake', hotspots: [], artifacts: [] }) + }) + })); + + const runner = require('../lib/perf/profiling-runner'); + // The cmd.exe hop is win32-only, so fake the platform instead of skipping. + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + + try { + expect(runner.runProfiling().ok).toBe(true); + } finally { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + } + + expect(execFileSync).toHaveBeenCalledWith( + 'cmd.exe', + ['/d', '/s', '/c', '""npx.cmd" "clinic" "doctor""'], + expect.objectContaining({ windowsVerbatimArguments: true }) + ); + + jest.dontMock('child_process'); + jest.dontMock('../lib/perf/profilers'); + jest.resetModules(); +}); + it('does not enforce timeout when timeoutMs is not provided', () => { jest.resetModules(); const execFileSync = jest.fn(); diff --git a/bin/cli.js b/bin/cli.js index 4a0bbc88..b6775492 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -20,7 +20,7 @@ const VERSION = require('../package.json').version; const PACKAGE_DIR = path.join(__dirname, '..'); const discovery = require('../lib/discovery'); const transforms = require('../lib/adapter-transforms'); -const { resolveExecutableForPlatform } = require('../lib/utils/command-parser'); +const { resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../lib/utils/command-parser'); // Valid tool names const VALID_TOOLS = ['claude', 'opencode', 'codex', 'cursor', 'kiro']; @@ -100,23 +100,21 @@ function pickClaudeExecutable(platform, whereOutput) { /** * Build the file and argv for one Claude Code invocation. * - * A batch shim cannot be handed to execFileSync at all, so it is launched - * through cmd.exe. cmd.exe re-parses its command line, so every argument is - * checked first: callers only ever pass literal subcommands and validated - * `@` ids, so anything else is a bug rather than a string - * to escape. The quoting mirrors how Node wraps a shell command - /s strips the - * outer quote pair, leaving the quoted shim path as the first token. + * planShimSpawn does the cmd.exe routing a batch shim needs, on win32 only. + * Every argument is checked against a stricter rule than that quoting requires: + * callers only ever pass literal subcommands and validated + * `@` ids, so anything carrying whitespace or a shell + * metacharacter is a bug rather than a string to escape. The platform parameter + * exists so the win32 path stays testable off Windows. */ -function claudeSpawnPlan(executable, args, comspec) { - if (!WINDOWS_BATCH_SHIM.test(executable)) { - return { file: executable, args }; - } - const unsafe = args.find(arg => !CMD_SAFE_ARG.test(arg)); - if (unsafe !== undefined) { - throw new Error(`Refusing to pass ${JSON.stringify(unsafe)} to cmd.exe`); +function claudeSpawnPlan(executable, args, comspec, platform = process.platform) { + if (platform === 'win32' && WINDOWS_BATCH_SHIM.test(executable)) { + const unsafe = args.find(arg => !CMD_SAFE_ARG.test(arg)); + if (unsafe !== undefined) { + throw new Error(`Refusing to pass ${JSON.stringify(unsafe)} to cmd.exe`); + } } - const command = [`"${executable}"`, ...args].join(' '); - return { file: comspec || 'cmd.exe', args: ['/d', '/s', '/c', `"${command}"`], verbatim: true }; + return planShimSpawn(executable, args, { comspec, platform }); } let claudeBinCache; @@ -144,7 +142,7 @@ function claudeExecutable() { */ function claudeSpawn(args, options) { const plan = claudeSpawnPlan(claudeExecutable(), args, process.env.comspec); - return execFileSync(plan.file, plan.args, plan.verbatim ? { ...options, windowsVerbatimArguments: true } : options); + return execFileSync(plan.file, plan.args, shimSpawnOptions(plan, options)); } function copyDirRecursive(src, dest) { diff --git a/bin/dev-cli.js b/bin/dev-cli.js index acc3f27d..aa37d1ed 100755 --- a/bin/dev-cli.js +++ b/bin/dev-cli.js @@ -14,7 +14,7 @@ const path = require('path'); const { execSync, spawnSync } = require('child_process'); -const { resolveExecutableForPlatform } = require('../lib/utils/command-parser'); +const { resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../lib/utils/command-parser'); const VERSION = require('../package.json').version; const ROOT_DIR = path.join(__dirname, '..'); @@ -284,17 +284,23 @@ const COMMANDS = { cmdArgs.push(...args); } const npmExecutable = resolveExecutableForPlatform('npm'); - const result = spawnSync(npmExecutable, cmdArgs, { + // npm resolves to npm.cmd on Windows, which cannot be spawned directly. + const plan = planShimSpawn(npmExecutable, cmdArgs); + const result = spawnSync(plan.file, plan.args, shimSpawnOptions(plan, { cwd: ROOT_DIR, stdio: 'inherit', shell: false, windowsHide: true - }); + })); if (result.error) { throw result.error; } return typeof result.status === 'number' ? result.status : 1; } catch (err) { + // planShimSpawn refuses arguments cmd.exe cannot carry, and everything + // after `agentsys-dev test --` reaches it, so this path is user-facing: + // exiting 1 in silence would leave nothing to act on. + console.error(`[ERROR] ${err.message}`); return err.status || 1; } } diff --git a/lib/perf/benchmark-runner.js b/lib/perf/benchmark-runner.js index e571eb6e..6793882b 100644 --- a/lib/perf/benchmark-runner.js +++ b/lib/perf/benchmark-runner.js @@ -6,7 +6,7 @@ const { execFileSync } = require('child_process'); const { validateBaseline } = require('./schemas'); -const { parseCommand, resolveExecutableForPlatform } = require('../utils/command-parser'); +const { parseCommand, resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../utils/command-parser'); const DEFAULT_MIN_DURATION = 60; const BINARY_SEARCH_MIN_DURATION = 30; @@ -74,13 +74,15 @@ function runBenchmark(command, options = {}) { const start = Date.now(); let output; try { - output = execFileSync(executable, parsedCommand.args, { + // A .cmd/.bat shim cannot be spawned directly; route it through cmd.exe. + const plan = planShimSpawn(executable, parsedCommand.args); + output = execFileSync(plan.file, plan.args, shimSpawnOptions(plan, { stdio: 'pipe', encoding: 'utf8', env, windowsHide: true, cwd: options.cwd || process.cwd() - }); + })); } catch (error) { const stderr = error.stderr ? String(error.stderr).trim() : ''; const stdout = error.stdout ? String(error.stdout).trim() : ''; diff --git a/lib/perf/profiling-runner.js b/lib/perf/profiling-runner.js index 946fe8eb..0aaa4ca5 100644 --- a/lib/perf/profiling-runner.js +++ b/lib/perf/profiling-runner.js @@ -6,7 +6,7 @@ const { execFileSync } = require('child_process'); const profilers = require('./profilers'); -const { parseCommand, resolveExecutableForPlatform } = require('../utils/command-parser'); +const { parseCommand, resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../utils/command-parser'); /** * Run a profiling command and return artifacts/hotspots metadata. @@ -48,7 +48,9 @@ function runProfiling(options = {}) { execOptions.timeout = timeoutMs; } - execFileSync(executable, parsedCommand.args, execOptions); + // A .cmd/.bat shim cannot be spawned directly; route it through cmd.exe. + const plan = planShimSpawn(executable, parsedCommand.args); + execFileSync(plan.file, plan.args, shimSpawnOptions(plan, execOptions)); } catch (error) { const stderr = error.stderr ? String(error.stderr).trim() : ''; const stdout = error.stdout ? String(error.stdout).trim() : ''; diff --git a/lib/sources/custom-handler.js b/lib/sources/custom-handler.js index 0c00ade3..b9abfa02 100644 --- a/lib/sources/custom-handler.js +++ b/lib/sources/custom-handler.js @@ -7,6 +7,11 @@ const { execFileSync } = require('child_process'); const sourceCache = require('./source-cache'); +const { + resolveExecutableForPlatform, + planShimSpawn, + shimSpawnOptions +} = require('../utils/command-parser'); /** * Validate tool name to prevent command injection @@ -98,8 +103,12 @@ function probeCLI(toolName) { } try { - // Check if tool exists using execFileSync (prevents command injection) - execFileSync(toolName, ['--version'], { encoding: 'utf8', stdio: 'pipe' }); + // Check if tool exists using execFileSync (prevents command injection). + // execFileSync applies no PATHEXT, so an npm-shipped tool has to be named + // as npm.cmd on Windows - and a .cmd needs the cmd.exe hop to spawn at all. + const executable = resolveExecutableForPlatform(toolName); + const plan = planShimSpawn(executable, ['--version']); + execFileSync(plan.file, plan.args, shimSpawnOptions(plan, { encoding: 'utf8', stdio: 'pipe' })); capabilities.available = true; } catch { return capabilities; diff --git a/lib/utils/command-parser.js b/lib/utils/command-parser.js index a8b184b1..91996cab 100644 --- a/lib/utils/command-parser.js +++ b/lib/utils/command-parser.js @@ -47,6 +47,112 @@ function resolveExecutableForPlatform(executable, platform = process.platform) { : executable; } +const WINDOWS_BATCH_SHIM = /\.(cmd|bat)$/i; + +// Characters no amount of quoting carries through cmd.exe: % is expanded even +// inside double quotes, a literal " ends the quoting, and CR or LF ends the +// command line cmd.exe parses - anything after it would be dropped or run as a +// separate command. +const CMD_UNPASSABLE = /["%\r\n]/; + +function assertCmdPassable(value, executable, label) { + if (CMD_UNPASSABLE.test(value)) { + throw new Error( + `Cannot run ${executable} with ${label} ${JSON.stringify(value)}: ` + + 'a literal ", % or newline is not representable through cmd.exe' + ); + } +} + +/** + * Quote one token for a cmd.exe command line. + * + * Inside double quotes cmd.exe leaves & | < > ^ ( ) alone, so quoting every + * token is what keeps it from reinterpreting the command. Trailing backslashes + * are doubled: otherwise the closing quote reads as escaped when the child's + * CRT parses the command line back into argv. + * + * The run of trailing backslashes is counted rather than matched with /(\\+)$/, + * which backtracks once per start position and so is quadratic in the length of + * an all-backslash token. + */ +function quoteForCmd(token) { + let trailing = 0; + while (trailing < token.length && token[token.length - 1 - trailing] === '\\') { + trailing += 1; + } + return `"${token}${'\\'.repeat(trailing)}"`; +} + +/** + * Plan a spawn for an executable that may be a Windows batch shim. + * + * Node's src has disallowed direct .bat and .cmd spawning since the + * CVE-2024-27980 fix (18.20.2 / 20.12.2 / 21.7.3), so handing a shim to + * spawnSync or execFileSync fails with EINVAL instead of running it. Route it + * through cmd.exe, whose command line is rebuilt here rather than delegated to + * `shell: true` - that option concatenates arguments unquoted, which is the + * injection the CVE was about (Node warns about it as DEP0190). + * + * A literal ", % or newline cannot be carried across cmd.exe faithfully (% is + * expanded even inside quotes, a " ends the quoting, and a newline ends the + * command line), so those are refused rather than silently mangled. That covers + * the executable too: it lands on the same command line, so a shim path holding + * a % would be expanded before cmd.exe resolved it. + * + * Only win32 gets the rewrite. The extension alone is not evidence a file needs + * cmd.exe - a repo-local `build.cmd` on Linux is spawnable as it stands, and + * routing it through a cmd.exe that does not exist would only turn a working + * command into ENOENT. Pass options.platform to exercise the win32 path. + * + * Returns { file, args, verbatim }. When verbatim is true the caller must pass + * windowsVerbatimArguments so Node does not re-quote the payload. + */ +function planShimSpawn(executable, args = [], options = {}) { + assertValidToken(executable, 'Executable'); + + const platform = options.platform || process.platform; + + if (platform !== 'win32' || !WINDOWS_BATCH_SHIM.test(executable)) { + return { file: executable, args, verbatim: false }; + } + + assertCmdPassable(executable, executable, 'executable path'); + + for (const arg of args) { + if (typeof arg !== 'string') { + throw new Error('Spawn argument must be a string'); + } + if (arg.includes('\0')) { + throw new Error('Spawn argument contains invalid null byte'); + } + assertCmdPassable(arg, executable, 'argument'); + } + + const command = [executable, ...args].map(quoteForCmd).join(' '); + + return { + // /s strips the outer quote pair, leaving the quoted shim path as the first + // token - the same shape Node builds for a shell command. + file: options.comspec || process.env.comspec || 'cmd.exe', + // CodeQL reports the concatenation below as a shell command built from input + // (js/shell-command-constructed-from-input), which is what a /c payload is: + // cmd.exe takes one command-line string and nothing else. The per-token + // quoting and the "/%/newline refusals above are the mitigation, which the + // query cannot model. Suppression comments are ignored by default-setup code + // scanning, so the alert is dismissed in the security tab instead. + args: ['/d', '/s', '/c', `"${command}"`], + verbatim: true + }; +} + +/** + * Merge a spawn plan's verbatim flag into caller-supplied spawn options. + */ +function shimSpawnOptions(plan, options = {}) { + return plan.verbatim ? { ...options, windowsVerbatimArguments: true } : options; +} + function tokenize(command) { const trimmed = command.trim(); if (!trimmed) { @@ -156,5 +262,7 @@ function parseCommand(command, label = 'Command') { module.exports = { parseCommand, - resolveExecutableForPlatform + resolveExecutableForPlatform, + planShimSpawn, + shimSpawnOptions }; diff --git a/scripts/bump-version.js b/scripts/bump-version.js index 4a5679c6..533508d7 100644 --- a/scripts/bump-version.js +++ b/scripts/bump-version.js @@ -17,6 +17,11 @@ const { execFileSync } = require('child_process'); const path = require('path'); +const { + resolveExecutableForPlatform, + planShimSpawn, + shimSpawnOptions +} = require('../lib/utils/command-parser'); const VERSION_PATTERN = /^\d+\.\d+\.\d+(-[a-zA-Z0-9.]+)?$/; function main(args) { @@ -58,13 +63,15 @@ Files updated (via npm version + stamp-version.js): // npm version updates package.json + package-lock.json, then triggers // the "version" lifecycle script which runs stamp-version.js. // Version is validated by VERSION_PATTERN above (safe for shell use). - // On Windows, execFileSync does not resolve npm.cmd via PATHEXT. - // Prefer the explicit .cmd suffix when running on win32. - const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm'; - execFileSync(npmCommand, ['version', newVersion, '--no-git-tag-version'], { + // On Windows, execFileSync does not resolve npm via PATHEXT - and the + // npm.cmd shim it needs cannot be spawned directly either, so route it + // through the same planner the CLI and the perf runners use. + const npmCommand = resolveExecutableForPlatform('npm'); + const plan = planShimSpawn(npmCommand, ['version', newVersion, '--no-git-tag-version']); + execFileSync(plan.file, plan.args, shimSpawnOptions(plan, { cwd: path.join(__dirname, '..'), stdio: 'inherit' - }); + })); } catch (err) { console.error(`[ERROR] npm version failed: ${err.message}`); return 1; diff --git a/tests/sources/custom-handler.test.js b/tests/sources/custom-handler.test.js index ac05e085..89605d57 100644 --- a/tests/sources/custom-handler.test.js +++ b/tests/sources/custom-handler.test.js @@ -174,6 +174,39 @@ describe('Custom Handler', () => { expect(result.tool).toBe('nonexistent-tool'); }); + it('should probe an npm-shipped tool through its Windows shim', () => { + // execFileSync applies no PATHEXT, so a bare 'npx' is ENOENT on Windows - + // and npx.cmd cannot be spawned directly since the CVE-2024-27980 fix, so + // the probe would report an installed tool as unavailable either way. + execFileSync.mockReturnValue('11.0.0'); + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + + let result; + try { + result = customHandler.probeCLI('npx'); + } finally { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + } + + expect(result.available).toBe(true); + expect(execFileSync).toHaveBeenCalledWith( + 'cmd.exe', + ['/d', '/s', '/c', '""npx.cmd" "--version""'], + expect.objectContaining({ windowsVerbatimArguments: true }) + ); + }); + + it('should probe a plain executable directly off Windows', () => { + execFileSync.mockReturnValue('gh version 2.0.0'); + + customHandler.probeCLI('gh'); + + expect(execFileSync).toHaveBeenCalledWith('gh', ['--version'], expect.objectContaining({ + stdio: 'pipe' + })); + }); + it('should return known patterns for gh tool', () => { execFileSync.mockReturnValue('gh version 2.0.0');