fix(security): harden CLI cleanup kill and matching

This commit is contained in:
Peter Steinberger
2026-02-14 16:49:33 +01:00
parent 9e147f00b4
commit eb60e2e1b2
2 changed files with 66 additions and 15 deletions

View File

@@ -36,7 +36,8 @@ describe("runCliAgent resume cleanup", () => {
].join("\n"),
stderr: "",
}) // cleanupResumeProcesses (ps)
.mockResolvedValueOnce({ stdout: "", stderr: "" }); // cleanupResumeProcesses (kill)
.mockResolvedValueOnce({ stdout: "", stderr: "" }) // cleanupResumeProcesses (kill -TERM)
.mockResolvedValueOnce({ stdout: "", stderr: "" }); // cleanupResumeProcesses (kill -9)
runCommandWithTimeoutMock.mockResolvedValueOnce({
stdout: "ok",
stderr: "",
@@ -62,19 +63,23 @@ describe("runCliAgent resume cleanup", () => {
return;
}
expect(runExecMock).toHaveBeenCalledTimes(3);
expect(runExecMock).toHaveBeenCalledTimes(4);
// Second call: cleanupResumeProcesses ps
const psCall = runExecMock.mock.calls[1] ?? [];
expect(psCall[0]).toBe("ps");
// Third call: kill with only the child PID
const killCall = runExecMock.mock.calls[2] ?? [];
// Third call: TERM, only the child PID
const termCall = runExecMock.mock.calls[2] ?? [];
expect(termCall[0]).toBe("kill");
const termArgs = termCall[1] as string[];
expect(termArgs).toEqual(["-TERM", String(selfPid + 1)]);
// Fourth call: KILL, only the child PID
const killCall = runExecMock.mock.calls[3] ?? [];
expect(killCall[0]).toBe("kill");
const killArgs = killCall[1] as string[];
expect(killArgs[0]).toBe("-9");
expect(killArgs[1]).toBe(String(selfPid + 1));
expect(killArgs).toHaveLength(2); // only -9 + one PID
expect(killArgs).toEqual(["-9", String(selfPid + 1)]);
});
it("falls back to per-agent workspace when workspaceDir is missing", async () => {
@@ -318,6 +323,7 @@ describe("cleanupResumeProcesses", () => {
].join("\n"),
stderr: "",
})
.mockResolvedValueOnce({ stdout: "", stderr: "" })
.mockResolvedValueOnce({ stdout: "", stderr: "" });
await cleanupResumeProcesses(
@@ -333,8 +339,13 @@ describe("cleanupResumeProcesses", () => {
return;
}
expect(runExecMock).toHaveBeenCalledTimes(2);
const killCall = runExecMock.mock.calls[1] ?? [];
expect(runExecMock).toHaveBeenCalledTimes(3);
const termCall = runExecMock.mock.calls[1] ?? [];
expect(termCall[0]).toBe("kill");
expect(termCall[1]).toEqual(["-TERM", String(selfPid + 1)]);
const killCall = runExecMock.mock.calls[2] ?? [];
expect(killCall[0]).toBe("kill");
expect(killCall[1]).toEqual(["-9", String(selfPid + 1)]);
});

View File

@@ -18,6 +18,31 @@ import { buildAgentSystemPrompt } from "../system-prompt.js";
const CLI_RUN_QUEUE = new Map<string, Promise<unknown>>();
function buildLooseArgOrderRegex(tokens: string[]): RegExp {
// Scan `ps` output lines. Keep matching flexible, but require whitespace arg boundaries
// to avoid substring matches like `codexx` or `/path/to/codexx`.
const [head, ...rest] = tokens.map((t) => String(t ?? "").trim()).filter(Boolean);
if (!head) {
return /$^/;
}
const headEscaped = escapeRegExp(head);
const headFragment = `(?:^|\\s)(?:${headEscaped}|\\S+\\/${headEscaped})(?=\\s|$)`;
const restFragments = rest.map((t) => `(?:^|\\s)${escapeRegExp(t)}(?=\\s|$)`);
return new RegExp([headFragment, ...restFragments].join(".*"));
}
async function psWithFallback(argsA: string[], argsB: string[]): Promise<string> {
try {
const { stdout } = await runExec("ps", argsA);
return stdout;
} catch {
// fallthrough
}
const { stdout } = await runExec("ps", argsB);
return stdout;
}
export async function cleanupResumeProcesses(
backend: CliBackendConfig,
sessionId: string,
@@ -47,9 +72,11 @@ export async function cleanupResumeProcesses(
}
try {
// Use wide output to reduce false negatives from argv truncation.
const { stdout } = await runExec("ps", ["-axww", "-o", "pid=,ppid=,command="]);
const patternRegex = new RegExp(pattern);
const stdout = await psWithFallback(
["-axww", "-o", "pid=,ppid=,command="],
["-ax", "-o", "pid=,ppid=,command="],
);
const patternRegex = buildLooseArgOrderRegex([commandToken, ...resumeTokens]);
const toKill: number[] = [];
for (const line of stdout.split("\n")) {
@@ -77,7 +104,18 @@ export async function cleanupResumeProcesses(
}
if (toKill.length > 0) {
await runExec("kill", ["-9", ...toKill.map((pid) => String(pid))]);
const pidArgs = toKill.map((pid) => String(pid));
try {
await runExec("kill", ["-TERM", ...pidArgs]);
} catch {
// ignore
}
await new Promise((resolve) => setTimeout(resolve, 250));
try {
await runExec("kill", ["-9", ...pidArgs]);
} catch {
// ignore
}
}
} catch {
// ignore errors - best effort cleanup
@@ -146,8 +184,10 @@ export async function cleanupSuspendedCliProcesses(
}
try {
// Use wide output to reduce false negatives from argv truncation.
const { stdout } = await runExec("ps", ["-axww", "-o", "pid=,ppid=,stat=,command="]);
const stdout = await psWithFallback(
["-axww", "-o", "pid=,ppid=,stat=,command="],
["-ax", "-o", "pid=,ppid=,stat=,command="],
);
const suspended: number[] = [];
for (const line of stdout.split("\n")) {
const trimmed = line.trim();