diff --git a/CHANGELOG.md b/CHANGELOG.md index d9ae571f5..17dbe506e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A fractional or infinite snapshot id is refused as malformed, not stale + +A `snapshotId` of `1.5` or `Infinity` passed the acting routes and never matched the stored +integer, so the answer was a 409 stale snapshot and the caller retried a request that was +malformed. Non-integer ids are refused with a 400 naming the ref and its snapshot before any +decision or audit row. ### A `DATABASE_URL` with a port of zero is refused at start-up `postgres://…:0/…` parsed and booted, and every query then failed against a port nothing listens diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index ba0338250..0a08bd981 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -431,7 +431,10 @@ export function createComputerRoutes( "Say which field the value goes in, using a ref from your snapshot.", }; } - if (typeof body?.snapshotId !== "number") { + if ( + typeof body?.snapshotId !== "number" || + !Number.isInteger(body.snapshotId) + ) { return { error: "The snapshotId the ref came from is required." }; } return gateway.requestSecret(botId, actor, { @@ -816,7 +819,18 @@ function asRef( body: Record | null, ): { ref: string; snapshotId: number } | undefined { if (typeof body?.ref !== "string" || !body.ref) return undefined; - if (typeof body?.snapshotId !== "number") return undefined; + /* + * A snapshot id is an integer the snapshot store handed out. `typeof` alone accepts `1.5` and + * `Infinity` (valid JSON: `1e999` parses to it), which then never equals the stored integer, so + * the gateway reports a stale snapshot and the caller retries a request that was malformed. + * Malformed input is a 400 here, not a 409 staleness. + */ + if ( + typeof body?.snapshotId !== "number" || + !Number.isInteger(body.snapshotId) + ) { + return undefined; + } return { ref: body.ref, snapshotId: body.snapshotId }; } diff --git a/server/tests/computer-snapshot-id.test.ts b/server/tests/computer-snapshot-id.test.ts new file mode 100644 index 000000000..263e6edee --- /dev/null +++ b/server/tests/computer-snapshot-id.test.ts @@ -0,0 +1,85 @@ +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: unknown[]) { + const gateway = { + click: async (_botId: string, _actor: unknown, ref: unknown) => { + calls.push(ref); + return { action: "click", 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, + ); +} + +/** + * A snapshot id is an integer the store handed out, not any number. + * + * `typeof 1.5 === "number"` and `1e999` parses to `Infinity`, so both used to pass the edge + * check and then never equal the stored integer. The gateway answered a stale snapshot (409) + * and the caller retried a request that was malformed. Malformed input is a 400 here. + */ +async function postClick( + app: ReturnType, + body: string, +) { + return app.request("http://openbot.test/bot-1/click", { + method: "POST", + headers: { "content-type": "application/json" }, + body, + }); +} + +describe("POST /:botId/click snapshotId", () => { + test("an integer snapshotId reaches the gateway", async () => { + const calls: unknown[] = []; + const response = await postClick( + appWith(calls), + '{"ref":"e5","snapshotId":12}', + ); + + expect(response.status).toBe(200); + expect(calls).toEqual([{ ref: "e5", snapshotId: 12 }]); + }); + + test.each([ + ["a float", "1.5"], + ["Infinity", "1e999"], + ["negative Infinity", "-1e999"], + ["a string", '"12"'], + ["null", "null"], + ["a boolean", "true"], + ])("rejects %s with 400 and never reaches the gateway", async (_n, raw) => { + const calls: unknown[] = []; + const response = await postClick( + appWith(calls), + `{"ref":"e5","snapshotId":${raw}}`, + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: + "A ref and the snapshotId it came from are both required. Take a snapshot first.", + }); + expect(calls).toEqual([]); + }); +});