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/CLAUDE.md b/CLAUDE.md index 3782bc4..5a970af 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,81 @@ 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. + +**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: + +``` +$ 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 @@ -405,7 +495,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 @@ - + /// + /// 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 7c0a7a7..65a4b79 100644 --- a/src/CodeShellManager/Services/GitRepoWatcher.cs +++ b/src/CodeShellManager/Services/GitRepoWatcher.cs @@ -55,6 +55,10 @@ private GitRepoWatcher(string gitDir) /// Creates a watcher for the repo containing , 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,117 @@ 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; + // 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)) + { + Shared[gitDir] = (existing.Watcher, existing.RefCount + 1); + return existing.Watcher; + } + } + + 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); + loser = candidate; + result = raced.Watcher; + } + else + { + candidate._sharedKey = gitDir; + Shared[gitDir] = (candidate, 1); + result = 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. + 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; + + // 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) + { + Shared[key] = (entry.Watcher, entry.RefCount - 1); + return; + } + + Shared.Remove(key); + entry.Watcher._sharedKey = null; + 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. + 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..9042bcd 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,18 @@ 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 `"`. + // `--` 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 }; var (output, stderr, exit) = await RunGitFullAsync(repoRoot, args, timeoutMs: 30_000).ConfigureAwait(false); if (exit == 0) return (true, ""); @@ -159,33 +169,114 @@ 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. + /// + /// + /// 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 = "\u001eCSM-GIT\u001e"; + + /// + /// 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) + { + // `-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", WslGitScript, "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 +305,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); @@ -235,20 +326,32 @@ public static async Task> ListBranchesAsync(string folderP } /// - /// 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 /// 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") { @@ -273,7 +376,10 @@ public static async Task> ListBranchesAsync(string folderP 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); } @@ -286,39 +392,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 +399,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/RunInstance.cs b/src/CodeShellManager/Services/RunInstance.cs index 6361f7a..a3e18ae 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) { @@ -287,15 +306,26 @@ 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}"); - sb.Append(" \""); + // 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(' '); + + // 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)) - sb.Append($"cd '{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/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/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 653c262..50995c4 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -389,6 +389,74 @@ 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) + { + // 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 (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. + // 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 + // 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); @@ -502,18 +570,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; } @@ -598,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/src/CodeShellManager/ViewModels/SessionViewModel.cs b/src/CodeShellManager/ViewModels/SessionViewModel.cs index 7f65b07..80c3081 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; @@ -239,11 +239,28 @@ 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. } + /// 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; + GitRepoWatcher.Release(_gitWatcher); + _gitWatcher = null; + } + StartGitWatcher(); + } + private void OnGitDirChanged() { if (_gitPollCts.IsCancellationRequested) return; @@ -390,6 +407,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(); } @@ -401,15 +422,26 @@ 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(); 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/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 new file mode 100644 index 0000000..1f5e61c --- /dev/null +++ b/tests/CodeShellManager.Tests/DroppedPathsTests.cs @@ -0,0 +1,124 @@ +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 AQuoteInAPathIsRejected() + { + // `"` is legal in a URI-derived string even though Win32 forbids it in a filename. + // 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("\"" + path + "\"", 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[] { "", "" })); + } + + [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/GitRepoWatcherTests.cs b/tests/CodeShellManager.Tests/GitRepoWatcherTests.cs index 29edf44..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,10 +143,98 @@ 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] + 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"); + + // Last reference released — the entry is gone, not merely decremented. + int before = GitRepoWatcher.SharedCount; + GitRepoWatcher.Release(b); + Assert.Equal(before - 1, GitRepoWatcher.SharedCount); + } + + [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); } + [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..1417ee7 --- /dev/null +++ b/tests/CodeShellManager.Tests/GitServiceInjectionTests.cs @@ -0,0 +1,296 @@ +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", + }; + + /// + /// 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" }); + + Assert.Equal( + new[] { "git", "-C", "/home/alice/repo", + "worktree", "add", "-b", payload, "--", "/home/alice/wt" }, + GitArgv(cmd)); + } + + [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 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.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(GitService.WslGitScript, 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 == 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] + 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] + 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" }); + + Assert.Equal( + new[] { "git", "-C", "/home/alice/proj$(curl evil|sh)", "status", "--porcelain" }, + GitArgv(cmd)); + } + + [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" }); + + Assert.Equal( + new[] { "git", "-C", "/repo", "for-each-ref", "--format=%(refname:short)", "refs/heads" }, + GitArgv(cmd)); + } + + [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]); + } + + [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/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/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.Tests/UntrustedInputTests.cs b/tests/CodeShellManager.Tests/UntrustedInputTests.cs new file mode 100644 index 0000000..be3625c --- /dev/null +++ b/tests/CodeShellManager.Tests/UntrustedInputTests.cs @@ -0,0 +1,174 @@ +using System.Linq; +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 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); + } + + [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")); + } + + [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() + { + 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!); + } +} 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(); } }