diff --git a/CHANGELOG.md b/CHANGELOG.md index c5ceb8024add..ef6174b6b5e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2789,6 +2789,7 @@ Docs: https://docs.openclaw.ai ### Fixes - CLI/message: skip eager model context warmup and preserve channel-declared gateway execution for Discord and Telegram message actions, avoiding Codex app-server/model discovery during simple send/read commands. Thanks @fuller-stack-dev. +- Agents/exec approvals: parse exec approval result metadata with balanced parentheses so nested-paren denial and finished payloads such as `Exec denied (gateway id=req-1, approval-timeout (allowlist-miss)): ...` are matched and routed to the denied followup branch instead of falling through to the generic followup path. (#72268) Thanks @amittell. - Codex/app-server: resolve managed binaries from bundled `dist` chunks and from the `@openai/codex` package bin when installs do not provide a nearby `.bin/codex` shim, avoiding false missing-binary startup failures. - Plugins/ClawHub: use the ClawHub artifact resolver response as the install decision before downloading, keeping legacy ZIP fallback and future ClawPack npm-pack installs on the same explicit resolver path. Thanks @vincentkoc. - Plugins/ClawHub: keep bare plugin package specs on npm for the launch cutover and reserve ClawHub resolution for explicit `clawhub:` specs until ClawHub pack readiness is deployed. Thanks @vincentkoc. diff --git a/src/agents/bash-tools.exec-approval-followup.test.ts b/src/agents/bash-tools.exec-approval-followup.test.ts index 0dd664c0fb3b..b9da116077e4 100644 --- a/src/agents/bash-tools.exec-approval-followup.test.ts +++ b/src/agents/bash-tools.exec-approval-followup.test.ts @@ -80,6 +80,16 @@ describe("exec approval followup", () => { expect(prompt).not.toContain("already approved has completed"); }); + it("uses the denied followup branch for nested-parentheses denial metadata", () => { + const prompt = buildExecApprovalFollowupPrompt( + "Exec denied (gateway id=req-1, approval-timeout (allowlist-miss)): uname -a", + ); + + expect(prompt).toContain("did not run"); + expect(prompt).toContain("Do not mention, summarize, or reuse output"); + expect(prompt).not.toContain("already approved has completed"); + }); + it("tells the agent to continue the task before replying when the command succeeds", () => { const prompt = buildExecApprovalFollowupPrompt("Exec finished (gateway id=req-1, code 0)\nok"); @@ -295,6 +305,29 @@ describe("exec approval followup", () => { expect(callGatewayTool).not.toHaveBeenCalled(); }); + it("uses safe denied copy for nested-parentheses denial metadata when session resume fails", async () => { + vi.mocked(callGatewayTool).mockRejectedValueOnce(new Error("session missing")); + + await sendExecApprovalFollowup({ + approvalId: "req-denied-resume-failed-nested", + sessionKey: "agent:main:telegram:-100123", + turnSourceChannel: "telegram", + turnSourceTo: "-100123", + turnSourceAccountId: "default", + turnSourceThreadId: "789", + resultText: + "Exec denied (gateway id=req-denied-resume-failed-nested, approval-timeout (allowlist-miss)): uname -a", + }); + + expect(sendMessage).toHaveBeenCalledWith( + expect.objectContaining({ + content: + "Automatic session resume failed, so sending the status directly.\n\nCommand did not run: approval timed out.", + idempotencyKey: "exec-approval-followup:req-denied-resume-failed-nested", + }), + ); + }); + it("suppresses denied followups for subagent sessions", async () => { await expect( sendExecApprovalFollowup({ diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index dc60d7b1d6c9..0c902f4abdaf 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -619,7 +619,10 @@ export async function processGatewayAllowlist( if (baseDecision.timedOut && askFallback === "allowlist") { if (!analysisOk || !allowlistSatisfied) { - deniedReason = "approval-timeout (allowlist-miss)"; + // Use a colon separator rather than nested parens so the + // `Exec denied (gateway id=..., ): cmd` wire format + // stays unambiguous for parsers that close on the first `):`. + deniedReason = "approval-timeout: allowlist-miss"; } else { approvedByAsk = true; } diff --git a/src/agents/exec-approval-result.test.ts b/src/agents/exec-approval-result.test.ts index 4ea821a38c59..1b1c72cf87bf 100644 --- a/src/agents/exec-approval-result.test.ts +++ b/src/agents/exec-approval-result.test.ts @@ -17,6 +17,33 @@ describe("parseExecApprovalResultText", () => { }); }); + it("parses denied results with nested parentheses in metadata", () => { + const input = + "Exec denied (gateway id=req-1, approval-timeout (allowlist-miss)): source ~/.zprofile && kubectl get pods"; + + expect(parseExecApprovalResultText(input)).toEqual({ + kind: "denied", + raw: input, + metadata: "gateway id=req-1, approval-timeout (allowlist-miss)", + body: "source ~/.zprofile && kubectl get pods", + }); + }); + + it("parses denied results with the canonical colon-separated deniedReason", () => { + // Producer (src/agents/bash-tools.exec-host-gateway.ts) emits a colon + // separator instead of nested parens to keep the (...)-delimited wire + // format unambiguous. This is the format real timeouts now produce. + const input = + "Exec denied (gateway id=req-1, approval-timeout: allowlist-miss): source ~/.zprofile && kubectl get pods"; + + expect(parseExecApprovalResultText(input)).toEqual({ + kind: "denied", + raw: input, + metadata: "gateway id=req-1, approval-timeout: allowlist-miss", + body: "source ~/.zprofile && kubectl get pods", + }); + }); + it("parses finished results", () => { expect( parseExecApprovalResultText("Exec finished (gateway id=req-1, code 0)\nall good"), @@ -28,6 +55,17 @@ describe("parseExecApprovalResultText", () => { }); }); + it("parses finished results with nested parentheses in metadata", () => { + const input = "Exec finished (gateway id=req-1, note (nested), code 0)\nall good"; + + expect(parseExecApprovalResultText(input)).toEqual({ + kind: "finished", + raw: input, + metadata: "gateway id=req-1, note (nested), code 0", + body: "all good", + }); + }); + it("parses completed results", () => { expect(parseExecApprovalResultText("Exec completed: done")).toEqual({ kind: "completed", @@ -42,12 +80,31 @@ describe("parseExecApprovalResultText", () => { raw: "some random text", }); }); + + it.each([ + "Exec denied (anything): bar", + "Exec denied (just-text): foo", + "Exec denied (request-id=abc, denied): cmd", + "Exec denied (id=req-1, user-denied): cmd", + "Exec finished (anything)\nbody", + "Exec finished (status: ok)\nbody", + ])( + "returns other when metadata is not gateway/node sourced (CWE-841 spoof guard): %s", + (input) => { + expect(parseExecApprovalResultText(input)).toEqual({ + kind: "other", + raw: input, + }); + }, + ); }); describe("isExecDeniedResultText", () => { it.each([ "Exec denied (gateway id=req-1, approval-timeout): uname -a", "exec denied (gateway id=req-1, approval-timeout): uname -a", + "Exec denied (gateway id=req-1, approval-timeout (allowlist-miss)): uname -a", + "Exec denied (gateway id=req-1, approval-timeout: allowlist-miss): uname -a", ])("matches denied payloads: %s", (input) => { expect(isExecDeniedResultText(input)).toBe(true); }); @@ -63,6 +120,14 @@ describe("formatExecDeniedUserMessage", () => { "Exec denied (gateway id=req-1, approval-timeout): uname -a", "Command did not run: approval timed out.", ], + [ + "Exec denied (gateway id=req-1, approval-timeout (allowlist-miss)): uname -a", + "Command did not run: approval timed out.", + ], + [ + "Exec denied (gateway id=req-1, approval-timeout: allowlist-miss): uname -a", + "Command did not run: approval timed out.", + ], [ "Exec denied (gateway id=req-1, user-denied): uname -a", "Command did not run: approval was denied.", diff --git a/src/agents/exec-approval-result.ts b/src/agents/exec-approval-result.ts index d5364e5b4035..9522e0c611c3 100644 --- a/src/agents/exec-approval-result.ts +++ b/src/agents/exec-approval-result.ts @@ -23,33 +23,98 @@ type ExecApprovalResult = raw: string; }; -const EXEC_DENIED_RE = /^exec denied \(([^)]*)\):(?:\s*([\s\S]*))?$/i; -const EXEC_FINISHED_RE = /^exec finished \(([^)]*)\)(?:\n([\s\S]*))?$/i; const EXEC_COMPLETED_RE = /^exec completed:\s*([\s\S]*)$/i; +// Approval-system-generated wrappers always start with either `gateway id=` or +// `node=` inside the parenthesized metadata (see bash-tools.exec-host-gateway.ts, +// bash-tools.exec-host-node.ts, and gateway/server-node-events.ts). Untrusted +// command stdout that happens to start with "Exec denied (...)" or +// "Exec finished (...)" should be rejected by the parser to prevent CWE-841 +// spoofed approval events from arbitrary tool output. +const APPROVAL_METADATA_SOURCE_RE = /^(?:gateway\s+id=|node=)/i; + +function parseExecApprovalResultWithMetadata( + raw: string, + prefix: string, + bodySeparator: ":" | "\n", +): { metadata: string; body: string } | null { + const normalizedRaw = normalizeLowercaseStringOrEmpty(raw); + const normalizedPrefix = normalizeLowercaseStringOrEmpty(prefix); + if (!normalizedRaw.startsWith(normalizedPrefix)) { + return null; + } + + const metadataStart = prefix.length; + let depth = 1; + let metadataEnd = -1; + for (let index = metadataStart; index < raw.length; index += 1) { + const char = raw[index]; + if (char === "(") { + depth += 1; + continue; + } + if (char === ")") { + depth -= 1; + if (depth === 0) { + metadataEnd = index; + break; + } + } + } + + if (metadataEnd < 0) { + return null; + } + + const metadata = raw.slice(metadataStart, metadataEnd).trim(); + if (!APPROVAL_METADATA_SOURCE_RE.test(metadata)) { + return null; + } + + const remainder = raw.slice(metadataEnd + 1); + if (bodySeparator === ":") { + if (!remainder.startsWith(":")) { + return null; + } + return { + metadata, + body: remainder.slice(1).trim(), + }; + } + + if (remainder && !remainder.startsWith("\n")) { + return null; + } + + return { + metadata, + body: remainder.startsWith("\n") ? remainder.slice(1).trim() : "", + }; +} + export function parseExecApprovalResultText(resultText: string): ExecApprovalResult { const raw = resultText.trim(); if (!raw) { return { kind: "other", raw }; } - const deniedMatch = EXEC_DENIED_RE.exec(raw); - if (deniedMatch) { + const deniedResult = parseExecApprovalResultWithMetadata(raw, "Exec denied (", ":"); + if (deniedResult) { return { kind: "denied", raw, - metadata: deniedMatch[1]?.trim() ?? "", - body: deniedMatch[2]?.trim() ?? "", + metadata: deniedResult.metadata, + body: deniedResult.body, }; } - const finishedMatch = EXEC_FINISHED_RE.exec(raw); - if (finishedMatch) { + const finishedResult = parseExecApprovalResultWithMetadata(raw, "Exec finished (", "\n"); + if (finishedResult) { return { kind: "finished", raw, - metadata: finishedMatch[1]?.trim() ?? "", - body: finishedMatch[2]?.trim() ?? "", + metadata: finishedResult.metadata, + body: finishedResult.body, }; } diff --git a/src/agents/pi-embedded-helpers/errors.ts b/src/agents/pi-embedded-helpers/errors.ts index fbe786028139..292ff364dc89 100644 --- a/src/agents/pi-embedded-helpers/errors.ts +++ b/src/agents/pi-embedded-helpers/errors.ts @@ -330,7 +330,7 @@ const INTERRUPTED_NETWORK_ERROR_RE = const REPLAY_INVALID_RE = /\bprevious_response_id\b.*\b(?:invalid|unknown|not found|does not exist|expired|mismatch)\b|\btool_(?:use|call)\.(?:input|arguments)\b.*\b(?:missing|required)\b|\bincorrect role information\b|\broles must alternate\b|\binput item id does not belong to this connection\b/i; const SANDBOX_BLOCKED_RE = - /\bapproval is required\b|\bapproval timed out\b|\bapproval was denied\b|\bblocked by sandbox\b|\bsandbox\b.*\b(?:blocked|denied|forbidden|disabled|not allowed)\b/i; + /\bapproval is required\b|\bapproval timed out\b|\bapproval was denied\b|\bblocked by sandbox\b|\bsandbox\b.*\b(?:blocked|denied|forbidden|disabled|not allowed)\b|\bexec denied\s*\(/i; const NO_BODY_HTTP_WRAPPER_RE = /^(?:no body(?: response)?|no response body|status code \(no body\))$/i; diff --git a/src/gateway/server-node-events.ts b/src/gateway/server-node-events.ts index 460fb020681a..45c33b0a38a2 100644 --- a/src/gateway/server-node-events.ts +++ b/src/gateway/server-node-events.ts @@ -733,7 +733,15 @@ export const handleNodeEvent = async ( : undefined; const timedOut = obj.timedOut === true; const output = sanitizeInboundSystemTags(normalizeOptionalString(obj.output) ?? ""); - const reason = sanitizeInboundSystemTags(normalizeOptionalString(obj.reason) ?? ""); + // Strip parens from the untrusted RAW reason before sanitizeInboundSystemTags + // runs: the `Exec denied (node=..., ): cmd` wire format is parsed by + // matching the first balanced `(...)` and stray parens in user-supplied + // input would break the metadata/body boundary. We strip pre-sanitize so + // that legitimate `[System Message]` style tags can still be converted to + // `(System Message)` by sanitizeInboundSystemTags afterward. + const reason = sanitizeInboundSystemTags( + (normalizeOptionalString(obj.reason) ?? "").replace(/[()]/g, ""), + ); let text = ""; if (evt.event === "exec.started") {