diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4ac45813c7..63491c0b30 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -40,6 +40,31 @@ jobs: - name: Run tests run: pnpm run test + # The main test job runs on Linux, where the real cmd.exe spawn tests are + # skipped. Run them on Windows so command quoting stays covered. + test-windows-spawn: + runs-on: windows-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v6 + + - name: Setup Node.js + uses: actions/setup-node@v6 + with: + node-version: '22' + + - name: Setup pnpm + uses: pnpm/action-setup@v4 + with: + version: 10.12.1 + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Run Windows spawn tests + run: pnpm exec vitest run src/__tests__/commands/setup-windows-spawn.test.ts + test-binary: if: github.event_name == 'pull_request' strategy: diff --git a/src/__tests__/commands/setup-windows-spawn.test.ts b/src/__tests__/commands/setup-windows-spawn.test.ts new file mode 100644 index 0000000000..4b87600ccf --- /dev/null +++ b/src/__tests__/commands/setup-windows-spawn.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'vitest'; +import { + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'fs'; +import os from 'os'; +import path from 'path'; +import { runClientCommand } from '../../commands/setup'; + +// Spawns a real cmd.exe, unlike setup.test.ts which mocks child_process. +describe.runIf(process.platform === 'win32')( + 'runClientCommand on Windows (real cmd.exe)', + () => { + it.each([ + ['spaces and parentheses', 'Program Files (x86)'], + // cmd.exe expands %VAR% even inside quotes, so the path must not be + // rewritten to the variable's value. + ['a %VAR% sequence', 'dir %USERNAME% x'], + ['a %VAR% sequence without spaces', 'dir%USERNAME%x'], + ])( + 'launches a .cmd shim under a path with %s and passes argv through exactly', + (_, dirName) => { + const root = mkdtempSync(path.join(os.tmpdir(), 'firecrawl-spawn-')); + const bin = path.join(root, dirName, 'nodejs'); + const outFile = path.join(root, 'argv.json'); + try { + mkdirSync(bin, { recursive: true }); + writeFileSync( + path.join(bin, 'record-argv.js'), + 'require("fs").writeFileSync(process.env.ARGV_OUT, JSON.stringify(process.argv.slice(2)));\n' + ); + writeFileSync( + path.join(bin, 'record-argv.cmd'), + '@echo off\r\nnode "%~dp0record-argv.js" %*\r\n' + ); + const args = [ + '--name', + 'firecrawl', + 'https://mcp.firecrawl.dev/v2/mcp?a=1&b=2', + 'Authorization: Bearer ${FIRECRAWL_API_KEY}', + ]; + + runClientCommand(path.join(bin, 'record-argv.cmd'), args, { + stdio: 'pipe', + env: { ...process.env, ARGV_OUT: outFile }, + }); + + expect(JSON.parse(readFileSync(outFile, 'utf8'))).toEqual(args); + } finally { + rmSync(root, { recursive: true, force: true }); + } + } + ); + } +); diff --git a/src/__tests__/commands/setup.test.ts b/src/__tests__/commands/setup.test.ts index 530062c681..e02947a845 100644 --- a/src/__tests__/commands/setup.test.ts +++ b/src/__tests__/commands/setup.test.ts @@ -886,7 +886,11 @@ describe('handleSetupCommand', () => { expect(command).toBe('cmd.exe'); expect(passthruArgs.slice(0, 3)).toEqual(['/d', '/s', '/c']); expect(opts?.windowsVerbatimArguments).toBe(true); - expect(passthruArgs[3]).toContain(`^\"${path.join(bin, 'npx.CMD')}^\"`); + // cmd.exe parses the command token itself, so its quotes must stay real + // quotes: a caret-escaped ^" is a literal character there, and the path + // would split at the space in "Program Files". + expect(passthruArgs[3]).toContain(`""${path.join(bin, 'npx.CMD')}" `); + expect(passthruArgs[3]).not.toContain(`^\"${path.join(bin, 'npx.CMD')}`); expect(passthruArgs[3]).toContain('add-mcp@1.14.0'); expect(passthruArgs[3]).toContain( '^"Authorization: Bearer ${FIRECRAWL_API_KEY}^"' diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 0b444a4ee2..8e3ce6e068 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -115,6 +115,21 @@ function escapeCmdArg(arg: string): string { return quoted.replace(CMD_META_CHARS, '^$1'); } +/** Quote the program path that cmd.exe itself resolves. Unlike arguments, this + * token is parsed by cmd.exe, where a caret-escaped ^" is a literal character + * rather than a quote, so escaping it would split paths such as + * `C:\Program Files\nodejs\npx.cmd` at the space. Windows paths cannot contain + * `"`. cmd.exe still expands `%VAR%` (and `!VAR!` under delayed expansion) + * inside quotes, and a caret is literal there, so each `%`/`!` is placed + * outside the quotes and caret-escaped: `"C:\a"^%"X"^%"b\npx.cmd"`. */ +function quoteCmdCommand(command: string): string { + rejectCommandControlCharacters(command, 'Command'); + if (command.includes('"')) { + throw new Error('Command path contains an unsupported quote character.'); + } + return `"${command.replace(/[%!]/g, '"^$&"')}"`; +} + function windowsPathExtensions(env: NodeJS.ProcessEnv): string[] { const configured = env.PATHEXT ?? '.COM;.EXE;.BAT;.CMD'; return configured @@ -168,7 +183,7 @@ function resolveWindowsCommand( * On every other platform we spawn the binary directly with no shell, exactly as * `execFileSync` did before. */ -function runClientCommand( +export function runClientCommand( command: string, args: string[], options: Parameters[2] @@ -189,7 +204,7 @@ function runClientCommand( return; } - const line = [escapeCmdArg(resolved), ...args.map(escapeCmdArg)].join(' '); + const line = [quoteCmdCommand(resolved), ...args.map(escapeCmdArg)].join(' '); const comspec = env.ComSpec ?? env.COMSPEC ?? 'cmd.exe'; const windowsOptions = { ...options,