From 7946675347c24c279af7eed97f38cd3683a75099 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:41:06 -0500 Subject: [PATCH] Refuse a ref this server cannot resolve, instead of acting blind A click citing a ref the server cannot resolve was carried out, and every deny rule keyed on the element was silently unable to match it. `resolve` answering undefined binds an all-empty element into the policy context. Empty is the right neutral value for an action that names no element, which is what stops a rule about one action surface throwing on another, and it is the wrong answer for an action that named a ref and did not get one. `contains(element.name, "submit")` against empty strings is false, the shipped default allows, and the click lands on whatever that ref points at 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: the store keeps the higher generation and refuses every snapshot until the counter climbs back past it, while the model is handed those refs anyway. Driven against a real computer and a real browser: the same click on the same button, refused by the rule before the restart and performed after it. Narrow on purpose, in three ways, each of which turns a test red if it goes. Only a cited ref, so a scroll, a page-level keypress, a shell call and a file read are untouched. Only against a snapshot this server holds, because with no stored page there is nothing here to judge the citation against, the computer is the only party that can answer, and refusing locally would take its answer away, including the one that says a person has the wheel. And raised after the decision row is written, because an action whose ref resolves to nothing is still an action somebody tried to take. A comment in the tests claimed the empty element was "the one a deny rule treats as a match". That was the misconception underneath this, and it is corrected where it sat. --- server/src/computer/gateway.ts | 39 ++++++++- server/tests/computer-gateway.test.ts | 110 +++++++++++++++++++++----- 2 files changed, 129 insertions(+), 20 deletions(-) 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"]); + }); +});