Public: anyone can read this; the crew can change it.
A list route given a limit that is not a whole number from 1 to 200 answers 422 invalid_limit, not a silent clamp #8505
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8490-a-list-route-given-a-limit-that-is-not-a → main 1 commit fork
10 files
changed +65
−26
scripts/cold-read.ts +1 −1
| @@ -137,7 +137,7 @@ if (parent !== undefined) { | ||
| 137 | 137 | console.error(early.error); |
| 138 | 138 | process.exit(2); |
| 139 | 139 | } |
| 140 | const list = await fetch(`${early.url}?state=open&limit=500`, { headers: auth }); | |
| 140 | const list = await fetch(`${early.url}?state=open&limit=200`, { headers: auth }); | |
| 141 | 141 | if (!list.ok) { |
| 142 | 142 | console.error(`could not list open issues (${list.status}); filing nothing`); |
| 143 | 143 | process.exit(1); |
src/contract/openapi/routes/getApiV1FleetOps.ts +1
| @@ -6,4 +6,5 @@ export const route: Route = { | ||
| 6 | 6 | summary: "List the fleet operations, newest first; answers { items, next_cursor }", |
| 7 | 7 | auth: true, |
| 8 | 8 | query: { limit: "integer", cursor: "string" }, |
| 9 | refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)" }, | |
| 9 | 10 | }; |
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 }; | |
| 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)" } }; |
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" } }; | |
| 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)" } }; |
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 }; | |
| 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)" } }; |
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" } }; | |
| 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)" } }; |
src/http/api.ts +34 −14
| @@ -818,9 +818,26 @@ export function contributeFailure(error: unknown): Response { | ||
| 818 | 818 | |
| 819 | 819 | // ---- query parsing ---- |
| 820 | 820 | |
| 821 | /** | |
| 822 | * `?limit=`: absent is `fallback`, a whole number from 1 to 200 is itself, | |
| 823 | * anything else is the 422 to answer. | |
| 824 | */ | |
| 825 | export function readLimit(ctx: Ctx, fallback: number): number | Response; | |
| 826 | export function readLimit(ctx: Ctx, fallback: undefined): number | undefined | Response; | |
| 827 | export function readLimit(ctx: Ctx, fallback: number | undefined): number | undefined | Response { | |
| 828 | const rawLimit = ctx.url.searchParams.get("limit"); | |
| 829 | if (rawLimit === null) return fallback; | |
| 830 | const limit = /^\d{1,4}$/.test(rawLimit) ? Number(rawLimit) : 0; | |
| 831 | if (limit < 1 || limit > 200) | |
| 832 | return apiError("invalid_limit", 422, "limit is a whole number from 1 to 200"); | |
| 833 | return limit; | |
| 834 | } | |
| 835 | ||
| 821 | 836 | /** `?state=&label=&author=&limit=&cursor=` for issue and pull lists. */ |
| 822 | 837 | export function listQuery(ctx: Ctx) { |
| 823 | 838 | const q = ctx.url.searchParams; |
| 839 | const limit = readLimit(ctx, undefined); | |
| 840 | if (limit instanceof Response) return limit; | |
| 824 | 841 | const stateParam = q.get("state"); |
| 825 | 842 | const state: IssueState | "all" = |
| 826 | 843 | stateParam === "closed" || stateParam === "all" ? stateParam : "open"; |
| @@ -829,7 +846,7 @@ export function listQuery(ctx: Ctx) { | ||
| 829 | 846 | const author = q.get("author"); |
| 830 | 847 | return { |
| 831 | 848 | state, |
| 832 | limit: pageLimit(q.has("limit") ? Number(q.get("limit")) : undefined), | |
| 849 | limit: pageLimit(limit), | |
| 833 | 850 | ...(Number.isInteger(cursorParam) && cursorParam > 0 |
| 834 | 851 | ? { cursor: cursorParam } |
| 835 | 852 | : {}), |
| @@ -872,11 +889,8 @@ export function pageParams( | ||
| 872 | 889 | ctx: Ctx, |
| 873 | 890 | ): { limit: number; cursor: string | null } | Response { |
| 874 | 891 | const q = ctx.url.searchParams; |
| 875 | const rawLimit = q.get("limit"); | |
| 876 | const limit = | |
| 877 | rawLimit === null ? 50 : /^\d{1,4}$/.test(rawLimit) ? Number(rawLimit) : 0; | |
| 878 | if (limit < 1 || limit > 200) | |
| 879 | return apiError("invalid_limit", 422, "limit is a whole number from 1 to 200"); | |
| 892 | const limit = readLimit(ctx, 50); | |
| 893 | if (limit instanceof Response) return limit; | |
| 880 | 894 | const cursor = q.get("cursor"); |
| 881 | 895 | if (cursor !== null && !/^[A-Za-z0-9_-]{1,64}$/.test(cursor)) |
| 882 | 896 | return apiError("invalid_cursor", 422, "cursor is the next_cursor of a previous page"); |
| @@ -921,8 +935,10 @@ export function keyedPage<T>( | ||
| 921 | 935 | function repoQuery(ctx: Ctx) { |
| 922 | 936 | const q = ctx.url.searchParams; |
| 923 | 937 | const cursor = q.get("cursor"); |
| 938 | const limit = readLimit(ctx, undefined); | |
| 939 | if (limit instanceof Response) return limit; | |
| 924 | 940 | return { |
| 925 | limit: pageLimit(q.has("limit") ? Number(q.get("limit")) : undefined), | |
| 941 | limit: pageLimit(limit), | |
| 926 | 942 | ...(cursor ? { cursor } : {}), |
| 927 | 943 | }; |
| 928 | 944 | } |
| @@ -1115,6 +1131,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1115 | 1131 | const user = getUserByName(ctx.app.db, ctx.params.username!); |
| 1116 | 1132 | if (!user || hiddenFrom(user, ctx.auth.user)) return notFound(); |
| 1117 | 1133 | const query = repoQuery(ctx); |
| 1134 | if (query instanceof Response) return query; | |
| 1118 | 1135 | const items = timePhase(ctx.request, "repos", () => |
| 1119 | 1136 | listReposByOwner(ctx.app.db, user.username, ctx.auth.user, query), |
| 1120 | 1137 | ); |
| @@ -1405,6 +1422,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1405 | 1422 | router.add("GET", "/api/v1/repos", (ctx) => { |
| 1406 | 1423 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 1407 | 1424 | const query = repoQuery(ctx); |
| 1425 | if (query instanceof Response) return query; | |
| 1408 | 1426 | const items = timePhase(ctx.request, "repos", () => listReposVisibleTo(ctx.app.db, ctx.auth.user, query)); |
| 1409 | 1427 | return timePhase(ctx.request, "render", () => json(paged(items, query.limit, (r) => r.id))); |
| 1410 | 1428 | }); |
| @@ -2242,14 +2260,14 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 2242 | 2260 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 2243 | 2261 | const repo = loadRepo(ctx); |
| 2244 | 2262 | if (!repo) return notFound(); |
| 2245 | const { query, pulls } = timePhase(ctx.request, "list", () => { | |
| 2246 | const query = listQuery(ctx); | |
| 2247 | const pulls = listPulls(ctx.app.db, repo.id, { | |
| 2263 | const query = listQuery(ctx); | |
| 2264 | if (query instanceof Response) return query; | |
| 2265 | const pulls = timePhase(ctx.request, "list", () => | |
| 2266 | listPulls(ctx.app.db, repo.id, { | |
| 2248 | 2267 | ...query, |
| 2249 | 2268 | ...sealedScope(ctx.app.db, repo, ctx.auth.user), |
| 2250 | }); | |
| 2251 | return { query, pulls }; | |
| 2252 | }); | |
| 2269 | }), | |
| 2270 | ); | |
| 2253 | 2271 | const states = await timePhaseAsync(ctx.request, "merge", async () => |
| 2254 | 2272 | pullMergeStates( |
| 2255 | 2273 | await Promise.all( |
| @@ -3022,7 +3040,9 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3022 | 3040 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 3023 | 3041 | if (!ctx.auth.user) return unauthorized(); |
| 3024 | 3042 | const q = ctx.url.searchParams; |
| 3025 | const limit = pageLimit(q.has("limit") ? Number(q.get("limit")) : undefined); | |
| 3043 | const asked = readLimit(ctx, undefined); | |
| 3044 | if (asked instanceof Response) return asked; | |
| 3045 | const limit = pageLimit(asked); | |
| 3026 | 3046 | const ops = timePhase(ctx.request, "ops", () => |
| 3027 | 3047 | listFleetOps(ctx.app, ctx.auth.user!, limit, q.get("cursor") ?? undefined), |
| 3028 | 3048 | ); |
src/http/api/issues.ts +1
| @@ -137,6 +137,7 @@ export function issueRoutes(router: Router<Handler>): void { | ||
| 137 | 137 | if (fields !== null && fields !== "brief") |
| 138 | 138 | return apiError("invalid_fields", 422, "fields must be brief, or left out"); |
| 139 | 139 | const query = listQuery(ctx); |
| 140 | if (query instanceof Response) return query; | |
| 140 | 141 | const listed = timePhase(ctx.request, "list", () => |
| 141 | 142 | listIssues(ctx.app.db, repo.id, { |
| 142 | 143 | ...query, |
test/list-pages.test.ts +18
| @@ -138,6 +138,24 @@ describe("forks, labels and webhooks are pages", () => { | ||
| 138 | 138 | } |
| 139 | 139 | }); |
| 140 | 140 | |
| 141 | test("repo, pull, issue and fleet-op lists refuse a bad limit with a 422, and take the bounds", async () => { | |
| 142 | const base = `${servers.url}/api/v1`; | |
| 143 | const headers = { authorization: `Bearer ${tokens["alice"]}` }; | |
| 144 | const paths = ["/repos", "/users/alice/repos", "/repos/alice/lib/pulls", "/repos/alice/lib/issues", "/fleet/ops"]; | |
| 145 | for (const path of paths) { | |
| 146 | for (const bad of ["abc", "0", "-1", "1.5", "201", ""]) { | |
| 147 | const res = await fetch(`${base}${path}?limit=${bad}`, { headers }); | |
| 148 | expect(res.status).toBe(422); | |
| 149 | expect(await res.json()).toEqual({ | |
| 150 | error: "invalid_limit", | |
| 151 | message: "limit is a whole number from 1 to 200", | |
| 152 | }); | |
| 153 | } | |
| 154 | for (const ok of ["", "?limit=1", "?limit=200"]) | |
| 155 | expect((await fetch(`${base}${path}${ok}`, { headers })).status).toBe(200); | |
| 156 | } | |
| 157 | }); | |
| 158 | ||
| 141 | 159 | test("deleting the row a page ended on makes the next read start over", async () => { |
| 142 | 160 | const first = await get("/labels?limit=1", "alice"); |
| 143 | 161 | expect(first.body.next_cursor).toBeTruthy(); |
test/parity.test.ts +6 −7
| @@ -1089,15 +1089,14 @@ describe("parity: full control through actions and the API", () => { | ||
| 1089 | 1089 | expect(seen).toHaveLength(total); |
| 1090 | 1090 | expect([...seen].sort((a, b) => b - a)).toEqual(seen); |
| 1091 | 1091 | expect(new Set(seen).size).toBe(total); |
| 1092 | // Limits are clamped, never trusted. | |
| 1092 | // Limits are validated, never trusted: past the ceiling is a 422. | |
| 1093 | expect( | |
| 1094 | (await call("GET", "/api/v1/repos/alice/pub/issues?state=all&limit=99999")).status, | |
| 1095 | ).toBe(422); | |
| 1093 | 1096 | expect( |
| 1094 | 1097 | ( |
| 1095 | ( | |
| 1096 | await call( | |
| 1097 | "GET", | |
| 1098 | "/api/v1/repos/alice/pub/issues?state=all&limit=99999", | |
| 1099 | ) | |
| 1100 | ).json.items as Json[] | |
| 1098 | (await call("GET", "/api/v1/repos/alice/pub/issues?state=all&limit=200")) | |
| 1099 | .json.items as Json[] | |
| 1101 | 1100 | ).length, |
| 1102 | 1101 | ).toBe(total); |
| 1103 | 1102 |
new above
offline