diff --git a/CHANGELOG.md b/CHANGELOG.md index b847e6d5a..6e2f952b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A malformed page size is refused instead of silently coerced + +`GET /channels` and `GET /api/admin/people` read `?limit=` with `Number.parseInt`, which +coerces: `?limit=12abc` arrived as 12, `?limit=3.9` as 3, and each answered 200 with a silently +wrong page. Both now share one strict parser with the audit list's rule: absent or blank leaves +the store default alone, a run of digits is clamped into range against the same ceiling the store +enforces, and anything else is a 400 naming the parameter, before the database is reached. ### A skill written with a non-string slug or summary is refused instead of failing the insert `POST /api/plugins/skills` checked presence with truthiness and then ran the slug regex, which diff --git a/server/src/app.ts b/server/src/app.ts index 44e205ade..c89789acf 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -47,7 +47,8 @@ import type { CredentialAdminService, CredentialInput } from "./credentials"; import type { Database } from "./db/client"; import { createIntelligenceClient } from "./intelligence-client"; import type { OnboardingStore } from "./people/onboarding"; -import type { PeopleStore } from "./people/store"; +import { parsePageLimit } from "./paging"; +import { type PeopleStore, MAX_PAGE } from "./people/store"; import { createPluginRoutes } from "./plugins/routes"; import type { PluginStore } from "./plugins/store"; import { REFUSAL_MARKER } from "./plugins/tools"; @@ -580,12 +581,17 @@ export function createApp( /* * A page, not the deployment. * - * `limit` is clamped by the store, so a caller cannot ask for everybody by naming a large - * number. `search` is what makes paging usable: an administrator looking for one colleague - * should not have to walk pages to reach them. + * `limit` is parsed strictly and clamped into range at the edge, against the same ceiling the + * store enforces, so a caller cannot ask for everybody by naming a large number and a typo + * like `12abc` is a 400 rather than a silently coerced page. `search` is what makes paging + * usable: an administrator looking for one colleague should not have to walk pages to reach + * them. */ const url = new URL(context.req.url); - const limit = Number.parseInt(url.searchParams.get("limit") ?? "", 10); + const parsed = parsePageLimit(url.searchParams.get("limit"), MAX_PAGE); + if (!parsed.ok) { + return context.json({ error: parsed.error }, 400); + } return context.json( await peopleStore.list({ @@ -595,7 +601,7 @@ export function createApp( ...(url.searchParams.get("cursor") ? { cursor: url.searchParams.get("cursor") as string } : {}), - ...(Number.isFinite(limit) ? { limit } : {}), + ...(parsed.limit !== undefined ? { limit: parsed.limit } : {}), }), ); }); diff --git a/server/src/channels/routes.ts b/server/src/channels/routes.ts index c129b3d18..1ecd39b79 100644 --- a/server/src/channels/routes.ts +++ b/server/src/channels/routes.ts @@ -20,6 +20,7 @@ import type { AgentActor, AgentProfile } from "../agents/profile-types"; import { type AuditStore, recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; import type { Database } from "../db/client"; +import { parsePageLimit } from "../paging"; import { agentProfiles, channelAgents, @@ -1064,12 +1065,21 @@ export function createChannelRoutes( routes.get("/", requireUser, async (context) => { try { const url = new URL(context.req.url); - const limit = Number.parseInt(url.searchParams.get("limit") ?? "", 10); + /* + * Parsed strictly, not coerced: `Number.parseInt` reads `"12abc"` as 12 and `"3.9"` + * as 3, so a typo silently returned the wrong page. A run of digits is clamped into + * range like the store already does; anything else is a 400 naming the parameter. + */ + const parsed = parsePageLimit( + url.searchParams.get("limit"), + MAX_CHANNEL_PAGE, + ); + if (!parsed.ok) return context.json({ error: parsed.error }, 400); const page = await store.list(context.var.actor, { ...(url.searchParams.get("cursor") ? { cursor: url.searchParams.get("cursor") as string } : {}), - ...(Number.isFinite(limit) ? { limit } : {}), + ...(parsed.limit !== undefined ? { limit: parsed.limit } : {}), }); return context.json({ diff --git a/server/src/paging.ts b/server/src/paging.ts new file mode 100644 index 000000000..7319fe31e --- /dev/null +++ b/server/src/paging.ts @@ -0,0 +1,28 @@ +/** + * A page size from a `?limit=` query param, parsed strictly. + * + * `Number.parseInt` coerces: `"12abc"` reads as 12, `"3.9"` as 3, `"0x10"` as 0, so a typo + * silently returns the wrong page and there is no 400 path at all. The audit list already parses + * strictly (`auditQueryFromUrl` trims and requires `/^\d+$/`); this is the same rule factored out + * so every paged list answers the same way. + * + * Absent or blank means the caller did not ask, and the store's own default applies. A run of + * digits is clamped into `1..max`, because the store clamps that way too and the edge saying the + * same thing keeps a huge but well-formed ask from ever reaching the database as one. Anything + * else is a caller error and answers 400 naming the parameter. + */ +export const PAGE_LIMIT_ERROR = + 'Query parameter "limit" must be a positive integer.'; + +export function parsePageLimit( + raw: string | null, + max: number, +): { ok: true; limit?: number } | { ok: false; error: string } { + if (raw === null || raw.trim() === "") return { ok: true }; + const trimmed = raw.trim(); + if (!/^\d+$/.test(trimmed)) return { ok: false, error: PAGE_LIMIT_ERROR }; + return { + ok: true, + limit: Math.min(Math.max(Number.parseInt(trimmed, 10), 1), max), + }; +} diff --git a/server/src/people/store.ts b/server/src/people/store.ts index 026972491..e62ec1828 100644 --- a/server/src/people/store.ts +++ b/server/src/people/store.ts @@ -85,8 +85,11 @@ const DEFAULT_PAGE = 50; * * A ceiling rather than a suggestion, because the limit arrives over HTTP and the whole point of * paging is that no single request can be made to read the entire deployment. + * + * Exported so the route parses against the same ceiling the store enforces, rather than the two + * drifting apart unnoticed. */ -const MAX_PAGE = 200; +export const MAX_PAGE = 200; /** Where a page stopped. Both halves of the sort, because either alone is ambiguous. */ type Cursor = { lastSignedInAt: string | null; email: string }; diff --git a/server/tests/channel-routes.test.ts b/server/tests/channel-routes.test.ts index ff0cdcae2..fb8c1a701 100644 --- a/server/tests/channel-routes.test.ts +++ b/server/tests/channel-routes.test.ts @@ -158,6 +158,62 @@ describe("channel input parser", () => { }); }); +describe("channel list limit", () => { + /** + * `?limit=` used to be read with `Number.parseInt`, which coerces: `"12abc"` arrived as 12 + * and `"3.9"` as 3, and every one of them answered 200 with a silently coerced page. A run of + * digits is clamped into range like the store already does; anything else is a 400 naming the + * parameter, before the store is reached. + */ + function listApp(calls: { queries: unknown[] }) { + const store = fakeStore({ + async list(_actor, query) { + calls.queries.push(query); + return { channels: [], nextCursor: null }; + }, + }); + return appFor(store); + } + + test.each([["12abc"], ["3.9"], ["-5"], ["0x10"], ["%2B5"]])( + "refuses a coerced limit %p with 400 and never reaches the store", + async (limit) => { + const calls = { queries: [] as unknown[] }; + const response = await listApp(calls).request( + `http://openbot.test/?limit=${limit}`, + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: 'Query parameter "limit" must be a positive integer.', + }); + expect(calls.queries).toEqual([]); + }, + ); + + test.each([ + ["10", { limit: 10 }], + ["999999", { limit: 200 }], + ["0", { limit: 1 }], + ])("passes a well-formed limit %p through as %p", async (limit, query) => { + const calls = { queries: [] as unknown[] }; + const response = await listApp(calls).request( + `http://openbot.test/?limit=${limit}`, + ); + + expect(response.status).toBe(200); + expect(calls.queries).toEqual([query]); + }); + + test("leaves an absent limit to the store default", async () => { + const calls = { queries: [] as unknown[] }; + const response = await listApp(calls).request("http://openbot.test/"); + + expect(response.status).toBe(200); + expect(calls.queries).toEqual([{}]); + }); +}); + describe("channel routes", () => { test("attaches authentication middleware to every route before calling the store", async () => { const store = fakeStore(); diff --git a/server/tests/paging.test.ts b/server/tests/paging.test.ts new file mode 100644 index 000000000..dd94b9935 --- /dev/null +++ b/server/tests/paging.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, test } from "bun:test"; +import { PAGE_LIMIT_ERROR, parsePageLimit } from "../src/paging"; + +/** + * `?limit=` used to be read with `Number.parseInt`, which coerces: `"12abc"` arrived as 12, + * `"3.9"` as 3, `"0x10"` as 0, and every one of them answered 200 with a silently coerced page. + * The parser requires a run of digits and answers 400 otherwise; well-formed values are clamped + * into range the same way the stores already clamp them. + */ +describe("parsePageLimit", () => { + test("leaves an absent or blank limit to the store default", () => { + expect(parsePageLimit(null, 200)).toEqual({ ok: true }); + expect(parsePageLimit("", 200)).toEqual({ ok: true }); + expect(parsePageLimit(" ", 200)).toEqual({ ok: true }); + }); + + test.each([ + ["10", 10], + ["1", 1], + ["200", 200], + [" 25 ", 25], + ])("passes a well-formed limit through: %p", (raw, limit) => { + expect(parsePageLimit(raw, 200)).toEqual({ ok: true, limit }); + }); + + test.each([ + ["0", 1], + ["999999", 200], + ])("clamps a well-formed limit into range: %p", (raw, limit) => { + expect(parsePageLimit(raw, 200)).toEqual({ ok: true, limit }); + }); + + test.each([["12abc"], ["3.9"], ["-5"], ["0x10"], ["+5"], ["1e3"], ["NaN"]])( + "refuses a coerced limit with 400: %p", + (raw) => { + expect(parsePageLimit(raw, 200)).toEqual({ + ok: false, + error: PAGE_LIMIT_ERROR, + }); + }, + ); +});