Public: anyone can read this; the crew can change it.
A cursor-paged API list answers `?page=` with 400 naming `cursor`, instead of page one again #8396
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8386-a-cursor-paged-api-list-answers-page-wit → main 2 commits fork 2 threads · 2 unresolved
7 files
changed +133
−4
docs/features/api-reference.md +2
| @@ -19,3 +19,5 @@ operations: they live on another origin. | ||
| 19 | 19 | path parameter runs it as the person at the terminal and shows the status, time, Server-Timing and body. |
| 20 | 20 | |
| 21 | 21 | `/api` answers `no_route` in JSON to a caller whose `Accept` prefers JSON, as the rest of `/api` does. |
| 22 | ||
| 23 | A list the contract gives a `cursor` pages by it: `?page=` or `?offset=` on one is a 400 `invalid_query` ("This list pages by cursor: pass next_cursor as ?cursor="), answered by one guard in `src/http/cursor_lists.ts` before any handler runs, and named in the document as the route's 400. |
src/contract/openapi.ts +9 −2
| @@ -17,6 +17,11 @@ import * as each from "./openapi/routes/index.ts"; | ||
| 17 | 17 | /** Every row, sorted by operationId: the index's order. */ |
| 18 | 18 | export const ROUTES: readonly Route[] = Object.values(each); |
| 19 | 19 | |
| 20 | /** A GET that declares a `cursor` pages by it; `?page=` and `?offset=` are refused with a 400 (`http/cursor_lists.ts`). */ | |
| 21 | export const pagesByCursor = (r: Pick<Route, "method" | "query">): boolean => r.method === "get" && r.query !== undefined && "cursor" in r.query; | |
| 22 | ||
| 23 | const PAGE_REFUSAL = "page or offset was sent: this list pages by cursor, pass next_cursor as ?cursor= (400)"; | |
| 24 | ||
| 20 | 25 | /** A route's operationId, and the name of the file that holds it. */ |
| 21 | 26 | export function operationId(r: Pick<Route, "method" | "path">): string { |
| 22 | 27 | return `${r.method}${r.path.replace(/[^A-Za-z0-9]+(.)?/g, (_, c: string | undefined) => (c ?? "").toUpperCase())}`; |
| @@ -28,6 +33,7 @@ const schema = (t: string) => (t === "object" ? { type: "object" } : t === "arra | ||
| 28 | 33 | export function openapi(origin: string, version: string): Record<string, unknown> { |
| 29 | 34 | const paths: Record<string, Record<string, unknown>> = {}; |
| 30 | 35 | for (const r of ROUTES) { |
| 36 | const refusals = pagesByCursor(r) ? { invalid_query: PAGE_REFUSAL, ...r.refusals } : r.refusals; | |
| 31 | 37 | const params = [...r.path.matchAll(/\{(\w+)\}/g)].map((m) => ({ |
| 32 | 38 | name: m[1], |
| 33 | 39 | in: "path", |
| @@ -86,12 +92,13 @@ export function openapi(origin: string, version: string): Record<string, unknown | ||
| 86 | 92 | }), |
| 87 | 93 | }, |
| 88 | 94 | ...Object.fromEntries((r.alsoStatus ?? []).map((c) => [String(c), { description: "Done" }])), |
| 95 | ...(pagesByCursor(r) ? { "400": { description: PAGE_REFUSAL } } : {}), | |
| 89 | 96 | default: { |
| 90 | 97 | description: |
| 91 | 98 | "An error: { error, message }; `error` is a stable code" + |
| 92 | (r.refusals === undefined | |
| 99 | (refusals === undefined | |
| 93 | 100 | ? "" |
| 94 | : `. Refusals: ${Object.entries(r.refusals).map(([c, w]) => `\`${c}\` (${w})`).join("; ")}`), | |
| 101 | : `. Refusals: ${Object.entries(refusals).map(([c, w]) => `\`${c}\` (${w})`).join("; ")}`), | |
| 95 | 102 | }, |
| 96 | 103 | }, |
| 97 | 104 | }; |
src/contract/openapi/routes/getApiV1ReposOwnerNameMilestones.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import { REPO, type Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/milestones`, summary: "Read the plan: { items, next_cursor }, one limb per milestone with its weights (and askToLive for a pillar's umbrella); limit 1–200 (default 50), cursor is the next_cursor of the page before; a private repository needs a token", auth: false }; | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/milestones`, summary: "Read the plan: { items, next_cursor }, one limb per milestone with its weights (and askToLive for a pillar's umbrella); limit 1–200 (default 50), cursor is the next_cursor of the page before; a private repository needs a token", auth: false, query: { limit: "integer", cursor: "string" } }; |
src/contract/openapi/routes/getApiV1ReposOwnerNameTags.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import { REPO, type Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/tags`, summary: "List a repository's tags, newest first; answers { items: [{ name, oid, commit }], next_cursor }; limit 1 to 200 (default 50), cursor from next_cursor; a bad limit or cursor is a 422", auth: false }; | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/tags`, summary: "List a repository's tags, newest first; answers { items: [{ name, oid, commit }], next_cursor }; limit 1 to 200 (default 50), cursor from next_cursor; a bad limit or cursor is a 422", auth: false, query: { limit: "integer", cursor: "string" } }; |
src/http/cursor_lists.ts added +23
| @@ -0,0 +1,23 @@ | ||
| 1 | /** The one guard for `?page=` and `?offset=`: a list the contract says pages by `cursor` refuses them, so no handler can forget (#8386). */ | |
| 2 | ||
| 3 | import { pagesByCursor, ROUTES } from "../contract/openapi.ts"; | |
| 4 | import { apiError } from "./respond.ts"; | |
| 5 | ||
| 6 | export const PAGE_PARAM_SAYS = "This list pages by cursor: pass next_cursor as ?cursor=."; | |
| 7 | ||
| 8 | /** The router pattern of a contract path: `{owner}` is `:owner`. */ | |
| 9 | const patternOf = (path: string): string => path.replace(/\{(\w+)\}/g, ":$1"); | |
| 10 | ||
| 11 | const CURSOR_LISTS: ReadonlySet<string> = new Set( | |
| 12 | ROUTES.filter(pagesByCursor).map((r) => patternOf(r.path)), | |
| 13 | ); | |
| 14 | ||
| 15 | /** True when the caller paged the way most APIs do (`?page=` or `?offset=`). */ | |
| 16 | export function pagedByNumber(q: URLSearchParams): boolean { | |
| 17 | return q.has("page") || q.has("offset"); | |
| 18 | } | |
| 19 | ||
| 20 | /** A 400 when this route pages by cursor and the query pages by number; undefined otherwise. */ | |
| 21 | export function pageParamRefused(pattern: string, q: URLSearchParams): Response | undefined { | |
| 22 | return CURSOR_LISTS.has(pattern) && pagedByNumber(q) ? apiError("invalid_query", 400, PAGE_PARAM_SAYS) : undefined; | |
| 23 | } |
src/http/server.ts +7
| @@ -498,6 +498,7 @@ import { pushThrough } from "../origins/push-through.ts"; | ||
| 498 | 498 | import { notFoundPage } from "../web/chrome.ts"; |
| 499 | 499 | import { targetForShortCode } from "../origins/oauth.ts"; |
| 500 | 500 | import { apiRoutes } from "./api.ts"; |
| 501 | import { pageParamRefused } from "./cursor_lists.ts"; | |
| 501 | 502 | import { inNamespace } from "../core/proposal_token.ts"; |
| 502 | 503 | import { proposalTokenMint, proposalTokenRevoke } from "../web/pages/proposal-tokens.ts"; |
| 503 | 504 | import { adminGate } from "./admin_gate.ts"; |
| @@ -2729,6 +2730,12 @@ async function answerGet( | ||
| 2729 | 2730 | ); |
| 2730 | 2731 | } |
| 2731 | 2732 | |
| 2733 | // A list that pages by cursor says so to `?page=`, one guard for all of them. | |
| 2734 | if (isApiPath && routeMethod === "GET") { | |
| 2735 | const byNumber = pageParamRefused(matched.pattern, url.searchParams); | |
| 2736 | if (byNumber !== undefined) return withRateHeaders(byNumber, verdict); | |
| 2737 | } | |
| 2738 | ||
| 2732 | 2739 | const themeCookie = cookieValue(request, "forge_theme") ?? ""; |
| 2733 | 2740 | if (!isApiPath) { |
| 2734 | 2741 | const clean = withoutMode(url, request, app.config.url); |
test/api-page-param.test.ts added +90
| @@ -0,0 +1,90 @@ | ||
| 1 | /** A cursor-paged API list refuses `?page=` and `?offset=` with a 400 that says how it pages (#8386). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { createRepo } from "../src/actions/repos.ts"; | |
| 8 | import { createSession } from "../src/core/sessions.ts"; | |
| 9 | import { createUser } from "../src/core/users.ts"; | |
| 10 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 11 | import { openapi, pagesByCursor, ROUTES } from "../src/contract/openapi.ts"; | |
| 12 | import { pageParamRefused } from "../src/http/cursor_lists.ts"; | |
| 13 | import { quietLogs } from "./helpers/globals.ts"; | |
| 14 | ||
| 15 | quietLogs(); | |
| 16 | ||
| 17 | let dataDir: string; | |
| 18 | let app: App; | |
| 19 | let servers: Servers; | |
| 20 | let cookie = ""; | |
| 21 | const cells = process.env["FORGE_CELLS"]; | |
| 22 | const SAYS = { error: "invalid_query", message: "This list pages by cursor: pass next_cursor as ?cursor=." }; | |
| 23 | ||
| 24 | beforeAll(async () => { | |
| 25 | process.env["FORGE_CELLS"] = "off"; | |
| 26 | dataDir = await mkdtemp(join(tmpdir(), "forge-api-page-param-")); | |
| 27 | app = createApp({ dataDir, port: 0 }); | |
| 28 | servers = await serve(app); | |
| 29 | const made = await createUser(app.db, { | |
| 30 | origin: "seed", | |
| 31 | trusted: true, | |
| 32 | username: "alice", | |
| 33 | email: "[email protected]", | |
| 34 | password: "a-good-passphrase", | |
| 35 | adminIfFirst: true, | |
| 36 | }); | |
| 37 | if (!made.ok) throw new Error(String(made.error)); | |
| 38 | cookie = `forge_session=${createSession(app.db, made.value.id).token}`; | |
| 39 | const repo = await createRepo(app, made.value, { name: "r", visibility: "public" }); | |
| 40 | if (!repo.ok) throw new Error(String(repo.error)); | |
| 41 | }); | |
| 42 | ||
| 43 | afterAll(async () => { | |
| 44 | await servers.stop(); | |
| 45 | app.close(); | |
| 46 | if (cells === undefined) delete process.env["FORGE_CELLS"]; | |
| 47 | else process.env["FORGE_CELLS"] = cells; | |
| 48 | await rm(dataDir, { recursive: true, force: true }); | |
| 49 | }); | |
| 50 | ||
| 51 | const get = (path: string) => fetch(`${servers.url}${path}`, { headers: { cookie } }); | |
| 52 | ||
| 53 | const lists = [ | |
| 54 | "/api/v1/repos/alice/r/issues", | |
| 55 | "/api/v1/repos/alice/r/pulls", | |
| 56 | "/api/v1/repos/alice/r/tags", | |
| 57 | "/api/v1/repos/alice/r/milestones", | |
| 58 | "/api/v1/admin/accounts", | |
| 59 | "/api/v1/admin/people", | |
| 60 | "/api/v1/admin/invites", | |
| 61 | "/api/v1/explore", | |
| 62 | ]; | |
| 63 | ||
| 64 | for (const path of lists) { | |
| 65 | test(`${path} refuses page and offset, and still pages by cursor`, async () => { | |
| 66 | for (const param of ["page=2", "offset=50"]) { | |
| 67 | const res = await get(`${path}?limit=100&${param}`); | |
| 68 | expect(res.status).toBe(400); | |
| 69 | expect(await res.json()).toEqual(SAYS); | |
| 70 | } | |
| 71 | expect((await get(`${path}?foo=1`)).status).toBe(200); | |
| 72 | }); | |
| 73 | } | |
| 74 | ||
| 75 | test("a list that does not page by cursor ignores page", async () => { | |
| 76 | expect((await get("/api/v1/user")).status).not.toBe(400); | |
| 77 | }); | |
| 78 | ||
| 79 | test("the contract names the 400 on every route the guard covers", () => { | |
| 80 | const doc = openapi("http://x", "0") as { paths: Record<string, Record<string, { responses: Record<string, unknown> }>> }; | |
| 81 | let seen = 0; | |
| 82 | for (const r of ROUTES) { | |
| 83 | const refused = r.method !== "get" ? undefined : pageParamRefused(r.path.replace(/\{(\w+)\}/g, ":$1"), new URLSearchParams("page=2")); | |
| 84 | expect(refused !== undefined).toBe(pagesByCursor(r)); | |
| 85 | if (!pagesByCursor(r)) continue; | |
| 86 | seen++; | |
| 87 | expect(doc.paths[r.path]?.[r.method]?.responses["400"]).toBeDefined(); | |
| 88 | } | |
| 89 | expect(seen).toBeGreaterThan(10); | |
| 90 | }); |
new above
offline