From 0c6db05cc05cf9c92e49e8e4e2024bd8b98f399d Mon Sep 17 00:00:00 2001 From: root Date: Mon, 2 Mar 2026 14:39:46 +0800 Subject: [PATCH] fix(agents): add strict format validation to sessions_spawn for agentId Implements a strict format validation for the agentId parameter in sessions_spawn to fully resolve the ghost workspace creation bug reported in #31311. This fix introduces a regex format gate at the entry point to immediately reject malformed agentId strings. This prevents error messages (e.g., 'Agent not found: xyz') or path traversals from being mangled by normalizeAgentId into seemingly valid IDs (e.g., 'agent-not-found--xyz'), which was the root cause of the bug. The validation is placed before normalization and does not interfere with existing workflows, including delegating to agents that are allowlisted but not globally configured. New, non-redundant tests are added to sessions-spawn.allowlist.test.ts to cover format validation and ensure no regressions in allowlist behavior. Fixes #31311 --- ...subagents.sessions-spawn.allowlist.test.ts | 120 ++++++++++++++++++ src/agents/subagent-spawn.ts | 24 +++- 2 files changed, 143 insertions(+), 1 deletion(-) diff --git a/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts b/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts index d46beb61d..e46dab37b 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts @@ -223,4 +223,124 @@ describe("openclaw-tools: subagents (sessions_spawn allowlist)", () => { expect(details.error).toContain('sandbox="require"'); expect(callGatewayMock).not.toHaveBeenCalled(); }); + + // --------------------------------------------------------------------------- + // agentId format validation (#31311) + // --------------------------------------------------------------------------- + + it("rejects error-message-like strings as agentId (#31311)", async () => { + setSessionsSpawnConfigOverride({ + session: { mainKey: "main", scope: "per-sender" }, + agents: { + list: [{ id: "main", subagents: { allowAgents: ["*"] } }, { id: "research" }], + }, + }); + const tool = await getSessionsSpawnTool({ + agentSessionKey: "main", + agentChannel: "whatsapp", + }); + const result = await tool.execute("call-err-msg", { + task: "do thing", + agentId: "Agent not found: xyz", + }); + const details = result.details as { status?: string; error?: string }; + expect(details.status).toBe("error"); + expect(details.error).toContain("Invalid agentId"); + expect(details.error).toContain("agents_list"); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("rejects agentId containing path separators (#31311)", async () => { + setSessionsSpawnConfigOverride({ + session: { mainKey: "main", scope: "per-sender" }, + agents: { + list: [{ id: "main", subagents: { allowAgents: ["*"] } }], + }, + }); + const tool = await getSessionsSpawnTool({ + agentSessionKey: "main", + agentChannel: "whatsapp", + }); + const result = await tool.execute("call-path", { + task: "do thing", + agentId: "../../../etc/passwd", + }); + const details = result.details as { status?: string; error?: string }; + expect(details.status).toBe("error"); + expect(details.error).toContain("Invalid agentId"); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("rejects agentId exceeding 64 characters (#31311)", async () => { + setSessionsSpawnConfigOverride({ + session: { mainKey: "main", scope: "per-sender" }, + agents: { + list: [{ id: "main", subagents: { allowAgents: ["*"] } }], + }, + }); + const tool = await getSessionsSpawnTool({ + agentSessionKey: "main", + agentChannel: "whatsapp", + }); + const result = await tool.execute("call-long", { + task: "do thing", + agentId: "a".repeat(65), + }); + const details = result.details as { status?: string; error?: string }; + expect(details.status).toBe("error"); + expect(details.error).toContain("Invalid agentId"); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("accepts well-formed agentId with hyphens and underscores (#31311)", async () => { + setSessionsSpawnConfigOverride({ + session: { mainKey: "main", scope: "per-sender" }, + agents: { + list: [{ id: "main", subagents: { allowAgents: ["*"] } }, { id: "my-research_agent01" }], + }, + }); + callGatewayMock.mockImplementation(async () => ({ + runId: "run-1", + status: "accepted", + acceptedAt: 1000, + })); + const tool = await getSessionsSpawnTool({ + agentSessionKey: "main", + agentChannel: "whatsapp", + }); + const result = await tool.execute("call-valid", { + task: "do thing", + agentId: "my-research_agent01", + }); + const details = result.details as { status?: string }; + expect(details.status).toBe("accepted"); + }); + + it("allows allowlisted-but-unconfigured agentId (#31311)", async () => { + setSessionsSpawnConfigOverride({ + session: { mainKey: "main", scope: "per-sender" }, + agents: { + list: [ + { id: "main", subagents: { allowAgents: ["research"] } }, + // "research" is NOT in agents.list — only in allowAgents + ], + }, + }); + callGatewayMock.mockImplementation(async () => ({ + runId: "run-1", + status: "accepted", + acceptedAt: 1000, + })); + const tool = await getSessionsSpawnTool({ + agentSessionKey: "main", + agentChannel: "whatsapp", + }); + const result = await tool.execute("call-unconfigured", { + task: "do thing", + agentId: "research", + }); + const details = result.details as { status?: string }; + // Must pass: "research" is in allowAgents even though not in agents.list + expect(details.status).toBe("accepted"); + }); }); diff --git a/src/agents/subagent-spawn.ts b/src/agents/subagent-spawn.ts index a1389841b..5a7b5b162 100644 --- a/src/agents/subagent-spawn.ts +++ b/src/agents/subagent-spawn.ts @@ -31,6 +31,16 @@ export type SpawnSubagentMode = (typeof SUBAGENT_SPAWN_MODES)[number]; export const SUBAGENT_SPAWN_SANDBOX_MODES = ["inherit", "require"] as const; export type SpawnSubagentSandboxMode = (typeof SUBAGENT_SPAWN_SANDBOX_MODES)[number]; +/** + * Strict format gate for user-supplied agentId values. + * Rejects error-message-like strings, path traversals, and other + * values that would be silently mangled by {@link normalizeAgentId} + * and could create ghost workspace directories. + * + * Must stay in sync with the canonical `VALID_ID_RE` in session-key.ts. + */ +const STRICT_AGENT_ID_RE = /^[a-z0-9][a-z0-9_-]{0,63}$/i; + export function decodeStrictBase64(value: string, maxDecodedBytes: number): Buffer | null { const maxEncodedBytes = Math.ceil(maxDecodedBytes / 3) * 4; if (value.length > maxEncodedBytes * 2) { @@ -248,7 +258,19 @@ export async function spawnSubagentDirect( ): Promise { const task = params.task; const label = params.label?.trim() || ""; - const requestedAgentId = params.agentId; + const requestedAgentId = params.agentId?.trim(); + + // Reject malformed agentId before normalizeAgentId can mangle it. + // Without this gate, error-message strings like "Agent not found: xyz" pass + // through normalizeAgentId and become "agent-not-found--xyz", which later + // creates ghost workspace directories and triggers cascading cron loops (#31311). + if (requestedAgentId && !STRICT_AGENT_ID_RE.test(requestedAgentId)) { + return { + status: "error", + error: `Invalid agentId "${requestedAgentId}". Agent IDs must match [a-z0-9][a-z0-9_-]{0,63}. Use agents_list to discover valid targets.`, + }; + } + const modelOverride = params.model; const thinkingOverrideRaw = params.thinking; const requestThreadBinding = params.thread === true;