diff --git a/CHANGELOG.md b/CHANGELOG.md index c851fcd3..255ccac5 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 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 +duplicate: a permitted action that fails is recorded both as the decision that allowed it and as a +separate failure row, so every failed action was scanned and scored twice. Worse, a dry-run policy +carries a refused action out, so a refused action can fail too — and its two rows disagree, the +decision row saying "refused" and the failure row reading as "allowed", so a candidate policy that +refused it identically was reported as a new refusal it never introduced. The replay now scores each +action once, from the row that recorded its decision. + ### A Bot's shell can no longer reach the embedded database without a password In the all-in-one image the cluster was `trust`-auth on loopback, and the Bot's shell runs in the diff --git a/server/src/computer/policy-dry-run.ts b/server/src/computer/policy-dry-run.ts index ba291d61..c79972d0 100644 --- a/server/src/computer/policy-dry-run.ts +++ b/server/src/computer/policy-dry-run.ts @@ -20,11 +20,17 @@ import { type PolicyContext, } from "./policy"; -/** The event types the gateway writes for a judged computer action. In one place, for the query. */ +/** + * The event types that record a policy DECISION about a computer action. In one place, for the query. + * + * Not `computer.action_failed`: the gateway writes that beside the decision row when a permitted + * action is attempted and does not succeed, so it is a second row for an action already recorded + * here, not a decision of its own. Replaying it too would score the action twice — see + * {@link dryRunAgainstHistory}. + */ export const REPLAYABLE_EVENT_TYPES = [ "computer.action_allowed", "computer.action_refused", - "computer.action_failed", ] as const; /** One action the candidate policy would have decided differently. */ @@ -38,7 +44,7 @@ export type DryRunChange = { element: { role: string; name: string } | null; command: string | null; file: string | null; - /** What actually happened, from the trail. A failed action was permitted first, so it was allowed. */ + /** What the policy in force decided, from the action's decision row. */ was: "allowed" | "refused"; would: "allowed" | "refused"; /** The candidate rule that decided it, or null for the default refusal. */ @@ -120,6 +126,14 @@ export function contextFromAuditPayload( * a dry-run policy's refusals, which were recorded and then carried out. That is the honest * baseline: the question this answers is "what would decide differently than was decided", not * "what would run differently than ran". + * + * A `computer.action_failed` row is skipped rather than scored. It is the outcome of an action whose + * decision row is already in this history, so counting it would score that action twice — and worse, + * a dry-run policy carries a refused action out, so a refused-then-failed action would arrive as a + * decision row that says "refused" and a failure row that reads as "allowed", inventing a change no + * policy made. The decision is on the decision row; the failure row only says it did not finish. + * Callers should exclude it from the query too ({@link REPLAYABLE_EVENT_TYPES}); this guards the + * function against being handed one regardless. */ export function dryRunAgainstHistory( policy: ActionPolicy, @@ -134,6 +148,7 @@ export function dryRunAgainstHistory( }; for (const event of events) { + if (event.eventType === "computer.action_failed") continue; const context = contextFromAuditPayload(event.payload); if (!context) continue; report.scanned += 1; diff --git a/server/tests/policy-dry-run.test.ts b/server/tests/policy-dry-run.test.ts index f6e3a07e..e5933733 100644 --- a/server/tests/policy-dry-run.test.ts +++ b/server/tests/policy-dry-run.test.ts @@ -122,7 +122,11 @@ describe("dryRunAgainstHistory", () => { expect(report.changes[0]?.would).toBe("allowed"); }); - test("a failed action was permitted first, so it counts as allowed", () => { + test("a permitted action that failed is counted once, from its decision row", () => { + // The gateway records a permitted-but-failed action twice: the decision row written before the + // attempt, and a failure row written when it did not succeed. Both carry the same action, and + // both are returned by the trail query, so scoring the failure row too would count the action a + // second time. Its baseline is still "allowed" — from the decision row that permitted it. const deny: ActionPolicy = { mode: "enforce", deny: ['tool.name == "computer_click"'], @@ -130,15 +134,50 @@ describe("dryRunAgainstHistory", () => { }; const report = dryRunAgainstHistory(deny, [ event({ - id: "f", + id: "dec", + eventType: "computer.action_allowed", + payload: CLICK_SUBMIT, + }), + event({ + id: "fail", eventType: "computer.action_failed", payload: CLICK_SUBMIT, }), ]); + expect(report.scanned).toBe(1); expect(report.wouldRefuse).toBe(1); + expect(report.changes).toHaveLength(1); + expect(report.changes[0]?.id).toBe("dec"); expect(report.changes[0]?.was).toBe("allowed"); }); + test("a dry-run refusal that was carried out and then failed invents no change", () => { + // In dry-run mode a refused action is still carried out, so a refused action can also fail. The + // decision row says "refused"; the failure row, read on its own, would read as "allowed" and a + // candidate that refuses the same action would then look like a new refusal. Skipping the failure + // row leaves only the honest baseline: it was refused, a policy that refuses it changes nothing. + const denyClicks: ActionPolicy = { + mode: "enforce", + deny: ['tool.name == "computer_click"'], + allow: ["true"], + }; + const report = dryRunAgainstHistory(denyClicks, [ + event({ + id: "dec", + eventType: "computer.action_refused", + payload: CLICK_SUBMIT, + }), + event({ + id: "fail", + eventType: "computer.action_failed", + payload: CLICK_SUBMIT, + }), + ]); + expect(report.scanned).toBe(1); + expect(report.wouldRefuse).toBe(0); + expect(report.unchanged).toBe(1); + }); + test("a rule naming a command does not refuse a click, because absent facts are neutral", () => { const candidate: ActionPolicy = { mode: "enforce",