From 4b4180373f5be86d83f94bdf904572a1d33c4447 Mon Sep 17 00:00:00 2001 From: kevin9327 <5299031+kevin9327@users.noreply.github.com> Date: Wed, 9 Sep 2026 07:10:52 +0900 Subject: [PATCH] Refuse a scroll whose deltaY is not a finite number `POST /:botId/scroll` and `POST /:botId/human/scroll` guarded `deltaY` with `typeof value === "number"`, which is true of `Infinity`. `1e999` is valid JSON and parses to exactly that, so the value passed the check, travelled to the gateway, and was written by `JSON.stringify` as `null` on the hop to the Bot's computer -- where `typeof body.deltaY === "number"` is now false and the computer scrolls its own default instead (600 pixels for the tool path, 400 for a person's). The caller got 200 and a scroll it did not ask for. Every other number on this surface is already checked at the edge: `timeoutMs` on `exec` is rejected unless it is a whole number in range (#427), and the coordinates behind `human/click` are rejected unless `Number.isFinite` accepts them. `deltaY` was the one that was not, so it is checked the same way, at the same place, with the same 400. Wrong types are refused rather than silently dropped, which is what `exec` already does with a `timeoutMs` of `"3000"`. Measured: with the routes unchanged and only the new test applied, 10 of 14 cases fail -- every rejection case on both paths answers 200 and reaches the gateway. With the change, 14 pass. --- CHANGELOG.md | 9 ++ server/src/computer/routes.ts | 30 +++++- server/tests/computer-scroll-delta.test.ts | 105 +++++++++++++++++++++ 3 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 server/tests/computer-scroll-delta.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index dba57a687..503acc133 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ 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. + ### One command to stop what `start.sh` started Stopping the local stack meant four commands read off the end of a successful start, and the one 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([]); + }, + ); +});