diff --git a/docs/.generated/plugin-sdk-api-baseline.sha256 b/docs/.generated/plugin-sdk-api-baseline.sha256 index d8d4ab046bbd..a46d6d49a0c6 100644 --- a/docs/.generated/plugin-sdk-api-baseline.sha256 +++ b/docs/.generated/plugin-sdk-api-baseline.sha256 @@ -1,2 +1,2 @@ -634a2cda48d53c9a234d59200b4666c105615da4d64612ea30facb42df9c4d93 plugin-sdk-api-baseline.json -f5bc2ec06dbdc26736f8d810d75f72f378ffd2111c2a143ed153a284b6d89d25 plugin-sdk-api-baseline.jsonl +d6747bc65237cf327f9f307e405347969292aee5f711ad41a1c8f6c5bc29e7ce plugin-sdk-api-baseline.json +678d8dddf40f8a0830ae2fe678a40af17a4dfbd550cfa41cc0a5e25f12ecfcb3 plugin-sdk-api-baseline.jsonl diff --git a/docs/plugins/hooks.md b/docs/plugins/hooks.md index 5bb9c74a207c..77d2b9999138 100644 --- a/docs/plugins/hooks.md +++ b/docs/plugins/hooks.md @@ -40,7 +40,6 @@ export default definePluginEntry({ description: `Allow search query: ${String(event.params.query ?? "")}`, severity: "info", timeoutMs: 60_000, - timeoutBehavior: "deny", }, }; }, @@ -251,6 +250,7 @@ type BeforeToolCallResult = { description: string; severity?: "info" | "warning" | "critical"; timeoutMs?: number; + /** @deprecated Unresolved approvals always deny. */ timeoutBehavior?: "allow" | "deny"; allowedDecisions?: Array<"allow-once" | "allow-always" | "deny">; pluginId?: string; diff --git a/docs/plugins/plugin-permission-requests.md b/docs/plugins/plugin-permission-requests.md index 58d9019a2f95..c7f8247eca47 100644 --- a/docs/plugins/plugin-permission-requests.md +++ b/docs/plugins/plugin-permission-requests.md @@ -63,7 +63,6 @@ export default definePluginEntry({ ? ["allow-once", "deny"] : ["allow-once", "allow-always", "deny"], timeoutMs: 120_000, - timeoutBehavior: "deny", onResolution(decision) { console.log(`deploy approval resolved: ${decision}`); }, @@ -99,10 +98,15 @@ available approval surfaces, and waits for a decision. | `allow-once` | The current call continues. | | `allow-always` | The current call continues and the decision is passed to the plugin. | | `deny` | The call is blocked with a denied tool result. | -| Timeout | The call is blocked unless `timeoutBehavior` is `"allow"`. | +| Timeout | The call is blocked. | | Cancellation | The call is blocked when the run is aborted. | | No approval route | The call is blocked because no connected approval surface can resolve it. | +Only the exact `allow-once` and `allow-always` decisions permitted by the +request allow execution. Unknown, malformed, mismatched, missing, and timed-out +decisions fail closed. The legacy `timeoutBehavior` field remains accepted for +plugin compatibility but is deprecated and ignored; do not set it in new hooks. + `allow-always` is only durable when the requesting plugin or runtime implements that persistence. For ordinary `before_tool_call.requireApproval` hooks, OpenClaw treats `allow-once` and `allow-always` as approval decisions for the diff --git a/docs/refactor/operator-approvals.md b/docs/refactor/operator-approvals.md index b4fa92e07454..48859b2256f3 100644 --- a/docs/refactor/operator-approvals.md +++ b/docs/refactor/operator-approvals.md @@ -335,13 +335,17 @@ Final strict behavior: - malformed trusted verdict -> `denied`, deny; - only an allowed explicit allow decision -> `allowed`. -Current shipped behavior conflicts with this contract: +Current shipped exec behavior still conflicts with this contract: - `src/agents/bash-tools.exec-host-shared.ts` may apply `askFallback`. -- `src/agents/agent-tools.before-tool-call.ts` may honor `timeoutBehavior: "allow"`. -- `docs/tools/exec-approvals.md`, `docs/cli/approvals.md`, and `docs/plugins/plugin-permission-requests.md` document those surfaces. +- `docs/tools/exec-approvals.md` and `docs/cli/approvals.md` document that surface. -Do not silently change them in the storage PR. The strict-semantics PR must update code, types, docs, tests, and changelog together, with explicit owner/security review. `askFallback` may continue to describe pre-gate policy selection during migration, but it must not turn a created pending record's timeout into approval. +Plugin approvals now fail closed on timeout and malformed verdicts; the legacy +`timeoutBehavior` field remains accepted but ignored. The exec strict-semantics +follow-up must update code, types, docs, tests, and changelog together, with +explicit owner/security review. `askFallback` may continue to describe +pre-gate policy selection during migration, but it must not turn a created +pending record's timeout into approval. ## Compatibility plan @@ -460,5 +464,5 @@ A committed transition is success even if later event delivery fails. Lifecycle ## Open decisions 1. **Externally reachable Control UI origin.** Every snapshot carries the stable relative `urlPath`. An absolute URL may be advertised only from a cached Tailscale Serve/Funnel location after Gateway exposure succeeds; `allowedOrigins`, request Host headers, `gateway.remote.url`, and display-only loopback/LAN candidates are not canonical origins. Telegram can use its authenticated Mini App wrapper to retain the approval path through bootstrap. Arbitrary reverse proxies remain relative-only until a separately reviewed explicit public-URL contract exists. Never let a channel guess the origin. -2. **Strict timeout compatibility cutover.** The target is fail-closed, but `askFallback` and plugin `timeoutBehavior: "allow"` are shipped contracts. Recommended: make the behavior change in PR 6 with explicit owner/security approval, changelog, docs, and a migration/deprecation decision rather than hiding it in PR 1. +2. **Exec strict timeout compatibility cutover.** Plugin approval timeouts now fail closed and `timeoutBehavior` is deprecated. The remaining shipped `askFallback` contract needs explicit owner/security review, changelog, docs, and a migration/deprecation decision before it stops authorizing execution after a pending ask times out. 3. **Gatewayless embedded mode.** Recommended: keep it local-only initially, then make it a client of the canonical service when a Gateway exists. Do not advertise a deep link that no server can resolve. diff --git a/extensions/codex/src/app-server/approval-bridge.test.ts b/extensions/codex/src/app-server/approval-bridge.test.ts index c817a08e946b..e7d5f9d6b4ed 100644 --- a/extensions/codex/src/app-server/approval-bridge.test.ts +++ b/extensions/codex/src/app-server/approval-bridge.test.ts @@ -2248,6 +2248,36 @@ describe("Codex app-server approval bridge", () => { }); }); + it("ignores waitDecision replies bound to a different approval id", async () => { + const params = createParams(); + const onNativeToolFailureDisposition = vi.fn(); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-mismatch", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-other", decision: "allow-once" }); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-mismatch", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + onNativeToolFailureDisposition, + }); + + // A misrouted allow for another approval must not release this gate. + expect(result).toEqual({ decision: "decline" }); + expect(onNativeToolFailureDisposition).toHaveBeenCalledWith("cmd-mismatch", "failed"); + findApprovalEvent(params, { + status: "unavailable", + approvalId: "plugin:approval-mismatch", + }); + }); + it("sanitizes reason previews before forwarding approval text and events", async () => { const params = createParams(); mockCallGatewayTool.mockResolvedValueOnce({ diff --git a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts index cc5bea9bdc25..827f330fa9c2 100644 --- a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts +++ b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts @@ -93,8 +93,14 @@ export async function waitForPluginApprovalDecision(params: { { timeoutMs: resolveCodexGatewayTimeoutWithGraceMs(timeoutMs) }, { id: params.approvalId }, ); + // Bind the verdict to the approval that parked this prompt. A stale or + // misrouted reply maps to "unavailable" instead of releasing another gate. + const bindDecision = ( + result: ApprovalWaitResult | undefined, + ): ExecApprovalDecision | null | undefined => + result?.id === params.approvalId ? result.decision : undefined; if (!params.signal) { - return (await waitPromise)?.decision; + return bindDecision(await waitPromise); } let onAbort: (() => void) | undefined; const abortPromise = new Promise((_, reject) => { @@ -106,7 +112,7 @@ export async function waitForPluginApprovalDecision(params: { params.signal!.addEventListener("abort", onAbort, { once: true }); }); try { - return (await Promise.race([waitPromise, abortPromise]))?.decision; + return bindDecision(await Promise.race([waitPromise, abortPromise])); } finally { if (onAbort) { params.signal.removeEventListener("abort", onAbort); diff --git a/extensions/file-transfer/src/shared/node-invoke-policy.test.ts b/extensions/file-transfer/src/shared/node-invoke-policy.test.ts index 9b62c1095732..1c0aa7b0e323 100644 --- a/extensions/file-transfer/src/shared/node-invoke-policy.test.ts +++ b/extensions/file-transfer/src/shared/node-invoke-policy.test.ts @@ -270,11 +270,95 @@ describe("file-transfer node invoke policy", () => { expect(invokeNode).not.toHaveBeenCalled(); }); - it("uses plugin approvals for ask-on-miss before invoking the node", async () => { + it.each(["allow-once", "allow-always"] as const)( + "uses exact %s plugin approval once across preflight and final invoke", + async (decision) => { + const policy = createFileTransferNodeInvokePolicy(); + const approvals = { + request: vi.fn(async () => ({ id: "approval-1", decision })), + }; + const { ctx, invokeNode } = createCtx({ + params: { path: "/tmp/new.txt" }, + pluginConfig: { + nodes: { + "node-1": { + ask: "on-miss", + allowReadPaths: ["/allowed/**"], + maxBytes: 256, + }, + }, + }, + approvals, + }); + + const result = await policy.handle(ctx); + + expect(result.ok).toBe(true); + expect(approvals.request).toHaveBeenCalledTimes(1); + expect(invokeNode).toHaveBeenCalledTimes(2); + const approvalCalls = approvals.request.mock.calls as unknown[][]; + const approvalRequest = requireRecord(approvalCalls[0]?.[0], "approval request"); + expectRecordFields(approvalRequest, { + title: "Read file: /tmp/new.txt", + severity: "info", + toolName: "file.fetch", + }); + expect(invokeNode).toHaveBeenNthCalledWith(1, { + params: { + path: "/tmp/new.txt", + followSymlinks: false, + maxBytes: 256, + preflightOnly: true, + }, + }); + expect(invokeNode).toHaveBeenNthCalledWith(2, { + params: { + path: "/tmp/new.txt", + followSymlinks: false, + maxBytes: 256, + }, + }); + }, + ); + + it.each([ + { + label: "explicit deny", + decision: "deny", + code: "APPROVAL_DENIED", + message: "file.fetch APPROVAL_DENIED: operator denied the prompt", + }, + { + label: "null decision", + decision: null, + code: "APPROVAL_UNAVAILABLE", + message: + "file.fetch APPROVAL_UNAVAILABLE: no operator client connected to approve the request", + }, + { + label: "undefined decision", + decision: undefined, + code: "APPROVAL_UNAVAILABLE", + message: + "file.fetch APPROVAL_UNAVAILABLE: no operator client connected to approve the request", + }, + { + label: "arbitrary truthy string", + decision: "accept", + code: "APPROVAL_DENIED", + message: "file.fetch APPROVAL_DENIED: invalid approval decision", + }, + { + label: "arbitrary truthy object", + decision: { action: "accept" }, + code: "APPROVAL_DENIED", + message: "file.fetch APPROVAL_DENIED: invalid approval decision", + }, + ])("fails closed for $label", async ({ decision, code, message }) => { const policy = createFileTransferNodeInvokePolicy(); const approvals = { - request: vi.fn(async () => ({ id: "approval-1", decision: "allow-once" as const })), - }; + request: vi.fn(async () => ({ id: "approval-1", decision })), + } as unknown as NonNullable; const { ctx, invokeNode } = createCtx({ params: { path: "/tmp/new.txt" }, pluginConfig: { @@ -282,7 +366,6 @@ describe("file-transfer node invoke policy", () => { "node-1": { ask: "on-miss", allowReadPaths: ["/allowed/**"], - maxBytes: 256, }, }, }, @@ -291,29 +374,9 @@ describe("file-transfer node invoke policy", () => { const result = await policy.handle(ctx); - expect(result.ok).toBe(true); - const approvalCalls = approvals.request.mock.calls as unknown[][]; - const approvalRequest = requireRecord(approvalCalls[0]?.[0], "approval request"); - expectRecordFields(approvalRequest, { - title: "Read file: /tmp/new.txt", - severity: "info", - toolName: "file.fetch", - }); - expect(invokeNode).toHaveBeenNthCalledWith(1, { - params: { - path: "/tmp/new.txt", - followSymlinks: false, - maxBytes: 256, - preflightOnly: true, - }, - }); - expect(invokeNode).toHaveBeenNthCalledWith(2, { - params: { - path: "/tmp/new.txt", - followSymlinks: false, - maxBytes: 256, - }, - }); + expectResultFields(result, { ok: false, code, message }); + expect(approvals.request).toHaveBeenCalledTimes(1); + expect(invokeNode).not.toHaveBeenCalled(); }); it("marks node transport failures as unavailable", async () => { diff --git a/extensions/file-transfer/src/shared/node-invoke-policy.ts b/extensions/file-transfer/src/shared/node-invoke-policy.ts index 0089206aa8e6..6a4e31b76581 100644 --- a/extensions/file-transfer/src/shared/node-invoke-policy.ts +++ b/extensions/file-transfer/src/shared/node-invoke-policy.ts @@ -154,28 +154,37 @@ async function requestApproval(input: { severity: input.kind === "write" ? "warning" : "info", toolName: input.op, }); + const approvalDecision: unknown = approval.decision; - if (approval.decision === "deny" || approval.decision === null || !approval.decision) { + if (approvalDecision !== "allow-once" && approvalDecision !== "allow-always") { + const unavailable = approvalDecision === null || approvalDecision === undefined; + const deniedByOperator = approvalDecision === "deny"; + const reason = deniedByOperator + ? "operator denied" + : unavailable + ? "no operator available" + : "invalid approval decision"; await appendFileTransferAudit({ op: input.op, nodeId: input.ctx.nodeId, nodeDisplayName, requestedPath: input.path, decision: "denied:approval", - reason: approval.decision === "deny" ? "operator denied" : "no operator available", + reason, durationMs: Date.now() - input.startedAt, }); return { ok: false, - code: approval.decision === "deny" ? "APPROVAL_DENIED" : "APPROVAL_UNAVAILABLE", - message: - approval.decision === "deny" + code: unavailable ? "APPROVAL_UNAVAILABLE" : "APPROVAL_DENIED", + message: unavailable + ? `${input.op} APPROVAL_UNAVAILABLE: no operator client connected to approve the request` + : deniedByOperator ? `${input.op} APPROVAL_DENIED: operator denied the prompt` - : `${input.op} APPROVAL_UNAVAILABLE: no operator client connected to approve the request`, + : `${input.op} APPROVAL_DENIED: invalid approval decision`, }; } - if (approval.decision === "allow-always") { + if (approvalDecision === "allow-always") { try { await persistAllowAlways({ nodeId: input.ctx.nodeId, @@ -228,7 +237,7 @@ async function requestApproval(input: { nodeId: input.ctx.nodeId, nodeDisplayName, requestedPath: input.path, - decision: approval.decision === "allow-always" ? "allowed:always" : "allowed:once", + decision: approvalDecision === "allow-always" ? "allowed:always" : "allowed:once", durationMs: Date.now() - input.startedAt, }); return { diff --git a/src/agents/agent-tools.before-tool-call.e2e.test.ts b/src/agents/agent-tools.before-tool-call.e2e.test.ts index 6321e59f69cb..a88642ba0a42 100644 --- a/src/agents/agent-tools.before-tool-call.e2e.test.ts +++ b/src/agents/agent-tools.before-tool-call.e2e.test.ts @@ -19,6 +19,7 @@ import { } from "../infra/diagnostic-events.js"; import { MAX_PLUGIN_APPROVAL_TIMEOUT_MS } from "../infra/plugin-approvals.js"; import { resetDiagnosticSessionStateForTest } from "../logging/diagnostic-session-state.js"; +import { PluginApprovalResolutions } from "../plugins/hook-before-tool-call-result.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { createEmptyPluginRegistry } from "../plugins/registry-empty.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; @@ -2037,18 +2038,25 @@ describe("before_tool_call requireApproval handling", () => { ); }); - it("allows on timeout when timeoutBehavior is allow and preserves hook params", async () => { + it.each([ + ["a timeout", null], + ["an explicit timeout decision", PluginApprovalResolutions.TIMEOUT], + ["an unknown decision", "approved"], + ["a malformed truthy decision", true as unknown as string], + ])("blocks on %s even when deprecated timeoutBehavior is allow", async (_label, decision) => { + const onResolution = vi.fn(); hookRunner.runBeforeToolCall.mockResolvedValue({ params: { command: "safe-command" }, requireApproval: { title: "Lenient timeout", - description: "Should allow on timeout", + description: "Must fail closed", timeoutBehavior: "allow", + onResolution, }, }); mockCallGateway.mockResolvedValueOnce({ id: "server-id-4", status: "accepted" }); - mockCallGateway.mockResolvedValueOnce({ id: "server-id-4", decision: null }); + mockCallGateway.mockResolvedValueOnce({ id: "server-id-4", decision }); const result = await runBeforeToolCallHook({ toolName: "bash", @@ -2056,10 +2064,78 @@ describe("before_tool_call requireApproval handling", () => { ctx: { agentId: "main", sessionKey: "main" }, }); - expect(result.blocked).toBe(false); - if (!result.blocked) { - expect(result.params).toEqual({ command: "safe-command" }); - } + expect(result).toMatchObject({ + blocked: true, + kind: "failure", + disposition: "timed_out", + deniedReason: "plugin-approval", + reason: "Approval timed out", + params: { command: "rm -rf /" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + }); + + it("blocks exact allow decisions excluded by the request", async () => { + const onResolution = vi.fn(); + hookRunner.runBeforeToolCall.mockResolvedValue({ + params: { command: "safe-command" }, + requireApproval: { + title: "Restricted approval", + description: "Allow once only", + allowedDecisions: ["allow-once", "deny"], + onResolution, + }, + }); + mockCallGateway.mockResolvedValueOnce({ id: "server-id-restricted", status: "accepted" }); + mockCallGateway.mockResolvedValueOnce({ + id: "server-id-restricted", + decision: "allow-always", + }); + + const result = await runBeforeToolCallHook({ + toolName: "bash", + params: { command: "unsafe-command" }, + ctx: { agentId: "main", sessionKey: "main" }, + }); + + expect(result).toMatchObject({ + blocked: true, + disposition: "timed_out", + reason: "Approval timed out", + params: { command: "unsafe-command" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + }); + + it("blocks a wait decision bound to another approval id", async () => { + const onResolution = vi.fn(); + hookRunner.runBeforeToolCall.mockResolvedValue({ + params: { command: "safe-command" }, + requireApproval: { + title: "Bound approval", + description: "Must match the request id", + onResolution, + }, + }); + mockCallGateway.mockResolvedValueOnce({ id: "server-id-bound", status: "accepted" }); + mockCallGateway.mockResolvedValueOnce({ + id: "server-id-other", + decision: "allow-once", + }); + + const result = await runBeforeToolCallHook({ + toolName: "bash", + params: { command: "unsafe-command" }, + ctx: { agentId: "main", sessionKey: "main" }, + }); + + expect(result).toMatchObject({ + blocked: true, + disposition: "timed_out", + reason: "Approval timed out", + params: { command: "unsafe-command" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); }); it("falls back to block on gateway error", async () => { diff --git a/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts b/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts index 58450c9fd194..6d5c55056c88 100644 --- a/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts +++ b/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts @@ -39,6 +39,19 @@ vi.mock("./tools/gateway.js", () => ({ callGatewayTool: vi.fn(), })); +const agentToolsWarnSpy = vi.hoisted(() => vi.fn()); +vi.mock("../logging/subsystem.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + createSubsystemLogger: (subsystem: string) => { + const logger = actual.createSubsystemLogger(subsystem); + // Capture agents/tools warnings so the deprecation signal is assertable. + return subsystem === "agents/tools" ? { ...logger, warn: agentToolsWarnSpy } : logger; + }, + }; +}); + const mockGetGlobalHookRunner = vi.mocked(getGlobalHookRunner); const mockCallGatewayTool = vi.mocked(callGatewayTool); @@ -220,6 +233,120 @@ describe("runBeforeToolCallHook — embedded mode approvals", () => { expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.CANCELLED); }); + it("blocks embedded approvals on timeout even when deprecated timeoutBehavior is allow", async () => { + setEmbeddedMode(true); + const broker = new EmbeddedPluginApprovalBroker(); + setEmbeddedPluginApprovalBroker(broker); + const onResolution = vi.fn(); + runBeforeToolCallMock.mockResolvedValue({ + requireApproval: { + pluginId: "test-plugin", + title: "Needs approval", + description: "Test approval request", + timeoutMs: 1, + timeoutBehavior: "allow", + onResolution, + }, + params: { adjusted: true }, + }); + + const result = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "ls" }, + toolCallId: "call-skill-timeout", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + + expect(result).toEqual({ + blocked: true, + kind: "failure", + disposition: "timed_out", + deniedReason: "plugin-approval", + reason: "Approval timed out", + params: { command: "ls" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + expect(mockCallGatewayTool).not.toHaveBeenCalled(); + }); + + it("warns once per plugin when deprecated timeoutBehavior allow arrives, still failing closed", async () => { + agentToolsWarnSpy.mockClear(); + setEmbeddedMode(true); + const broker = new EmbeddedPluginApprovalBroker(); + setEmbeddedPluginApprovalBroker(broker); + runBeforeToolCallMock.mockResolvedValue({ + requireApproval: { + pluginId: "deprecated-timeout-plugin", + title: "Needs approval", + description: "Test approval request", + timeoutMs: 1, + timeoutBehavior: "allow", + }, + }); + + const first = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "ls" }, + toolCallId: "call-deprecated-warn-1", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + const second = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "ls" }, + toolCallId: "call-deprecated-warn-2", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + + expect(first).toMatchObject({ blocked: true, disposition: "timed_out" }); + expect(second).toMatchObject({ blocked: true, disposition: "timed_out" }); + const deprecationWarnings = agentToolsWarnSpy.mock.calls.filter( + ([message]) => + typeof message === "string" && + message.includes("deprecated-timeout-plugin") && + message.includes("timeoutBehavior"), + ); + expect(deprecationWarnings).toHaveLength(1); + }); + + it("blocks embedded allow decisions excluded by the request", async () => { + setEmbeddedMode(true); + const broker = new EmbeddedPluginApprovalBroker(); + setEmbeddedPluginApprovalBroker(broker); + vi.spyOn(broker, "request").mockResolvedValue({ + id: "plugin:unexpected-decision", + decision: PluginApprovalResolutions.ALLOW_ALWAYS, + }); + const onResolution = vi.fn(); + runBeforeToolCallMock.mockResolvedValue({ + requireApproval: { + pluginId: "test-plugin", + title: "Restricted approval", + description: "Allow once only", + allowedDecisions: ["allow-once", "deny"], + onResolution, + }, + params: { adjusted: true }, + }); + + const result = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "unsafe-command" }, + toolCallId: "call-restricted-approval", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + + expect(result).toEqual({ + blocked: true, + kind: "failure", + disposition: "timed_out", + deniedReason: "plugin-approval", + reason: "Approval timed out", + params: { command: "unsafe-command" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + expect(mockCallGatewayTool).not.toHaveBeenCalled(); + }); + it("reports approval-required tools without opening an approval request", async () => { runBeforeToolCallMock.mockResolvedValue({ requireApproval: { diff --git a/src/agents/agent-tools.before-tool-call.ts b/src/agents/agent-tools.before-tool-call.ts index fbf19e3d67cd..081e65d41713 100644 --- a/src/agents/agent-tools.before-tool-call.ts +++ b/src/agents/agent-tools.before-tool-call.ts @@ -40,6 +40,7 @@ import { describeNativePluginApprovalClientSetup, resolveApprovalInitiatingSurfaceState, } from "../infra/exec-approval-surface.js"; +import { resolveCanonicalPluginApprovalRequestAllowedDecisions } from "../infra/plugin-approval-canonical-decisions.js"; import { DEFAULT_PLUGIN_APPROVAL_TIMEOUT_MS, MAX_PLUGIN_APPROVAL_TIMEOUT_MS, @@ -828,6 +829,24 @@ function emitToolBlockedSecurityEvent(params: { }); } +// Once-per-plugin-per-process deprecation signal; the field is ignored at +// runtime because unresolved approvals always fail closed on timeout. +const warnedDeprecatedTimeoutBehaviorPluginIds = new Set(); + +function warnDeprecatedApprovalTimeoutBehavior(approval: PluginApprovalRequest): void { + if (approval.timeoutBehavior !== "allow") { + return; + } + const pluginId = approval.pluginId ?? "unknown-plugin"; + if (warnedDeprecatedTimeoutBehaviorPluginIds.has(pluginId)) { + return; + } + warnedDeprecatedTimeoutBehaviorPluginIds.add(pluginId); + log.warn( + `plugin '${pluginId}' sets deprecated requireApproval.timeoutBehavior:"allow"; the field is ignored and approvals fail closed on timeout (see docs/plugins/plugin-permission-requests.md)`, + ); +} + function notifyPluginApprovalResolution( approval: PluginApprovalRequest, resolution: PluginApprovalResolution, @@ -845,6 +864,21 @@ function notifyPluginApprovalResolution( } } +function resolvePermittedPluginApprovalResolution( + decision: unknown, + allowedDecisions: readonly string[], +): PluginApprovalResolution { + if ( + (decision === PluginApprovalResolutions.ALLOW_ONCE || + decision === PluginApprovalResolutions.ALLOW_ALWAYS || + decision === PluginApprovalResolutions.DENY) && + allowedDecisions.includes(decision) + ) { + return decision; + } + return PluginApprovalResolutions.TIMEOUT; +} + function buildPluginApprovalFailureReason(params: { fallbackReason: string; ctx?: HookContext; @@ -894,6 +928,7 @@ async function requestPluginToolApproval(params: { const approval = params.approval; const timeoutMs = resolvePluginToolApprovalTimeoutMs(approval); const gatewayTimeoutMs = resolvePluginToolApprovalGatewayTimeoutMs(timeoutMs); + const allowedDecisions = resolveCanonicalPluginApprovalRequestAllowedDecisions(approval); let gatewayApprovalPhase: "none" | "request" | "wait" = "none"; try { const embeddedApprovalBroker = isEmbeddedMode() ? getEmbeddedPluginApprovalBroker() : null; @@ -918,16 +953,11 @@ async function requestPluginToolApproval(params: { signal: params.signal, }); const decision = result.decision; - const resolution: PluginApprovalResolution = - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS || - decision === PluginApprovalResolutions.DENY - ? decision - : PluginApprovalResolutions.TIMEOUT; + const resolution = resolvePermittedPluginApprovalResolution(decision, allowedDecisions); notifyPluginApprovalResolution(approval, resolution); if ( - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS + resolution === PluginApprovalResolutions.ALLOW_ONCE || + resolution === PluginApprovalResolutions.ALLOW_ALWAYS ) { return { blocked: false, @@ -935,7 +965,7 @@ async function requestPluginToolApproval(params: { approvalResolution: resolution, }; } - if (decision === PluginApprovalResolutions.DENY) { + if (resolution === PluginApprovalResolutions.DENY) { return { blocked: true, kind: "failure", @@ -945,13 +975,6 @@ async function requestPluginToolApproval(params: { params: params.baseParams, }; } - if (approval.timeoutBehavior === "allow") { - return { - blocked: false, - params: mergeParamsWithApprovalOverrides(params.baseParams, params.overrideParams), - approvalResolution: resolution, - }; - } // Veto carries the plugin-supplied reason; plain timeouts record a // timed_out failure disposition for the audit ledger. return approval.timeoutReason @@ -976,7 +999,7 @@ async function requestPluginToolApproval(params: { const requestResult: { id?: string; status?: string; - decision?: string | null; + decision?: unknown; deliveryRoute?: string; } = await callGatewayTool( "plugin.approval.request", @@ -1019,7 +1042,7 @@ async function requestPluginToolApproval(params: { }; } const hasImmediateDecision = Object.hasOwn(requestResult ?? {}, "decision"); - let decision: string | null | undefined; + let decision: unknown; if (hasImmediateDecision) { decision = requestResult?.decision; if (decision === null) { @@ -1042,7 +1065,7 @@ async function requestPluginToolApproval(params: { gatewayApprovalPhase = "wait"; const waitPromise: Promise<{ id?: string; - decision?: string | null; + decision?: unknown; }> = callGatewayTool( "plugin.approval.waitDecision", // Buffer beyond the approval timeout so the gateway can clean up @@ -1050,7 +1073,7 @@ async function requestPluginToolApproval(params: { { timeoutMs: gatewayTimeoutMs }, { id }, ); - let waitResult: { id?: string; decision?: string | null } | undefined; + let waitResult: { id?: string; decision?: unknown } | undefined; if (params.signal) { let onAbort: (() => void) | undefined; const abortPromise = new Promise((_, reject) => { @@ -1071,18 +1094,15 @@ async function requestPluginToolApproval(params: { } else { waitResult = await waitPromise; } - decision = waitResult?.decision; + // Bind the verdict to the request that parked this call. A stale or + // misrouted reply must never release a different tool gate. + decision = waitResult?.id === id ? waitResult.decision : undefined; } - const resolution: PluginApprovalResolution = - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS || - decision === PluginApprovalResolutions.DENY - ? decision - : PluginApprovalResolutions.TIMEOUT; + const resolution = resolvePermittedPluginApprovalResolution(decision, allowedDecisions); notifyPluginApprovalResolution(approval, resolution); if ( - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS + resolution === PluginApprovalResolutions.ALLOW_ONCE || + resolution === PluginApprovalResolutions.ALLOW_ALWAYS ) { return { blocked: false, @@ -1090,7 +1110,7 @@ async function requestPluginToolApproval(params: { approvalResolution: resolution, }; } - if (decision === PluginApprovalResolutions.DENY) { + if (resolution === PluginApprovalResolutions.DENY) { return { blocked: true, kind: "failure", @@ -1100,14 +1120,6 @@ async function requestPluginToolApproval(params: { params: params.baseParams, }; } - const timeoutBehavior = approval.timeoutBehavior ?? "deny"; - if (timeoutBehavior === "allow") { - return { - blocked: false, - params: mergeParamsWithApprovalOverrides(params.baseParams, params.overrideParams), - approvalResolution: resolution, - }; - } const fallbackTimeoutReason = approval.timeoutReason ?? "Approval timed out"; const timeoutReason = requestResult?.deliveryRoute === "turn-source" @@ -1201,6 +1213,7 @@ async function resolveBeforeToolCallApprovalOutcome(params: { if (!approval) { return undefined; } + warnDeprecatedApprovalTimeoutBehavior(approval); if (params.approvalMode === "defer") { return { blocked: false, diff --git a/src/agents/harness/native-hook-relay.approval-binding.test.ts b/src/agents/harness/native-hook-relay.approval-binding.test.ts new file mode 100644 index 000000000000..217fd2b2586d --- /dev/null +++ b/src/agents/harness/native-hook-relay.approval-binding.test.ts @@ -0,0 +1,86 @@ +// Covers gateway waitDecision id binding for native hook relay permission approvals. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { callGatewayTool } from "../tools/gateway.js"; +import { invokeNativeHookRelay, registerNativeHookRelay, testing } from "./native-hook-relay.js"; + +vi.mock("../tools/gateway.js", async (importOriginal) => ({ + ...(await importOriginal()), + callGatewayTool: vi.fn(), +})); + +const mockCallGatewayTool = vi.mocked(callGatewayTool); + +afterEach(() => { + // restoreAllMocks does not clear call history on module-mock vi.fn()s. + mockCallGatewayTool.mockReset(); + vi.restoreAllMocks(); + testing.clearNativeHookRelaysForTests(); +}); + +function mockGatewayApproval(waitResult: { id?: string; decision?: string | null }) { + mockCallGatewayTool.mockImplementation(async (method: string) => { + if (method === "plugin.approval.request") { + return { id: "approval-1", status: "accepted" }; + } + if (method === "plugin.approval.waitDecision") { + return waitResult; + } + throw new Error(`unexpected gateway method: ${method}`); + }); +} + +async function invokePermissionRequest(relayId: string) { + return invokeNativeHookRelay({ + provider: "codex", + relayId, + event: "permission_request", + rawPayload: { + hook_event_name: "PermissionRequest", + cwd: "/repo", + tool_name: "Bash", + tool_use_id: "native-binding-call-1", + tool_input: { command: "printf binding" }, + }, + }); +} + +describe("native hook relay approval id binding", () => { + it("accepts a waitDecision reply bound to the requested approval id", async () => { + mockGatewayApproval({ id: "approval-1", decision: "allow-once" }); + const relay = registerNativeHookRelay({ + provider: "codex", + relayId: "codex-approval-binding-match", + sessionId: "session-1", + runId: "run-1", + }); + + const response = await invokePermissionRequest(relay.relayId); + + expect(JSON.parse(response.stdout)).toEqual({ + hookSpecificOutput: { + hookEventName: "PermissionRequest", + decision: { behavior: "allow" }, + }, + }); + }); + + it("defers when a waitDecision reply carries a different approval id", async () => { + mockGatewayApproval({ id: "approval-other", decision: "allow-once" }); + const relay = registerNativeHookRelay({ + provider: "codex", + relayId: "codex-approval-binding-mismatch", + sessionId: "session-1", + runId: "run-1", + }); + + const response = await invokePermissionRequest(relay.relayId); + + // A misrouted reply must never release the gate; the relay falls back to + // the provider's own approval path via the noop response. + expect(response).toEqual({ stdout: "", stderr: "", exitCode: 0 }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + }); +}); diff --git a/src/agents/harness/native-hook-relay.ts b/src/agents/harness/native-hook-relay.ts index fce3379bd243..121854c65001 100644 --- a/src/agents/harness/native-hook-relay.ts +++ b/src/agents/harness/native-hook-relay.ts @@ -2132,7 +2132,9 @@ async function requestNativeHookRelayPermissionApproval( signal: request.signal, timeoutMs, }); - decision = waitResult?.decision; + // Bind the verdict to the request that parked this call. A stale or + // misrouted reply must never release a different tool gate. + decision = waitResult?.id === approvalId ? waitResult.decision : undefined; } if (decision === PluginApprovalResolutions.ALLOW_ONCE) { return "allow"; diff --git a/src/gateway/node-invoke-plugin-policy.test.ts b/src/gateway/node-invoke-plugin-policy.test.ts index e61a5f546c08..97c849bfb8f6 100644 --- a/src/gateway/node-invoke-plugin-policy.test.ts +++ b/src/gateway/node-invoke-plugin-policy.test.ts @@ -217,6 +217,8 @@ async function expectApprovalResolution( ok: true, payload: { id: record.id, decision: "allow-once" }, }); + expect(manager.getSnapshot(record.id)?.consumedDecision).toBe("allow-once"); + expect(manager.consumeAllowOnce(record.id)).toBe(false); } describe("applyPluginNodeInvokePolicy", () => { @@ -526,6 +528,31 @@ describe("applyPluginNodeInvokePolicy", () => { await expectApprovalResolution(resultPromise, manager, record); }); + it("fails closed when the allow-once claim cannot be consumed", async () => { + const manager = new ExecApprovalManager(); + vi.spyOn(manager, "consumeAllowOnce").mockReturnValue(false); + setDangerousDemoCommandRegistry([createApprovalRequestPolicy()]); + const { context } = createContext({ + pluginApprovalManager: manager, + getApprovalClientConnIds: createApprovalClientLookup([ + createApprovalClient({ + connId: "conn-owner-approval", + clientId: "client-owner", + deviceId: "device-owner", + }), + ]), + }); + const resultPromise = invokeDemoPolicy(context, createOperatorClient()); + + const record = await expectSinglePendingApproval(manager); + expect(manager.resolve(record.id, "allow-once")).toBe(true); + + await expect(resultPromise).resolves.toStrictEqual({ + ok: true, + payload: { id: record.id, decision: null }, + }); + }); + it("fails closed before routing an unrenderable persistent policy approval", async () => { const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-node-policy-approval-")); tempDirs.push(stateDir); diff --git a/src/gateway/node-invoke-plugin-policy.ts b/src/gateway/node-invoke-plugin-policy.ts index 4a510c57cda4..5184f82fbfeb 100644 --- a/src/gateway/node-invoke-plugin-policy.ts +++ b/src/gateway/node-invoke-plugin-policy.ts @@ -145,7 +145,16 @@ function createApprovalRuntime(params: { }); }, }); - return { id: record.id, decision: await decisionPromise }; + const decision = await decisionPromise; + // This return hands execution authority to the plugin policy. Claim a + // one-shot decision here so observation or retry cannot replay it. + if ( + decision === "allow-once" && + !manager.consumeAllowOnce(record.id, `plugin.node.invoke:${record.id}`) + ) { + return { id: record.id, decision: null }; + } + return { id: record.id, decision }; }, }; } diff --git a/src/plugins/hook-before-tool-call-result.ts b/src/plugins/hook-before-tool-call-result.ts index f297641b2ec9..d244f7d68382 100644 --- a/src/plugins/hook-before-tool-call-result.ts +++ b/src/plugins/hook-before-tool-call-result.ts @@ -18,6 +18,10 @@ export type PluginHookBeforeToolCallResult = { description: string; severity?: "info" | "warning" | "critical"; timeoutMs?: number; + /** + * @deprecated Unresolved approvals always deny; retained for plugin API + * compatibility. The field will be removed after one deprecation release train. + */ timeoutBehavior?: "allow" | "deny"; /** Override timeout text and return the timeout as a blocked tool result. */ timeoutReason?: string;