From 56417bf7b56f8cd24dedf9ff1ba338574450b8b0 Mon Sep 17 00:00:00 2001 From: KassaSana Date: Thu, 24 Sep 2026 04:39:30 -0400 Subject: [PATCH 1/3] fix(cli): quote the cmd.exe command path with real quotes on Windows runClientCommand() launches .cmd shims such as npx.cmd through `cmd.exe /d /s /c`, and escaped the resolved program path with the same escapeCmdArg() used for arguments. That produces ^"C:\Program Files\...^". For arguments this works, because cmd.exe strips the carets and passes the quotes on to the program. The program path, though, is parsed by cmd.exe itself, where ^" is a literal character rather than a quote, so the path splits at the first space: '"C:\Program' is not recognized as an internal or external command This broke `firecrawl setup mcp`, skills installs and every other setup step that runs npx whenever Node lives under C:\Program Files, which is the default install location. Quote the program path with plain quotes (Windows paths cannot contain `"`) and leave argument escaping unchanged. Escaping the arguments with unescaped outer quotes as well would stop the carets from being consumed, so an argument like `https://host/mcp?a=1&b=2` would arrive as `a=1^&b=2`. The existing win32 test mocked child_process and asserted the ^"...^" form, so it could not see the failure. Update that assertion, and add a win32-only test that spawns a real cmd.exe with a .cmd shim under "Program Files (x86)" and checks the argv it receives. Fixes #188 --- .../commands/setup-windows-spawn.test.ts | 49 +++++++++++++++++++ src/__tests__/commands/setup.test.ts | 6 ++- src/commands/setup.ts | 17 ++++++- 3 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 src/__tests__/commands/setup-windows-spawn.test.ts 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..e8dca0c080 --- /dev/null +++ b/src/__tests__/commands/setup-windows-spawn.test.ts @@ -0,0 +1,49 @@ +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('launches a .cmd shim under a path with spaces and parentheses and passes argv through exactly', () => { + const root = mkdtempSync(path.join(os.tmpdir(), 'firecrawl-spawn-')); + const bin = path.join(root, 'Program Files (x86)', '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..2fdfac643a 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -115,6 +115,19 @@ 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 + * `"`, so plain quotes are unambiguous here. */ +function quoteCmdCommand(command: string): string { + rejectCommandControlCharacters(command, 'Command'); + if (command.includes('"')) { + throw new Error('Command path contains an unsupported quote character.'); + } + return `"${command}"`; +} + function windowsPathExtensions(env: NodeJS.ProcessEnv): string[] { const configured = env.PATHEXT ?? '.COM;.EXE;.BAT;.CMD'; return configured @@ -168,7 +181,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 +202,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, From 81eeaffd2161e48dbe226d41436d759114b6cfbc Mon Sep 17 00:00:00 2001 From: KassaSana Date: Thu, 24 Sep 2026 13:00:19 -0400 Subject: [PATCH 2/3] fix(cli): keep %VAR% in the cmd.exe command path from expanding cmd.exe expands %VAR% (and !VAR! under delayed expansion) even inside double quotes, where a caret is a literal character. With the command path now in plain quotes, a launcher under a directory such as `dir%USERNAME%x` was rewritten to the variable's value and failed with "The system cannot find the path specified" (the previous caret-escaped form handled this, but only for paths without spaces). Close the quotes around each % and ! and caret-escape it outside them, e.g. "C:\a"^%"X"^%"b\npx.cmd", so the path is passed through literally whether or not it contains spaces. Extend the real cmd.exe test to cover %USERNAME% in the path, with and without spaces. --- .../commands/setup-windows-spawn.test.ts | 65 +++++++++++-------- src/commands/setup.ts | 6 +- 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/src/__tests__/commands/setup-windows-spawn.test.ts b/src/__tests__/commands/setup-windows-spawn.test.ts index e8dca0c080..4b87600ccf 100644 --- a/src/__tests__/commands/setup-windows-spawn.test.ts +++ b/src/__tests__/commands/setup-windows-spawn.test.ts @@ -14,36 +14,45 @@ import { runClientCommand } from '../../commands/setup'; describe.runIf(process.platform === 'win32')( 'runClientCommand on Windows (real cmd.exe)', () => { - it('launches a .cmd shim under a path with spaces and parentheses and passes argv through exactly', () => { - const root = mkdtempSync(path.join(os.tmpdir(), 'firecrawl-spawn-')); - const bin = path.join(root, 'Program Files (x86)', '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}', - ]; + 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 }, - }); + 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 }); + expect(JSON.parse(readFileSync(outFile, 'utf8'))).toEqual(args); + } finally { + rmSync(root, { recursive: true, force: true }); + } } - }); + ); } ); diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 2fdfac643a..8e3ce6e068 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -119,13 +119,15 @@ function escapeCmdArg(arg: string): string { * 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 - * `"`, so plain quotes are unambiguous here. */ + * `"`. 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}"`; + return `"${command.replace(/[%!]/g, '"^$&"')}"`; } function windowsPathExtensions(env: NodeJS.ProcessEnv): string[] { From 178bf160d22bc0a312b379605cb5035c3c8cfc28 Mon Sep 17 00:00:00 2001 From: KassaSana Date: Thu, 24 Sep 2026 13:29:14 -0400 Subject: [PATCH 3/3] ci: run the real cmd.exe spawn tests on Windows setup-windows-spawn.test.ts only runs on win32, and the main test job runs on ubuntu-latest, so those tests were always skipped in CI. Add a windows-latest job that runs just that file, reusing the existing Node and pnpm setup steps. It targets the one file rather than adding Windows to the main job, because the full suite has pre-existing Windows-only failures (path assumptions in setup, credentials and web-defaults tests) that are identical on main. --- .github/workflows/test.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) 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: