Refuse a malformed list limit instead of silently coercing it

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

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 } : {}),
       }),
     );
   });

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({

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),
+  };
+}

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 };

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();

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,
+      });
+    },
+  );
+});