From 4e14978f8ceedeccdb78e93a9a06d091c0d15e34 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 9 Jul 2026 05:48:02 -0700 Subject: [PATCH] fix(security): fail closed on approval persistence errors --- CHANGELOG.md | 1 + .../bash-tools.exec-host-gateway.test.ts | 27 +++++++++++ src/agents/bash-tools.exec-host-gateway.ts | 45 +++++++++++++++---- src/infra/exec-approvals-config.test.ts | 4 +- src/infra/exec-approvals-store.test.ts | 26 +++++------ src/infra/exec-approvals.ts | 5 +-- 6 files changed, 82 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74d9908736e..701883b4c0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai - **Lean local model shell access:** keep `exec` directly visible beside the default structured Tool Search controls so coding-tuned local models can use their shell fallback instead of searching for missing domain tools. (#87587) Thanks @vincentkoc. - **OAuth refresh contention diagnostics:** keep local lock paths out of user-facing refresh failures and avoid duplicate failure prefixes while preserving structured provider and profile classification. (#83383) Thanks @vincentkoc. - **Exec approval prompts:** keep background-disabled fallback warnings out of pending gateway/node approvals and show them only after a command actually runs in the foreground. (#78184) Thanks @vincentkoc. +- **Exec approval revocation races:** serialize CLI, runtime, and macOS approval mutations against the current on-disk policy so stale metadata or settings snapshots cannot restore revoked execution permissions. (#78225, #78226) Thanks @coygeek. - **Direct poll delivery:** route direct and hybrid channel polls through the owning outbound adapter while preserving gateway-mode routing and channel option checks. (#99950) Thanks @NianJiuZst. - **Agent wait hard-timeout snapshots:** preserve canonical hard-timeout phase and timestamps when the outer `agent.wait` timer wins the retry-grace race, while leaving queue, draining, and restart-cancelled waits correctable. (#89367) Thanks @Pick-cat. - **Control UI typed approvals:** send `/approve` commands immediately through the authorized Gateway command path while an agent run is blocked instead of queueing the command behind that run. (#77672) Thanks @vincentkoc. diff --git a/src/agents/bash-tools.exec-host-gateway.test.ts b/src/agents/bash-tools.exec-host-gateway.test.ts index 9787ec50c93..61b9a143dc1 100644 --- a/src/agents/bash-tools.exec-host-gateway.test.ts +++ b/src/agents/bash-tools.exec-host-gateway.test.ts @@ -1471,6 +1471,33 @@ EOF`, expect(requireSentFollowupText(0)).toContain("done"); }); + it("fails closed when detached approval metadata cannot be persisted", async () => { + resolveApprovalDecisionOrUndefinedMock.mockResolvedValue("allow-once"); + createExecApprovalDecisionStateMock.mockReturnValue({ + baseDecision: { timedOut: false }, + approvedByAsk: true, + deniedReason: null, + }); + recordAllowlistMatchesUseMock.mockRejectedValueOnce(new Error("approval lock unavailable")); + buildExecApprovalFollowupTargetMock.mockImplementation((value) => value); + + const result = await runGatewayAllowlist({ command: "echo approved" }); + + expect(result.pendingResult?.details.status).toBe("approval-pending"); + await vi.waitFor(() => { + expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledTimes(1); + }); + expect(requireSentFollowupText(0)).toContain("approval-state-write-failed"); + expect(runExecProcessMock).not.toHaveBeenCalled(); + expect(emitTrustedSecurityEventMock).toHaveBeenCalledWith( + expect.objectContaining({ + action: "exec.approval.denied", + outcome: "error", + policy: expect.objectContaining({ reason: "approval-state-write-failed" }), + }), + ); + }); + it("waits inline for webchat approval so the exec tool can return real output to the model", async () => { resolveApprovalDecisionOrUndefinedMock.mockResolvedValue("allow-once"); createExecApprovalDecisionStateMock.mockReturnValue({ diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index dbc81f65a87..567035a4dab 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -925,15 +925,39 @@ export async function processGatewayAllowlist( turnSourceThreadId: params.turnSourceThreadId, direct: params.approvalFollowupMode === "direct", }); + const denyApprovalStateWriteFailure = async () => { + emitGatewayExecApprovalSecurityEvent({ + action: "exec.approval.denied", + outcome: "error", + severity: "high", + agentId: params.agentId, + reason: "approval-state-write-failed", + hostSecurity, + hostAsk, + host: "gateway", + segmentCount: allowlistEval.segments.length, + trigger: params.trigger, + }); + await sendExecApprovalFollowupResult( + followupTarget, + `Exec denied (gateway id=${approvalId}, approval-state-write-failed): ${params.command}`, + ); + }; void (async () => { - const approvalDecision = await resolveApprovalForExecution( - () => - void sendExecApprovalFollowupResult( - followupTarget, - `Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`, - ), - ); + let approvalDecision: Awaited>; + try { + approvalDecision = await resolveApprovalForExecution( + () => + void sendExecApprovalFollowupResult( + followupTarget, + `Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`, + ), + ); + } catch { + await denyApprovalStateWriteFailure(); + return; + } if (approvalDecision.requestFailed) { return; } @@ -946,7 +970,12 @@ export async function processGatewayAllowlist( return; } - await recordMatchedAllowlistUse(resolvedPath ?? undefined); + try { + await recordMatchedAllowlistUse(resolvedPath ?? undefined); + } catch { + await denyApprovalStateWriteFailure(); + return; + } let run: Awaited> | null; try { diff --git a/src/infra/exec-approvals-config.test.ts b/src/infra/exec-approvals-config.test.ts index 199995bc7c5..4f8033d1acc 100644 --- a/src/infra/exec-approvals-config.test.ts +++ b/src/infra/exec-approvals-config.test.ts @@ -16,7 +16,7 @@ import { } from "./exec-approvals.js"; describe("exec approvals wildcard agent", () => { - it("merges wildcard allowlist entries with agent entries", async () => { + it("merges wildcard allowlist entries with agent entries", () => { const dir = makeTempDir(); const prevOpenClawHome = process.env.OPENCLAW_HOME; @@ -39,7 +39,7 @@ describe("exec approvals wildcard agent", () => { ), ); - const resolved = await resolveExecApprovals("main"); + const resolved = resolveExecApprovals("main"); expect(resolved.allowlist.map((entry) => entry.pattern)).toEqual([ "/bin/hostname", "/usr/bin/uname", diff --git a/src/infra/exec-approvals-store.test.ts b/src/infra/exec-approvals-store.test.ts index 696967ba375..c4a887991ac 100644 --- a/src/infra/exec-approvals-store.test.ts +++ b/src/infra/exec-approvals-store.test.ts @@ -137,7 +137,7 @@ describe("exec approvals store helpers", () => { expect(resolveExecApprovalsDisplayPath()).toBe("~/.openclaw/exec-approvals.json"); }); - it("uses OPENCLAW_STATE_DIR for default file and socket paths", async () => { + it("uses OPENCLAW_STATE_DIR for default file and socket paths", () => { const dir = createHomeDir(); const stateDir = path.join(dir, "custom-state"); setTestEnvValue("OPENCLAW_STATE_DIR", stateDir); @@ -151,7 +151,7 @@ describe("exec approvals store helpers", () => { expect(resolveExecApprovalsDisplayPath()).toBe(stateApprovalsFilePath(stateDir)); expect(resolveExecApprovalsTranscriptPath()).toBe("$OPENCLAW_STATE_DIR/exec-approvals.json"); - const ensured = await ensureExecApprovals(); + const ensured = ensureExecApprovals(); expect(ensured.socket?.path).toBe(resolveExecApprovalsSocketPath()); expect(fs.existsSync(stateApprovalsFilePath(stateDir))).toBe(true); @@ -183,7 +183,7 @@ describe("exec approvals store helpers", () => { it("persists synchronous compatibility writes without restoring revoked policy", async () => { const dir = createHomeDir(); - await ensureExecApprovals(); + ensureExecApprovals(); await updateExecApprovals({ update: (file) => ({ ...file, @@ -320,7 +320,7 @@ describe("exec approvals store helpers", () => { expect(fs.existsSync(stateApprovalsFilePath(stateDir))).toBe(false); expect(fs.existsSync(approvalsFilePath(dir))).toBe(true); - const ensured = await ensureExecApprovals(); + const ensured = ensureExecApprovals(); expect(ensured.defaults).toEqual({ security: "deny", @@ -338,13 +338,13 @@ describe("exec approvals store helpers", () => { expect(fs.existsSync(stateApprovalsFilePath(stateDir))).toBe(false); }); - it("keeps the default approvals path when only legacy state exists", async () => { + it("keeps the default approvals path when only legacy state exists", () => { const dir = createHomeDir(); fs.mkdirSync(path.join(dir, ".clawdbot"), { recursive: true }); expect(path.normalize(resolveExecApprovalsPath())).toBe(path.normalize(approvalsFilePath(dir))); - await ensureExecApprovals(); + ensureExecApprovals(); expect(fs.existsSync(approvalsFilePath(dir))).toBe(true); expect(fs.existsSync(path.join(dir, ".clawdbot", "exec-approvals.json"))).toBe(false); @@ -405,10 +405,10 @@ describe("exec approvals store helpers", () => { expect(invalid.file).toEqual(normalizeExecApprovals({ version: 1, agents: {} })); }); - it("ensures approvals file with default socket path and generated token", async () => { + it("ensures approvals file with default socket path and generated token", () => { const dir = createHomeDir(); - const ensured = await ensureExecApprovals(); + const ensured = ensureExecApprovals(); const raw = fs.readFileSync(approvalsFilePath(dir), "utf8"); expect(ensured.socket?.path).toBe(resolveExecApprovalsSocketPath()); @@ -843,7 +843,7 @@ describe("exec approvals store helpers", () => { const dir = createHomeDir(); vi.spyOn(Date, "now").mockReturnValue(321_000); - await ensureExecApprovals(); + ensureExecApprovals(); await persistAllowAlwaysDecision({ agentId: "worker", decision: { @@ -864,7 +864,7 @@ describe("exec approvals store helpers", () => { it("applies allow-always grants to the latest file after a concurrent policy write", async () => { const dir = createHomeDir(); - await ensureExecApprovals(); + ensureExecApprovals(); const snapshot = readExecApprovalsSnapshot(); const policyWrite = updateExecApprovals({ @@ -912,7 +912,7 @@ describe("exec approvals store helpers", () => { const dir = createHomeDir(); vi.spyOn(Date, "now").mockReturnValue(321_000); - await ensureExecApprovals(); + ensureExecApprovals(); await persistAllowAlwaysDecision({ agentId: "worker", decision: { @@ -1070,7 +1070,7 @@ describe("exec approvals store helpers", () => { const dir = createHomeDir(); vi.spyOn(Date, "now").mockReturnValue(654_321); - await ensureExecApprovals(); + ensureExecApprovals(); const patterns = await persistAllowAlwaysPatterns({ agentId: "worker", platform: "win32", @@ -1114,7 +1114,7 @@ describe("exec approvals store helpers", () => { const dir = createHomeDir(); vi.spyOn(Date, "now").mockReturnValue(654_322); - await ensureExecApprovals(); + ensureExecApprovals(); const completePatterns = await persistAllowAlwaysPatterns({ agentId: "worker", commandText: "/usr/bin/tool ok", diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 1bb2eaa366b..d8a8792882f 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -1843,13 +1843,12 @@ function applyRecordedAllowlistUse(params: { return entry; } changed = true; - return { - ...entry, + return Object.assign({}, entry, { id: entry.id ?? crypto.randomUUID(), lastUsedAt: Date.now(), lastUsedCommand: params.command, lastResolvedPath: params.resolvedPath, - }; + }); }); return changed ? {