Fix shell command injection in desktop openUrl - #191
Merged
Conversation
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.
openUrl()previously concatenated its input intostart,open, orxdg-openshell commands. A normal URL such ashttps://example.com/?first=1&second=2could be split at&, and shell metacharacters in an untrusted URL could execute additional commands.Pass the URL directly to
ShellExecuteWusing Nim's existingstd/winleanbinding andnewWideCStringon Windows. On macOS and Linux, launchopen/xdg-openwith 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
startProcesslaunches executables and cannot resolve a URL to its default browser. Unlike the POSIX openers,startis acmd.exebuilt-in. Nim's ownstd/browsershelper also usesShellExecuteWon Windows; this PR uses the existing binding directly to preserve links such asmailto: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.nimpassed.nim c tests/test.nimandnim c examples/openurl.nimpassed.nim checkpassed for the regression test and the openurl example.