diff --git a/server/src/audit.ts b/server/src/audit.ts index ada81df10..48d181246 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -1,6 +1,7 @@ import { and, desc, eq, gt, inArray, lt, or } from "drizzle-orm"; import type { Database } from "./db/client"; import { auditEvents } from "./db/schema"; +import { parsePageLimit } from "./paging"; const sensitiveKeys = new Set([ "access_token", @@ -645,14 +646,17 @@ export function createAuditReader(database: Database): AuditReader { } export function auditQueryFromUrl(url: URL): AuditEventQuery { - const rawLimit = url.searchParams.get("limit") ?? "50"; - const trimmedLimit = rawLimit.trim(); - const requestedLimit = /^\d+$/.test(trimmedLimit) - ? Number.parseInt(trimmedLimit, 10) - : Number.NaN; - const limit = Number.isFinite(requestedLimit) - ? Math.min(Math.max(requestedLimit, 1), 100) - : 50; + /* + * Parsed strictly, like every other paged list. This used to trim and require `/^\d+$/` + * but fall back to 50 on anything else, so `?limit=abc`, `?limit=12abc`, `?limit=3.9` + * and `?limit=` all silently returned the default page while `?from=garbage` on the same + * endpoint answered 400. A typo in the page size is a caller error and names the parameter. + */ + const parsedLimit = parsePageLimit(url.searchParams.get("limit"), 100); + if (!parsedLimit.ok) { + throw new AuditQueryError(parsedLimit.error); + } + const limit = parsedLimit.limit ?? 50; const optional = (name: string) => url.searchParams.get(name) ?? undefined; const from = optional("from"); diff --git a/server/src/paging.ts b/server/src/paging.ts index 7319fe31e..08796352c 100644 --- a/server/src/paging.ts +++ b/server/src/paging.ts @@ -2,9 +2,8 @@ * 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. + * silently returns the wrong page and there is no 400 path at all. The audit list, the channel + * list and the people list all share this rule, 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 diff --git a/server/tests/audit-limit.test.ts b/server/tests/audit-limit.test.ts new file mode 100644 index 000000000..47445e31f --- /dev/null +++ b/server/tests/audit-limit.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, test } from "bun:test"; +import { createApp } from "../src/app"; +import { AuditQueryError, auditQueryFromUrl } from "../src/audit"; +import { loadConfig } from "../src/config"; +import { PAGE_LIMIT_ERROR } from "../src/paging"; +import { testEnvironment } from "./support/environment"; + +/** + * The audit trail's page size, held to the same rule as every other list. + * + * `auditQueryFromUrl` matched `/^\d+$/` but fell back to 50 on anything else, so + * `?limit=abc`, `?limit=12abc`, `?limit=3.9` and `?limit=-5` all silently returned the + * default page while `?from=garbage` on the same endpoint answered 400. The channel and + * people lists already answer 400 through `parsePageLimit`; this endpoint was the odd one. + */ + +const config = loadConfig({ ...testEnvironment() }); + +const adminAuth = { + handler: () => new Response(null, { status: 204 }), + api: { + getSession: async () => ({ + user: { id: "admin", email: "admin@openbot.test" }, + }), + }, +}; + +function query(search: string) { + return auditQueryFromUrl( + new URL(`http://openbot.local/api/admin/audit-events${search}`), + ); +} + +describe("audit-events limit validation", () => { + test("an absent limit still reads the default page", () => { + expect(query("").limit).toBe(50); + }); + + test("a blank limit still reads the default page", () => { + expect(query("?limit=").limit).toBe(50); + expect(query("?limit=%20%20").limit).toBe(50); + }); + + test.each([[1], [10], [50], [100]])("a limit of %s still parses", (limit) => { + expect(query(`?limit=${limit}`).limit).toBe(limit); + }); + + test("a well-formed but huge limit is clamped to the audit maximum", () => { + expect(query("?limit=99999").limit).toBe(100); + }); + + test("a padded limit is read for its digits", () => { + expect(query("?limit=%20%2010%20%20").limit).toBe(10); + }); + + test.each([ + ["letters", "abc"], + ["a mixed typo", "12abc"], + ["a decimal", "3.9"], + ["a negative", "-5"], + ["a hex literal", "0x10"], + ["a plus sign", "+10"], + ])("a limit of %s is a query error naming the parameter", (_name, limit) => { + expect(() => query(`?limit=${encodeURIComponent(limit)}`)).toThrow( + AuditQueryError, + ); + expect(() => query(`?limit=${encodeURIComponent(limit)}`)).toThrow( + PAGE_LIMIT_ERROR, + ); + }); + + test("the admin route answers 400 for a malformed limit and never reads", async () => { + const app = createApp( + config, + adminAuth, + { rolesForUser: async () => ["admin"] }, + { + list: async () => { + throw new Error("must not reach the store with a bad limit"); + }, + }, + ); + + const response = await app.request( + "http://openbot.local/api/admin/audit-events?limit=12abc", + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: PAGE_LIMIT_ERROR, + }); + }); + + test("the admin route still pages with a valid limit", async () => { + const queries: { limit?: number }[] = []; + const app = createApp( + config, + adminAuth, + { rolesForUser: async () => ["admin"] }, + { + list: async (received) => { + queries.push({ limit: (received as { limit: number }).limit }); + return { events: [], nextCursor: undefined }; + }, + }, + ); + + const response = await app.request( + "http://openbot.local/api/admin/audit-events?limit=10", + ); + + expect(response.status).toBe(200); + expect(queries).toEqual([{ limit: 10 }]); + }); +});