From 6084d13b956119e3cf95daaf9a1cae1670ea3557 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 16:43:23 +0100 Subject: [PATCH] fix(security): scope CLI cleanup to owned child PIDs --- CHANGELOG.md | 1 + src/agents/cli-runner.e2e.test.ts | 172 +++++++++++++++++++++++++++--- src/agents/cli-runner/helpers.ts | 48 +++++++-- 3 files changed, 199 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26a61db32e..cb8c9e3ee4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. - Security: fix Chutes manual OAuth login state validation (thanks @aether-ai-agent). (#16058) - macOS: hard-limit unkeyed `openclaw://agent` deep links and ignore `deliver` / `to` / `channel` unless a valid unattended key is provided. Thanks @Cillian-Collins. - Plugins: suppress false duplicate plugin id warnings when the same extension is discovered via multiple paths (config/workspace/global vs bundled), while still warning on genuine duplicates. (#16222) Thanks @shadril238. diff --git a/src/agents/cli-runner.e2e.test.ts b/src/agents/cli-runner.e2e.test.ts index b5f5e5ba52..8c5acd97d6 100644 --- a/src/agents/cli-runner.e2e.test.ts +++ b/src/agents/cli-runner.e2e.test.ts @@ -5,7 +5,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import type { CliBackendConfig } from "../config/types.js"; import { runCliAgent } from "./cli-runner.js"; -import { cleanupSuspendedCliProcesses } from "./cli-runner/helpers.js"; +import { cleanupResumeProcesses, cleanupSuspendedCliProcesses } from "./cli-runner/helpers.js"; const runCommandWithTimeoutMock = vi.fn(); const runExecMock = vi.fn(); @@ -22,12 +22,21 @@ describe("runCliAgent resume cleanup", () => { }); it("kills stale resume processes for codex sessions", async () => { + const selfPid = process.pid; + runExecMock .mockResolvedValueOnce({ - stdout: " 1 S /bin/launchd\n", + stdout: " 1 999 S /bin/launchd\n", stderr: "", - }) // cleanupSuspendedCliProcesses (ps) - .mockResolvedValueOnce({ stdout: "", stderr: "" }); // cleanupResumeProcesses (pkill) + }) // cleanupSuspendedCliProcesses (ps) — ppid 999 != selfPid, no match + .mockResolvedValueOnce({ + stdout: [ + ` ${selfPid + 1} ${selfPid} codex exec resume thread-123 --color never --sandbox read-only --skip-git-repo-check`, + ` ${selfPid + 2} 999 codex exec resume thread-123 --color never --sandbox read-only --skip-git-repo-check`, + ].join("\n"), + stderr: "", + }) // cleanupResumeProcesses (ps) + .mockResolvedValueOnce({ stdout: "", stderr: "" }); // cleanupResumeProcesses (kill) runCommandWithTimeoutMock.mockResolvedValueOnce({ stdout: "ok", stderr: "", @@ -53,14 +62,19 @@ describe("runCliAgent resume cleanup", () => { return; } - expect(runExecMock).toHaveBeenCalledTimes(2); - const pkillCall = runExecMock.mock.calls[1] ?? []; - expect(pkillCall[0]).toBe("pkill"); - const pkillArgs = pkillCall[1] as string[]; - expect(pkillArgs[0]).toBe("-f"); - expect(pkillArgs[1]).toContain("codex"); - expect(pkillArgs[1]).toContain("resume"); - expect(pkillArgs[1]).toContain("thread-123"); + expect(runExecMock).toHaveBeenCalledTimes(3); + + // 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] ?? []; + 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 }); it("falls back to per-agent workspace when workspaceDir is missing", async () => { @@ -165,11 +179,12 @@ describe("cleanupSuspendedCliProcesses", () => { }); it("matches sessionArg-based commands", async () => { + const selfPid = process.pid; runExecMock .mockResolvedValueOnce({ stdout: [ - " 40 T+ claude --session-id thread-1 -p", - " 41 S claude --session-id thread-2 -p", + ` 40 ${selfPid} T+ claude --session-id thread-1 -p`, + ` 41 ${selfPid} S claude --session-id thread-2 -p`, ].join("\n"), stderr: "", }) @@ -195,11 +210,12 @@ describe("cleanupSuspendedCliProcesses", () => { }); it("matches resumeArgs with positional session id", async () => { + const selfPid = process.pid; runExecMock .mockResolvedValueOnce({ stdout: [ - " 50 T codex exec resume thread-99 --color never --sandbox read-only", - " 51 T codex exec resume other --color never --sandbox read-only", + ` 50 ${selfPid} T codex exec resume thread-99 --color never --sandbox read-only`, + ` 51 ${selfPid} T codex exec resume other --color never --sandbox read-only`, ].join("\n"), stderr: "", }) @@ -223,4 +239,128 @@ describe("cleanupSuspendedCliProcesses", () => { expect(killCall[0]).toBe("kill"); expect(killCall[1]).toEqual(["-9", "50", "51"]); }); + + it("only kills child processes of current process (ppid validation)", async () => { + const selfPid = process.pid; + const childPid = selfPid + 1; + const unrelatedPid = 9999; + + runExecMock + .mockResolvedValueOnce({ + stdout: [ + ` ${childPid} ${selfPid} T claude --session-id thread-1 -p`, + ` ${unrelatedPid} 100 T claude --session-id thread-2 -p`, + ].join("\n"), + stderr: "", + }) + .mockResolvedValueOnce({ stdout: "", stderr: "" }); + + await cleanupSuspendedCliProcesses( + { + command: "claude", + sessionArg: "--session-id", + } as CliBackendConfig, + 0, + ); + + if (process.platform === "win32") { + expect(runExecMock).not.toHaveBeenCalled(); + return; + } + + expect(runExecMock).toHaveBeenCalledTimes(2); + const killCall = runExecMock.mock.calls[1] ?? []; + expect(killCall[0]).toBe("kill"); + // Only childPid killed; unrelatedPid (ppid=100) excluded + expect(killCall[1]).toEqual(["-9", String(childPid)]); + }); + + it("skips all processes when none are children of current process", async () => { + runExecMock.mockResolvedValueOnce({ + stdout: [ + " 200 100 T claude --session-id thread-1 -p", + " 201 100 T claude --session-id thread-2 -p", + ].join("\n"), + stderr: "", + }); + + await cleanupSuspendedCliProcesses( + { + command: "claude", + sessionArg: "--session-id", + } as CliBackendConfig, + 0, + ); + + if (process.platform === "win32") { + expect(runExecMock).not.toHaveBeenCalled(); + return; + } + + // Only ps called — no kill because no matching ppid + expect(runExecMock).toHaveBeenCalledTimes(1); + }); +}); + +describe("cleanupResumeProcesses", () => { + beforeEach(() => { + runExecMock.mockReset(); + }); + + it("only kills resume processes owned by current process", async () => { + const selfPid = process.pid; + + runExecMock + .mockResolvedValueOnce({ + stdout: [ + ` ${selfPid + 1} ${selfPid} codex exec resume abc-123`, + ` ${selfPid + 2} 999 codex exec resume abc-123`, + ].join("\n"), + stderr: "", + }) + .mockResolvedValueOnce({ stdout: "", stderr: "" }); + + await cleanupResumeProcesses( + { + command: "codex", + resumeArgs: ["exec", "resume", "{sessionId}"], + } as CliBackendConfig, + "abc-123", + ); + + if (process.platform === "win32") { + expect(runExecMock).not.toHaveBeenCalled(); + return; + } + + expect(runExecMock).toHaveBeenCalledTimes(2); + const killCall = runExecMock.mock.calls[1] ?? []; + expect(killCall[0]).toBe("kill"); + expect(killCall[1]).toEqual(["-9", String(selfPid + 1)]); + }); + + it("skips kill when no resume processes match ppid", async () => { + runExecMock.mockResolvedValueOnce({ + stdout: [" 300 100 codex exec resume abc-123", " 301 200 codex exec resume abc-123"].join( + "\n", + ), + stderr: "", + }); + + await cleanupResumeProcesses( + { + command: "codex", + resumeArgs: ["exec", "resume", "{sessionId}"], + } as CliBackendConfig, + "abc-123", + ); + + if (process.platform === "win32") { + expect(runExecMock).not.toHaveBeenCalled(); + return; + } + + // Only ps called — no kill because no matching ppid + expect(runExecMock).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/agents/cli-runner/helpers.ts b/src/agents/cli-runner/helpers.ts index 3674d8f2ed..5b7acd3560 100644 --- a/src/agents/cli-runner/helpers.ts +++ b/src/agents/cli-runner/helpers.ts @@ -47,9 +47,40 @@ export async function cleanupResumeProcesses( } try { - await runExec("pkill", ["-f", pattern]); + // 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 toKill: number[] = []; + + for (const line of stdout.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) { + continue; + } + const match = /^(\d+)\s+(\d+)\s+(.*)$/.exec(trimmed); + if (!match) { + continue; + } + const pid = Number(match[1]); + const ppid = Number(match[2]); + const cmd = match[3] ?? ""; + if (!Number.isFinite(pid)) { + continue; + } + if (ppid !== process.pid) { + continue; + } + if (!patternRegex.test(cmd)) { + continue; + } + toKill.push(pid); + } + + if (toKill.length > 0) { + await runExec("kill", ["-9", ...toKill.map((pid) => String(pid))]); + } } catch { - // ignore missing pkill or no matches + // ignore errors - best effort cleanup } } @@ -115,23 +146,28 @@ export async function cleanupSuspendedCliProcesses( } try { - const { stdout } = await runExec("ps", ["-ax", "-o", "pid=,stat=,command="]); + // Use wide output to reduce false negatives from argv truncation. + const { stdout } = await runExec("ps", ["-axww", "-o", "pid=,ppid=,stat=,command="]); const suspended: number[] = []; for (const line of stdout.split("\n")) { const trimmed = line.trim(); if (!trimmed) { continue; } - const match = /^(\d+)\s+(\S+)\s+(.*)$/.exec(trimmed); + const match = /^(\d+)\s+(\d+)\s+(\S+)\s+(.*)$/.exec(trimmed); if (!match) { continue; } const pid = Number(match[1]); - const stat = match[2] ?? ""; - const command = match[3] ?? ""; + const ppid = Number(match[2]); + const stat = match[3] ?? ""; + const command = match[4] ?? ""; if (!Number.isFinite(pid)) { continue; } + if (ppid !== process.pid) { + continue; + } if (!stat.includes("T")) { continue; }