-
Notifications
You must be signed in to change notification settings - Fork 0
[WRONG BRANCH] fix(adapters): terminate Windows coding-agent process trees #476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { spawn as nodeSpawn, type ChildProcess, type SpawnOptions } from "node:child_process"; | ||
| import { execFileSync, spawn as nodeSpawn, type ChildProcess, type SpawnOptions } from "node:child_process"; | ||
| import type { AdapterEvent, OcxParsedRequest, OcxProviderConfig } from "../../types"; | ||
| import { commandInvocation } from "../../lib/win-exec"; | ||
| import type { IncomingMeta } from "../base"; | ||
|
|
@@ -8,6 +8,9 @@ import { resolveCodingAgentBinary, resolveProfileByBaseUrl, type CodingAgentProv | |
| /** Injectable spawn for tests; production uses node:child_process. */ | ||
| export type SpawnFn = (command: string, args: readonly string[], options: SpawnOptions) => ChildProcess; | ||
|
|
||
| /** Injectable Windows process-tree terminator; production uses taskkill /T /F. */ | ||
| export type KillWindowsProcessTreeFn = (pid: number) => void; | ||
|
|
||
| /** Per-turn injectables: spawn/which seams for tests plus wall-clock ceilings for timeout, kill grace, and bounded reap. */ | ||
| export interface CodingAgentDeps { | ||
| spawn?: SpawnFn; | ||
|
|
@@ -20,13 +23,23 @@ export interface CodingAgentDeps { | |
| reapTimeoutMs?: number; | ||
| /** Test seam for Windows command-shim invocation. */ | ||
| platform?: NodeJS.Platform; | ||
| /** Test seam for terminating a Windows CLI and all descendants. */ | ||
| killWindowsProcessTree?: KillWindowsProcessTreeFn; | ||
| } | ||
|
|
||
| const DEFAULT_TIMEOUT_MS = 300_000; | ||
| const DEFAULT_KILL_GRACE_MS = 2_000; | ||
| /** Bound captured stderr so an error message can never carry an unbounded (or secret) payload. */ | ||
| const MAX_STDERR_BYTES = 8 * 1024; | ||
|
|
||
| function killWindowsProcessTree(pid: number): void { | ||
| const taskkill = `${process.env.SystemRoot ?? "C:\\Windows"}\\System32\\taskkill.exe`; | ||
| execFileSync(taskkill, ["/PID", String(pid), "/T", "/F"], { | ||
| stdio: "pipe", | ||
| windowsHide: true, | ||
| }); | ||
| } | ||
|
|
||
| /** Env keys a CLI needs to run; everything else is dropped so the child env is scoped and deterministic. */ | ||
| const INHERITED_ENV_KEYS = [ | ||
| "PATH", "HOME", "USERPROFILE", "LANG", "LC_ALL", "LC_CTYPE", "TMPDIR", "TEMP", "TMP", | ||
|
|
@@ -93,6 +106,7 @@ export async function runCodingAgentTurn(input: CodingAgentTurnInput): Promise<v | |
| const timeoutMs = deps.timeoutMs ?? DEFAULT_TIMEOUT_MS; | ||
| const killGraceMs = deps.killGraceMs ?? DEFAULT_KILL_GRACE_MS; | ||
| const reapTimeoutMs = deps.reapTimeoutMs ?? (killGraceMs * 2 + 250); | ||
| const platform = deps.platform ?? process.platform; | ||
|
|
||
| if (incoming.abortSignal?.aborted) { | ||
| emit({ type: "error", message: "Coding-agent turn was aborted before start." }); | ||
|
|
@@ -140,7 +154,7 @@ export async function runCodingAgentTurn(input: CodingAgentTurnInput): Promise<v | |
|
|
||
| const args = buildArgs(profile, parsed, provider); | ||
| const env = buildEnv(profile, apiKey); | ||
| const invocation = commandInvocation(binary, args, deps.platform ?? process.platform, { env }); | ||
| const invocation = commandInvocation(binary, args, platform, { env }); | ||
|
|
||
| let child: ChildProcess; | ||
| try { | ||
|
|
@@ -197,6 +211,12 @@ export async function runCodingAgentTurn(input: CodingAgentTurnInput): Promise<v | |
| const kill = (): void => { | ||
| if (killed || child.killed) return; | ||
| killed = true; | ||
| if (platform === "win32" && child.pid !== undefined) { | ||
| try { | ||
| (deps.killWindowsProcessTree ?? killWindowsProcessTree)(child.pid); | ||
|
Comment on lines
+214
to
+216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the direct child has emitted AGENTS.md reference: src/AGENTS.md:L17-L20 Useful? React with 👍 / 👎. |
||
| return; | ||
| } catch { /* fall back to terminating the direct child */ } | ||
| } | ||
| try { child.kill("SIGTERM"); } catch { /* already gone */ } | ||
| killTimer = setTimeout(() => { | ||
| try { child.kill("SIGKILL"); } catch { /* already gone */ } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When OpenCodex is launched with a poisoned
SystemRootvalue, this constructs an absolute-looking path under the attacker-controlled directory and executes itstaskkill.exeon the first abort or timeout with the proxy user's privileges and inherited environment. The repository already providesresolveTrustedWindowsTaskkillExe(), backed byGetSystemDirectoryW, specifically to avoid selecting system executables through caller-controlled environment variables; use that resolver or the same trusted-resolution logic here.AGENTS.md reference: AGENTS.md:L357-L363
Useful? React with 👍 / 👎.