From 2bbbd99c0d1980197aff416554808fca966b99b5 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:33:19 -0700 Subject: [PATCH] test(wsl): close the two ratchet gaps that let planted spawns pass - variable-indirected wsl.exe (`const b = 'wsl.exe'; spawnProcess(b)`) is now tracked, so the 5 files recorded only in a comment become real allowlist entries. Three actually spawn that way; the other two never spawned wsl.exe at all, so the prose record was wrong by three in the hiding direction. - promisify(renamedAlias) is now resolved, so `const run = promisify(execFile)` behind an `execFile as x` import can no longer skip windowsHide. Each verified by planting the violation, watching it fail, restoring, watching it pass. Credit: GPT-Sol. --- .../__fixtures__/wsl-invocation-allowlist.txt | 13 ++-- src/main/wsl/wsl-invocation-boundary.test.ts | 68 ++++++++++++++----- .../windows-console-visibility.test.ts | 22 +++++- 3 files changed, 78 insertions(+), 25 deletions(-) diff --git a/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt b/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt index 7e9689ad9e4..b9cfe66addb 100644 --- a/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt +++ b/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt @@ -1,11 +1,11 @@ -# Files that spawn wsl.exe directly instead of through src/main/wsl/wsl-runner.ts. -# Enforced by ../wsl-invocation-boundary.test.ts. This list only shrinks -- its -# length is the W3 goalpost. # +# Enforced by ../wsl-invocation-boundary.test.ts. This list only shrinks -- its +# express because it always prepends --exec. +# Files that spawn wsl.exe directly instead of through src/main/wsl/wsl-runner.ts. +# host-level flag like --status / --list that the guest-command API cannot +# length is the W3 goalpost. # Remaining entries need a runner mode that does not exist: a long-lived # streaming child (OAuth logins, the hook relay), a synchronous caller, or a -# host-level flag like --status / --list that the guest-command API cannot -# express because it always prepends --exec. main/agent-hooks/wsl-hook-relay-launch.ts main/claude-accounts/service.ts main/codex-accounts/runtime-home-service.ts @@ -13,10 +13,13 @@ main/codex-accounts/service.ts main/codex/codex-state-db-backfill-recovery.ts main/codex/codex-trust-grant-host.ts main/codex/codex-wsl-hook-install-plan.ts +main/daemon/pty-subprocess.ts main/git/runner.ts main/git/wsl-git-read-environment.ts main/ipc/filesystem-watcher-wsl.ts main/local-worktree-filesystem.ts +main/providers/local-pty-provider.ts +main/rate-limits/claude-pty.ts main/rate-limits/codex-fetcher.ts main/wsl-availability.ts main/wsl-unc-delete.ts diff --git a/src/main/wsl/wsl-invocation-boundary.test.ts b/src/main/wsl/wsl-invocation-boundary.test.ts index a29bc880f18..c8be06882a4 100644 --- a/src/main/wsl/wsl-invocation-boundary.test.ts +++ b/src/main/wsl/wsl-invocation-boundary.test.ts @@ -45,28 +45,19 @@ const IGNORED = new Set(['node_modules', 'dist', 'out', 'build', '.git', '__fixt * * Any identifier followed by `(` counts as an opener, and the assignment-style * fields are matched by name. Indirection through a variable - * (`const f = cond ? 'wsl.exe' : x`) is still invisible; those files are listed - * explicitly below so the gap is recorded rather than implied. + * (`const f = cond ? 'wsl.exe' : x`) is caught separately, by + * `bindsWslBinaryToASpawnedIdentifier`. */ const SPAWN_OPENER = /\b[A-Za-z_$][\w$]*\s*\(\s*$|(?:program|binary|command|file|shellPath):\s*$/ /* - * Known blind spot, recorded rather than implied: these files assign - * `'wsl.exe'` to a variable and spawn it elsewhere, which no regex over the - * literal's neighbourhood can see. They are NOT in the allowlist, because the - * stale-entry check would reject an entry the scanner cannot find -- so the - * guard's count under-reports by these five, and this comment is the record. - * - * main/rate-limits/claude-pty.ts - * main/providers/local-pty-provider.ts - * main/daemon/pty-subprocess.ts - * relay/pty-shell-launch.ts - * relay/pty-handler.ts - * - * All five are PTY-pane launches, which are out of W3's scope anyway: they go - * through node-pty, not a child-process wrapper. + * The five files this comment used to list as an unscannable blind spot are + * now handled: three bind `wsl.exe` to a variable and spawn it, and are real + * allowlist entries; the other two never spawned it at all -- one compares a + * basename, one lists it among accepted shells. Recording a gap in prose was + * worse than it looked, because the count is the goalpost and it was wrong by + * three in the direction that hides offenders. */ - function isTestFile(path: string): boolean { return ( /\.(?:test|spec)\.tsx?$/.test(path) || @@ -92,6 +83,46 @@ function collectSourceFiles(root: string): string[] { return found } +/** + * `const binary = 'wsl.exe'` ... `spawn(binary)`, which no test over the + * literal's neighbourhood can see. + * + * Why it earns its place: the neighbourhood test was the whole guard, and a + * planted `const p = 'wsl.exe'; spawnProcess(p)` passed it. Five files were + * already known to spawn this way and were recorded in a comment instead of + * the allowlist, which means the count -- the actual goalpost -- was wrong by + * five and any NEW indirect spawner would have been invisible. + */ +function bindsWslBinaryToASpawnedIdentifier(source: string): boolean { + const bound = new Set() + // Covers `const x = 'wsl.exe'`, a ternary picking it, and `binary: 'wsl.exe'`. + for (const match of source.matchAll( + /(?:const|let|var)\s+([A-Za-z_$][\w$]*)[^=;\n]*=[^;\n]*['"]wsl\.exe['"]/g + )) { + bound.add(match[1]!) + } + for (const match of source.matchAll( + /\b([A-Za-z_$][\w$]*)\s*:\s*[^,;\n]*['"]wsl\.exe['"]/g + )) { + bound.add(match[1]!) + } + for (const name of bound) { + const identifier = name.replace(/[$]/g, '\\$&') + // The identifier reaching a call opener, a spawn-style field, or the first + // argument of a spawn-style call. + if ( + new RegExp(`\\b${identifier}\\s*\\(`).test(source) || + new RegExp(`(?:program|binary|command|file|shellPath)\\s*:\\s*${identifier}\\b`).test( + source + ) || + new RegExp(`\\b\\w*(?:spawn|exec|run)\\w*\\s*\\(\\s*${identifier}\\b`, 'i').test(source) + ) { + return true + } + } + return false +} + function findSpawnSites(): string[] { const offenders = new Set() for (const path of collectSourceFiles(SOURCE_ROOT)) { @@ -108,6 +139,9 @@ function findSpawnSites(): string[] { offenders.add(relativePath) } } + if (bindsWslBinaryToASpawnedIdentifier(source)) { + offenders.add(relativePath) + } } return [...offenders].sort() } diff --git a/src/shared/child-process/windows-console-visibility.test.ts b/src/shared/child-process/windows-console-visibility.test.ts index cae64347875..3582a0c592e 100644 --- a/src/shared/child-process/windows-console-visibility.test.ts +++ b/src/shared/child-process/windows-console-visibility.test.ts @@ -74,9 +74,25 @@ function findOffenders(): string[] { const decommented = stripComments(file.source) // Resolve `import { spawn as sp }` so a renamed binding is still a spawn. // The previous comment claimed this; only three names were hardcoded. - const aliases = [...decommented.matchAll(/\b(?:spawn|spawnSync|execFile|execFileSync|exec|execSync|fork)\s+as\s+(\w+)/g)].map( - (match) => match[1] - ) + const aliases = [ + ...decommented.matchAll( + /\b(?:spawn|spawnSync|execFile|execFileSync|exec|execSync|fork)\s+as\s+(\w+)/g + ) + ].map((match) => match[1]!) + // `const run = promisify(execFile)` mints a third name, and it can wrap a + // renamed binding, so this has to run after the aliases are known. A + // planted `promisify(renamedExecFile)` spawn passed the guard without it. + for (const match of decommented.matchAll( + /(?:const|let|var)\s+(\w+)\s*=\s*promisify\s*\(\s*(\w+)\s*\)/g + )) { + const wrapped = match[2]! + if ( + aliases.includes(wrapped) || + /^(?:spawn|spawnSync|execFile|execFileSync|exec|execSync|fork)$/.test(wrapped) + ) { + aliases.push(match[1]!) + } + } // The import test needs the module name, which blanking would erase; the // call scan needs parens inside strings neutralised. Two views, one file. if (!CHILD_PROCESS_IMPORT.test(decommented)) {