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'"); + } + }); +});