diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index 9ab59b80..fc7df747 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -18,7 +18,11 @@ * The refs are opaque to the caller precisely so that the server holds the mapping. */ import { type AuditStore, recordAuditEvent } from "../audit"; -import { ComputerUnavailableError, createComputerTransport } from "./client"; +import { + ComputerUnavailableError, + createComputerTransport, + StaleSnapshotError, +} from "./client"; import { checkComputerAddress } from "./target"; export { @@ -480,6 +484,39 @@ export function createComputerGateway( let result: T; try { + /* + * A citation that was made and could not be honoured is refused, not carried out. + * + * `resolve` answering undefined leaves the element half of the context all-empty, and empty is + * the honest neutral value for an action that names no element: it is what stops a rule about + * one action surface throwing on another. It is the wrong answer for an action that named a ref + * and did not get one. Every element-keyed rule then evaluates against empty strings, so a deny + * that exists to stop this exact click does not match, the shipped default permits, and the + * click lands on whatever that ref is now. The rule did not decline to match. It was never + * shown the element. + * + * The computer's own staleness check does not make this safe. It compares the citation against + * its own counter, so it catches the cases where the two disagree; the case that matters is the + * one where the computer is content and only this server is out of step, which is what a + * computer restarting its generation counter under a stored row leaves behind. + * + * Only a cited ref, and only against a snapshot this server actually holds. Scroll, a + * page-level keypress, a shell call and a file read name no element, so they have nothing to + * have failed to resolve. A computer this server has no snapshot for at all is a different + * situation: there is no page here to judge the citation against, the computer is the only + * party that can, and refusing locally would take its answer away, including the one that says + * a person has taken the wheel. What is refused here is a citation this server can see is stale. + * + * Thrown from inside the attempt, after the decision row, on purpose: an action whose ref + * resolves to nothing is still an action somebody tried to take, and refusing it before the row + * was written would be a way to act without appearing on the trail. The failure row beside it is + * what stops the trail claiming a permitted action was carried out when nothing was sent. + */ + if (ref && stored && !element) { + throw new StaleSnapshotError( + `${ref} is not on the page this computer is showing, so nothing can be checked against it before acting. Take a fresh snapshot and use the refs it returns.`, + ); + } result = await run(); } catch (error) { /** diff --git a/server/tests/computer-gateway.test.ts b/server/tests/computer-gateway.test.ts index a71dc908..22bbbb59 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -10,6 +10,7 @@ import type { ComputerLocation, ComputerProvider, } from "../src/computer/provider"; +import { StaleSnapshotError } from "../src/computer/client"; import type { SnapshotResult } from "../src/computer/schema"; import { createInMemorySnapshotStore, @@ -519,12 +520,16 @@ describe("the computer gateway", () => { test("an action on an unresolvable ref is still decided and still recorded", async () => { const { gateway, rows } = await gatewayWith(PERMISSIVE); - await gateway.click("bot-1", ACTOR, { - ref: "e404", - snapshotId: 7, - }); - // Permitted here only because the shipped default permits; the row says plainly that the server - // could not identify what was touched, rather than omitting the field. + // The assertion this test was written for is unchanged: the decision is taken and the row says + // plainly that the server could not identify what was touched, rather than omitting the field. + // What changed is what happens after the row: the action used to be forwarded, so an + // element-keyed deny rule could not match and the click landed anyway. It is now refused. The + // row is still written first, which is what keeps the attempt on the trail. + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e404", snapshotId: 7 }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(StaleSnapshotError); expect(rows[0]?.payload.element).toBe("not in the current snapshot"); }); @@ -1049,12 +1054,14 @@ describe("resolving a ref across replicas", () => { }); expect(current.element?.name).toBe("Submit order"); - const superseded = await handlesClick.gateway.click("bot-1", ACTOR, { - ref: "e9", - snapshotId: 6, - }); - // Not resolved to snapshot 7's Submit button: the ref carried an older generation. - expect(superseded.element).toBeUndefined(); + // Not resolved to snapshot 7's Submit button: the ref carried an older generation. The claim is + // unchanged; the assertion moved because a citation that cannot be resolved is now refused rather + // than forwarded with an empty element, which is what let an element-keyed deny rule go inert. + const superseded = await handlesClick.gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: 6 }) + .catch((error: unknown) => error); + + expect(superseded).toBeInstanceOf(StaleSnapshotError); expect(handlesClick.rows[1]?.payload.element).toBe( "not in the current snapshot", ); @@ -1071,6 +1078,11 @@ describe("resolving a ref across replicas", () => { await tookSnapshot.gateway.snapshot("bot-1"); await tookSnapshot.gateway.resetComputer("bot-1", ACTOR); + // Still forwarded with the element unresolved, and deliberately so. A reset deletes the row, so + // this server holds no snapshot for the computer at all and has nothing to judge the citation + // against; the refusal added for a stale citation fires only where there is a stored page to see + // that it is stale. The computer is the party that can answer here, and taking its answer away + // would also take away the one that says a person has the wheel. const afterReset = await handlesClick.gateway.click("bot-1", ACTOR, { ref: "e9", snapshotId: 7, @@ -1112,14 +1124,19 @@ describe("a ref that outlived its computer", () => { /* * The same generation the model is still holding, now belonging to a different run. Resolving it - * would hand the policy an element from the dead page; refusing to resolve leaves the boundary - * deciding with no element, which is the honest answer and the one a deny rule treats as a - * match. + * would hand the policy an element from the dead page. + * + * This comment used to say that refusing to resolve leaves the boundary deciding with no element, + * "which is the honest answer and the one a deny rule treats as a match". The first half is right + * and the second was not: an all-empty element is what a deny rule keyed on `element.name` fails + * to match, so the action was permitted by the shipped default and carried out. The citation is + * refused now, which is what makes the sentence true. */ - await gateway.click("bot-1", ACTOR, { - ref: "e9", - snapshotId: taken.snapshotId, - }); + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: taken.snapshotId }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(StaleSnapshotError); // Recorded as unresolved rather than as the dead page's button, which is what the audit row // said before: `element: { name: "Confirm transfer" }` on a click nowhere near it. @@ -1150,3 +1167,58 @@ describe("a ref that outlived its computer", () => { }); }); }); + +describe("acting on a ref the server cannot resolve", () => { + const DENY_SUBMIT: ActionPolicy = { + ...PERMISSIVE, + deny: ['contains(element.name, "submit")'], + }; + + test("the rule refuses the click while it can see the element", async () => { + // The control. Without it a fix that refused everything would look identical to a fix that + // works, and the rule below would prove nothing about the element being visible to the policy. + const { gateway, calls } = await gatewayWith(DENY_SUBMIT); + + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: 7 }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(ActionRefusedError); + expect(calls).toEqual([]); + }); + + test("a cited ref that does not resolve is refused, not carried out with the rule blind", async () => { + // The same rule, the same button, and a citation the server cannot resolve. The element half of + // the policy context is then all-empty, so `contains(element.name, "submit")` is false and the + // shipped default permits: the click was forwarded and performed while the rule that exists to + // stop it never saw what it was about to touch. The audit row was honest about not identifying + // the element, which is what makes this a prevention failure rather than a detection one. + const { gateway, calls, rows } = await gatewayWith(DENY_SUBMIT); + + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: 2 }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(StaleSnapshotError); + expect(calls).toEqual([]); + // Still recorded. An action whose ref resolves to nothing is an action somebody tried to take, + // and refusing it before the row is written would be a way to act without appearing on the trail. + expect(rows.map((row) => row.eventType)).toEqual([ + "computer.action_allowed", + "computer.action_failed", + ]); + expect(rows[0]?.payload.element).toBe("not in the current snapshot"); + }); + + test("an action that names no ref is untouched, because it has nothing to have failed to resolve", async () => { + // The boundary. A scroll, a page-level keypress, a shell call and a file read legitimately carry + // no element, and the neutral empty element is the right answer for them: it is what keeps a rule + // about one action surface from throwing on another. Only a citation that was made and could not + // be honoured is refused here. + const { gateway, calls } = await gatewayWith(PERMISSIVE); + + await gateway.scroll("bot-1", ACTOR, { deltaY: 200 }); + + expect(calls).toEqual(["scroll"]); + }); +});