github/openclaw
1
1
Fork 0
mirror of https://github.com/openclaw/openclaw.git synced 2026-02-19 18:39:20 -05:00
Code Issues Packages Projects Releases Wiki Activity

fix(security): scope CLI cleanup to owned child PIDs

Browse Source
This commit is contained in:
Peter Steinberger
2026-02-14 16:43:23 +01:00
parent 5b4121d601
commit 6084d13b95
3 changed files with 199 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@@ -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.

172
src/agents/cli-runner.e2e.test.ts
View File

@@ -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);
});
});

48
src/agents/cli-runner/helpers.ts
View File

@@ -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;
}