From ecdbd8aa523d25f5da41d3984bfca72612628a95 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:12:22 +0000 Subject: [PATCH] fix(security): restrict leaf subagent control scope --- CHANGELOG.md | 1 + .../openclaw-tools.subagents.scope.test.ts | 152 ++++++++++++++++++ ...agents.sessions-spawn-depth-limits.test.ts | 5 + .../openclaw-tools.subagents.test-harness.ts | 5 + src/agents/pi-tools.policy.test.ts | 4 +- src/agents/pi-tools.policy.ts | 11 +- src/agents/tools/subagents-tool.ts | 70 +++++--- 7 files changed, 217 insertions(+), 31 deletions(-) create mode 100644 src/agents/openclaw-tools.subagents.scope.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index af544e1f6ff..b1b827ae6db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,7 @@ Docs: https://docs.openclaw.ai - Docs/Changelog: correct the contributor credit for the bundled Control UI global-install fix to @LarytheLord. (#40420) Thanks @velvet-shark. - Telegram/media downloads: time out only stalled body reads so polling recovers from hung file downloads without aborting slow downloads that are still streaming data. (#40098) thanks @tysoncung. - Docker/runtime image: prune dev dependencies, strip build-only dist metadata for smaller Docker images. (#40307) Thanks @vincentkoc. +- Subagents/sandboxing: restrict leaf subagents to their own spawned runs and remove leaf `subagents` control access so sandboxed leaf workers can no longer steer sibling sessions. Thanks @tdjackey. - Gateway/restart timeout recovery: exit non-zero when restart-triggered shutdown drains time out so launchd/systemd restart the gateway instead of treating the failed restart as a clean stop. Landed from contributor PR #40380 by @dsantoreis. Thanks @dsantoreis. - Gateway/config restart guard: validate config before service start/restart and keep post-SIGUSR1 startup failures from crashing the gateway process, reducing invalid-config restart loops and macOS permission loss. Landed from contributor PR #38699 by @lml2468. Thanks @lml2468. - Gateway/launchd respawn detection: treat `XPC_SERVICE_NAME` as a launchd supervision hint so macOS restarts exit cleanly under launchd instead of attempting detached self-respawn. Landed from contributor PR #20555 by @dimat. Thanks @dimat. diff --git a/src/agents/openclaw-tools.subagents.scope.test.ts b/src/agents/openclaw-tools.subagents.scope.test.ts new file mode 100644 index 00000000000..c58708ee6f4 --- /dev/null +++ b/src/agents/openclaw-tools.subagents.scope.test.ts @@ -0,0 +1,152 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { beforeEach, describe, expect, it } from "vitest"; +import { + callGatewayMock, + resetSubagentsConfigOverride, + setSubagentsConfigOverride, +} from "./openclaw-tools.subagents.test-harness.js"; +import { addSubagentRunForTests, resetSubagentRegistryForTests } from "./subagent-registry.js"; +import "./test-helpers/fast-core-tools.js"; +import { createPerSenderSessionConfig } from "./test-helpers/session-config.js"; +import { createSubagentsTool } from "./tools/subagents-tool.js"; + +function writeStore(storePath: string, store: Record) { + fs.mkdirSync(path.dirname(storePath), { recursive: true }); + fs.writeFileSync(storePath, JSON.stringify(store, null, 2), "utf-8"); +} + +describe("openclaw-tools: subagents scope isolation", () => { + let storePath = ""; + + beforeEach(() => { + resetSubagentRegistryForTests(); + resetSubagentsConfigOverride(); + callGatewayMock.mockReset(); + storePath = path.join( + os.tmpdir(), + `openclaw-subagents-scope-${Date.now()}-${Math.random().toString(16).slice(2)}.json`, + ); + setSubagentsConfigOverride({ + session: createPerSenderSessionConfig({ store: storePath }), + }); + writeStore(storePath, {}); + }); + + it("leaf subagents do not inherit parent sibling control scope", async () => { + const leafKey = "agent:main:subagent:leaf"; + const siblingKey = "agent:main:subagent:unsandboxed"; + + writeStore(storePath, { + [leafKey]: { + sessionId: "leaf-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + [siblingKey]: { + sessionId: "sibling-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + }); + + addSubagentRunForTests({ + runId: "run-leaf", + childSessionKey: leafKey, + requesterSessionKey: "agent:main:main", + requesterDisplayKey: "main", + task: "sandboxed leaf", + cleanup: "keep", + createdAt: Date.now() - 30_000, + startedAt: Date.now() - 30_000, + }); + addSubagentRunForTests({ + runId: "run-sibling", + childSessionKey: siblingKey, + requesterSessionKey: "agent:main:main", + requesterDisplayKey: "main", + task: "unsandboxed sibling", + cleanup: "keep", + createdAt: Date.now() - 20_000, + startedAt: Date.now() - 20_000, + }); + + const tool = createSubagentsTool({ agentSessionKey: leafKey }); + const result = await tool.execute("call-leaf-list", { action: "list" }); + + expect(result.details).toMatchObject({ + status: "ok", + requesterSessionKey: leafKey, + callerSessionKey: leafKey, + callerIsSubagent: true, + total: 0, + active: [], + recent: [], + }); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("orchestrator subagents still see children they spawned", async () => { + const orchestratorKey = "agent:main:subagent:orchestrator"; + const workerKey = `${orchestratorKey}:subagent:worker`; + const siblingKey = "agent:main:subagent:sibling"; + + writeStore(storePath, { + [orchestratorKey]: { + sessionId: "orchestrator-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + [workerKey]: { + sessionId: "worker-session", + updatedAt: Date.now(), + spawnedBy: orchestratorKey, + }, + [siblingKey]: { + sessionId: "sibling-session", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + }, + }); + + addSubagentRunForTests({ + runId: "run-worker", + childSessionKey: workerKey, + requesterSessionKey: orchestratorKey, + requesterDisplayKey: orchestratorKey, + task: "worker child", + cleanup: "keep", + createdAt: Date.now() - 30_000, + startedAt: Date.now() - 30_000, + }); + addSubagentRunForTests({ + runId: "run-sibling", + childSessionKey: siblingKey, + requesterSessionKey: "agent:main:main", + requesterDisplayKey: "main", + task: "sibling of orchestrator", + cleanup: "keep", + createdAt: Date.now() - 20_000, + startedAt: Date.now() - 20_000, + }); + + const tool = createSubagentsTool({ agentSessionKey: orchestratorKey }); + const result = await tool.execute("call-orchestrator-list", { action: "list" }); + const details = result.details as { + status?: string; + requesterSessionKey?: string; + total?: number; + active?: Array<{ sessionKey?: string }>; + }; + + expect(details.status).toBe("ok"); + expect(details.requesterSessionKey).toBe(orchestratorKey); + expect(details.total).toBe(1); + expect(details.active).toEqual([ + expect.objectContaining({ + sessionKey: workerKey, + }), + ]); + }); +}); diff --git a/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts b/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts index 7a5b93d7ae1..69a1a913b2c 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts @@ -6,6 +6,11 @@ import { addSubagentRunForTests, resetSubagentRegistryForTests } from "./subagen import { createPerSenderSessionConfig } from "./test-helpers/session-config.js"; import { createSessionsSpawnTool } from "./tools/sessions-spawn-tool.js"; +vi.mock("@mariozechner/pi-ai/oauth", () => ({ + getOAuthApiKey: () => undefined, + getOAuthProviders: () => [], +})); + const callGatewayMock = vi.fn(); vi.mock("../gateway/call.js", () => ({ diff --git a/src/agents/openclaw-tools.subagents.test-harness.ts b/src/agents/openclaw-tools.subagents.test-harness.ts index 44b6ea79118..36f00e22961 100644 --- a/src/agents/openclaw-tools.subagents.test-harness.ts +++ b/src/agents/openclaw-tools.subagents.test-harness.ts @@ -5,6 +5,11 @@ export type LoadedConfig = ReturnType<(typeof import("../config/config.js"))["lo export const callGatewayMock: MockFn = vi.fn(); +vi.mock("@mariozechner/pi-ai/oauth", () => ({ + getOAuthApiKey: () => undefined, + getOAuthProviders: () => [], +})); + const defaultConfig: LoadedConfig = { session: { mainKey: "main", diff --git a/src/agents/pi-tools.policy.test.ts b/src/agents/pi-tools.policy.test.ts index 0cdc572c448..ac31ca18694 100644 --- a/src/agents/pi-tools.policy.test.ts +++ b/src/agents/pi-tools.policy.test.ts @@ -144,9 +144,9 @@ describe("resolveSubagentToolPolicy depth awareness", () => { expect(isToolAllowedByPolicyName("sessions_spawn", policy)).toBe(false); }); - it("depth-2 leaf allows subagents (for visibility)", () => { + it("depth-2 leaf denies subagents", () => { const policy = resolveSubagentToolPolicy(baseCfg, 2); - expect(isToolAllowedByPolicyName("subagents", policy)).toBe(true); + expect(isToolAllowedByPolicyName("subagents", policy)).toBe(false); }); it("depth-2 leaf denies sessions_list and sessions_history", () => { diff --git a/src/agents/pi-tools.policy.ts b/src/agents/pi-tools.policy.ts index 61d037dd9f3..d5cf592428e 100644 --- a/src/agents/pi-tools.policy.ts +++ b/src/agents/pi-tools.policy.ts @@ -64,15 +64,20 @@ const SUBAGENT_TOOL_DENY_ALWAYS = [ * Additional tools denied for leaf sub-agents (depth >= maxSpawnDepth). * These are tools that only make sense for orchestrator sub-agents that can spawn children. */ -const SUBAGENT_TOOL_DENY_LEAF = ["sessions_list", "sessions_history", "sessions_spawn"]; +const SUBAGENT_TOOL_DENY_LEAF = [ + "subagents", + "sessions_list", + "sessions_history", + "sessions_spawn", +]; /** * Build the deny list for a sub-agent at a given depth. * * - Depth 1 with maxSpawnDepth >= 2 (orchestrator): allowed to use sessions_spawn, * subagents, sessions_list, sessions_history so it can manage its children. - * - Depth >= maxSpawnDepth (leaf): denied sessions_spawn and - * session management tools. Still allowed subagents (for list/status visibility). + * - Depth >= maxSpawnDepth (leaf): denied subagents, sessions_spawn, and + * session management tools. */ function resolveSubagentDenyList(depth: number, maxSpawnDepth: number): string[] { const isLeaf = depth >= Math.max(1, Math.floor(maxSpawnDepth)); diff --git a/src/agents/tools/subagents-tool.ts b/src/agents/tools/subagents-tool.ts index f2b073934ab..8735bc8809c 100644 --- a/src/agents/tools/subagents-tool.ts +++ b/src/agents/tools/subagents-tool.ts @@ -7,7 +7,6 @@ import { sortSubagentRuns, type SubagentTargetResolution, } from "../../auto-reply/reply/subagents-utils.js"; -import { DEFAULT_SUBAGENT_MAX_SPAWN_DEPTH } from "../../config/agent-limits.js"; import { loadConfig } from "../../config/config.js"; import type { SessionEntry } from "../../config/sessions.js"; import { loadSessionStore, resolveStorePath, updateSessionStore } from "../../config/sessions.js"; @@ -28,7 +27,6 @@ import { INTERNAL_MESSAGE_CHANNEL } from "../../utils/message-channel.js"; import { AGENT_LANE_SUBAGENT } from "../lanes.js"; import { abortEmbeddedPiRun } from "../pi-embedded.js"; import { optionalStringEnum } from "../schema/typebox.js"; -import { getSubagentDepthFromSessionStore } from "../subagent-depth.js"; import { clearSubagentRunSteerRestart, countPendingDescendantRuns, @@ -204,36 +202,28 @@ function resolveRequesterKey(params: { }; } - // Check if this sub-agent can spawn children (orchestrator). - // If so, it should see its own children, not its parent's children. - const callerDepth = getSubagentDepthFromSessionStore(callerSessionKey, { cfg: params.cfg }); - const maxSpawnDepth = - params.cfg.agents?.defaults?.subagents?.maxSpawnDepth ?? DEFAULT_SUBAGENT_MAX_SPAWN_DEPTH; - if (callerDepth < maxSpawnDepth) { - // Orchestrator sub-agent: use its own session key as requester - // so it sees children it spawned. - return { - requesterSessionKey: callerSessionKey, - callerSessionKey, - callerIsSubagent: true, - }; - } - - // Leaf sub-agent: walk up to its parent so it can see sibling runs. - const cache = new Map>(); - const callerEntry = resolveSessionEntryForKey({ - cfg: params.cfg, - key: callerSessionKey, - cache, - }).entry; - const spawnedBy = typeof callerEntry?.spawnedBy === "string" ? callerEntry.spawnedBy.trim() : ""; return { - requesterSessionKey: spawnedBy || callerSessionKey, + // Subagents can only control runs spawned from their own session key. + // Announce routing still uses SubagentRunRecord.requesterSessionKey elsewhere. + requesterSessionKey: callerSessionKey, callerSessionKey, callerIsSubagent: true, }; } +function ensureSubagentControlsOwnDescendants(params: { + requester: ResolvedRequesterKey; + entry: SubagentRunRecord; +}) { + if (!params.requester.callerIsSubagent) { + return undefined; + } + if (params.entry.requesterSessionKey === params.requester.callerSessionKey) { + return undefined; + } + return "Subagents can only control runs spawned from their own session."; +} + async function killSubagentRun(params: { cfg: ReturnType; entry: SubagentRunRecord; @@ -499,6 +489,20 @@ export function createSubagentsTool(opts?: { agentSessionKey?: string }): AnyAge error: resolved.error ?? "Unknown subagent target.", }); } + const ownershipError = ensureSubagentControlsOwnDescendants({ + requester, + entry: resolved.entry, + }); + if (ownershipError) { + return jsonResult({ + status: "forbidden", + action: "kill", + target, + runId: resolved.entry.runId, + sessionKey: resolved.entry.childSessionKey, + error: ownershipError, + }); + } const killCache = new Map>(); const stopResult = await killSubagentRun({ cfg, @@ -568,6 +572,20 @@ export function createSubagentsTool(opts?: { agentSessionKey?: string }): AnyAge error: resolved.error ?? "Unknown subagent target.", }); } + const ownershipError = ensureSubagentControlsOwnDescendants({ + requester, + entry: resolved.entry, + }); + if (ownershipError) { + return jsonResult({ + status: "forbidden", + action: "steer", + target, + runId: resolved.entry.runId, + sessionKey: resolved.entry.childSessionKey, + error: ownershipError, + }); + } if (resolved.entry.endedAt) { return jsonResult({ status: "done",