From a3169db770ea44054175ba8578cb35b418ae9324 Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Tue, 18 Aug 2026 09:01:38 +0900 Subject: [PATCH] fix(windows): one scheduler-wrapper killer, scoped to one installation src/service.ts and src/update/job.ts each carried a copy of the same teardown logic, and the copies drifted in both directions. service.ts matched canonical full paths as complete command-line tokens; update/job.ts matched the bare filenames with -like '*name*'. Meanwhile update/job.ts had received the #1589 argv cleanup that service.ts had not. The bare-filename matcher is the defect. Two OpenCodex homes under one Windows account means a dashboard update for home A can force-terminate home B's scheduler wrapper, and any unrelated process whose command line contains either filename matches as well. Extracted the service.ts implementation, which was the correct one, into src/lib/windows-service-wrappers.ts and pointed both callers at it. The updater now passes its own config dir instead of bare names. windowsWrapperKillScript is exported because the matching rule is the entire point of the module and the spawn reports nothing: the script it builds is the only observable surface, which is the same source-level convention windows-deploy-close-regressions.test.ts already uses. New tests/windows-service-wrappers.test.ts pins that another home's path is not among the patterns, that matching is token-bounded rather than substring, that the caller excludes itself, and that neither file keeps a private matcher. The last assertion was driven red first: both files failed it before the extraction. windows-deploy-close-regressions.test.ts asserted "$_.ProcessId -eq $PID" against update/job.ts. That string moved, so the assertion follows it to the shared module rather than being dropped. Verification: bun test over service, windows-deploy-close-regressions, windows-popup-fix and the new file (143 pass), bun run typecheck clean. --- src/lib/windows-service-wrappers.ts | 72 ++++++++++ src/service.ts | 48 ++----- src/update/job.ts | 27 ++-- tests/cli-ready.test.ts | 21 ++- .../windows-deploy-close-regressions.test.ts | 7 +- tests/windows-service-wrappers.test.ts | 125 ++++++++++++++++++ 6 files changed, 235 insertions(+), 65 deletions(-) create mode 100644 src/lib/windows-service-wrappers.ts create mode 100644 tests/windows-service-wrappers.test.ts diff --git a/src/lib/windows-service-wrappers.ts b/src/lib/windows-service-wrappers.ts new file mode 100644 index 0000000000..78c63174ba --- /dev/null +++ b/src/lib/windows-service-wrappers.ts @@ -0,0 +1,72 @@ +/** + * Shared termination of surviving Windows scheduler launcher/wrapper processes. + * + * `schtasks /end` ends the task instance but often leaves wscript/cmd running the + * `:loop` batch, which brings the proxy back during a stop, a restart, or + * post-update reclaim. Both the service teardown path and the update job need + * that guarantee, and they used to carry a copy each. + * + * The copies drifted in both directions, which is why this module exists rather + * than a second careful implementation: the updater received the #1589 argv + * cleanup that service.ts missed, and service.ts received the canonical-path + * scoping the updater missed. One of those gaps could force-terminate a wrapper + * belonging to a DIFFERENT OpenCodex home. + * + * Matching is scoped to the CANONICAL paths of one installation, never a bare + * filename, and the path must appear as a COMPLETE command-line token + * (wscript.exe spawns the .vbs as an argument; cmd.exe /c runs the .cmd). A + * substring match is excluded: an unrelated process whose command line merely + * contains the filename, and a wrapper under another home whose path ends with + * the same name, must both survive. + */ +import { spawnSync } from "node:child_process"; + +import { resolveTrustedWindowsPowerShellExe } from "./windows-elevation"; + +/** Quote for PowerShell: single-quote the value and double any embedded quote. */ +function quoteForPowerShell(value: string): string { + return `'${value.replace(/'/g, "''")}'`; +} + +/** + * The PowerShell that finds and stops the wrappers for exactly these paths. + * Exported for tests: the matching rule is the whole point of this module, and + * the spawn itself is best-effort and unobservable. + */ +export function windowsWrapperKillScript(paths: readonly string[]): string { + return [ + `$pats = @(${paths.map(quoteForPowerShell).join(", ")});`, + "Get-CimInstance Win32_Process | Where-Object {", + " if ($_.ProcessId -eq $PID) { return $false };", + " $c = $_.CommandLine; if (-not $c) { return $false };", + " foreach ($p in $pats) {", + " $i = $c.IndexOf($p, [System.StringComparison]::OrdinalIgnoreCase);", + " if ($i -lt 0) { continue };", + " $before = if ($i -gt 0) { $c.Substring($i - 1, 1) } else { ' ' };", + " $end = $i + $p.Length;", + " $after = if ($end -lt $c.Length) { $c.Substring($end, 1) } else { ' ' };", + " if ($before -match '[\\s\"'']' -and $after -match '[\\s\"'']') { return $true };", + " };", + " $false", + "} | ForEach-Object { Stop-Process -Id $_.ProcessId -Force -ErrorAction SilentlyContinue }", + ].join(" "); +} + +/** + * Best-effort: never throws, never reports. A wrapper that survives is handled + * by the caller's own stop verification. + */ +export function killWindowsSchedulerWrappers(paths: { + scriptPath: string; + launcherPath: string; +}): void { + if (process.platform !== "win32") return; + try { + spawnSync(resolveTrustedWindowsPowerShellExe(), [ + // No `-WindowStyle Hidden` here: Bun 1.3.14 can fail that direct CLI pair + // before the command runs (#1589). `windowsHide` below is sufficient. + "-NoProfile", "-NoLogo", "-NonInteractive", + "-Command", windowsWrapperKillScript([paths.scriptPath, paths.launcherPath]), + ], { stdio: "ignore", timeout: 5000, windowsHide: true }); + } catch { /* best-effort */ } +} diff --git a/src/service.ts b/src/service.ts index 08d4de4292..9adc59fee3 100644 --- a/src/service.ts +++ b/src/service.ts @@ -45,6 +45,7 @@ import { } from "./lib/windows-secret-acl"; import { windowsEnvIndirectBatchPathList, windowsEnvIndirectBatchValue } from "./lib/win-paths"; import { recordOwnedConfigPath } from "./lib/config-ownership"; +import { killWindowsSchedulerWrappers } from "./lib/windows-service-wrappers"; import { maybeShowStarPrompt } from "./cli/star-prompt"; const LABEL = "com.opencodex.proxy"; @@ -2323,48 +2324,17 @@ function statusWindowsXml(): string { try { return schtasks(["/query", "/tn", TA /** * Best-effort termination of surviving Windows scheduler launcher/wrapper processes. * `schtasks /end` ends the task instance but often leaves wscript/cmd running the - * `:loop` batch, which brings the proxy back during a stop or restart. Same killer - * the update job uses, so both teardown paths share the guarantee. + * `:loop` batch, which brings the proxy back during a stop or restart. * - * Matching is scoped to the CANONICAL paths of THIS installation (opencodex-service.cmd - * and opencodex-service-launcher.vbs under the current config dir), never a bare - * filename: a wrapper from another OpenCodex home — or an unrelated process whose - * command line merely contains the filename — must not be force-terminated. - * The path must appear as a COMPLETE command-line token (wscript.exe spawns the - * .vbs as an argument; cmd.exe /c runs the .cmd), so a substring-only match is - * excluded. + * The matching rule — canonical paths of THIS installation, as complete + * command-line tokens — lives in lib/windows-service-wrappers so the update job + * cannot drift away from it again. */ function killWindowsServiceWrapperProcesses(): void { - if (process.platform !== "win32") return; - try { - const script = windowsServiceScriptPath(); - const launcher = windowsLauncherVbsPath(); - // Quote for PowerShell: single-quote the value and double any embedded quote. - const quote = (value: string) => `'${value.replace(/'/g, "''")}'`; - const ps = [ - `$pats = @(${quote(script)}, ${quote(launcher)});`, - "Get-CimInstance Win32_Process | Where-Object {", - " if ($_.ProcessId -eq $PID) { return $false };", - " $c = $_.CommandLine; if (-not $c) { return $false };", - " foreach ($p in $pats) {", - " $i = $c.IndexOf($p, [System.StringComparison]::OrdinalIgnoreCase);", - " if ($i -lt 0) { continue };", - " $before = if ($i -gt 0) { $c.Substring($i - 1, 1) } else { ' ' };", - " $end = $i + $p.Length;", - " $after = if ($end -lt $c.Length) { $c.Substring($end, 1) } else { ' ' };", - " if ($before -match '[\\s\"'']' -and $after -match '[\\s\"'']') { return $true };", - " };", - " $false", - "} | ForEach-Object { Stop-Process -Id $_.ProcessId -Force -ErrorAction SilentlyContinue }", - ].join(" "); - spawnSync(resolveTrustedWindowsPowerShellExe(), [ - // No `-WindowStyle Hidden` here: Bun 1.3.14 can fail that direct CLI pair - // before the command runs (#1589). `windowsHide` below is what actually - // suppresses the console window, and it is sufficient. - "-NoProfile", "-NoLogo", "-NonInteractive", - "-Command", ps, - ], { stdio: "ignore", timeout: 5000, windowsHide: true }); - } catch { /* best-effort */ } + killWindowsSchedulerWrappers({ + scriptPath: windowsServiceScriptPath(), + launcherPath: windowsLauncherVbsPath(), + }); } function uninstallWindows(): void { const probe = probeWindowsSchedulerTask(TASK); diff --git a/src/update/job.ts b/src/update/job.ts index c95a9ff2d5..b1d17bcf2b 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -14,6 +14,7 @@ import { } from "../config"; import { isProcessAlive, killProxy } from "../lib/process-control"; import { selfLaunchArgv } from "../lib/self-launch-argv"; +import { killWindowsSchedulerWrappers } from "../lib/windows-service-wrappers"; import { buildWindowsElevatedArgumentList, resolveTrustedWindowsPowerShellExe, @@ -1371,26 +1372,16 @@ function stopWindowsServiceWrappersBestEffort(): void { * Best-effort termination of surviving Windows scheduler launcher/wrapper processes. * `schtasks /end` ends the task instance but often leaves wscript/cmd running the * `:loop` batch, which brings the proxy back during post-update reclaim. + * + * This used to match the bare filenames with -like '*name*', which could stop a + * wrapper belonging to a DIFFERENT OpenCodex home under the same account. The + * shared killer scopes to this home's canonical paths as complete tokens. */ function killWindowsServiceWrapperProcesses(): void { - if (process.platform !== "win32") return; - try { - const ps = [ - "$pats = @('opencodex-service.cmd','opencodex-service-launcher.vbs');", - "Get-CimInstance Win32_Process | Where-Object {", - " if ($_.ProcessId -eq $PID) { return $false };", - " $c = $_.CommandLine; if (-not $c) { return $false };", - " foreach ($p in $pats) { if ($c -like ('*' + $p + '*')) { return $true } };", - " $false", - "} | ForEach-Object { Stop-Process -Id $_.ProcessId -Force -ErrorAction SilentlyContinue }", - ].join(" "); - spawnSync(resolveTrustedWindowsPowerShellExe(), [ - "-NoProfile", "-NoLogo", "-NonInteractive", - "-Command", ps, - ], { stdio: "ignore", timeout: 5000, windowsHide: true }); - } catch { - /* best-effort */ - } + killWindowsSchedulerWrappers({ + scriptPath: join(getConfigDir(), "opencodex-service.cmd"), + launcherPath: join(getConfigDir(), "opencodex-service-launcher.vbs"), + }); } /** Exposed for tests: drives the non-service restart path with injected io. */ diff --git a/tests/cli-ready.test.ts b/tests/cli-ready.test.ts index 9e02e5f6ac..cab4c1ab4c 100644 --- a/tests/cli-ready.test.ts +++ b/tests/cli-ready.test.ts @@ -881,12 +881,19 @@ describe("handleStart OCX_SERVICE exit guard (source-level)", () => { // process whose command line merely contains the canonical path. The // PowerShell filter must check token boundaries (whitespace/quote before // and after the path), not a bare IndexOf. - const serviceSource = readFileSync(join(import.meta.dir, "../src/service.ts"), "utf8"); - const killBody = serviceSource.match(/function killWindowsServiceWrapperProcesses\(\)[\s\S]*?\n}/); - expect(killBody, "killWindowsServiceWrapperProcesses body must exist").not.toBeNull(); - expect(killBody![0]).not.toMatch(/IndexOf\(\$p, \[System\.StringComparison\]::OrdinalIgnoreCase\) -ge 0/); - expect(killBody![0]).toMatch(/Substring\(/); - expect(killBody![0]).toMatch(/before/); - expect(killBody![0]).toMatch(/after/); + // + // The script itself now lives in lib/windows-service-wrappers, shared with + // the update job so the two teardown paths cannot drift apart again, so the + // token-boundary rule is asserted where it is implemented. + const sharedSource = readFileSync( + join(import.meta.dir, "../src/lib/windows-service-wrappers.ts"), + "utf8", + ); + const killScript = sharedSource.match(/export function windowsWrapperKillScript\([\s\S]*?\n}/); + expect(killScript, "windowsWrapperKillScript body must exist").not.toBeNull(); + expect(killScript![0]).not.toMatch(/IndexOf\(\$p, \[System\.StringComparison\]::OrdinalIgnoreCase\) -ge 0/); + expect(killScript![0]).toMatch(/Substring\(/); + expect(killScript![0]).toMatch(/before/); + expect(killScript![0]).toMatch(/after/); }); }); diff --git a/tests/windows-deploy-close-regressions.test.ts b/tests/windows-deploy-close-regressions.test.ts index f3e3097ecc..f917b604ed 100644 --- a/tests/windows-deploy-close-regressions.test.ts +++ b/tests/windows-deploy-close-regressions.test.ts @@ -52,7 +52,12 @@ describe("update-job restart avoids the shell-less .cmd EINVAL (Windows, bun/sou // Native WinSW installs must stop via stopWinswService, not Task Scheduler /end only. expect(src).toContain("readServiceBackend"); expect(src).toContain("stopWinswService"); - expect(src).toContain("$_.ProcessId -eq $PID"); + // The wrapper-killer script (and its self-exclusion) moved to the shared + // lib/windows-service-wrappers so the updater and the service teardown path + // cannot drift apart again. Follow the invariant to where it lives; the + // matching rules themselves are covered by windows-service-wrappers.test.ts. + expect(src).toContain("killWindowsSchedulerWrappers"); + expect(read("src/lib/windows-service-wrappers.ts")).toContain("$_.ProcessId -eq $PID"); expect(src).toContain("lastChild?.pid && aliveFn(lastChild.pid)"); }); }); diff --git a/tests/windows-service-wrappers.test.ts b/tests/windows-service-wrappers.test.ts new file mode 100644 index 0000000000..b7c3e7bd67 --- /dev/null +++ b/tests/windows-service-wrappers.test.ts @@ -0,0 +1,125 @@ +/** + * The scheduler-wrapper killer must terminate THIS installation's wrappers and + * nothing else. + * + * Two copies of this logic existed. src/service.ts matched canonical full paths + * as complete command-line tokens; src/update/job.ts matched the bare filenames + * with -like '*name*'. On a machine with two OpenCodex homes under one account, + * a dashboard update for home A could force-terminate home B's wrapper, and any + * unrelated process whose command line contained either filename matched too. + * + * The killer spawns PowerShell and reports nothing, so the generated script is + * the only observable surface. Asserting that the script merely *contains* + * IndexOf/before/after would pass for a broken matcher that kept those tokens, + * so these cases port the rule to JS and run real command lines through it. The + * port is pinned to the shipped script by `matchRuleMatchesScript` below: if + * the PowerShell changes shape, that test fails and this file must be revisited. + */ +import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { windowsWrapperKillScript } from "../src/lib/windows-service-wrappers"; + +const read = (rel: string) => readFileSync(join(import.meta.dir, "..", rel), "utf8"); + +const HOME_A = "C:\\Users\\ocx\\.opencodex"; +const HOME_B = "C:\\Users\\ocx\\other-home\\.opencodex"; +const script = (home: string) => join(home, "opencodex-service.cmd"); +const launcher = (home: string) => join(home, "opencodex-service-launcher.vbs"); + +/** + * The shipped rule, in JS: find the pattern case-insensitively, then require the + * characters on both sides to be whitespace or a quote (start/end of the line + * counts as whitespace). Mirrors the PowerShell at + * src/lib/windows-service-wrappers.ts. + */ +function killsCommandLine(commandLine: string, patterns: readonly string[]): boolean { + const boundary = /[\s"']/; + for (const pattern of patterns) { + const at = commandLine.toLowerCase().indexOf(pattern.toLowerCase()); + if (at < 0) continue; + const before = at > 0 ? commandLine[at - 1]! : " "; + const end = at + pattern.length; + const after = end < commandLine.length ? commandLine[end]! : " "; + if (boundary.test(before) && boundary.test(after)) return true; + } + return false; +} + +const patterns = [script(HOME_A), launcher(HOME_A)]; + +describe("which command lines the wrapper killer stops", () => { + test("this installation's own wrappers are killed", () => { + expect(killsCommandLine(`cmd.exe /c "${script(HOME_A)}"`, patterns)).toBe(true); + expect(killsCommandLine(`wscript.exe "${launcher(HOME_A)}" //B`, patterns)).toBe(true); + // Unquoted, as Task Scheduler may present it. + expect(killsCommandLine(`cmd.exe /c ${script(HOME_A)}`, patterns)).toBe(true); + }); + + test("another OpenCodex home under the same account survives", () => { + // The defect this replaces: -like '*opencodex-service.cmd*' matched here. + expect(killsCommandLine(`cmd.exe /c "${script(HOME_B)}"`, patterns)).toBe(false); + expect(killsCommandLine(`wscript.exe "${launcher(HOME_B)}" //B`, patterns)).toBe(false); + }); + + test("a longer path that merely ends with our path is not a token", () => { + expect(killsCommandLine(`cmd.exe /c "C:\\backup\\${script(HOME_A)}"`, patterns)).toBe(false); + }); + + test("a path that merely starts with ours is not a token", () => { + expect(killsCommandLine(`cmd.exe /c "${script(HOME_A)}.bak"`, patterns)).toBe(false); + }); + + test("an unrelated process merely naming the file is not killed", () => { + expect(killsCommandLine("notepad.exe opencodex-service.cmd", patterns)).toBe(false); + expect(killsCommandLine('findstr /c:"opencodex-service-launcher.vbs" log.txt', patterns)).toBe(false); + }); + + test("matching is case-insensitive, as Windows paths are", () => { + expect(killsCommandLine(`cmd.exe /c "${script(HOME_A).toUpperCase()}"`, patterns)).toBe(true); + }); +}); + +describe("the generated script still implements that rule", () => { + test("matchRuleMatchesScript", () => { + // Pins the JS port above to the shipped PowerShell. If the script stops + // using ordinal-insensitive IndexOf plus both boundary checks, the port is + // no longer a faithful model and the cases above prove nothing. + const ps = windowsWrapperKillScript(patterns); + expect(ps).toContain("IndexOf($p, [System.StringComparison]::OrdinalIgnoreCase)"); + expect(ps).toContain("$before = if ($i -gt 0)"); + expect(ps).toContain("$after = if ($end -lt $c.Length)"); + expect(ps).toContain("if ($before -match"); + expect(ps).toContain("-and $after -match"); + expect(ps).not.toContain("-like"); + }); + + test("the script carries this home's canonical paths, not bare filenames", () => { + const ps = windowsWrapperKillScript(patterns); + expect(ps).toContain(script(HOME_A)); + expect(ps).toContain(launcher(HOME_A)); + expect(ps).not.toContain(script(HOME_B)); + expect(ps).not.toContain("@('opencodex-service.cmd'"); + }); + + test("the caller's own process is always excluded", () => { + expect(windowsWrapperKillScript(patterns)).toContain("$_.ProcessId -eq $PID"); + }); + + test("a path containing a quote is escaped, not injected", () => { + const odd = "C:\\Users\\o'brien\\.opencodex\\opencodex-service.cmd"; + expect(windowsWrapperKillScript([odd])).toContain("C:\\Users\\o''brien\\.opencodex\\opencodex-service.cmd"); + }); +}); + +describe("both teardown paths use the shared killer", () => { + test("neither file keeps a private matcher", () => { + for (const rel of ["src/service.ts", "src/update/job.ts"]) { + const src = read(rel); + expect(src).toContain("killWindowsSchedulerWrappers"); + expect(src).not.toContain("-like ('*' + $p + '*')"); + expect(src).not.toContain("$pats = @('opencodex-service.cmd'"); + } + }); +});