Skip to content

Fix shell command injection in desktop openUrl - #191

Merged
treeform merged 2 commits into
masterfrom
fix-open-url-shell-injection
Sep 6, 2026
Merged

Fix shell command injection in desktop openUrl#191
treeform merged 2 commits into
masterfrom
fix-open-url-shell-injection

Conversation

@treeform

@treeform treeform commented Sep 6, 2026

Copy link
Copy Markdown
Owner

openUrl() previously concatenated its input into start, open, or xdg-open shell commands. A normal URL such as https://example.com/?first=1&second=2 could be split at &, and shell metacharacters in an untrusted URL could execute additional commands.

Pass the URL directly to ShellExecuteW using Nim's existing std/winlean binding and newWideCString on Windows. On macOS and Linux, launch open / xdg-open with explicit argument arrays and no shell evaluation; macOS also gets an end-of-options marker. The POSIX launchers still wait for the opener to exit and close its process handle.

Windows uses the association API because startProcess launches executables and cannot resolve a URL to its default browser. Unlike the POSIX openers, start is a cmd.exe built-in. Nim's own std/browsers helper also uses ShellExecuteW on Windows; this PR uses the existing binding directly to preserve links such as mailto: on older supported Nim versions.

Add a regression test to the three-platform CI matrix. It captures the Windows API call or runs a recording executable in place of the POSIX opener, so it checks the exact URL without opening a browser. Cases cover query-string ampersands, spaces, quotes, command substitution, separators, redirection, newlines, environment-variable syntax, backslashes, and Unicode.

Validation on Windows with Nim 2.2.10:

  • nim r tests/test_openurl.nim passed.
  • nim c tests/test.nim and nim c examples/openurl.nim passed.
  • Linux/amd64 and macOS/arm64 nim check passed for the regression test and the openurl example.
  • Native Linux and macOS execution is covered by the PR's CI jobs.

@treeform
treeform merged commit a466695 into master Sep 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant