From b152e1158e5af53aa2e6f087ad741b540c4ee1f6 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:44:22 -0500 Subject: [PATCH] Take the kind of human input from the path, not the body `/:botId/human/:kind` checks `kind` against the four gestures a person's mouse and keyboard produce, and then built the call as `{ kind, ...body }`. The spread came second, so a body carrying its own `kind` replaced the value that had just been checked. `humanInput` puts that value straight into the path it calls on the computer and the transport concatenates it onto the base address, so a fetch resolves the `..` away and the request lands on a different endpoint entirely. `{"kind":"../computers/reset"}` sent to `/bot-1/human/click` reaches `/computers/reset`, which wipes the profile and every login in it. `{"kind":"../exec","command":"..."}` reaches the shell. Both carry the deployment's computer token, because the transport attaches it to everything it sends. Three defences are not defeated so much as stepped around, since the request never enters the paths that carry them. This route deliberately skips the policy decision and the audit row, which is the price of a takeover: a person entering a password is exactly what must not be recorded. The computer's own `humanMayDrive` check is keyed to the four `/human/*` paths and never sees a request routed elsewhere. So the one route that writes nothing down is the one that could be pointed anywhere. The only gate in front of it is the Bot-access check every route under `/:botId/*` has, so this is reachable by any signed-in person who may act as the Bot, and a Bot whose visibility is public means everybody. Fixed at both layers, because they answer different questions. The route spreads the body first and puts the checked `kind` last, so the value it verified is the value it sends. The gateway checks the gesture against the set before interpolating, because that is where the string becomes a path: `kind` is typed as a union of four, and the route casts a parsed body to that shape, so the type is erased exactly where it would have helped. Reverting either one turns tests red on its own. The end-to-end test binds a socket and asks the far side what arrived, because the stubbed-fetch tests either side of it asserted on a URL this process had just built. The `..` is resolved by URL parsing inside fetch, so reading back the string passed in describes the argument rather than the request. With both layers reverted, a real server receives `/computers/reset`, `/exec` and `/health`. --- server/src/computer/gateway.ts | 24 +++ server/src/computer/routes.ts | 5 +- server/tests/computer-gateway.test.ts | 45 ++++++ server/tests/computer-routes.test.ts | 81 ++++++++++ server/tests/human-input-end-to-end.test.ts | 157 ++++++++++++++++++++ 5 files changed, 311 insertions(+), 1 deletion(-) create mode 100644 server/tests/human-input-end-to-end.test.ts diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index be73249d..732fb15a 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -623,6 +623,21 @@ export function createComputerGateway( input: HumanInput, ): Promise { const { kind, ...payload } = input; + /* + * Checked here as well as at the route, because this is where it becomes a path. + * + * `kind` is typed as one of four gestures and a type is not a check: the route casts a parsed + * body to this shape, so whatever arrived is whatever the caller sent. Interpolated into the + * path below, a value like `../exec` reaches a different endpoint of the computer's API + * altogether, carrying this deployment's computer token. This method is also the one acting + * path that writes no audit row, deliberately, so a call that went somewhere else leaves + * nothing behind that would say so. + */ + if (!HUMAN_GESTURES.has(kind)) { + throw new Error( + `A person's input is one of ${[...HUMAN_GESTURES].join(", ")}, not ${JSON.stringify(kind)}.`, + ); + } return post(botId, `/human/${kind}`, payload); }, @@ -824,6 +839,15 @@ function describeFile(path: string): { */ const ACTIVATING_KEYS = new Set(["Enter", "NumpadEnter", "Space", " "]); +/** + * What a person's mouse and keyboard produce, and the whole of what `/human/` may name. + * + * A set rather than the union type alone, because the type is erased before the value gets here: the + * route parses a JSON body and casts it to the input shape, so the check has to exist at runtime on + * the side that builds the path. + */ +const HUMAN_GESTURES = new Set(["click", "type", "key", "scroll"]); + function intentOf( toolName: string, key: string | undefined, diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 30f822b3..d50c663f 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -324,8 +324,11 @@ export function createComputerRoutes( try { return context.json( await gateway.humanInput(context.req.param("botId"), { - kind, ...(body ?? {}), + // Last, so the checked value wins. Spread over it, a body carrying its own `kind` replaced + // the one this route had just checked, and the gateway puts that value into the path it + // calls on the computer. + kind, } as Parameters[1]), ); } catch (error) { diff --git a/server/tests/computer-gateway.test.ts b/server/tests/computer-gateway.test.ts index d6d54ad6..f9bc4556 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -163,6 +163,9 @@ function fakeComputer(options?: { calls.push("supplySecret"); return Response.json({ supplied: true }); case "/human/click": + case "/human/type": + case "/human/key": + case "/human/scroll": case "/human/move": case "/human/button": case "/human/wheel": @@ -743,6 +746,48 @@ describe("the computer gateway", () => { }); }); +/** + * The gateway builds the computer path from `kind`, so `kind` cannot be a caller's string. + * + * The route ahead of it checks the four gestures, and this is the layer that holds when something + * gets past that: a value reaching here decides which endpoint of the computer's API the deployment + * calls, with its computer token attached. `humanInput` is also the one acting method that writes no + * audit row, by design, so a redirected call leaves nothing behind to read afterwards. + */ +describe("human input names a gesture, not a path", () => { + test.each([ + ["../computers/reset", "another endpoint of the computer's API"], + ["../exec", "the shell"], + ["click/../../health", "a traversal in the middle"], + ["", "nothing at all"], + ])("refuses %s (%s), and sends nothing", async (kind) => { + const { gateway, requests } = await gatewayWith(PERMISSIVE); + const before = requests.length; + + await expect( + gateway.humanInput("bot-1", { kind, x: 1, y: 1 } as never), + ).rejects.toThrow(); + // Nothing left this process. Asserting only that it threw would pass just as well when the + // request went out and the far side answered 404, which is the failure being fixed. + expect(requests.slice(before)).toEqual([]); + }); + + test.each(["click", "type", "key", "scroll"])( + "carries %s through, because that is what a person's hands do", + async (kind) => { + const { gateway, requests } = await gatewayWith(PERMISSIVE); + + await gateway.humanInput("bot-1", { kind, x: 1, y: 1 } as never); + + expect( + requests.some( + (request) => new URL(request.url).pathname === `/human/${kind}`, + ), + ).toBe(true); + }, + ); +}); + describe("resolving a computer's address", () => { test("a foreign address is refused, so nothing is sent to it", async () => { /* diff --git a/server/tests/computer-routes.test.ts b/server/tests/computer-routes.test.ts index e688f31f..36f82b4e 100644 --- a/server/tests/computer-routes.test.ts +++ b/server/tests/computer-routes.test.ts @@ -132,3 +132,84 @@ describe("computer fleet listing", () => { expect(listed()).toBe(1); }); }); + +/** + * The kind of human input is what the path says, and only what the path says. + * + * The route checks `:kind` against the four gestures a person's mouse and keyboard produce, and then + * built the call as `{ kind, ...body }`, so a body carrying its own `kind` replaced the value that + * had just been checked. The gateway puts that value straight into the path it calls on the + * computer, so the check decided one thing and the request went somewhere else. + * + * This route is the one that deliberately skips the policy decision and the audit row, because a + * takeover exists so a person can type the thing nothing should keep. That makes it the worst one to + * be able to redirect: nothing downstream writes the row that would have shown where it went. + */ +describe("human input", () => { + function recordingGateway() { + const calls: Array<{ botId: string; input: Record }> = []; + const gateway = { + humanInput: async (botId: string, input: Record) => { + calls.push({ botId, input }); + return { ok: true }; + }, + } as unknown as ComputerGateway; + return { + calls, + app: createComputerRoutes( + gateway, + {} as PolicyStore, + asActor(member), + async () => true, + ), + }; + } + + async function send(body: unknown, kind = "click") { + const { app, calls } = recordingGateway(); + const response = await app.request( + `http://openbot.test/bot-1/human/${kind}`, + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); + return { response, calls }; + } + + test("carries a real gesture through with its coordinates", async () => { + const { response, calls } = await send({ x: 10, y: 20 }); + + expect(response.status).toBe(200); + expect(calls).toHaveLength(1); + expect(calls[0]?.input.kind).toBe("click"); + expect(calls[0]?.input.x).toBe(10); + }); + + test("a body naming its own kind does not decide where the call goes", async () => { + // `../computers/reset` is the shape that matters: the gateway interpolates this into the path it + // calls, and a fetch resolves the `..` away, so the request lands on a different endpoint of the + // computer's API carrying the deployment's computer token. + const { calls } = await send({ kind: "../computers/reset", x: 1, y: 1 }); + + expect(calls[0]?.input.kind).toBe("click"); + }); + + test("a body naming its own kind cannot reach the shell either", async () => { + const { calls } = await send( + { kind: "../exec", command: "cat /workspace/notes" }, + "type", + ); + + expect(calls[0]?.input.kind).toBe("type"); + }, 10_000); + + test("a kind the path does not allow is still refused", async () => { + // The existing check, which must go on working: the four gestures are the whole surface. + const { response, calls } = await send({ x: 1 }, "screenshot"); + + expect(response.status).toBe(400); + expect(calls).toHaveLength(0); + }); +}); diff --git a/server/tests/human-input-end-to-end.test.ts b/server/tests/human-input-end-to-end.test.ts new file mode 100644 index 00000000..3e3bdc00 --- /dev/null +++ b/server/tests/human-input-end-to-end.test.ts @@ -0,0 +1,157 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import type { AppVariables, AuthenticatedActor } from "../src/auth/guards"; +import { createComputerGateway } from "../src/computer/gateway"; +import type { PolicyStore } from "../src/computer/policy-store"; +import type { ComputerProvider } from "../src/computer/provider"; +import { createComputerRoutes } from "../src/computer/routes"; + +/** + * The whole path a person's input takes, over a real socket. + * + * The unit tests either side of this one stub the fetch, which means the URL they assert on is a + * string this process built rather than a request anything received. The thing being prevented here + * is a request ARRIVING somewhere it should not, and only a listener can say where one arrived: the + * `..` in `/human/../exec` is resolved by URL parsing inside fetch, so a test that reads the string + * it passed in is describing its own argument. This one starts a server, drives the real router + * through the real gateway and the real transport, and asks the server what it got. + * + * It stands in for `agent-computer` rather than running it, because that process opens Chromium at + * import time. What it reproduces is the part that matters: an HTTP API on the other end of the + * transport, with endpoints beyond the four this route is allowed to reach. + */ + +const servers: Array<{ stop: (force?: boolean) => void }> = []; + +afterEach(() => { + for (const server of servers.splice(0)) server.stop(true); +}); + +const TOKEN = "computer-token-for-this-test"; + +/** What the far side actually received, in the order it arrived. */ +type Received = { path: string; token: string | null; body: unknown }; + +function serveComputer() { + const received: Received[] = []; + const server = Bun.serve({ + port: 0, + fetch: async (request) => { + received.push({ + path: new URL(request.url).pathname, + token: request.headers.get("x-openbot-computer-token"), + body: await request.json().catch(() => null), + }); + return Response.json({ ok: true }); + }, + }); + servers.push(server); + return { received, baseUrl: `http://127.0.0.1:${server.port}` }; +} + +function appFor(baseUrl: string) { + const rows: AuditEventInput[] = []; + const provider: ComputerProvider = { + name: "test", + isolation: "per-bot", + locate: async () => baseUrl, + status: async (botId) => ({ botId, state: "ready" }), + stop: async () => ({ wasRunning: true }), + reset: async () => ({ cleared: true }), + list: async () => [], + }; + // No fetchImpl: the transport uses the platform's own, which is what resolves the path. + const gateway = createComputerGateway({ + provider, + auditStore: { + insert: async (event: AuditEventInput) => { + rows.push(event); + }, + } as unknown as AuditStore, + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), + token: TOKEN, + }); + const actor: AuthenticatedActor = { + id: "user-1", + email: "member@openbot.test", + role: "user", + }; + const asActor: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", actor); + await next(); + }; + return { + rows, + app: createComputerRoutes( + gateway, + {} as PolicyStore, + asActor, + async () => true, + ), + }; +} + +async function drive(kind: string, body: unknown) { + const { received, baseUrl } = serveComputer(); + const { app, rows } = appFor(baseUrl); + const response = await app.request( + `http://openbot.test/bot-1/human/${kind}`, + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); + return { response, received, rows }; +} + +describe("a person's input, end to end", () => { + test("a click arrives at the click endpoint, carrying the computer token", async () => { + const { response, received } = await drive("click", { x: 10, y: 20 }); + + expect(response.status).toBe(200); + expect(received).toHaveLength(1); + expect(received[0]?.path).toBe("/human/click"); + expect(received[0]?.token).toBe(TOKEN); + expect(received[0]?.body).toEqual({ x: 10, y: 20 }); + }); + + test.each([ + [ + "../computers/reset", + "/computers/reset", + "wipes the profile and every login in it", + ], + ["../exec", "/exec", "runs a command on the computer"], + ["../../health", "/health", "leaves the Bot's own surface entirely"], + ])( + "a body asking for %s does not arrive at %s, which %s", + async (kind, forbidden) => { + const { received } = await drive("click", { + kind, + command: "cat /workspace/notes", + x: 1, + y: 1, + }); + + // The assertion is about what the far side was asked to do, not about what this process + // intended: every path below is one the transport would have reached with the deployment's + // token attached, and none of them is guarded by the takeover check that protects `/human/*`. + expect(received.map((request) => request.path)).not.toContain(forbidden); + }, + ); + + test("the four gestures still reach their own endpoints", async () => { + for (const kind of ["click", "type", "key", "scroll"]) { + const { received } = await drive(kind, { x: 1, y: 1, text: "hello" }); + + expect(received.map((request) => request.path)).toEqual([ + `/human/${kind}`, + ]); + } + }); +});