From 7b2b86c60abd7699cd9a19a66cd5ac994cc9acc3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 24 Feb 2026 03:22:05 +0000 Subject: [PATCH] fix(exec): add approval race changelog and regressions --- CHANGELOG.md | 1 + .../bash-tools.exec-approval-request.test.ts | 49 +++++++++++++++ .../bash-tools.exec.approval-id.test.ts | 62 +++++++++++++++++++ 3 files changed, 112 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d95bdeba..9a732763f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Docs: https://docs.openclaw.ai - Security/Shell env fallback: remove trusted-prefix shell-path fallback and only trust login shells explicitly registered in `/etc/shells`, defaulting to `/bin/sh` when `SHELL` is not registered. This ships in the next npm release. Thanks @tdjackey for reporting. - Security/Exec approvals: bind `host=node` approvals to explicit `nodeId`, reject cross-node replay of approved `system.run` requests, and include the target node in approval prompts. This ships in the next npm release. Thanks @tdjackey for reporting. +- Security/Exec approvals: restore two-phase approval registration + wait-decision handling for gateway/node exec paths, requiring approval IDs to be registered before returning `approval-pending` and honoring server-assigned approval IDs during wait resolution to prevent orphaned `/approve` flows and immediate-return races (`ask:on-miss`). This ships in the next npm release. Thanks @vitalyis for reporting. - Security/Voice Call: harden Twilio webhook replay handling by preserving provider event IDs through normalization, adding bounded replay dedupe, and enforcing per-call turn-token matching for call-state transitions. This ships in the next npm release. Thanks @jiseoung for reporting. - Security/Export session HTML: escape raw HTML markdown tokens in the exported session viewer, harden tree/header metadata rendering against HTML injection, and sanitize image data-URL MIME types in export output to prevent stored XSS when opening exported HTML files. This ships in the next npm release. Thanks @allsmog for reporting. - Security/iOS deep links: require local confirmation (or trusted key) before forwarding `openclaw://agent` requests from iOS to gateway `agent.request`, and strip unkeyed delivery-routing fields to reduce exfiltration risk. This ships in the next npm release. Thanks @GCXWLP for reporting. diff --git a/src/agents/bash-tools.exec-approval-request.test.ts b/src/agents/bash-tools.exec-approval-request.test.ts index 1cd221e30..c14a3f62b 100644 --- a/src/agents/bash-tools.exec-approval-request.test.ts +++ b/src/agents/bash-tools.exec-approval-request.test.ts @@ -102,6 +102,55 @@ describe("requestExecApprovalDecision", () => { ).resolves.toBeNull(); }); + it("uses registration response id when waiting for decision", async () => { + vi.mocked(callGatewayTool) + .mockResolvedValueOnce({ + status: "accepted", + id: "server-assigned-id", + expiresAtMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }) + .mockResolvedValueOnce({ decision: "allow-once" }); + + await expect( + requestExecApprovalDecision({ + id: "client-id", + command: "echo hi", + cwd: "/tmp", + host: "gateway", + security: "allowlist", + ask: "on-miss", + }), + ).resolves.toBe("allow-once"); + + expect(callGatewayTool).toHaveBeenNthCalledWith( + 2, + "exec.approval.waitDecision", + { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, + { id: "server-assigned-id" }, + ); + }); + + it("treats expired-or-missing waitDecision as null decision", async () => { + vi.mocked(callGatewayTool) + .mockResolvedValueOnce({ + status: "accepted", + id: "approval-id", + expiresAtMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }) + .mockRejectedValueOnce(new Error("approval expired or not found")); + + await expect( + requestExecApprovalDecision({ + id: "approval-id", + command: "echo hi", + cwd: "/tmp", + host: "gateway", + security: "allowlist", + ask: "on-miss", + }), + ).resolves.toBeNull(); + }); + it("returns final decision directly when gateway already replies with decision", async () => { vi.mocked(callGatewayTool).mockResolvedValue({ decision: "deny", id: "approval-id" }); diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 37a1215e5..fc04efc0a 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -196,6 +196,68 @@ describe("exec approvals", () => { expect(calls).toContain("exec.approval.waitDecision"); }); + it("waits for approval registration before returning approval-pending", async () => { + const calls: string[] = []; + let resolveRegistration: ((value: unknown) => void) | undefined; + const registrationPromise = new Promise((resolve) => { + resolveRegistration = resolve; + }); + + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { + calls.push(method); + if (method === "exec.approval.request") { + return await registrationPromise; + } + if (method === "exec.approval.waitDecision") { + return { decision: "deny" }; + } + return { ok: true, id: (params as { id?: string })?.id }; + }); + + const tool = createExecTool({ + host: "gateway", + ask: "on-miss", + security: "allowlist", + approvalRunningNoticeMs: 0, + }); + + let settled = false; + const executePromise = tool.execute("call-registration-gate", { command: "echo register" }); + void executePromise.finally(() => { + settled = true; + }); + + await Promise.resolve(); + await Promise.resolve(); + expect(settled).toBe(false); + + resolveRegistration?.({ status: "accepted", id: "approval-id" }); + const result = await executePromise; + expect(result.details.status).toBe("approval-pending"); + expect(calls[0]).toBe("exec.approval.request"); + expect(calls).toContain("exec.approval.waitDecision"); + }); + + it("fails fast when approval registration fails", async () => { + vi.mocked(callGatewayTool).mockImplementation(async (method) => { + if (method === "exec.approval.request") { + throw new Error("gateway offline"); + } + return { ok: true }; + }); + + const tool = createExecTool({ + host: "gateway", + ask: "on-miss", + security: "allowlist", + approvalRunningNoticeMs: 0, + }); + + await expect(tool.execute("call-registration-fail", { command: "echo fail" })).rejects.toThrow( + "Exec approval registration failed", + ); + }); + it("denies node obfuscated command when approval request times out", async () => { vi.mocked(detectCommandObfuscation).mockReturnValue({ detected: true,