PDX-504: quote sf executable and args for spaces on Windows#200
Conversation
RCA: On Windows the sf launcher is a .cmd run via shell:true, where Node concatenates executable + args unquoted, so any space (auto-discovered C:\Program Files\sf install, an explicit sf_path, or a --properties-file value under a Provar Manager directory) split the command — breaking the default Windows install case. Fix: Pre-quote the executable and every argument with quoteWindowsToken on the win32 shell branch (auto-resolved paths included, not gated on sf_path); Node's join + cmd /s outer-quote strip preserves per-token quoting. Metacharacter rejection still applies to user-supplied paths. Adds win32 unit tests and a docs note.
Quality Orchestrator🟢 LOW · 🧪 Tests to Run · Running 1 of 54 tests
▶ Run commandnpx vitest run \
unit/mcp/sfSpawn.test.ts⚡ quality-orchestrator · |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Windows-specific sf subprocess invocation bug where paths/arguments containing spaces were split when using shell: true (required for .cmd launchers). It introduces per-token quoting on the Windows shell path, adds unit coverage for spaced executable/argument cases, and documents that 8.3 short-name workarounds are no longer needed on Windows.
Changes:
- Add
quoteWindowsToken()and apply it to the executable + args whenneedsWindowsShell(...)is true. - Add 5 unit tests validating quoting behavior for spaced auto-resolved paths, explicit
sf_path, and spaced argument values. - Update MCP docs to clarify Windows spaced paths are handled automatically and metacharacter rejection still applies to user-provided
sf_path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/mcp/tools/sfSpawn.ts |
Adds Windows token-quoting helper and applies it for shell:true spawns to prevent space-splitting. |
test/unit/mcp/sfSpawn.test.ts |
Adds win32-focused tests verifying executable/args are not split by spaces and metachar injection guard remains. |
docs/mcp.md |
Documents the Windows quoting behavior and removal of the 8.3 short-name workaround requirement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function quoteWindowsToken(token: string): string { | ||
| if (token === '') return '""'; | ||
| if (/[\s"&|<>^()]/.test(token)) { | ||
| return `"${token.replace(/"/g, '""')}"`; | ||
| } | ||
| return token; | ||
| } |
There was a problem hiding this comment.
Thanks — but this is a false positive for cmd.exe, and applying the suggestion would introduce a regression.
In cmd.exe, ^ is an escape character only outside double quotes. Inside a double-quoted token the caret is literal: "abc^" does not let the caret escape the closing quote, and operator parsing is not re-enabled. So quoteWindowsToken wrapping a caret-containing token is already safe.
Escaping ^ → ^^ before wrapping would actively corrupt real inputs: ^ is a legal Windows filename character, so a path such as C:\allowed\weird^name\provardx-properties.json would become weird^^name inside the quotes and fail to resolve. Keeping ^ in the quote-trigger set is in fact the safer choice — an unquoted caret would be the real hazard, and we always quote it.
Injection surface is also nil here: a user-supplied sf_path containing shell metacharacters is rejected up front by assertShellSafePath (INVALID_SF_PATH), and the argument values are internally constructed (file paths already validated against --allowed-paths). Leaving the implementation as-is.
Summary
sfinvocation split on spaces. On Windows thesflauncher is a.cmdand must run throughcmd.exe(shell: true); Node then concatenates executable + args into one unquoted string, so any space broke the command ('C:\Program' is not recognized ...).getSfCommonPaths()auto-discoversC:\Program Files\sf\..., and arg values under aProvar Managerdirectory also split.quoteWindowsToken). Node joins them and wraps the line in cmd's outer quotes; cmd's/srule strips only that outermost pair, preserving our per-token quoting. Applied to auto-resolved paths too — not gated on a user-suppliedsf_path. The 3-argspawnSync(executable, args, opts)shape is kept, so space-free tokens are byte-identical (no behavioural change off the spaced-path case).assertShellSafePath→INVALID_SF_PATH) still applies to user-suppliedsf_path; spaces are now allowed (and quoted) where previously the 8.3 short-name workaround was needed.Jira
https://provartesting.atlassian.net/browse/PDX-504
Test plan
Tests (per acceptance criteria, win32 via
setSfPlatformForTesting)C:\Program Files\sf\client\bin\sf.cmdis quoted as a single token (not gated onsf_path)sf_pathis quoted--properties-fileunderProvar Manager) is quoted, not splitsf_pathwith shell metacharacters is still rejected; a spaced-but-clean one is acceptedChanges
src/mcp/tools/sfSpawn.ts:quoteWindowsTokenhelper; quote executable + args on the win32 shell branch.test/unit/mcp/sfSpawn.test.ts: 5 new win32 quoting tests.docs/mcp.md: note that Windows spaced paths are handled automatically (8.3 workaround no longer needed).