fix: v0.8.0 pre-release hardening — command injection, UI-thread spawns, zero-warning build - #129
Merged
Merged
Conversation
…s, warnings
Round 1 of the v0.8.0 pre-release review. Two release blockers, both instances of
a lesson the codebase had already learned somewhere else and not applied here.
## Command injection in the WSL git dispatch (HIGH)
$"-d {QuoteForCmd(distro)} -- git -C {QuoteForCmd(cwd)} {arguments}"
`wsl.exe … -- <tail>` runs the tail through the distro's DEFAULT LOGIN SHELL.
That is not a guess: ShellSession.BuildWslArgs documents it with an empirical
test, which is exactly why *that* method uses `-e`. QuoteForCmd is MSVCRT argv
quoting and cannot neutralise `$(…)`, backticks, `;`, `|` or `&`.
Two reachable sinks:
- CreateWorktreeAsync interpolated a branch name from the *cloned repo*, and
git ref names legally contain all of those characters. Open a hostile repo,
right-click a branch, make a worktree -> arbitrary execution in the distro.
- The working folder, reached UNATTENDED by the git poll on a timer. A
directory named `proj$(…)` is legal on Linux and creatable over \wsl$.
Fixed by making argv argv all the way down: RunGitFullAsync takes
IReadOnlyList<string>, every command line is assembled by JoinArgv (MSVCRT
quoting per element), and the WSL path execs `-e git` with no shell pass. The
local path is fixed too — a `"` in a branch previously injected extra git argv.
TranslateUncArgsToLinux (whole-string, two regex passes to cope with quoting)
is replaced by TranslateUncArgToLinux (per argument). An argument either is a
UNC or is not; no quote handling, no half-translated paths with spaces.
This also fixes a plain bug: `--format=%(refname:short)` is a bash syntax error
once a login shell sees it, so ListBranchesAsync never worked under WSL.
GitServiceInjectionTests round-trips hostile payloads through the real
CommandLineToArgvW. 18 of its 27 cases fail against a pre-fix simulation.
## WslDiscoveryService spawned processes on the UI thread
The exact defect #70 fixed in GitService, reintroduced in a new service three
times: Process.Start ahead of the first await, with every caller on the UI
thread — including LaunchSessionAsync inside the restore loop. wsl.exe is worse
than git here; it can boot a stopped distro VM. All three probes now share
RunWslCaptureAsync, which is Task.Run-wrapped with ConfigureAwait(false).
GetDistroHomeAsync was also using `--` where `-e` was intended.
## The rest
- SanitizeBranch had no length cap while SanitizeTitle did, so an OSC 9001
emitter could put a megabyte "branch name" into a sidebar row.
- UiThreadHeartbeat ticked unconditionally, checking the trace flag inside the
tick — four Normal-priority dispatcher items/sec forever, in the app whose
headline bug was UI-thread saturation. Now starts/stops with the setting.
- GitRepoWatcher is shared and reference-counted per .git directory instead of
one per session (~20 watchers rather than ~47 at the reporter's scale).
- wsl -l -v header detection matched the literal "NAME", so a localized header
parsed as a phantom distro. Now requires a parseable VERSION column.
- Version was still 0.6.0, two releases stale. Now 0.8.0.
- Three non-Catppuccin colour literals replaced with Mocha values.
- UITests: 14 nullable warnings fixed with a Require() helper that names the
missing AutomationId rather than `!` (issue #92). The solution now builds at
zero warnings for the first time.
- CLAUDE.md: documents Diagnostics/, the new services, the argv rule, and the
second UI-thread offence; corrects the stale `wsl … bash -lc` claim.
| Check | Result |
|---|---|
| Solution build (Release, clean) | 0 errors, 0 warnings |
| Unit tests | 529/529 (34 new) |
| Vulnerable packages | none |
| Injection tests vs pre-fix code | 18/27 fail, as intended |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
Two fresh reviewers ran against the round-1 tree: one adversarial pass trying to break the injection fix, one fresh sweep for what the first security review missed. Both found real things, and the adversarial pass found two defects in the fix itself. ## Found in round 1's own fix **`-e git` silently dropped the login shell.** Injection-safe, but it execs git directly, so PATH is the bare default. Anyone whose git comes from nix, asdf or linuxbrew — PATH set in a shell profile — would have lost WSL git entirely, and the symptom is "not a git repo", which points nowhere near PATH. The old `--` form did run a login shell, so this was a regression I introduced. Now `-e sh -lc 'exec "$0" "$@"' git …`, which gets the login PATH back without reopening the hole: the script is a fixed literal and every untrusted value arrives as a positional parameter, which "$0"/"$@" expand verbatim without re-parsing. TheShellScriptIsAlwaysTheSameLiteral pins that property. **`worktree add` had no `--`.** git uses permuting parse_options, so a ref legitimately named `--force` in refs/heads is consumed as an option. Not execution, but it is a repo choosing our git flags. ## Found elsewhere **Drag-and-drop wrote unfiltered paths to the PTY (MEDIUM).** The page derives paths from text/uri-list with decodeURIComponent, so `%0A` arrives as a real newline — and a newline written to a PTY is Enter. A drag source that controls the payload (hostile page dragstart, crafted .url, another local app) ran a command in the focused session on one drop, no keystroke, no confirmation. Control characters are now rejected — Win32 forbids them in filenames, so nothing legitimate is lost and rejection has no escaping bug to get wrong. Embedded quotes are escaped rather than allowed to break the quoting. **`SshRemoteFolder` was interpolated raw into a remote shell command**, in both ShellSession.BuildSshArgs and RunInstance.BuildSshArgs — the one value in those builders that wasn't escaped, while the command payload beside it correctly used SingleQuoteEscape. It comes from state.json, which that file's own header calls untrusted. Now goes through PosixSingleQuote. This also fixes legitimate paths containing an apostrophe, which were simply broken. **PostRunUrl validated one string and launched another.** Uri.TryCreate accepts and internally escapes characters the raw string still contains, and ShellExecute got the raw one. Now launches uri.AbsoluteUri — the string that was inspected. **GitRepoWatcher refcounting.** Release had no identity check, so a stale double-Release could dispose the *replacement* watcher for the same .git dir and silently kill events for a live session. The watcher was also constructed while holding the global SharedLock, on the UI thread — one slow repo path would stall every other session's Acquire/Release. Keys are now normalized through GetFullPath so C:/repo and C:\repo don't get two watchers. **SessionViewModel.Dispose was not idempotent** — a second call threw on the CTS before reaching the watcher release, leaking a shared reference. MainWindow disposes a VM from several paths. **Stale comments** in GitService, SessionViewModel and MainWindow still described `wsl.exe … -- git` and an always-on heartbeat. In a change whose thesis is "`-e`, not `--`", that is exactly how it regresses. ## Also CI has never run the test suite — it built the app and went straight to packaging. Every test in this repo has only ever run on a developer's machine, including the regression guards for the injection and UI-thread fixes. Added a `dotnet test` step. Unit tests only; the FlaUI project needs an interactive desktop a hosted runner doesn't reliably provide. | Check | Result | |---|---| | Solution build (Release, clean) | 0 errors, 0 warnings | | Unit tests | 569/569 (40 new this round) | Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
…CI blocker
Third round. Two fresh reviewers again; both found real defects, including one
release blocker introduced by round 2's own CI change.
## Release blocker (self-inflicted)
The `dotnet test` step added last round would have FAILED the release build.
GitServiceThreadingTests asserted a non-empty branch from the checked-out repo,
and actions/checkout leaves a detached HEAD for tag pushes and PR merge refs —
`branch --show-current` prints nothing there. It passes on push-to-main, which
is where it was seen green. The step sits ahead of every tag-gated step, so a
tag push would have aborted Publish/MSI/Release. The assertion is now completion,
which is the property the test was actually about.
## Worse than what round 2 fixed
`SshHost`/`SshUser` went into the ssh command line unquoted, outside the quoted
region. A host of `h -oProxyCommand=calc` splits into extra *ssh options*, and
ProxyCommand runs LOCALLY — so a crafted state.json got code execution on the
user's own machine without any remote host existing. Round 2 escaped
`SshRemoteFolder` and walked straight past the two values next to it.
Conditional QuoteForCmd is enough: the attack needs argv splitting, which needs
whitespace, and there is no shell on this path.
## Dropped-path quoting was POSIX-shaped
Round 2 quoted only on space and escaped `"` as `\"`. Panes commonly run cmd.exe
or PowerShell, where a perfectly legal filename `a&calc.txt` — no space — is a
command separator, and `\"` is not an escape at all. Now quoting triggers on the
union of shell metacharacters, and `"` is rejected outright: no escaping is
simultaneously correct for cmd, PowerShell and sh, and Win32 forbids `"` in
filenames anyway.
## Profile output could contaminate git's stdout
`-e sh -lc` sources /etc/profile and ~/.profile, and a profile that echoes
prepends to stdout — which callers parse. `branch --show-current` would return
the banner; worse, `status --porcelain` would be non-empty, pinning every WSL
repo to "dirty" forever. The old `--` form had the identical exposure, so this
would have been the WSL feature's debut rather than a regression. The script now
prints a distinctive marker before exec and the host strips everything up to it.
Verified on a real Ubuntu distro rather than reasoned about:
wsl -d Ubuntu -- echo '$(id)' -> uid=1000(thraen)…
wsl -d Ubuntu -e sh -lc 'exec "$0" "$@"' echo '$(id)' -> $(id)
The login shell genuinely adds ~/.local/bin to PATH, exec preserves exit codes
(0 and 128), and dash-leading arguments pass through as data.
## Also
- GitRepoWatcher disposed watchers while holding the global SharedLock, which
is what moving construction out of it was supposed to avoid.
- "Edit session…" never re-acquired the watcher, so moving a session to another
repo left it watching the old .git — silently poll-only, up to 120s stale.
- Three test defects of my own: a vacuous assertion (`DescribeShared()` can
never contain a path), a debounce test that assumed 20 real file writes land
inside a 400ms window, and a thread-identity test that passed or failed by
luck because xunit runs on the pool and Task.Run may reuse the caller's
thread. All three now assert the property rather than a coincidence.
- A quote-counting assertion I wrote was simply wrong: `'/x'\'''` is correct
POSIX escaping and contains an odd number of quotes.
Not fixed, filed instead: #127 (WSL git ignores WslUser — widens five
signatures, degraded display only) and #128 (UNC working folder in an imported
state.json triggers outbound SMB).
| Check | Result |
|---|---|
| Solution build (Release, clean) | 0 errors, 0 warnings |
| Unit tests | 581/581 |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
…inel hardening
Fourth round. One new HIGH, two hardening items, and one reported blocker that
was a false positive worth acting on anyway.
## The remote command escaped the WINDOWS argv layer (HIGH)
Round 3 quoted SshHost. Round 2 POSIX-escaped SshRemoteFolder. Both missed that
the remote command was hand-wrapped in `" … "`, and PosixSingleQuote escapes `'`
but not `"`:
SshRemoteFolder = /t" -oProxyCommand=calc "x
-> -t host "cd '/t" -oProxyCommand=calc "x' && bash"
CommandLineToArgvW splits that into separate arguments, ssh re-runs getopt after
the host, and ProxyCommand executes LOCALLY. Same escalation as the round-3
SshHost finding, reached through the field round 2 had already "fixed" — POSIX
quoting was the right escaping at the wrong layer.
Both builders now assemble the remote command and hand it to QuoteForCmd as one
argv element. Two layers, both explicit: POSIX inside for the remote shell,
Windows argv outside for CreateProcess. Output is byte-identical for ordinary
values, so no existing expectation changed.
## The sentinel: a false positive that was still worth fixing
Both round-4 reviewers reported that WslOutputSentinel lacked its 0x1E bytes and
that every WSL repo would read as permanently dirty. It did have them — their
file reads normalised the raw control characters away. Verified two ways rather
than argued: a codepoint dump (30, 30 present) and an end-to-end run against a
real Ubuntu distro —
clean status --porcelain -> "\u001eCSM-GIT\u001e" -> "" isDirty false
dirty status --porcelain -> "\u001eCSM-GIT\u001e?? f" -> "?? f" isDirty true
branch --show-current -> "…main\n" -> "main"
But a value invisible to tooling is one cleanup away from exactly the bug they
described, so the constant now uses escapes instead of raw bytes, and a
test pins it against what the printf in WslGitScript actually emits.
## Drop payload
- Rejects Unicode category Cf (U+202E and friends). char.IsControl catches
C0/C1 — the newline that made this executable — but not characters that
reorder how the path renders, and the user reads this text before pressing
Enter.
- Bounded: 64 paths, 1024 chars each. The page controls that array.
## Also
- RestartGitWatcher had no _disposed guard, so an edit racing a close could
re-Acquire a shared watcher nothing would release.
Filed, not fixed: #127 (WSL git ignores WslUser), #128 (UNC folder from an
imported state.json triggers outbound SMB).
| Check | Result |
|---|---|
| Solution build (Release, clean) | 0 errors, 0 warnings |
| Unit tests | 591/591 |
| Vulnerable packages | none |
| New ssh argv tests vs pre-fix code | fail, as intended |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
Round 5 was an exhaustive audit rather than a hunt: enumerate every site in src/
where a string becomes a process argument or reaches a PTY, and say for each
where its values come from and how they are escaped.
**Verdict: argument injection is systematically closed.** No untrusted origin
reaches a raw interpolation on any argv path. Every builder funnels through
QuoteForCmd / PosixSingleQuote / JoinArgv, and `--` is gone from every wsl.exe
invocation. The audit also independently walked the round-4 nesting with a value
carrying `'`, `"` and a trailing backslash and confirmed it lands as exactly one
argv element.
I verified both layers myself rather than taking that on trust:
- POSIX: 8 hostile values round-tripped through a real /bin/sh in a WSL distro
(`sh -c 'printf %s <escaped>'` returns the original for every one).
- Windows argv: the same values through the real CommandLineToArgvW, now
pinned by BothEscapingLayersNestCorrectly.
## The two residuals it did find, both inconsistencies
**`UpdateBadge_Click` reached ShellExecute with no scheme check** — the same
shape as `RunCommandItem.PostRunUrl`, which has had a guard for exactly this
reason, sitting a few files away. The value comes from a GitHub API response via
an AppData cache file, so the risk is low; the inconsistency is the problem. An
unguarded ShellExecute next to a guarded one is how the guard stops being the
rule. Now goes through the same TryGetLaunchableUrl.
**Run output was written to the PTY raw.** `SendRunOutputToTerminal` wrote
arbitrary run-command stdout with `SendToTerminal`, while the clipboard path
beside it correctly round-trips through the page so xterm applies bracketed
paste. Same primitive the drop fix was about — "a newline written to a PTY is
Enter" — just user-initiated. Added `TerminalBridge.PasteToTerminal` and used it
here; it falls back to a plain write before the page is up, so it is strictly no
worse than what it replaces.
CLAUDE.md gains both rules: that a value can cross two escaping layers and each
escaper only defends one, and that SendToTerminal types while PasteToTerminal
pastes.
| Check | Result |
|---|---|
| Solution build (Release, clean) | 0 errors, 0 warnings |
| Unit tests | 597/597 |
| Vulnerable packages | none |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
…posite The comment claimed "no trailing \r — leave it in Claude's input box for the user to submit". The string ends in \n, and a raw PTY write delivers that as Enter exactly like \r, so it submitted — as did every newline inside the run output. Round 5's switch to bracketed paste is what actually made the stated intent true; the comment now says so. 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.
Six rounds of pre-release review, each round re-reviewing the previous round's fixes with fresh reviewers. The loop stopped when a round came back clean.
The headline: v0.7.0 shipped a command-injection vulnerability, and it was live. Demonstrated end-to-end against a real Ubuntu distro, not reasoned about:
Why six rounds
Each round found defects in the previous round's fixes. That progression is the real finding:
WslDiscoveryServicereintroducing the UI-thread spawn #70 had just fixed--SshHostinjection worse than round 2's findSshRemoteFolderis the instructive one. Round 2 escaped it correctly for POSIX. Round 4 found it still exploitable, because the value crosses two escaping layers and only one had been fixed: the remote command was hand-wrapped in" … "for Windows argv, andPosixSingleQuoteescapes', not". A"broke out at the Windows layer, and ssh honours options after the host — so-oProxyCommand=ran locally.Five instances of one pattern is not five bugs. It is a missing convention, which is why CLAUDE.md now carries the rule and the tests round-trip through real tokenizers rather than asserting on strings I wrote.
What changed
Security
JoinArgv, MSVCRT quoting per element);wsl.exeuses-e, never--"rejected, quoting on the union of cmd/PowerShell/POSIX metacharacters, list boundedPostRunUrland the update badge both launch the validatedAbsoluteUri, not the raw stringgit-branchlength-capped liketitlealready wasCorrectness
WslDiscoveryServiceno longer spawnswsl.exeon the UI thread (it can boot a stopped distro VM)for-each-ref --format=%(refname:short)now works under WSL; it was a bash syntax error before, soListBranchesAsynchad never worked thereGitRepoWatchershared/refcounted per.gitdir, re-acquired when a session's folder changesHygiene
dotnet teststepEvidence, not assertion
Every regression test was checked against the pre-fix code:
GitServiceInjectionTests— 18 of 27 fail on a pre-fix simulationGitServiceThreadingTests— all 3 fail against pre-fixGitService, two by timing out at 20sShellSessionVerified against reality where a unit test cannot reach:
/bin/shin WSLCommandLineToArgvW?? f.txt, branch →mainexecpreserves exit codes (0 and 128); the login shell genuinely adds~/.local/binto PATHOne correction worth recording
Two round-4 reviewers independently reported a blocker: that
WslOutputSentinelwas missing its0x1Ebytes and every WSL repo would read as permanently dirty. It was not — their file reads normalise control characters away. I verified with a codepoint dump and an end-to-end run before acting.But I changed it anyway. A value two independent reviewers misread is one the next person will misread too, and the bug they described would have been severe. The constant now uses
�escapes, with a test pinning it against what the shell script actually prints.Deliberately not fixed
WslUser; widens five signatures, degrades display onlystate.jsontriggers outbound SMB; the sane fix (import-as-dormant) is a design choice, not a patchVerification
Not smoke-tested against the running app — an instance was live throughout and
output.dbis shared across processes, so I would not start a second one. That is the single thing to eyeball on merge, along with a WSL session's git label.🤖 Generated with Claude Code
https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be