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}`, + ]); + } + }); +});