Public: anyone can read this; the crew can change it.
A list route given a cursor or state it cannot read answers 422, never page one again #8624
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8504-a-list-route-given-a-cursor-or-state-it → main 1 commit fork
6 files
changed +138
−11
src/contract/openapi/routes/getApiV1Repos.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import type { Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/repos", summary: "List public repositories", auth: false, query: { limit: "integer", cursor: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)" } }; | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/repos", summary: "List public repositories", auth: false, query: { limit: "integer", cursor: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)", invalid_cursor: "cursor is not the next_cursor of a previous page (422)" } }; |
src/contract/openapi/routes/getApiV1ReposOwnerNameIssues.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import { LIST_QUERY, REPO, type Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/issues`, summary: "List a repository's issues; fields=brief leaves out bodies and comments, any other fields value is a 422 invalid_fields", auth: false, query: { ...LIST_QUERY, fields: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)" } }; | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/issues`, summary: "List a repository's issues; fields=brief leaves out bodies and comments, any other fields value is a 422 invalid_fields", auth: false, query: { ...LIST_QUERY, fields: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)", invalid_cursor: "cursor is not the next_cursor of a previous page (422)", invalid_state: "state is not open, closed or all (422)" } }; |
src/contract/openapi/routes/getApiV1ReposOwnerNamePulls.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import { LIST_QUERY, REPO, type Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/pulls`, summary: "List a repository's pulls", auth: false, query: LIST_QUERY, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)" } }; | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/pulls`, summary: "List a repository's pulls", auth: false, query: LIST_QUERY, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)", invalid_cursor: "cursor is not the next_cursor of a previous page (422)", invalid_state: "state is not open, closed or all (422)" } }; |
src/contract/openapi/routes/getApiV1UsersUsernameRepos.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import type { Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/users/{username}/repos", summary: "List the repositories a person owns that the caller may see (private ones only to those with access); answers { items, next_cursor }, paged by `limit` and `cursor`", auth: false, query: { limit: "integer", cursor: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)" } }; | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/users/{username}/repos", summary: "List the repositories a person owns that the caller may see (private ones only to those with access); answers { items, next_cursor }, paged by `limit` and `cursor`", auth: false, query: { limit: "integer", cursor: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)", invalid_cursor: "cursor is not the next_cursor of a previous page (422)" } }; |
src/http/api.ts +10 −7
| @@ -839,17 +839,18 @@ export function listQuery(ctx: Ctx) { | ||
| 839 | 839 | const limit = readLimit(ctx, undefined); |
| 840 | 840 | if (limit instanceof Response) return limit; |
| 841 | 841 | const stateParam = q.get("state"); |
| 842 | const state: IssueState | "all" = | |
| 843 | stateParam === "closed" || stateParam === "all" ? stateParam : "open"; | |
| 844 | const cursorParam = Number(q.get("cursor")); | |
| 842 | if (stateParam !== null && stateParam !== "open" && stateParam !== "closed" && stateParam !== "all") | |
| 843 | return apiError("invalid_state", 422, "state is open, closed or all"); | |
| 844 | const state: IssueState | "all" = stateParam ?? "open"; | |
| 845 | const rawCursor = q.get("cursor"); | |
| 846 | if (rawCursor !== null && !/^[1-9]\d{0,15}$/.test(rawCursor)) | |
| 847 | return apiError("invalid_cursor", 422, "cursor is the next_cursor of a previous page"); | |
| 845 | 848 | const label = q.get("label"); |
| 846 | 849 | const author = q.get("author"); |
| 847 | 850 | return { |
| 848 | 851 | state, |
| 849 | 852 | limit: pageLimit(limit), |
| 850 | ...(Number.isInteger(cursorParam) && cursorParam > 0 | |
| 851 | ? { cursor: cursorParam } | |
| 852 | : {}), | |
| 853 | ...(rawCursor !== null ? { cursor: Number(rawCursor) } : {}), | |
| 853 | 854 | ...(label ? { label } : {}), |
| 854 | 855 | ...(author ? { author } : {}), |
| 855 | 856 | }; |
| @@ -935,11 +936,13 @@ export function keyedPage<T>( | ||
| 935 | 936 | function repoQuery(ctx: Ctx) { |
| 936 | 937 | const q = ctx.url.searchParams; |
| 937 | 938 | const cursor = q.get("cursor"); |
| 939 | if (cursor !== null && !/^[A-Za-z0-9_-]{1,64}$/.test(cursor)) | |
| 940 | return apiError("invalid_cursor", 422, "cursor is the next_cursor of a previous page"); | |
| 938 | 941 | const limit = readLimit(ctx, undefined); |
| 939 | 942 | if (limit instanceof Response) return limit; |
| 940 | 943 | return { |
| 941 | 944 | limit: pageLimit(limit), |
| 942 | ...(cursor ? { cursor } : {}), | |
| 945 | ...(cursor !== null ? { cursor } : {}), | |
| 943 | 946 | }; |
| 944 | 947 | } |
| 945 | 948 |
test/list-cursor-state.test.ts added +124
| @@ -0,0 +1,124 @@ | ||
| 1 | /** | |
| 2 | * A list route given a cursor or state it cannot read answers 422 (#8504): | |
| 3 | * an agent paging with a mangled cursor is stopped, not handed page one again. | |
| 4 | */ | |
| 5 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 6 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 7 | import { tmpdir } from "node:os"; | |
| 8 | import { join } from "node:path"; | |
| 9 | import { createRepo } from "../src/actions/repos.ts"; | |
| 10 | import { createApp, type App } from "../src/app.ts"; | |
| 11 | import { createIssue } from "../src/core/issues.ts"; | |
| 12 | import { createToken } from "../src/core/tokens.ts"; | |
| 13 | import { createUser } from "../src/core/users.ts"; | |
| 14 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 15 | ||
| 16 | describe("list cursors and states are validated", () => { | |
| 17 | let dataDir: string; | |
| 18 | let app: App; | |
| 19 | let servers: Servers; | |
| 20 | let headers: Record<string, string>; | |
| 21 | ||
| 22 | beforeAll(async () => { | |
| 23 | dataDir = await mkdtemp(join(tmpdir(), "forge-list-cursor-")); | |
| 24 | app = createApp({ dataDir, port: 0 }); | |
| 25 | servers = await serve(app); | |
| 26 | const made = await createUser(app.db, { | |
| 27 | origin: "seed", | |
| 28 | trusted: true, | |
| 29 | username: "alice", | |
| 30 | email: "[email protected]", | |
| 31 | password: "a-good-passphrase", | |
| 32 | adminIfFirst: true, | |
| 33 | }); | |
| 34 | if (!made.ok) throw new Error(made.error); | |
| 35 | headers = { authorization: `Bearer ${createToken(app.db, made.value.id, "t", "admin").token}` }; | |
| 36 | const lib = await createRepo(app, made.value, { name: "lib", visibility: "public" }); | |
| 37 | if (!lib.ok) throw new Error(String(lib.error)); | |
| 38 | for (let n = 1; n <= 5; n++) { | |
| 39 | createIssue(app.db, { repoId: lib.value.id, authorId: made.value.id, title: `issue ${n}` }); | |
| 40 | const pull = createIssue(app.db, { | |
| 41 | repoId: lib.value.id, | |
| 42 | authorId: made.value.id, | |
| 43 | title: `pull ${n}`, | |
| 44 | kind: "pull", | |
| 45 | }); | |
| 46 | if (!pull.ok) throw new Error(String(pull.error)); | |
| 47 | app.db | |
| 48 | .query( | |
| 49 | `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id) | |
| 50 | VALUES (?1, 'main', ?2, 'a', 'b', ?3)`, | |
| 51 | ) | |
| 52 | .run(pull.value.id, `topic-${n}`, lib.value.id); | |
| 53 | const more = await createRepo(app, made.value, { name: `r${n}`, visibility: "public" }); | |
| 54 | if (!more.ok) throw new Error(String(more.error)); | |
| 55 | } | |
| 56 | }); | |
| 57 | ||
| 58 | afterAll(async () => { | |
| 59 | await servers.stop(); | |
| 60 | await rm(dataDir, { recursive: true, force: true }); | |
| 61 | }); | |
| 62 | ||
| 63 | const get = (path: string) => fetch(`${servers.url}/api/v1${path}`, { headers }); | |
| 64 | ||
| 65 | test("issue and pull lists refuse a cursor that is not a positive integer", async () => { | |
| 66 | for (const route of ["/repos/alice/lib/issues", "/repos/alice/lib/pulls"]) | |
| 67 | for (const bad of ["abc", "0", "-1", "1.5", "", "1e3", "%25%25"]) { | |
| 68 | const res = await get(`${route}?cursor=${bad}`); | |
| 69 | expect(res.status).toBe(422); | |
| 70 | expect(await res.json()).toEqual({ | |
| 71 | error: "invalid_cursor", | |
| 72 | message: "cursor is the next_cursor of a previous page", | |
| 73 | }); | |
| 74 | } | |
| 75 | }); | |
| 76 | ||
| 77 | test("issue and pull lists refuse a state other than open, closed or all", async () => { | |
| 78 | for (const route of ["/repos/alice/lib/issues", "/repos/alice/lib/pulls"]) { | |
| 79 | for (const bad of ["bogus", "Closed", "", "OPEN"]) { | |
| 80 | const res = await get(`${route}?state=${bad}`); | |
| 81 | expect(res.status).toBe(422); | |
| 82 | expect(await res.json()).toEqual({ error: "invalid_state", message: "state is open, closed or all" }); | |
| 83 | } | |
| 84 | for (const ok of ["", "?state=open", "?state=closed", "?state=all"]) | |
| 85 | expect((await get(`${route}${ok}`)).status).toBe(200); | |
| 86 | } | |
| 87 | }); | |
| 88 | ||
| 89 | test("repository lists refuse a cursor outside the id alphabet", async () => { | |
| 90 | for (const route of ["/repos", "/users/alice/repos"]) | |
| 91 | for (const bad of ["%25%25", "a.b", "", "a".repeat(65), "a%20b"]) { | |
| 92 | const res = await get(`${route}?cursor=${bad}`); | |
| 93 | expect(res.status).toBe(422); | |
| 94 | expect(await res.json()).toEqual({ | |
| 95 | error: "invalid_cursor", | |
| 96 | message: "cursor is the next_cursor of a previous page", | |
| 97 | }); | |
| 98 | } | |
| 99 | }); | |
| 100 | ||
| 101 | test("following next_cursor walks every page of each list", async () => { | |
| 102 | for (const [route, total] of [ | |
| 103 | ["/repos/alice/lib/issues", 5], | |
| 104 | ["/repos/alice/lib/pulls", 5], | |
| 105 | ["/repos", 6], | |
| 106 | ] as const) { | |
| 107 | const seen: unknown[] = []; | |
| 108 | let cursor: string | null = null; | |
| 109 | let pages = 0; | |
| 110 | do { | |
| 111 | const res = await get(`${route}?limit=2${cursor === null ? "" : `&cursor=${encodeURIComponent(cursor)}`}`); | |
| 112 | expect(res.status).toBe(200); | |
| 113 | const body = (await res.json()) as { items: { id: string }[]; next_cursor: string | number | null }; | |
| 114 | seen.push(...body.items.map((i) => i.id)); | |
| 115 | cursor = body.next_cursor === null ? null : String(body.next_cursor); | |
| 116 | pages++; | |
| 117 | } while (cursor !== null && pages < 10); | |
| 118 | expect(pages).toBeGreaterThanOrEqual(Math.ceil(total / 2)); | |
| 119 | expect(pages).toBeLessThan(10); | |
| 120 | expect(seen).toHaveLength(total); | |
| 121 | expect(new Set(seen).size).toBe(total); | |
| 122 | } | |
| 123 | }); | |
| 124 | }); |
new above
offline