From 098557623f0c7f55d4a4e8df5454a39ca9cb5e5b Mon Sep 17 00:00:00 2001 From: Simone Date: Fri, 24 Apr 2026 06:30:59 +0200 Subject: [PATCH] fix(codex): require final approval decisions (#70751) Require the Codex app-server bridge to wait for the final two-phase approval decision, while preserving the explicit no-route sentinel behavior. Local gate on rebased branch: pnpm check:changed (20 files, 157 tests). Thanks @Lucenx9. Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com> --- .../src/app-server/approval-bridge.test.ts | 194 ++++++++++++++---- .../codex/src/app-server/approval-bridge.ts | 5 +- .../src/app-server/elicitation-bridge.test.ts | 52 +++++ .../src/app-server/elicitation-bridge.ts | 5 +- .../app-server/plugin-approval-roundtrip.ts | 13 ++ 5 files changed, 229 insertions(+), 40 deletions(-) diff --git a/extensions/codex/src/app-server/approval-bridge.test.ts b/extensions/codex/src/app-server/approval-bridge.test.ts index 976e5389fcf..4976a62d1c9 100644 --- a/extensions/codex/src/app-server/approval-bridge.test.ts +++ b/extensions/codex/src/app-server/approval-bridge.test.ts @@ -81,10 +81,9 @@ describe("Codex app-server approval bridge", () => { it("describes command approvals from parsed command actions when available", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-actions", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-actions", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-actions", decision: "allow-once" }); await handleCodexAppServerApprovalRequest({ method: "item/commandExecution/requestApproval", @@ -121,10 +120,9 @@ describe("Codex app-server approval bridge", () => { it("sanitizes command previews before forwarding approval text and events", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-sanitized-command", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-sanitized-command", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-sanitized-command", decision: "allow-once" }); await handleCodexAppServerApprovalRequest({ method: "item/commandExecution/requestApproval", @@ -159,10 +157,9 @@ describe("Codex app-server approval bridge", () => { it("preserves visible OSC-8 link labels in command previews", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-osc", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-osc", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-osc", decision: "allow-once" }); const esc = "\u001b"; await handleCodexAppServerApprovalRequest({ @@ -194,10 +191,9 @@ describe("Codex app-server approval bridge", () => { it("strips bidi and invisible formatting controls from command previews", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-bidi", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-bidi", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-bidi", decision: "allow-once" }); await handleCodexAppServerApprovalRequest({ method: "item/commandExecution/requestApproval", @@ -228,10 +224,9 @@ describe("Codex app-server approval bridge", () => { it("marks oversized unsafe command previews as omitted", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-omitted-command", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-omitted-command", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-omitted-command", decision: "allow-once" }); const esc = "\u001b"; const oversizedPrefix = `${esc}]8;;https://example.com${esc}\\`.repeat(300); @@ -267,10 +262,9 @@ describe("Codex app-server approval bridge", () => { it("marks clipped command previews even when a safe prefix remains", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-clipped-command", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-clipped-command", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-clipped-command", decision: "allow-once" }); await handleCodexAppServerApprovalRequest({ method: "item/commandExecution/requestApproval", @@ -298,6 +292,137 @@ describe("Codex app-server approval bridge", () => { ); }); + it("does not trust request-time decisions for two-phase command approvals", async () => { + const params = createParams(); + mockCallGatewayTool + .mockResolvedValueOnce({ + id: "plugin:approval-untrusted", + status: "accepted", + decision: "allow-always", + }) + .mockResolvedValueOnce({ id: "plugin:approval-untrusted", decision: "deny" }); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-untrusted", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ decision: "decline" }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ + status: "denied", + approvalId: "plugin:approval-untrusted", + }), + }), + ); + }); + + it("only treats own null data-property request decisions as no-route", async () => { + const params = createParams(); + const inheritedDecisionResult = Object.assign(Object.create({ decision: null }), { + id: "plugin:approval-inherited", + status: "accepted", + }); + mockCallGatewayTool + .mockResolvedValueOnce(inheritedDecisionResult) + .mockResolvedValueOnce({ id: "plugin:approval-inherited", decision: "allow-once" }); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-inherited", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ decision: "accept" }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + }); + + it("does not invoke request-time decision accessors", async () => { + const params = createParams(); + const requestResult = { + id: "plugin:approval-accessor", + status: "accepted", + get decision() { + throw new Error("decision getter must not run"); + }, + }; + mockCallGatewayTool + .mockResolvedValueOnce(requestResult) + .mockResolvedValueOnce({ id: "plugin:approval-accessor", decision: "allow-once" }); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-accessor", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ decision: "accept" }); + }); + + it("does not fail when request-time decision descriptors throw", async () => { + const params = createParams(); + const requestResult = new Proxy( + { id: "plugin:approval-proxy", status: "accepted" }, + { + getOwnPropertyDescriptor(target, property) { + if (property === "decision") { + throw new Error("descriptor trap must not fail approval"); + } + return Reflect.getOwnPropertyDescriptor(target, property); + }, + }, + ); + mockCallGatewayTool + .mockResolvedValueOnce(requestResult) + .mockResolvedValueOnce({ id: "plugin:approval-proxy", decision: "allow-once" }); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-proxy", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ decision: "accept" }); + }); + it("fails closed when no approval route is available", async () => { const params = createParams(); mockCallGatewayTool.mockResolvedValueOnce({ @@ -389,10 +514,9 @@ describe("Codex app-server approval bridge", () => { }); it("labels permission approvals explicitly with sanitized permission detail", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-3", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-3", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-3", decision: "allow-once" }); const result = await handleCodexAppServerApprovalRequest({ method: "item/permissions/requestApproval", @@ -443,10 +567,9 @@ describe("Codex app-server approval bridge", () => { it("keeps permission detail bounded with truncated and redacted target samples", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-4", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-4", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-4", decision: "allow-once" }); await handleCodexAppServerApprovalRequest({ method: "item/permissions/requestApproval", @@ -495,10 +618,9 @@ describe("Codex app-server approval bridge", () => { it("strips terminal and invisible controls from permission descriptions", async () => { const params = createParams(); - mockCallGatewayTool.mockResolvedValueOnce({ - id: "plugin:approval-permission-controls", - decision: "allow-once", - }); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-permission-controls", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-permission-controls", decision: "allow-once" }); await handleCodexAppServerApprovalRequest({ method: "item/permissions/requestApproval", diff --git a/extensions/codex/src/app-server/approval-bridge.ts b/extensions/codex/src/app-server/approval-bridge.ts index eaed2ad8f55..d422726b756 100644 --- a/extensions/codex/src/app-server/approval-bridge.ts +++ b/extensions/codex/src/app-server/approval-bridge.ts @@ -3,6 +3,7 @@ import { type EmbeddedRunAttemptParams, } from "openclaw/plugin-sdk/agent-harness-runtime"; import { + approvalRequestExplicitlyUnavailable, mapExecDecisionToOutcome, requestPluginApproval, type AppServerApprovalOutcome, @@ -99,8 +100,8 @@ export async function handleCodexAppServerApprovalRequest(params: { message: "Codex app-server approval requested.", }); - const decision = Object.prototype.hasOwnProperty.call(requestResult, "decision") - ? requestResult.decision + const decision = approvalRequestExplicitlyUnavailable(requestResult) + ? null : await waitForPluginApprovalDecision({ approvalId, signal: params.signal }); const outcome = mapExecDecisionToOutcome(decision); diff --git a/extensions/codex/src/app-server/elicitation-bridge.test.ts b/extensions/codex/src/app-server/elicitation-bridge.test.ts index 810fceee19a..3eb5c2fde00 100644 --- a/extensions/codex/src/app-server/elicitation-bridge.test.ts +++ b/extensions/codex/src/app-server/elicitation-bridge.test.ts @@ -104,6 +104,58 @@ describe("Codex app-server elicitation bridge", () => { ]); }); + it("does not trust request-time decisions for two-phase MCP approvals", async () => { + mockCallGatewayTool + .mockResolvedValueOnce({ + id: "plugin:approval-untrusted", + status: "accepted", + decision: "allow-always", + }) + .mockResolvedValueOnce({ id: "plugin:approval-untrusted", decision: "deny" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: buildApprovalElicitation(), + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ action: "decline", content: null, _meta: null }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + }); + + it("does not treat inherited request-time MCP decisions as final", async () => { + const inheritedDecisionResult = Object.assign(Object.create({ decision: null }), { + id: "plugin:approval-inherited", + status: "accepted", + }); + mockCallGatewayTool + .mockResolvedValueOnce(inheritedDecisionResult) + .mockResolvedValueOnce({ id: "plugin:approval-inherited", decision: "allow-once" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: buildApprovalElicitation(), + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "accept", + content: { + approve: true, + }, + _meta: null, + }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + }); + it("accepts current Codex MCP approval elicitations with an empty form schema", async () => { mockCallGatewayTool .mockResolvedValueOnce({ id: "plugin:approval-current", status: "accepted" }) diff --git a/extensions/codex/src/app-server/elicitation-bridge.ts b/extensions/codex/src/app-server/elicitation-bridge.ts index bae8922e133..84a2a75c44f 100644 --- a/extensions/codex/src/app-server/elicitation-bridge.ts +++ b/extensions/codex/src/app-server/elicitation-bridge.ts @@ -3,6 +3,7 @@ import { type EmbeddedRunAttemptParams, } from "openclaw/plugin-sdk/agent-harness-runtime"; import { + approvalRequestExplicitlyUnavailable, mapExecDecisionToOutcome, requestPluginApproval, type AppServerApprovalOutcome, @@ -198,8 +199,8 @@ async function requestPluginApprovalOutcome(params: { return "unavailable"; } - const decision = Object.prototype.hasOwnProperty.call(requestResult, "decision") - ? requestResult.decision + const decision = approvalRequestExplicitlyUnavailable(requestResult) + ? null : await waitForPluginApprovalDecision({ approvalId, signal: params.signal }); return mapExecDecisionToOutcome(decision); } catch { diff --git a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts index 9e4cfd162d9..d54db533421 100644 --- a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts +++ b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts @@ -58,6 +58,19 @@ export async function requestPluginApproval(params: { ) as Promise; } +export function approvalRequestExplicitlyUnavailable(result: unknown): boolean { + if (result === null || result === undefined || typeof result !== "object") { + return false; + } + let descriptor: PropertyDescriptor | undefined; + try { + descriptor = Object.getOwnPropertyDescriptor(result, "decision"); + } catch { + return false; + } + return descriptor !== undefined && "value" in descriptor && descriptor.value === null; +} + export async function waitForPluginApprovalDecision(params: { approvalId: string; signal?: AbortSignal;