diff --git a/CHANGELOG.md b/CHANGELOG.md index 8911e11e2..daecc5e78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A scroll with an unusable `deltaY` is refused, rather than scrolling some other distance + +`POST /computers/:botId/scroll` and `POST /computers/:botId/human/scroll` accepted any JSON number +as `deltaY`, and `1e999` is a JSON number: it parses to `Infinity`, passes the `typeof` check, and is +turned back into `null` by the hop to the Bot's computer, which reads the field as absent and scrolls +its own default distance. The caller was answered 200 for a scroll it had not asked for. A `deltaY` +that is not a finite number now answers 400 and the page is not touched, the way the timeout on +`exec` and the coordinates behind a person's click already did. ### An MCP call carrying `x-api-key` is stopped the same as one carrying `api-key` The check that keeps credentials out of MCP tool arguments compared each argument name against a diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 2ff5ceaa3..ba0338250 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -331,11 +331,12 @@ export function createComputerRoutes( ); routes.post("/:botId/scroll", (context) => - act(context, (botId, actor, body) => - gateway.scroll(botId, actor, { + act(context, (botId, actor, body) => { + if (!usableDeltaY(body?.deltaY)) return badDeltaY; + return gateway.scroll(botId, actor, { ...(typeof body?.deltaY === "number" ? { deltaY: body.deltaY } : {}), - }), - ), + }); + }), ); /** @@ -481,6 +482,9 @@ export function createComputerRoutes( string, unknown > | null; + if (kind === "scroll" && !usableDeltaY(body?.deltaY)) { + return context.json(badDeltaY, 400); + } try { return context.json( await gateway.humanInput(context.req.param("botId"), { @@ -701,6 +705,24 @@ const badRef: BadRequest = { "A ref and the snapshotId it came from are both required. Take a snapshot first.", }; +const badDeltaY: BadRequest = { + error: "deltaY must be a finite number of pixels.", +}; + +/** + * Whether a wheel delta from an untrusted body can be carried out. + * + * `typeof value === "number"` is true of `Infinity`, and JSON carries it: `1e999` parses to it. It + * then survives every comparison on the way down and is erased by `JSON.stringify` on the hop to the + * computer, which reads the missing field as absent and scrolls its own default distance instead -- + * so the caller is answered 200 for a scroll nobody asked for. Every other number on this surface is + * already checked at the edge: the timeout on `exec`, the coordinates behind `human/click`. This one + * was not. + */ +function usableDeltaY(value: unknown): boolean { + return value === undefined || Number.isFinite(value); +} + /** * Shared plumbing for acting routes that use this helper: resolve who is asking, run, and map * failures onto statuses. diff --git a/server/tests/computer-scroll-delta.test.ts b/server/tests/computer-scroll-delta.test.ts new file mode 100644 index 000000000..4593d10fe --- /dev/null +++ b/server/tests/computer-scroll-delta.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import type { ComputerGateway } from "../src/computer/gateway"; +import type { PolicyStore } from "../src/computer/policy-store"; +import { createComputerRoutes } from "../src/computer/routes"; + +function appWith(calls: { deltaY?: number }[]) { + const gateway = { + scroll: async ( + _botId: string, + _actor: unknown, + input: { deltaY?: number }, + ) => { + calls.push({ + ...(input.deltaY !== undefined ? { deltaY: input.deltaY } : {}), + }); + return { action: "scroll", url: "https://openbot.test/" }; + }, + humanInput: async (_botId: string, input: { deltaY?: number }) => { + calls.push({ + ...(input.deltaY !== undefined ? { deltaY: input.deltaY } : {}), + }); + return { action: "human_scroll", url: "https://openbot.test/" }; + }, + } as unknown as ComputerGateway; + const requireUser: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", { + id: "user-1", + email: "user@openbot.test", + role: "admin", + }); + await next(); + }; + return createComputerRoutes( + gateway, + {} as PolicyStore, + requireUser, + async () => true, + ); +} + +/** + * The body is sent as text, not as a stringified object. + * + * `JSON.stringify({ deltaY: Infinity })` is `{"deltaY":null}`, so an object literal cannot express + * what a client actually puts on the wire. `1e999` is valid JSON and parses to `Infinity`, which is + * the value this endpoint has to answer for. + */ +async function postScroll( + app: ReturnType, + path: string, + body: string, +) { + return app.request(`http://openbot.test/bot-1${path}`, { + method: "POST", + headers: { "content-type": "application/json" }, + body, + }); +} + +describe.each(["/scroll", "/human/scroll"])("POST %s deltaY", (path) => { + test("a valid delta reaches the gateway", async () => { + const calls: { deltaY?: number }[] = []; + const response = await postScroll(appWith(calls), path, '{"deltaY":400}'); + + expect(response.status).toBe(200); + expect(calls).toEqual([{ deltaY: 400 }]); + }); + + test("an absent delta still scrolls the computer's own default", async () => { + const calls: { deltaY?: number }[] = []; + const response = await postScroll(appWith(calls), path, "{}"); + + expect(response.status).toBe(200); + expect(calls).toEqual([{}]); + }); + + test.each([ + ["Infinity", "1e999"], + ["negative Infinity", "-1e999"], + ["a string", '"400"'], + ["null", "null"], + ["a boolean", "true"], + ])( + "rejects %s with 400 and never reaches the gateway", + async (_name, raw) => { + const calls: { deltaY?: number }[] = []; + const response = await postScroll( + appWith(calls), + path, + `{"deltaY":${raw}}`, + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: "deltaY must be a finite number of pixels.", + }); + expect(calls).toEqual([]); + }, + ); +});