Repository navigation
fix(desktop): caret-escape and fail-close the WSL cmd.exe URL open - #134293
Yicheng-Mark wants to merge 1 commit into
Conversation
openViaWsl passed the URL unquoted into `cmd.exe /c start "" <url>`;
cmd re-parses the command line, so metacharacters in the URL enabled
command concatenation (& calc.exe) and %var% expansion (131561).
escapeUrlForCmdStart(): reject outright what caret-escaping cannot
neutralize (", %, !, whitespace/controls) -- refusing to open beats
executing a command -- and caret-escape the escapable metacharacters
(& | < > ^ ( ) , ; =). openViaWsl resolves invalid without spawning
on the reject tier. Scheme allowlist unchanged.
Fixes NousResearch#131561.
Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Heads-up on something odd: I have a follow-up fix ready for #134283 (the New Project dialog pathless create — branch and regression test are pushed to my fork), but every attempt to open that PR against this repo from my account is rejected (REST returns 404, GraphQL returns a permission error). It started on Oct 7, shortly after this PR was opened. Everything else still works from my side — commenting, reacting, editing, and PR creation on my own fork all succeed — and other external contributors are opening PRs normally. Could an automated restriction have been applied to my account on this repo? If it can be lifted, I'll open the #134283 PR right away. |
|
Closing as superseded: #126968 (opened 09-28) closes the same cmd.exe re-parsing hole on the same two files with the same fail-closed approach, and it got there first. Keeping both open only adds duplicate weight to the triage of #131561. The patch stays on the fork ( |
Symptom
On WSL, every external URL is opened via
cmd.exe /c start "" <url>with the URL passed unquoted in the argv (apps/desktop/electron/external-open.ts,openViaWsl).cmd.exere-parses the command line Node assembles from argv, so shell metacharacters in the URL enable command concatenation, exactly as reported in #131561:The
http/https/mailtoscheme allowlist does not filter& | < > ^ ( ) , ; =, and%var%expansion happens before caret-escaping, so%cannot be neutralized either.Cause
openViaWslhandsparsed.toString()straight todeps.spawn('cmd.exe', ['/c', 'start', '""', url]). Node only quotes argv values that contain spaces, so a URL without spaces reaches cmd.exe unquoted — and cmd re-parses every metacharacter in it as an operator.Fix
Two-tier, both fail-closed, in a new exported
escapeUrlForCmdStart(), as suggested in the issue:"(would need quotingstart's argv cannot express),%(variable expansion runs before cmd applies^escapes —%name%cannot be defused on a command line at all),!(delayed expansion), and any whitespace/control character (the URL would split across argv slots).openViaWslrefuses the open (invalid), logs, and never spawns.& | < > ^ ( ) , ; =, so they reachstartas literals instead of operators — this also keeps, ; =from splitting the URL across argv slots.A URL that survived
new URL().toString()never carries a raw quote, space or control character (the WHATWG parser percent-encodes them), so the reject tier is defense in depth;%(legit percent-encoded URLs) is the one real-world casualty — refusing to open beats executing a command.How to Verify
external-open.test.ts: a 20-case probe suite (8 attack URLs fully escaped, 8 unescapable rejected, 4 clean URLs unchanged), plus twoopenExternalUrlintegration tests — the spawned argv carries the caret-escaped URL, and an unescapable URL resolvesinvalidwith zero spawns and a log line.tsc), and the compiled module was run against the 20-probe matrix — all green. The suite runs in CI.Fixes #131561
Type of Change
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — TS files; local verification was transpile + 20-probe matrix against the compiled module (above); suite runs in CIDocumentation & Housekeeping
cli-config.yaml.example— N/ACONTRIBUTING.mdorAGENTS.md— N/Adeps.isWslbranch (WSL-only route); non-WSL platforms take the identical path as before