Skip to content

Commit 2e3be08

Browse files
steipetesteipete
committed
feat(gateway): fail-closed plugin and tool approval gates
Squash-rebased #103932 segment onto the ancestor-propagation tip on current main. Plugin node.invoke approvals claim a one-shot allow-once decision before handing execution authority to the policy, so observation or retry cannot replay a consumed approval; sibling tool gates bind approval ids to their originating reviewer identity. (cherry picked from commit 122df0d75281f572c012b6484ed5daf085d1d577) (cherry picked from commit f23d8ac240a8dcf2a42c4daaae962263975a0ae8) (cherry picked from commit 8632fb6436a224dac7a9a9ef0216884530de2c24) (cherry picked from commit d9fe2dd5c53665e5732f5f3da5ed1907a686ade8) (cherry picked from commit bb139f2c8aa36ddad70413e1ef79ff491a6f2ecd) (cherry picked from commit 9b3e056b68ea515c7d4baef269289b6670570480) (cherry picked from commit b11e66b59a7af1f3b08712de06864aed64d349e4) (cherry picked from commit a12916ee592d03dfa35e86a2aaede4476a5d4e5d) (cherry picked from commit d699de840fc8346892b9ae244fe3c3e8c4413032) (cherry picked from commit 9b7e5a9) (cherry picked from commit 4b507593f1b2f10b4496984c84a2803b853e5c5e) (cherry picked from commit 56408a186733c4eeb5aa76bd5907cde7b0cd3d5b) (cherry picked from commit a4051d6d8353d39d6fa0b5d69c36e06c3cd3e796) (cherry picked from commit 7682980) (cherry picked from commit a8b493f) (cherry picked from commit aceb990) (cherry picked from commit a29b0e7)
1 parent 93ba8c4 commit 2e3be08

16 files changed

Lines changed: 555 additions & 95 deletions

‎docs/.generated/plugin-sdk-api-baseline.sha256‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
634a2cda48d53c9a234d59200b4666c105615da4d64612ea30facb42df9c4d93 plugin-sdk-api-baseline.json
2-
f5bc2ec06dbdc26736f8d810d75f72f378ffd2111c2a143ed153a284b6d89d25 plugin-sdk-api-baseline.jsonl
1+
d6747bc65237cf327f9f307e405347969292aee5f711ad41a1c8f6c5bc29e7ce plugin-sdk-api-baseline.json
2+
678d8dddf40f8a0830ae2fe678a40af17a4dfbd550cfa41cc0a5e25f12ecfcb3 plugin-sdk-api-baseline.jsonl

