From bc67f2545aeea129b27c7ae776c8381ae205f69d Mon Sep 17 00:00:00 2001 From: treeform Date: Sat, 5 Sep 2026 22:04:12 -0700 Subject: [PATCH 1/2] Fix shell command injection in desktop openUrl --- .github/workflows/build.yml | 1 + src/windy/platforms/linux/x11.nim | 5 +- src/windy/platforms/macos/platform.nim | 7 ++- src/windy/platforms/win32/platform.nim | 12 +++- src/windy/platforms/win32/windefs.nim | 9 +++ tests/test_openurl.nim | 76 ++++++++++++++++++++++++++ tests/test_openurl.nims | 3 + 7 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 tests/test_openurl.nim create mode 100644 tests/test_openurl.nims diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 571c958..d939cac 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -25,6 +25,7 @@ jobs: # Run tests. - run: nim c tests/test.nim + - run: nim r tests/test_openurl.nim - run: nim r -d:useCpu tests/test_cpu_pixels.nim if: matrix.os == 'windows-latest' diff --git a/src/windy/platforms/linux/x11.nim b/src/windy/platforms/linux/x11.nim index f9aeeca..5b29e3c 100644 --- a/src/windy/platforms/linux/x11.nim +++ b/src/windy/platforms/linux/x11.nim @@ -1395,7 +1395,10 @@ proc setConfig*(appName: string, fileName: string, content: string) = proc openUrl*(url: string) = ## Open a URL in the default browser. - discard execShellCmd("xdg-open " & url) + let process = startProcess("xdg-open", args = [url], + options = {poUsePath, poParentStreams}) + defer: process.close() + discard process.waitForExit() proc openTempTextFile*(title, text: string) = ## Open a text file in the default text editor. diff --git a/src/windy/platforms/macos/platform.nim b/src/windy/platforms/macos/platform.nim index 98651f2..822f540 100644 --- a/src/windy/platforms/macos/platform.nim +++ b/src/windy/platforms/macos/platform.nim @@ -1,5 +1,5 @@ import - std/[os, strutils, times, unicode, pathnorm], + std/[os, osproc, strutils, times, unicode, pathnorm], pixie/fileformats/png, pixie/images, utils, vmath, ../../[common, internal], macdefs @@ -1572,7 +1572,10 @@ proc openTempTextFile*(title, text: string) = proc openUrl*(url: string) = ## Open a URL in the default web browser. - discard execShellCmd("open " & url) + let process = startProcess("open", args = ["--", url], + options = {poUsePath, poParentStreams}) + defer: process.close() + discard process.waitForExit() proc fileDialogExtensions(filters: seq[FileDialogFilter]): seq[string] = ## Collects unique file extensions without wildcards or dots. diff --git a/src/windy/platforms/win32/platform.nim b/src/windy/platforms/win32/platform.nim index 6aa59ed..8b8a8fe 100644 --- a/src/windy/platforms/win32/platform.nim +++ b/src/windy/platforms/win32/platform.nim @@ -2823,7 +2823,17 @@ proc setConfig*(appName: string, fileName: string, content: string) = proc openUrl*(url: string) = ## Open a URL in the default web browser. - discard execShellCmd("start " & url) + let + operation = wstr("open") + target = wstr(url) + discard ShellExecuteW( + 0, + cast[LPCWSTR](operation[0].addr), + cast[LPCWSTR](target[0].addr), + nil, + nil, + SW_SHOWNORMAL + ) proc openTempTextFile*(title, text: string) = ## Open a text file in the default text editor. diff --git a/src/windy/platforms/win32/windefs.nim b/src/windy/platforms/win32/windefs.nim index 8540d91..4873f1b 100644 --- a/src/windy/platforms/win32/windefs.nim +++ b/src/windy/platforms/win32/windefs.nim @@ -1051,6 +1051,15 @@ proc Shell_NotifyIconW*( lpData: PNOTIFYICONDATAW ): BOOL {.dynlib: "shell32".} +proc ShellExecuteW*( + hwnd: HWND, + lpOperation: LPCWSTR, + lpFile: LPCWSTR, + lpParameters: LPCWSTR, + lpDirectory: LPCWSTR, + nShowCmd: int32 +): HINSTANCE {.dynlib: "shell32".} + proc WinHttpOpen*( lpszAgent: LPCWSTR, dwAccessType: DWORD, diff --git a/tests/test_openurl.nim b/tests/test_openurl.nim new file mode 100644 index 0000000..778c34b --- /dev/null +++ b/tests/test_openurl.nim @@ -0,0 +1,76 @@ +import windy + +when defined(windows): + import windy/platforms/win32/[utils, windefs] + + var openedUrls: seq[string] + + proc recordOpenUrl( + hwnd: HWND, + operation, target, parameters, directory: LPCWSTR, + showCmd: int32 + ): HINSTANCE {.stdcall, exportc: "ShellExecuteW".} = + # Capture the native API boundary without launching a browser. + doAssert hwnd == 0 + doAssert $operation == "open" + doAssert parameters == nil + doAssert directory == nil + doAssert showCmd == SW_SHOWNORMAL + openedUrls.add($target) + return 33 +else: + import std/[json, os, tempfiles] + + # A copy of this executable stands in for open/xdg-open on PATH. + if getEnv("WINDY_OPENURL_RECORD") != "": + writeFile(getEnv("WINDY_OPENURL_RECORD"), $(%commandLineParams())) + quit(0) + +proc main() = + let urls = [ + "https://example.com/", + "https://example.com/?first=1&second=2#fragment", + "https://example.com/a b/\"quoted\"/'single'", + "https://example.com/$(echo injected > windy-openurl-injected)", + "https://example.com/`echo injected > windy-openurl-injected`", + "https://example.com/; echo injected > windy-openurl-injected", + "https://example.com/| echo injected > windy-openurl-injected", + "https://example.com/\necho injected > windy-openurl-injected", + "https://example.com/%PATH%/$HOME/!name!/a\\b", + "https://example.com/café/日本語/🌍?x=1&y=2" + ] + + when defined(windows): + for url in urls: + openUrl(url) + doAssert openedUrls == @urls + else: + let + tempDir = createTempDir("windy-openurl-", "") + originalDir = getCurrentDir() + originalPath = getEnv("PATH") + recordPath = tempDir / "args.json" + opener = tempDir / (when defined(macosx): "open" else: "xdg-open") + defer: + setCurrentDir(originalDir) + putEnv("PATH", originalPath) + delEnv("WINDY_OPENURL_RECORD") + removeDir(tempDir) + + copyFile(getAppFilename(), opener) + setFilePermissions(opener, {fpUserRead, fpUserWrite, fpUserExec}) + setCurrentDir(tempDir) + putEnv("PATH", tempDir & ":" & originalPath) + putEnv("WINDY_OPENURL_RECORD", recordPath) + + for url in urls: + if fileExists(recordPath): + removeFile(recordPath) + openUrl(url) + let expected = when defined(macosx): @["--", url] else: @[url] + doAssert parseFile(recordPath) == %expected + doAssert not fileExists("windy-openurl-injected") + + echo "Windy openUrl regression test passed" + +main() diff --git a/tests/test_openurl.nims b/tests/test_openurl.nims new file mode 100644 index 0000000..1582b55 --- /dev/null +++ b/tests/test_openurl.nims @@ -0,0 +1,3 @@ +when defined(windows): + # Link the test's ShellExecuteW recorder instead of loading shell32.dll. + switch("dynlibOverride", "shell32") From cd56ce1ac0126c7b7644f6dbcfa82681da56e877 Mon Sep 17 00:00:00 2001 From: treeform Date: Sat, 5 Sep 2026 22:09:49 -0700 Subject: [PATCH 2/2] Reuse Nim's standard Windows URL launcher binding --- src/windy/platforms/win32/platform.nim | 16 +++++----------- src/windy/platforms/win32/windefs.nim | 9 --------- tests/test_openurl.nim | 1 + tests/test_openurl.nims | 2 +- 4 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/windy/platforms/win32/platform.nim b/src/windy/platforms/win32/platform.nim index 8b8a8fe..e790375 100644 --- a/src/windy/platforms/win32/platform.nim +++ b/src/windy/platforms/win32/platform.nim @@ -4,6 +4,8 @@ import pixie/fileformats/bmp, pixie/images, urlly, utils, vmath, windefs, zippy +from std/winlean import shellExecuteW + when defined(useDirectX): {.hint: "Using DirectX backend".} elif defined(useVulkan): @@ -2823,17 +2825,9 @@ proc setConfig*(appName: string, fileName: string, content: string) = proc openUrl*(url: string) = ## Open a URL in the default web browser. - let - operation = wstr("open") - target = wstr(url) - discard ShellExecuteW( - 0, - cast[LPCWSTR](operation[0].addr), - cast[LPCWSTR](target[0].addr), - nil, - nil, - SW_SHOWNORMAL - ) + # Windows resolves URL associations through ShellExecute, not CreateProcess. + discard shellExecuteW(0, newWideCString("open"), newWideCString(url), + nil, nil, SW_SHOWNORMAL) proc openTempTextFile*(title, text: string) = ## Open a text file in the default text editor. diff --git a/src/windy/platforms/win32/windefs.nim b/src/windy/platforms/win32/windefs.nim index 4873f1b..8540d91 100644 --- a/src/windy/platforms/win32/windefs.nim +++ b/src/windy/platforms/win32/windefs.nim @@ -1051,15 +1051,6 @@ proc Shell_NotifyIconW*( lpData: PNOTIFYICONDATAW ): BOOL {.dynlib: "shell32".} -proc ShellExecuteW*( - hwnd: HWND, - lpOperation: LPCWSTR, - lpFile: LPCWSTR, - lpParameters: LPCWSTR, - lpDirectory: LPCWSTR, - nShowCmd: int32 -): HINSTANCE {.dynlib: "shell32".} - proc WinHttpOpen*( lpszAgent: LPCWSTR, dwAccessType: DWORD, diff --git a/tests/test_openurl.nim b/tests/test_openurl.nim index 778c34b..0e86eab 100644 --- a/tests/test_openurl.nim +++ b/tests/test_openurl.nim @@ -29,6 +29,7 @@ else: proc main() = let urls = [ "https://example.com/", + "mailto:test@example.com?subject=Hello&body=World", "https://example.com/?first=1&second=2#fragment", "https://example.com/a b/\"quoted\"/'single'", "https://example.com/$(echo injected > windy-openurl-injected)", diff --git a/tests/test_openurl.nims b/tests/test_openurl.nims index 1582b55..9b4d2cd 100644 --- a/tests/test_openurl.nims +++ b/tests/test_openurl.nims @@ -1,3 +1,3 @@ when defined(windows): # Link the test's ShellExecuteW recorder instead of loading shell32.dll. - switch("dynlibOverride", "shell32") + switch("dynlibOverride", "shell32.dll")