Repository navigation
Conversation
…tart The WSL branch of the external-open chokepoint spawned `cmd.exe /c start "" <url>`. cmd.exe parses `&` in a query string as a command separator and Node's Windows argument escaping never quotes it, so an agent-produced markdown link like `https://example.com/x&calc` executed `calc` on the Windows host, and every ordinary `?a=1&b=2` URL failed to open on WSL (NousResearch#126939). Quoting the argument is not a fix: cmd.exe still expands `%VAR%` inside quotes, and `%` is pervasive in percent-encoded URLs. Replace the sink with `rundll32.exe url.dll,FileProtocolHandler`, which is not a shell: it never treats `&`, `|`, or `^` as separators and never expands `%VAR%`, so the URL travels as a single argv element to the default protocol handler. A fail-closed guard additionally rejects quotes, whitespace, and control characters — the only characters that could split or escape the launch argument for this sink. The spawn-error fallback to xdg-open is unchanged.
- Pin the spawn options. Under WSL the Electron process is Linux, so a `shell: true` would route the URL through /bin/sh, which also splits on `&`. No existing test failed with that mutation. - Exercise the launch guard through openExternalUrl(). After URL normalization it is reachable only for a mailto address (opaque path) that keeps a quote or a space; the helper-level tests still passed with the guard call removed from openViaWsl(). - Cover raw quote/CRLF/`%PATH%` input with an expectation built from `new URL()`, so the test holds on every supported Node line (older releases keep `^` in paths, newer WHATWG parsers encode it as %5E). - Cover mailto query metacharacters reaching rundll32 unchanged. - Apply Prettier to the test file (the widened import line).
14 tasks done
|
🤖 Hermes Agent automated review: PR reviewed. Diff analyzed (237 lines changed). Check CI status and manual review recommended. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Security fix. Public counterpart of the private advisory
GHSA-333f-mm3q-xg95(severity High, CWE-78). It was reported privately first; the maintainers asked for the fix to be posted as a public PR.Supersedes #126974. It cherry-picks @liuhao1024's commit unchanged and adds regression tests that close gaps in that PR's coverage. Fixes #126939.
What does this PR do?
When Hermes Desktop runs inside WSL,
openExternalUrl()hands everyhttp:/https:/mailto:link to Windows ascmd.exe /c start "" <url>. cmd.exe parses that command line again, so&,|,^and%VAR%inside a valid URL are treated as command syntax. A link such ashttps://example.com/?q=1&calc.exestartscalc.exewhen the user clicks it, and ordinary?a=1&b=2links fail to open.The first commit is @liuhao1024's fix from #126974, unchanged:
rundll32.exe url.dll,FileProtocolHandler, which is not a shell.The second commit adds regression tests for two changes that #126974's tests do not catch: adding
shell: trueto the spawn, and removing the guard call fromopenViaWsl(). It also makes the new tests independent of the Node version.apps/desktop/electron/external-open.tshermes:openExternal(rendered links, previews)main@95ee74db64Impact
&and|are legal URL characters andnew URL()keeps them as they are.Root cause
apps/desktop/electron/external-open.ts::openViaWslonmain:spawnkeeps the URL as one argv element, butcmd.exe /ctakes the rest of its command line as a single string and parses it again. Quoting the URL would not help, because cmd.exe still expands%VAR%inside quotes.Related Issue
Fixes #126939. It was also reported privately as
GHSA-333f-mm3q-xg95(visible to maintainers only).Supersedes #126974. That PR's commit is included unchanged, with @liuhao1024 kept as its author.
Other open attempts on the same path:
cmd.exeand refuses any URL that contains a metacharacter, so ordinary?a=1&b=2links stop opening.cmd.exeand quotes the URL, but%VAR%still expands inside quotes.Type of Change
Changes Made
Commit 1, @liuhao1024 (from #126974, unchanged):
fix(desktop): hand WSL external URLs to rundll32 instead of cmd.exe startapps/desktop/electron/external-open.ts:openViaWsl()spawnsrundll32.exe url.dll,FileProtocolHandler <url>. The fallback to xdg-open when the spawn fails is unchanged.isUnsafeWslLaunchArgument()guard. URLs containing quotes, whitespace or control characters resolve asinvalid, and nothing is spawned.apps/desktop/electron/external-open.test.ts: tests for the exact rundll32 argv, for a URL with&and^staying one argument, and unit tests for the guard helper.Commit 2, FroRaut:
test(desktop): pin the WSL rundll32 hand-off end to end. Changes toapps/desktop/electron/external-open.test.tsonly:{ detached, stdio, windowsHide }, noshell). Under WSL the Electron process runs on Linux, soshell: truewould pass the URL to/bin/sh, which also splits on&. Withshell: trueadded, every fix(desktop): hand WSL external URLs to rundll32 instead of cmd.exe start #126974 test still passed; only this new test fails.openExternalUrl(). After URL normalization, the guard can only be reached by amailto:address that keeps a quote or space:http(s)URLs get those characters percent-encoded, andmailto:addresses do not. The new test checks that such a link is refused, nothing is spawned, and nothing falls back to xdg-open. With the guard call removed fromopenViaWsl(), all of fix(desktop): hand WSL external URLs to rundll32 instead of cmd.exe start #126974's tests still passed, because they only test the helper. The new test fails in that case.&,|,^and%PATH%. The expected argument is built withnew URL(), because Node releases differ on^in paths (older ones keep it, newer ones write%5E). The test also checks that the quote is encoded, CR/LF are gone, and&,|and%PATH%remain URL data.mailto:query covered.&and|in amailto:query reach rundll32 unchanged.prettier --check.How to Test
cd apps/desktop npx vitest run electron/external-open.test.ts electron/preload-external-open.test.ts npx eslint electron/external-open.ts electron/external-open.test.ts npx prettier --check electron/external-open.ts electron/external-open.test.ts npx tsc -p tsconfig.electron.json --noEmitexternal-open.test.ts18/18,preload-external-open.test.ts1/1). ESLint, Prettier andtscare clean.external-open.tsfrommain, 7 tests fail: the 4 from fix(desktop): hand WSL external URLs to rundll32 instead of cmd.exe start #126974 and the 3 added here.shell: trueto the spawn fails 1 test, the new options pin. fix(desktop): hand WSL external URLs to rundll32 instead of cmd.exe start #126974's tests alone still passed.Tested on macOS 27.2 with Node 22.23.2, focused suites only. Tests replace the spawn function with a stub. I did not run a click-through on a real WSL/Windows host.
Reproduction on
main(before this fix)This script calls the real
openExternalUrl()withisWsl: trueand aspawnstub that prints its arguments (run withnode --experimental-strip-types):Output on
main:On Windows, cmd.exe reads this as three commands:
start "" https://example.com/?q=1, thencalc.exe, thenx=2.Output with this PR:
Checklist
Code
pytest tests/ -qand all tests passDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Ashell.openExternal.Acknowledgements
Based on work by @liuhao1024 (#126974): the rundll32 hand-off and the launch-argument guard are theirs, cherry-picked with authorship preserved.
Source and authorship
If this change is salvaged or reimplemented: please preserve the existing commit authorship. Keep @liuhao1024 as the author of the first commit. For FroRaut's contribution, cherry-pick the authored commit(s) or include:
Co-authored-by: FroRaut <79215537+Froraut@users.noreply.github.com>