diff --git a/CHANGELOG.md b/CHANGELOG.md index a6663c5a3..bfd002ee9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A rule tried in dry-run now says what it would have refused a Bot's tools + +`dry-run` exists so a boundary can be measured against live traffic before it starts refusing +anybody. It worked that way for the browser, and not for connectors: a tool call the rule matched was +recorded only as the call that then went out, so `Blocked` on the audit page — and any query behind +it — answered "this rule would have refused none of them" about calls it would have refused. A rule +about `mcp.server`, `mcp.tool` or `mcp.effect` therefore looked inert, and enforcing it started +refusing Bots with nothing in the trail to have warned anybody. A refused tool call is now recorded +whatever the mode does with it, carrying `carriedOut` so a reader can tell a call this deployment +stopped from one dry-run recorded and let past. Enforcing deployments behave exactly as before. ### A policy dry-run no longer counts a failed action twice, or invents a change it did not make Testing a boundary against recent history replayed three kinds of audit row, and one of them is a diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 4164c39a6..1b8ca1bb1 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -2875,18 +2875,34 @@ export function createPluginStore(options: PluginStoreOptions) { }; /* - * A refusal is written here, because there is no attempt to wait for. + * A refusal is written on the POLICY's answer, not on whether the call was then let through. * * This deployment declining is the whole event, and it is recorded before the throw so that a * refusal cannot be lost by the caller's error handling. + * + * In `dry-run` the policy still refuses and the mode forwards anyway, which is the whole point + * of the mode: `evaluateActionPolicy` returns `allowed: false` with `forward: true` so a rule + * can be tried against live traffic before it starts refusing anybody. Writing this row on + * `forward` therefore recorded nothing at all on this surface for exactly the traffic an + * operator switched dry-run on to measure — the browser gateway keys its row on + * `decision.allowed` and does record it — so `Blocked` on the audit page, and every + * `eventType=mcp.call_rejected` query behind it, answered "this rule would refuse none of your + * tool calls" about calls it would refuse. The rule then looked inert, and enforcing it + * started refusing Bots with no warning in the trail. + * + * `decision.carriedOut` is what tells the two rows apart: false is a call this deployment + * stopped, true is one dry-run recorded and let past. The outcome row below is unchanged, so a + * forwarded call still says separately whether the vendor answered. */ - if (!verdict.forward) { + if (!verdict.allowed) { await recordAuditEvent(auditStore, { eventType: "mcp.call_rejected", targetType: "mcp_tool", targetId: input.ref, payload: decided, }); + } + if (!verdict.forward) { throw new PluginRefusedError(verdict.reason, verdict.matched); } diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index c82eb3d7f..f05914ffc 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -383,6 +383,54 @@ describe("the policy is asked as well as the grant", () => { expect((thrown as PluginRefusedError).rule).toBeNull(); expect((thrown as PluginRefusedError).message).toContain("connected"); }); + + test("a dry-run refusal is recorded, even though the call is let through", async () => { + await store.grant("mcp", ref, holderId, "admin@openbot.local"); + /* + * The mode an operator switches on to size a rule before enforcing it, and the only mode in + * which the policy refuses and the call still goes out. Its whole value is the row: without one + * the report reads "this rule would refuse nothing" about traffic it would refuse. + */ + const rule = `mcp.tool == "${toolName}"`; + policy = { mode: "dry-run", deny: [rule], allow: ["true"] }; + + try { + await store + .callTool({ + ref, + args: {}, + botId: holderId, + actorId: "someone@openbot.local", + }) + // Forwarded past the policy, so what happens next is the vendor's business and not this + // test's: nobody has connected an account, so it fails there. Swallowed deliberately. + .catch(() => undefined); + } finally { + policy = { mode: "enforce", deny: [], allow: ["true"] }; + } + + const rows = await auditRowsFor(ref); + const recorded = rows.filter( + (row) => + row.eventType === "mcp.call_rejected" && + (row.payload as { decision?: { rule?: string } }).decision?.rule === + rule, + ); + expect(recorded.length).toBeGreaterThan(0); + /* + * What tells this row apart from a call this deployment actually stopped. `allowed` is the + * policy's answer and `carriedOut` is what the mode did with it, so a reader counting what a + * rule would have refused finds this one, and a reader counting what was refused does not. + */ + const decision = ( + recorded[0].payload as { + decision?: { allowed?: boolean; mode?: string; carriedOut?: boolean }; + } + ).decision; + expect(decision?.allowed).toBe(false); + expect(decision?.mode).toBe("dry-run"); + expect(decision?.carriedOut).toBe(true); + }); }); describe("the trail says what happened, not what was permitted", () => {