Public: anyone can read this; the crew can change it.
The API's own root answers 33 KB of HTML to a client that asked for JSON #314
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/279-the-api-s-own-root-answers-33-kb-of-html → main 1 commit fork
3 files
changed +86
−14
src/http/respond.ts +1 −1
| @@ -87,7 +87,7 @@ function acceptQuality(accept: string, type: string): number { | ||
| 87 | 87 | * not a script or `curl` that never said otherwise. |
| 88 | 88 | */ |
| 89 | 89 | export function prefersHtml(request: Request, pathname: string): boolean { |
| 90 | if (pathname.startsWith("/api/")) return false; | |
| 90 | if (pathname === "/api" || pathname.startsWith("/api/")) return false; | |
| 91 | 91 | const accept = request.headers.get("accept"); |
| 92 | 92 | if (!accept) return false; |
| 93 | 93 | return ( |
src/http/server.ts +38 −13
| @@ -536,8 +536,9 @@ export function buildRouter(): Router<Handler> { | ||
| 536 | 536 | ), |
| 537 | 537 | ); |
| 538 | 538 | |
| 539 | // The JSON API lives in api.ts; its routes all sit under /api/v1. | |
| 540 | apiRoutes(router); | |
| 539 | // The JSON API lives in api.ts, on its own router (`buildApiRouter` | |
| 540 | // below) — never here, so a `/:owner`-shaped studio route can never | |
| 541 | // reclaim a path under `/api/`. | |
| 541 | 542 | |
| 542 | 543 | // Issues (web). The literal /new is registered before /:number so it can |
| 543 | 544 | // never be captured as a number. |
| @@ -746,6 +747,18 @@ export function buildRouter(): Router<Handler> { | ||
| 746 | 747 | return router; |
| 747 | 748 | } |
| 748 | 749 | |
| 750 | /** | |
| 751 | * The JSON API's own router, matched before the studio's when a path starts | |
| 752 | * with `/api` — so a miss under `/api/` can never fall through to a | |
| 753 | * `/:owner`-shaped page route and answer with a repo's or a profile's 404 | |
| 754 | * page instead of the API's own. | |
| 755 | */ | |
| 756 | export function buildApiRouter(): Router<Handler> { | |
| 757 | const router = new Router<Handler>(); | |
| 758 | apiRoutes(router); | |
| 759 | return router; | |
| 760 | } | |
| 761 | ||
| 749 | 762 | async function withGitAccess( |
| 750 | 763 | ctx: Ctx, |
| 751 | 764 | service: GitService, |
| @@ -1120,6 +1133,7 @@ async function clearStaleSockets(app: App): Promise<void> { | ||
| 1120 | 1133 | |
| 1121 | 1134 | export async function serve(app: App): Promise<Servers> { |
| 1122 | 1135 | const router = buildRouter(); |
| 1136 | const apiRouter = buildApiRouter(); | |
| 1123 | 1137 | await clearStaleSockets(app); |
| 1124 | 1138 | // One limiter per listener: its state is a minute of timestamps, and two |
| 1125 | 1139 | // forges in one process (every test file) must not share a budget. |
| @@ -1147,7 +1161,9 @@ export async function serve(app: App): Promise<Servers> { | ||
| 1147 | 1161 | } |
| 1148 | 1162 | // Everything the forge answers for itself leaves through this one |
| 1149 | 1163 | // door, so a responder added next year cannot forget the floor. |
| 1150 | return securityFloor(await answer(app, router, limiter, request)); | |
| 1164 | return securityFloor( | |
| 1165 | await answer(app, router, apiRouter, limiter, request), | |
| 1166 | ); | |
| 1151 | 1167 | }, |
| 1152 | 1168 | error(error) { |
| 1153 | 1169 | app.log.error("unhandled request error", { error: String(error) }); |
| @@ -1193,10 +1209,11 @@ export async function serve(app: App): Promise<Servers> { | ||
| 1193 | 1209 | async function answer( |
| 1194 | 1210 | app: App, |
| 1195 | 1211 | router: Router<Handler>, |
| 1212 | apiRouter: Router<Handler>, | |
| 1196 | 1213 | limiter: RateLimiter, |
| 1197 | 1214 | request: Request, |
| 1198 | 1215 | ): Promise<Response> { |
| 1199 | const response = await answerGet(app, router, limiter, request); | |
| 1216 | const response = await answerGet(app, router, apiRouter, limiter, request); | |
| 1200 | 1217 | return request.method === "HEAD" |
| 1201 | 1218 | ? new Response(null, { status: response.status, headers: response.headers }) |
| 1202 | 1219 | : response; |
| @@ -1205,24 +1222,32 @@ async function answer( | ||
| 1205 | 1222 | async function answerGet( |
| 1206 | 1223 | app: App, |
| 1207 | 1224 | router: Router<Handler>, |
| 1225 | apiRouter: Router<Handler>, | |
| 1208 | 1226 | limiter: RateLimiter, |
| 1209 | 1227 | request: Request, |
| 1210 | 1228 | ): Promise<Response> { |
| 1211 | 1229 | const url = publicUrl(request, app.config.url); |
| 1212 | 1230 | if (url.pathname === "/healthz") return healthResponse(); |
| 1213 | 1231 | const routeMethod = request.method === "HEAD" ? "GET" : request.method; |
| 1214 | const matched = router.match(routeMethod, url.pathname); | |
| 1232 | // The one place a path decides its own shape of miss: anything under | |
| 1233 | // `/api` matches only against the API's own routes, so a studio route as | |
| 1234 | // generic as `/:owner/:name` can never claim it — a route added there | |
| 1235 | // next year still can't bring the HTML 404 back for an API caller. | |
| 1236 | const isApiPath = url.pathname === "/api" || url.pathname.startsWith("/api/"); | |
| 1237 | const matched = isApiPath | |
| 1238 | ? apiRouter.match(routeMethod, url.pathname) | |
| 1239 | : router.match(routeMethod, url.pathname); | |
| 1215 | 1240 | if (!matched) { |
| 1216 | 1241 | // A path under no route at all is what a mistyped or stale studio |
| 1217 | 1242 | // URL lands on. A browser gets the forge's own page for it, same as |
| 1218 | // any other 404; an API client — anything outside `/api/` that never | |
| 1219 | // said it wanted HTML — keeps the `no_route` body, distinct from | |
| 1220 | // `not_found` so it can tell a typo from a resource that is really | |
| 1221 | // gone. The page is rendered from a viewer-less `Ctx`: resolving a | |
| 1222 | // session or token here would spend a database read on a path that, | |
| 1223 | // by construction, cannot belong to a signed-in-only route, at the | |
| 1224 | // cost of the topbar reading "Sign in" even for someone who is. | |
| 1225 | if (prefersHtml(request, url.pathname)) { | |
| 1243 | // any other 404; an API client — anything under `/api/` — keeps the | |
| 1244 | // `no_route` body, distinct from `not_found` so it can tell a typo | |
| 1245 | // from a resource that is really gone. The page is rendered from a | |
| 1246 | // viewer-less `Ctx`: resolving a session or token here would spend a | |
| 1247 | // database read on a path that, by construction, cannot belong to a | |
| 1248 | // signed-in-only route, at the cost of the topbar reading "Sign in" | |
| 1249 | // even for someone who is. | |
| 1250 | if (!isApiPath && prefersHtml(request, url.pathname)) { | |
| 1226 | 1251 | const themeCookie = cookieValue(request, "forge_theme") ?? ""; |
| 1227 | 1252 | const modeCookieValue = cookieValue(request, "forge_mode") ?? ""; |
| 1228 | 1253 | const anonCtx: Ctx = { |
test/api-errors.test.ts +47
| @@ -74,4 +74,51 @@ describe("API errors are distinguishable and self-describing", () => { | ||
| 74 | 74 | expect(noAuth.error).toBe("unauthorized"); |
| 75 | 75 | expect(noAuth.message).toBeTruthy(); |
| 76 | 76 | }); |
| 77 | ||
| 78 | // The API's own base, and any other path under `/api/` that no route | |
| 79 | // claims, used to fall through to the studio's `/:owner`-shaped catch-all | |
| 80 | // page routes ("api" and "v1" read as an owner and a repo name) and answer | |
| 81 | // with thirty-three kilobytes of the not-found *page* — wrong shape, wrong | |
| 82 | // content-type, to a caller whose `accept` said `application/json`. | |
| 83 | test("/api and /api/v1 are JSON 404s, not the studio's HTML page", async () => { | |
| 84 | for (const path of ["/api", "/api/v1", "/api/v2/anything"]) { | |
| 85 | const res = await fetch(`${base}${path}`, { | |
| 86 | headers: { accept: "application/json" }, | |
| 87 | }); | |
| 88 | expect(res.status).toBe(404); | |
| 89 | expect(res.headers.get("content-type")).toStartWith("application/json"); | |
| 90 | const body = (await res.json()) as { error: string }; | |
| 91 | expect(body.error).toBe("no_route"); | |
| 92 | } | |
| 93 | }); | |
| 94 | ||
| 95 | // Content negotiation never rescues it either: a bare curl with no | |
| 96 | // `accept` header at all must still get JSON under `/api/`. | |
| 97 | test("/api/v1 is JSON even with no accept header", async () => { | |
| 98 | const res = await fetch(`${base}/api/v1`); | |
| 99 | expect(res.status).toBe(404); | |
| 100 | expect(res.headers.get("content-type")).toStartWith("application/json"); | |
| 101 | }); | |
| 102 | ||
| 103 | // A page path outside `/api/` keeps the forge's own not-found page, | |
| 104 | // unchanged, for a browser that asks for HTML. | |
| 105 | test("a page path with accept: text/html still gets the HTML page", async () => { | |
| 106 | const res = await fetch(`${base}/this-owner-does-not-exist`, { | |
| 107 | headers: { accept: "text/html" }, | |
| 108 | }); | |
| 109 | expect(res.status).toBe(404); | |
| 110 | expect(res.headers.get("content-type")).toStartWith("text/html"); | |
| 111 | }); | |
| 112 | ||
| 113 | // The security floor is not the API's own responder's business — it is | |
| 114 | // applied once, above every response the forge writes — but a miss under | |
| 115 | // `/api/` must not be the one path that skips it. | |
| 116 | test("the security floor still covers a miss under /api/", async () => { | |
| 117 | const res = await fetch(`${base}/api/v1`, { | |
| 118 | headers: { accept: "application/json" }, | |
| 119 | }); | |
| 120 | expect(res.headers.get("x-content-type-options")).toBe("nosniff"); | |
| 121 | expect(res.headers.get("referrer-policy")).toBe("same-origin"); | |
| 122 | expect(res.headers.get("content-security-policy")).toBeTruthy(); | |
| 123 | }); | |
| 77 | 124 | }); |
new above
offline