fix(run): only ShellExecute http/https post-run URLs - #87
Merged
Conversation
PostRunUrl went straight to Process.Start with UseShellExecute=true and nothing validated it. ShellExecute resolves far more than web URLs — a local executable, a .ps1, a UNC path, or any registered protocol handler (ms-settings:, steam://) would all be launched. This fires automatically when a run exits 0, with no confirmation step. And ImportExportService deserializes an entire AppState — run commands included — from any JSON file the user points at, so an imported state file could carry a run command that launches a program on its first successful run. Import is a trust boundary either way (CommandLine is just as arbitrary), but the automatic trigger is worth closing. RunInstance.IsLaunchableUrl now gates the launch on an absolute http or https URI. Rejections are logged to crash.log alongside launch failures, since we can't pop UI from the PTY-exit callback thread. Scheme-less input like "localhost:5173" is rejected rather than silently upgraded to http:// — guessing would defeat the check. The dialog tooltip now says so explicitly. 19 new tests cover the accepted schemes, the rejected ones, and the empty/scheme-less cases. Suite is 241/241. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
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
1 participant
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.
Found during the pre-release review for v0.6.0.
Problem
PostRunUrlwent straight toProcess.StartwithUseShellExecute = true, and nothing anywhere validated it. ShellExecute resolves far more than web URLs — a local executable, a.ps1, a UNC path, or any registered protocol handler (ms-settings:,steam://) would all be launched.Two things make this worth closing rather than filing as "user's own config":
ImportExportService.ImportAsyncdeserializes an entireAppState— run commands included — from any JSON file the user points at. An imported state file can therefore carry a run command that launches a program on its first successful run.Import is a trust boundary either way (
CommandLineis just as arbitrary), so this doesn't make importing untrusted state safe. It closes the automatic trigger.Fix
RunInstance.IsLaunchableUrlgates the launch on an absolutehttporhttpsURI. Rejections are logged tocrash.logalongside launch failures — we can't pop UI from the PTY-exit callback thread.Scheme-less input like
localhost:5173is rejected rather than silently upgraded tohttp://; guessing on the user's behalf would defeat the check. The dialog tooltip now states the requirement explicitly, since that's the one input a user might reasonably expect to work.Tests
19 new cases in
PostRunUrlTests— accepted schemes (incl. case-insensitivity), rejected schemes (Windows paths, UNC,file:,javascript:, protocol handlers), and the empty / scheme-less cases.Note
The solution build reports 14 warnings, all pre-existing nullable warnings in
CodeShellManager.UITests, untouched by this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be