diff --git a/.env.example b/.env.example index 016e81df..05f4a2d3 100644 --- a/.env.example +++ b/.env.example @@ -162,12 +162,13 @@ AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true # # Name every route to the same effect. A form submits from a keypress in any of its fields, so a rule # that only blocks a Submit button does not block Enter from another field. The example below refuses -# Enter outright for that reason. +# Enter outright for that reason, from both tools that can press it: a key action, and a type action +# carrying `submit`. # Functions: contains(haystack, needle) and matches(value, pattern), both case-insensitive. # `enforce` blocks; `dry-run` decides and records but lets everything through, so a new rule can be # tried against real traffic before it starts refusing anybody's work. # -# AGENT_COMPUTER_POLICY={"mode":"enforce","deny":["(intent == \"activate\" && contains(element.name, \"submit\")) || (tool.name == \"computer_key\" && key == \"Enter\")"],"allow":["true"]} +# AGENT_COMPUTER_POLICY={"mode":"enforce","deny":["(intent == \"activate\" && contains(element.name, \"submit\")) || ((tool.name == \"computer_key\" || tool.name == \"computer_type\") && key == \"Enter\")"],"allow":["true"]} # How long one action waits for its element, in ms. Read by agent-computer, not the server. # ACTION_TIMEOUT_MS=10000 diff --git a/app/src/routes/_authed/admin/boundaries.tsx b/app/src/routes/_authed/admin/boundaries.tsx index 66644bd0..cb6faca6 100644 --- a/app/src/routes/_authed/admin/boundaries.tsx +++ b/app/src/routes/_authed/admin/boundaries.tsx @@ -23,8 +23,11 @@ import { Input } from "@/components/ui/input"; const PRESETS: { label: string; rule: string; cost?: string }[] = [ { label: "Never submit a form", - // `key` exists only on keypress actions; guard it by tool name to keep other actions evaluable. - rule: '(intent == "activate" && contains(element.name, "submit")) || (tool.name == "computer_key" && key == "Enter")', + // `key` is guarded by tool name so the clause short-circuits before it on actions that have no + // keypress in them. Both tools that can press Enter are named: `computer_type` takes a `submit` + // flag that presses it once the text is in, and a rule naming only `computer_key` left that door + // open. + rule: '(intent == "activate" && contains(element.name, "submit")) || ((tool.name == "computer_key" || tool.name == "computer_type") && key == "Enter")', cost: "Also stops the Bot pressing Enter for anything else, because a form submits from Enter in any of its fields.", }, { diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index be73249d..63b60984 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -674,6 +674,15 @@ export function createComputerGateway( { ref: input.ref, snapshotId: input.snapshotId, + /* + * `submit` presses Enter once the text is in, so this call is a keypress as well as a + * typing one and the policy has to see both halves. Without the key here, an agent refused + * on clicking the button and refused on pressing Enter types into the field with + * `submit: true` instead, and the form goes through against a rule written to stop exactly + * that. It is what the trail is missing too: a row with no key says a field was filled in, + * not that the form was sent. + */ + ...(input.submit ? { key: "Enter" } : {}), ...(signal ? { signal } : {}), }, () => post(botId, "/type", input, signal), @@ -832,9 +841,10 @@ function intentOf( case "computer_click": return "activate"; case "computer_key": - return key && ACTIVATING_KEYS.has(key) ? "activate" : "type"; + // A type carrying `submit` ends in Enter, and the same reasoning applies to it: what the + // keypress does is press whatever the form activates, whichever tool asked for it. case "computer_type": - return "type"; + return key && ACTIVATING_KEYS.has(key) ? "activate" : "type"; case "computer_navigate": return "navigate"; case "computer_read": diff --git a/server/src/computer/policy.ts b/server/src/computer/policy.ts index ec9d39e2..93b06137 100644 --- a/server/src/computer/policy.ts +++ b/server/src/computer/policy.ts @@ -60,9 +60,10 @@ export type PolicyContext = { * click is refused and audited, the keypress is allowed, because nothing in the context could tell * one keypress from another. * - * A form has two doors and the policy could only see one. Now a rule can say - * `tool.name == "computer_key" && key == "Enter"`, and an operator blocking a submit button knows - * to block both routes, which the deny example in `.env.example` now does. + * A form has three doors, and this is set for two of them. `computer_type` takes a `submit` flag + * that presses Enter once the text is in, so it carries the key as well; a rule naming only + * `computer_key` was refused at the button and at the keypress and let through the third way in. + * The deny example in `.env.example` and the Boundaries preset both name both tools. */ key?: string; /** @@ -78,7 +79,8 @@ export type PolicyContext = { * `read`, looking at the page or listing what is on it. * `write_file` / `read_file` / `list_files`, the workspace. * - * It still cannot see whether a keypress will submit a form. A browser submits + * It still cannot see whether a keypress will submit a form, only that one is coming: a type + * carrying `submit` reports `activate` because it ends in Enter, but a browser submits * from Enter in any field of it, and the element a keypress names is the field, not the form. The * gateway would need to know the page's structure at decision time, which it does not, refs are * held off-DOM by Playwright and the policy runs before the action reaches the browser. So a rule diff --git a/server/tests/computer-gateway.test.ts b/server/tests/computer-gateway.test.ts index d6d54ad6..d8413d08 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -726,6 +726,134 @@ describe("the computer gateway", () => { expect(calls).toEqual(["click"]); }); + test("a submit typed into a field is the same Enter, and the rule catches it", async () => { + /* + * The second door, reached through the first tool. + * + * `computer_type` takes `submit`, which presses Enter when the text is in. A rule written + * about Enter saw a keypress and a click and not this, so an agent refused at both doors + * typed into the field with `submit: true` and the form went through. The preset shipped in + * `.env.example` and on the Boundaries page is exactly this rule, so the deployment that + * followed the product's own advice was the one with the hole in it. + */ + const { gateway, calls, rows } = await gatewayWith({ + ...PERMISSIVE, + deny: ['key == "Enter"'], + }); + + await expect( + gateway.type("bot-1", ACTOR, { + ref: "e1", + snapshotId: 7, + text: "Ada", + submit: true, + }), + ).rejects.toThrow(); + expect(calls).toEqual([]); + expect(rows[0]?.eventType).toBe("computer.action_refused"); + // The key belongs in the trail for the same reason it belongs in the decision: without it the + // row says somebody filled in a field, and not that they submitted the form. + expect((rows[0]?.payload as { key?: string } | undefined)?.key).toBe( + "Enter", + ); + }); + + test("typing without a submit is still typing", async () => { + // The other direction. A rule about Enter must not start refusing ordinary text, which is what + // a fix that simply reported every type as a keypress would do. + const { gateway, calls } = await gatewayWith({ + ...PERMISSIVE, + deny: ['key == "Enter"'], + }); + + await gateway.type("bot-1", ACTOR, { + ref: "e1", + snapshotId: 7, + text: "Ada", + }); + + expect(calls).toEqual(["type"]); + }); + + test("a submit is an activation, so an intent rule catches it too", async () => { + // `intent` is the other way an operator writes this, and it is the one the preset leads with. + // Enter pressed in a field presses whatever the form activates, the same as Space or a click. + const { gateway, calls } = await gatewayWith({ + ...PERMISSIVE, + deny: ['intent == "activate"'], + }); + + await expect( + gateway.type("bot-1", ACTOR, { + ref: "e1", + snapshotId: 7, + text: "Ada", + submit: true, + }), + ).rejects.toThrow(); + expect(calls).toEqual([]); + }); + + /* + * The rule the product ships, run against every door it claims to close. + * + * Written out here rather than imported because the two places it lives, `.env.example` and the + * Boundaries preset list, are a string in a comment and a string in the browser bundle. What + * this pins is the behaviour an operator gets by taking the product's own advice, so a change to + * either copy that reopens a door fails here. + */ + const SHIPPED_NEVER_SUBMIT = + '(intent == "activate" && contains(element.name, "submit")) || ((tool.name == "computer_key" || tool.name == "computer_type") && key == "Enter")'; + + test("the shipped preset closes the button, the keypress and the submit flag", async () => { + const shipped = { ...PERMISSIVE, deny: [SHIPPED_NEVER_SUBMIT] }; + + const click = await gatewayWith(shipped); + await expect( + click.gateway.click("bot-1", ACTOR, { ref: "e9", snapshotId: 7 }), + ).rejects.toThrow(); + expect(click.calls).toEqual([]); + + const enter = await gatewayWith(shipped); + await expect( + enter.gateway.key("bot-1", ACTOR, { + ref: "e1", + snapshotId: 7, + key: "Enter", + }), + ).rejects.toThrow(); + expect(enter.calls).toEqual([]); + + const submit = await gatewayWith(shipped); + await expect( + submit.gateway.type("bot-1", ACTOR, { + ref: "e1", + snapshotId: 7, + text: "Ada", + submit: true, + }), + ).rejects.toThrow(); + expect(submit.calls).toEqual([]); + }); + + test("the shipped preset still lets the Bot fill the form in", async () => { + // A boundary that stopped a Bot typing its way through a form would be refused by the first + // deployment to try it, and an operator would take the whole rule off rather than narrow it. + const shipped = { ...PERMISSIVE, deny: [SHIPPED_NEVER_SUBMIT] }; + + const typing = await gatewayWith(shipped); + await typing.gateway.type("bot-1", ACTOR, { + ref: "e1", + snapshotId: 7, + text: "Ada", + }); + expect(typing.calls).toEqual(["type"]); + + const looking = await gatewayWith(shipped); + await looking.gateway.screenshot("bot-1", ACTOR, {}); + expect(looking.calls).toEqual(["screenshot"]); + }); + test("but the rule still refuses the action it is about", async () => { // The point is not that these rules stop working. A neutral value answers honestly; it does // not answer no.