‎docs/plugins/hooks.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ export default definePluginEntry({
4040
description: `Allow search query: ${String(event.params.query ?? "")}`,
4141
severity: "info",
4242
timeoutMs: 60_000,
43-
timeoutBehavior: "deny",
4443
},
4544
};
4645
},
@@ -251,6 +250,7 @@ type BeforeToolCallResult = {
251250
description: string;
252251
severity?: "info" | "warning" | "critical";
253252
timeoutMs?: number;
253+
/** @deprecated Unresolved approvals always deny. */
254254
timeoutBehavior?: "allow" | "deny";
255255
allowedDecisions?: Array<"allow-once" | "allow-always" | "deny">;
256256
pluginId?: string;

‎docs/plugins/plugin-permission-requests.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ export default definePluginEntry({
6363
? ["allow-once", "deny"]
6464
: ["allow-once", "allow-always", "deny"],
6565
timeoutMs: 120_000,
66-
timeoutBehavior: "deny",
6766
onResolution(decision) {
6867
console.log(`deploy approval resolved: ${decision}`);
6968
},
@@ -99,10 +98,15 @@ available approval surfaces, and waits for a decision.
9998
| `allow-once` | The current call continues. |
10099
| `allow-always` | The current call continues and the decision is passed to the plugin. |
101100
| `deny` | The call is blocked with a denied tool result. |
102-
| Timeout | The call is blocked unless `timeoutBehavior` is `"allow"`. |
101+
| Timeout | The call is blocked. |
103102
| Cancellation | The call is blocked when the run is aborted. |
104103
| No approval route | The call is blocked because no connected approval surface can resolve it. |
105104

105+
Only the exact `allow-once` and `allow-always` decisions permitted by the
106+
request allow execution. Unknown, malformed, mismatched, missing, and timed-out
107+
decisions fail closed. The legacy `timeoutBehavior` field remains accepted for
108+
plugin compatibility but is deprecated and ignored; do not set it in new hooks.
109+
106110
`allow-always` is only durable when the requesting plugin or runtime implements
107111
that persistence. For ordinary `before_tool_call.requireApproval` hooks,
108112
OpenClaw treats `allow-once` and `allow-always` as approval decisions for the

‎docs/refactor/operator-approvals.md‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -335,13 +335,17 @@ Final strict behavior:
335335
- malformed trusted verdict -> `denied`, deny;
336336
- only an allowed explicit allow decision -> `allowed`.
337337

338-
Current shipped behavior conflicts with this contract:
338+
Current shipped exec behavior still conflicts with this contract:
339339

340340
- `src/agents/bash-tools.exec-host-shared.ts` may apply `askFallback`.
341-
- `src/agents/agent-tools.before-tool-call.ts` may honor `timeoutBehavior: "allow"`.
342-
- `docs/tools/exec-approvals.md`, `docs/cli/approvals.md`, and `docs/plugins/plugin-permission-requests.md` document those surfaces.
341+
- `docs/tools/exec-approvals.md` and `docs/cli/approvals.md` document that surface.
343342

344-
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.
343+
Plugin approvals now fail closed on timeout and malformed verdicts; the legacy
344+
`timeoutBehavior` field remains accepted but ignored. The exec strict-semantics
345+
follow-up must update code, types, docs, tests, and changelog together, with
346+
explicit owner/security review. `askFallback` may continue to describe
347+
pre-gate policy selection during migration, but it must not turn a created
348+
pending record's timeout into approval.
345349

346350
## Compatibility plan
347351

@@ -460,5 +464,5 @@ A committed transition is success even if later event delivery fails. Lifecycle
460464
## Open decisions
461465

462466
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.
463-
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.
467+
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.
464468
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.

‎extensions/codex/src/app-server/approval-bridge.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2248,6 +2248,36 @@ describe("Codex app-server approval bridge", () => {
22482248
});
22492249
});
22502250

2251+
it("ignores waitDecision replies bound to a different approval id", async () => {
2252+
const params = createParams();
2253+
const onNativeToolFailureDisposition = vi.fn();
2254+
mockCallGatewayTool
2255+
.mockResolvedValueOnce({ id: "plugin:approval-mismatch", status: "accepted" })
2256+
.mockResolvedValueOnce({ id: "plugin:approval-other", decision: "allow-once" });
2257+
2258+
const result = await handleCodexAppServerApprovalRequest({
2259+
method: "item/commandExecution/requestApproval",
2260+
requestParams: {
2261+
threadId: "thread-1",
2262+
turnId: "turn-1",
2263+
itemId: "cmd-mismatch",
2264+
command: "pnpm test",
2265+
},
2266+
paramsForRun: params,
2267+
threadId: "thread-1",
2268+
turnId: "turn-1",
2269+
onNativeToolFailureDisposition,
2270+
});
2271+
2272+
// A misrouted allow for another approval must not release this gate.
2273+
expect(result).toEqual({ decision: "decline" });
2274+
expect(onNativeToolFailureDisposition).toHaveBeenCalledWith("cmd-mismatch", "failed");
2275+
findApprovalEvent(params, {
2276+
status: "unavailable",
2277+
approvalId: "plugin:approval-mismatch",
2278+
});
2279+
});
2280+
22512281
it("sanitizes reason previews before forwarding approval text and events", async () => {
22522282
const params = createParams();
22532283
mockCallGatewayTool.mockResolvedValueOnce({

‎extensions/codex/src/app-server/plugin-approval-roundtrip.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,14 @@ export async function waitForPluginApprovalDecision(params: {
9393
{ timeoutMs: resolveCodexGatewayTimeoutWithGraceMs(timeoutMs) },
9494
{ id: params.approvalId },
9595
);
96+
// Bind the verdict to the approval that parked this prompt. A stale or
97+
// misrouted reply maps to "unavailable" instead of releasing another gate.
98+
const bindDecision = (
99+
result: ApprovalWaitResult | undefined,
100+
): ExecApprovalDecision | null | undefined =>
101+
result?.id === params.approvalId ? result.decision : undefined;
96102
if (!params.signal) {
97-
return (await waitPromise)?.decision;
103+
return bindDecision(await waitPromise);
98104
}
99105
let onAbort: (() => void) | undefined;
100106
const abortPromise = new Promise<never>((_, reject) => {
@@ -106,7 +112,7 @@ export async function waitForPluginApprovalDecision(params: {
106112
params.signal!.addEventListener("abort", onAbort, { once: true });
107113
});
108114
try {
109-
return (await Promise.race([waitPromise, abortPromise]))?.decision;
115+
return bindDecision(await Promise.race([waitPromise, abortPromise]));
110116
} finally {
111117
if (onAbort) {
112118
params.signal.removeEventListener("abort", onAbort);

‎extensions/file-transfer/src/shared/node-invoke-policy.test.ts‎

Lines changed: 90 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -270,19 +270,102 @@ describe("file-transfer node invoke policy", () => {
270270
expect(invokeNode).not.toHaveBeenCalled();
271271
});
272272

273-
it("uses plugin approvals for ask-on-miss before invoking the node", async () => {
273+
it.each(["allow-once", "allow-always"] as const)(
274+
"uses exact %s plugin approval once across preflight and final invoke",
275+
async (decision) => {
276+
const policy = createFileTransferNodeInvokePolicy();
277+
const approvals = {
278+
request: vi.fn(async () => ({ id: "approval-1", decision })),
279+
};
280+
const { ctx, invokeNode } = createCtx({
281+
params: { path: "/tmp/new.txt" },
282+
pluginConfig: {
283+
nodes: {
284+
"node-1": {
285+
ask: "on-miss",
286+
allowReadPaths: ["/allowed/**"],
287+
maxBytes: 256,
288+
},
289+
},
290+
},
291+
approvals,
292+
});
293+
294+
const result = await policy.handle(ctx);
295+
296+
expect(result.ok).toBe(true);
297+
expect(approvals.request).toHaveBeenCalledTimes(1);
298+
expect(invokeNode).toHaveBeenCalledTimes(2);
299+
const approvalCalls = approvals.request.mock.calls as unknown[][];
300+
const approvalRequest = requireRecord(approvalCalls[0]?.[0], "approval request");
301+
expectRecordFields(approvalRequest, {
302+
title: "Read file: /tmp/new.txt",
303+
severity: "info",
304+
toolName: "file.fetch",
305+
});
306+
expect(invokeNode).toHaveBeenNthCalledWith(1, {
307+
params: {
308+
path: "/tmp/new.txt",
309+
followSymlinks: false,
310+
maxBytes: 256,
311+
preflightOnly: true,
312+
},
313+
});
314+
expect(invokeNode).toHaveBeenNthCalledWith(2, {
315+
params: {
316+
path: "/tmp/new.txt",
317+
followSymlinks: false,
318+
maxBytes: 256,
319+
},
320+
});
321+
},
322+
);
323+
324+
it.each([
325+
{
326+
label: "explicit deny",
327+
decision: "deny",
328+
code: "APPROVAL_DENIED",
329+
message: "file.fetch APPROVAL_DENIED: operator denied the prompt",
330+
},
331+
{
332+
label: "null decision",
333+
decision: null,
334+
code: "APPROVAL_UNAVAILABLE",
335+
message:
336+
"file.fetch APPROVAL_UNAVAILABLE: no operator client connected to approve the request",
337+
},
338+
{
339+
label: "undefined decision",
340+
decision: undefined,
341+
code: "APPROVAL_UNAVAILABLE",
342+
message:
343+
"file.fetch APPROVAL_UNAVAILABLE: no operator client connected to approve the request",
344+
},
345+
{
346+
label: "arbitrary truthy string",
347+
decision: "accept",
348+
code: "APPROVAL_DENIED",
349+
message: "file.fetch APPROVAL_DENIED: invalid approval decision",
350+
},
351+
{
352+
label: "arbitrary truthy object",
353+
decision: { action: "accept" },
354+
code: "APPROVAL_DENIED",
355+
message: "file.fetch APPROVAL_DENIED: invalid approval decision",
356+
},
357+
])("fails closed for $label", async ({ decision, code, message }) => {
274358
const policy = createFileTransferNodeInvokePolicy();
275359
const approvals = {
276-
request: vi.fn(async () => ({ id: "approval-1", decision: "allow-once" as const })),
277-
};
360+
request: vi.fn(async () => ({ id: "approval-1", decision })),
361+
} as unknown as NonNullable<OpenClawPluginNodeInvokePolicyContext["approvals"]>;
278362
const { ctx, invokeNode } = createCtx({
279363
params: { path: "/tmp/new.txt" },
280364
pluginConfig: {
281365
nodes: {
282366
"node-1": {
283367
ask: "on-miss",
284368
allowReadPaths: ["/allowed/**"],
285-
maxBytes: 256,
286369
},
287370
},
288371
},
@@ -291,29 +374,9 @@ describe("file-transfer node invoke policy", () => {
291374

292375
const result = await policy.handle(ctx);
293376

294-
expect(result.ok).toBe(true);
295-
const approvalCalls = approvals.request.mock.calls as unknown[][];
296-
const approvalRequest = requireRecord(approvalCalls[0]?.[0], "approval request");
297-
expectRecordFields(approvalRequest, {
298-
title: "Read file: /tmp/new.txt",
299-
severity: "info",
300-
toolName: "file.fetch",
301-
});
302-
expect(invokeNode).toHaveBeenNthCalledWith(1, {
303-
params: {
304-
path: "/tmp/new.txt",
305-
followSymlinks: false,
306-
maxBytes: 256,
307-
preflightOnly: true,
308-
},
309-
});
310-
expect(invokeNode).toHaveBeenNthCalledWith(2, {
311-
params: {
312-
path: "/tmp/new.txt",
313-
followSymlinks: false,
314-
maxBytes: 256,
315-
},
316-
});
377+
expectResultFields(result, { ok: false, code, message });
378+
expect(approvals.request).toHaveBeenCalledTimes(1);
379+
expect(invokeNode).not.toHaveBeenCalled();
317380
});
318381

319382
it("marks node transport failures as unavailable", async () => {

‎extensions/file-transfer/src/shared/node-invoke-policy.ts‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -154,28 +154,37 @@ async function requestApproval(input: {
154154
severity: input.kind === "write" ? "warning" : "info",
155155
toolName: input.op,
156156
});
157-
158-
if (approval.decision === "deny" || approval.decision === null || !approval.decision) {
157+
const approvalDecision: unknown = approval.decision;
158+
159+
if (approvalDecision !== "allow-once" && approvalDecision !== "allow-always") {
160+
const unavailable = approvalDecision === null || approvalDecision === undefined;
161+
const deniedByOperator = approvalDecision === "deny";
162+
const reason = deniedByOperator
163+
? "operator denied"
164+
: unavailable
165+
? "no operator available"
166+
: "invalid approval decision";
159167
await appendFileTransferAudit({
160168
op: input.op,
161169
nodeId: input.ctx.nodeId,
162170
nodeDisplayName,
163171
requestedPath: input.path,
164172
decision: "denied:approval",
165-
reason: approval.decision === "deny" ? "operator denied" : "no operator available",
173+
reason,
166174
durationMs: Date.now() - input.startedAt,
167175
});
168176
return {
169177
ok: false,
170-
code: approval.decision === "deny" ? "APPROVAL_DENIED" : "APPROVAL_UNAVAILABLE",
171-
message:
172-
approval.decision === "deny"
178+
code: unavailable ? "APPROVAL_UNAVAILABLE" : "APPROVAL_DENIED",
179+
message: unavailable
180+
? `${input.op} APPROVAL_UNAVAILABLE: no operator client connected to approve the request`
181+
: deniedByOperator
173182
? `${input.op} APPROVAL_DENIED: operator denied the prompt`
174-
: `${input.op} APPROVAL_UNAVAILABLE: no operator client connected to approve the request`,
183+
: `${input.op} APPROVAL_DENIED: invalid approval decision`,
175184
};
176185
}
177186

178-
if (approval.decision === "allow-always") {
187+
if (approvalDecision === "allow-always") {
179188
try {
180189
await persistAllowAlways({
181190
nodeId: input.ctx.nodeId,
@@ -228,7 +237,7 @@ async function requestApproval(input: {
228237
nodeId: input.ctx.nodeId,
229238
nodeDisplayName,
230239
requestedPath: input.path,
231-
decision: approval.decision === "allow-always" ? "allowed:always" : "allowed:once",
240+
decision: approvalDecision === "allow-always" ? "allowed:always" : "allowed:once",
232241
durationMs: Date.now() - input.startedAt,
233242
});
234243
return {

0 commit comments

Comments
 (0)