Public: anyone can read this; the crew can change it.
Every kind of response carries the security floor and every mutation checks its origin, proven by two route-walking tests #10931
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10918-every-kind-of-response-carries-the-secur → main 1 commit fork
2 files
changed +212
−16
test/cross-origin-mutations.test.ts → test/origin-every-mutation.test.ts renamed +37 −16
| @@ -1,11 +1,16 @@ | ||
| 1 | 1 | /** |
| 2 | * Same-origin on every mutation, tested as a whole (#5848). Every POST, PUT, | |
| 3 | * PATCH and DELETE either router registers is sent signed in with a session | |
| 4 | * cookie and `Origin: https://evil.example`; each must answer 403 and leave | |
| 5 | * the event log as it was. Routes a bearer token alone opens are exempt by | |
| 6 | * one rule (BEARER_ONLY) and proven to refuse a cookie-only request. Routes | |
| 7 | * the test cannot reach are named in UNCALLABLE with why, and that list is | |
| 8 | * checked against the routers both ways. | |
| 2 | * Same-origin on every mutation, tested as a whole (#5848, #10918). Every POST, | |
| 3 | * PUT, PATCH and DELETE either router registers is sent signed in with a | |
| 4 | * session cookie and `Origin: https://evil.example`; each must answer 403, | |
| 5 | * leave the event log as it was, and have been refused by an origin check | |
| 6 | * (`crossOrigin` in src/http/context.ts, which the gate in | |
| 7 | * src/http/origin_gate.ts and the handlers' second locks share) — a 403 for | |
| 8 | * any other reason would hide a route with no check at all. | |
| 9 | * | |
| 10 | * A route with no origin check is named in EXEMPT, one line each with its | |
| 11 | * reason, and proven to refuse a cookie-only request. Routes the test cannot | |
| 12 | * reach are named in UNCALLABLE with why. Both lists are checked against the | |
| 13 | * routers, so neither can name a route that is gone or grow without a reason. | |
| 9 | 14 | */ |
| 10 | 15 | import { afterAll, beforeAll, expect, test } from "bun:test"; |
| 11 | 16 | import { mkdtemp, rm } from "node:fs/promises"; |
| @@ -23,11 +28,16 @@ import { quietLogs } from "./helpers/globals.ts"; | ||
| 23 | 28 | quietLogs(); |
| 24 | 29 | |
| 25 | 30 | /** |
| 26 | * The rule: these routes read only `Authorization: Bearer`, never a cookie, so | |
| 27 | * a browser cannot be made to call them as someone. A cookie alone must get a | |
| 28 | * 401 from them, which the second test proves. | |
| 31 | * Mutations with no origin check, each with why. Every one reads only | |
| 32 | * `Authorization: Bearer`, never a cookie, so a browser cannot be made to call | |
| 33 | * it as someone: a cookie alone must get a 401 (or 405), which a test proves. | |
| 34 | * Add a line here only with a reason a reviewer can check. | |
| 29 | 35 | */ |
| 30 | const BEARER_ONLY = new Set(["POST /mcp", "DELETE /mcp", "POST /oauth/userinfo"]); | |
| 36 | const EXEMPT: Record<string, string> = { | |
| 37 | "POST /mcp": "the MCP endpoint reads a bearer token only; a cookie is never an identity there", | |
| 38 | "DELETE /mcp": "ends an MCP session named by a bearer's session header; a cookie is never an identity there", | |
| 39 | "POST /oauth/userinfo": "OAuth userinfo answers the bearer access token it is sent, never a cookie", | |
| 40 | }; | |
| 31 | 41 | |
| 32 | 42 | /** Routes the test cannot call, with why. Each must be a registered mutation. */ |
| 33 | 43 | const UNCALLABLE: Record<string, string> = {}; |
| @@ -45,6 +55,8 @@ const mutations = (): { method: string; pattern: string }[] => | ||
| 45 | 55 | [...buildRouter().routes(), ...buildApiRouter().routes()].filter((r) => MUTATING.has(r.method)); |
| 46 | 56 | |
| 47 | 57 | let dataDir = "", app: App, cookie = ""; |
| 58 | /** How many times an origin check has refused a request, read from the log line `crossOrigin` writes. */ | |
| 59 | let originRefusals = 0; | |
| 48 | 60 | const cells = process.env["FORGE_CELLS"]; |
| 49 | 61 | |
| 50 | 62 | beforeAll(async () => { |
| @@ -66,6 +78,11 @@ beforeAll(async () => { | ||
| 66 | 78 | const pull = await openPull(app, admin.value, repo.value, { title: "t", body: "", baseBranch: "main", headBranch: "topic" }); |
| 67 | 79 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); |
| 68 | 80 | cookie = `forge_session=${createSession(app.db, admin.value.id).token}`; |
| 81 | const warn = app.log.warn.bind(app.log); | |
| 82 | app.log.warn = (message, fields) => { | |
| 83 | if (message === "refused cross-origin mutation") originRefusals++; | |
| 84 | warn(message, fields); | |
| 85 | }; | |
| 69 | 86 | }); |
| 70 | 87 | |
| 71 | 88 | afterAll(async () => { |
| @@ -92,18 +109,22 @@ test("every mutating route refuses a signed-in request from a foreign Origin, wr | ||
| 92 | 109 | expect(routes.length).toBeGreaterThan(100); |
| 93 | 110 | const wrong: string[] = []; |
| 94 | 111 | for (const r of routes) { |
| 95 | if (BEARER_ONLY.has(key(r)) || key(r) in UNCALLABLE) continue; | |
| 112 | if (key(r) in EXEMPT || key(r) in UNCALLABLE) continue; | |
| 96 | 113 | const before = events(); |
| 114 | const refusedBefore = originRefusals; | |
| 97 | 115 | const status = await send(r, "https://evil.example"); |
| 98 | if (status !== 403 || events() !== before) wrong.push(`${key(r)} → ${status}, ${events() - before} events`); | |
| 116 | const checked = originRefusals > refusedBefore; | |
| 117 | if (status !== 403 || events() !== before || !checked) | |
| 118 | wrong.push(`${key(r)} → ${status}, ${events() - before} events${checked ? "" : ", no origin check"}`); | |
| 99 | 119 | } |
| 100 | 120 | expect(wrong).toEqual([]); |
| 101 | 121 | }); |
| 102 | 122 | |
| 103 | test("bearer-only routes carry no cookie: a cookie alone is refused", async () => { | |
| 123 | test("every exempt route is a registered mutation with a reason, and a cookie alone is refused there", async () => { | |
| 104 | 124 | const registered = new Set(mutations().map(key)); |
| 105 | for (const route of BEARER_ONLY) { | |
| 125 | for (const [route, why] of Object.entries(EXEMPT)) { | |
| 106 | 126 | expect(registered.has(route)).toBe(true); |
| 127 | expect(why.length).toBeGreaterThan(20); | |
| 107 | 128 | const [method, pattern] = route.split(" ") as [string, string]; |
| 108 | 129 | const before = events(); |
| 109 | 130 | // 401 for no credential; 405 where the route answers nothing but a bearer's POST. |
| @@ -116,7 +137,7 @@ test("the uncallable list names only registered routes, each with a reason", () | ||
| 116 | 137 | const registered = new Set(mutations().map(key)); |
| 117 | 138 | for (const [route, why] of Object.entries(UNCALLABLE)) { |
| 118 | 139 | expect(registered.has(route)).toBe(true); |
| 119 | expect(why.length).toBeGreaterThan(0); | |
| 140 | expect(why.length).toBeGreaterThan(20); | |
| 120 | 141 | } |
| 121 | 142 | }); |
| 122 | 143 |
test/security-floor-every-route.test.ts added +175
| @@ -0,0 +1,175 @@ | ||
| 1 | /** | |
| 2 | * The security floor (`securityFloor` in src/http/respond.ts) on every kind of | |
| 3 | * response, not just every GET route (#10918): a page, a JSON answer, each | |
| 4 | * shape of refusal and error, the 404s, the 304s, the assets, HEAD and | |
| 5 | * OPTIONS, a redirect, the probes and a 500. `security-floor-routes.test.ts` | |
| 6 | * walks the GET route table; this one walks the kinds of answer, which is | |
| 7 | * where a responder that skips the floor would hide — a 304 with no body, a | |
| 8 | * refusal written before any route, a page that writes its own policy. | |
| 9 | * | |
| 10 | * Each sample names its kind and the status it must answer with, so a sample | |
| 11 | * that drifted to another kind of answer fails loudly instead of proving | |
| 12 | * nothing. The three headers asserted are the issue's; HTML writes its own | |
| 13 | * nonce'd policy and the floor fills the rest. | |
| 14 | */ | |
| 15 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 16 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 17 | import { tmpdir } from "node:os"; | |
| 18 | import { join } from "node:path"; | |
| 19 | import { createApp, type App } from "../src/app.ts"; | |
| 20 | import { commitToBranch } from "../src/actions/repos.ts"; | |
| 21 | import { createRepo } from "../src/core/repos.ts"; | |
| 22 | import { createSession } from "../src/core/sessions.ts"; | |
| 23 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 24 | import { spriteAsset, stylesheetAsset } from "../src/web/asset.ts"; | |
| 25 | import { seedAdmin } from "./helpers/admin.ts"; | |
| 26 | import { quietLogs } from "./helpers/globals.ts"; | |
| 27 | ||
| 28 | quietLogs(); | |
| 29 | ||
| 30 | const FLOOR = ["content-security-policy", "x-content-type-options", "referrer-policy"] as const; | |
| 31 | ||
| 32 | type Sample = { | |
| 33 | kind: string; | |
| 34 | path: string | (() => string); | |
| 35 | status: number; | |
| 36 | init?: RequestInit; | |
| 37 | signedIn?: boolean; | |
| 38 | /** Headers the sample needs that only exist once the first answer is in (a 304's tag). */ | |
| 39 | conditional?: boolean; | |
| 40 | }; | |
| 41 | ||
| 42 | let dataDir = ""; | |
| 43 | let app: App, servers: Servers; | |
| 44 | let cookie = ""; | |
| 45 | ||
| 46 | beforeAll(async () => { | |
| 47 | dataDir = await mkdtemp(join(tmpdir(), "forge-floor-kinds-")); | |
| 48 | app = createApp({ dataDir, port: 0 }); | |
| 49 | servers = await serve(app); | |
| 50 | const admin = await seedAdmin(app, { username: "ada", email: "[email protected]", password: "correct-horse-battery" }); | |
| 51 | if (!admin.ok) throw new Error(admin.error); | |
| 52 | const made = await createRepo(app.db, app.reposRoot, { ownerId: admin.value.id, name: "notes", visibility: "public" }); | |
| 53 | if (!made.ok) throw new Error(String(made.error)); | |
| 54 | const seed = await commitToBranch(app, admin.value, made.value, { message: "seed", files: [{ path: "a.txt", content: "a\n" }] }); | |
| 55 | if (!seed.ok) throw new Error(String(seed.error)); | |
| 56 | cookie = `forge_session=${createSession(app.db, admin.value.id).token}`; | |
| 57 | }, 30_000); | |
| 58 | ||
| 59 | afterAll(async () => { | |
| 60 | servers?.stop(); | |
| 61 | app?.close(); | |
| 62 | await rm(dataDir, { recursive: true, force: true }); | |
| 63 | }); | |
| 64 | ||
| 65 | const FOREIGN = { origin: "https://evil.example", "content-type": "application/json" }; | |
| 66 | ||
| 67 | const SAMPLES: Sample[] = [ | |
| 68 | { kind: "page", path: "/", status: 200 }, | |
| 69 | { kind: "page, signed in", path: "/", status: 200, signedIn: true }, | |
| 70 | { kind: "repository page", path: "/ada/notes", status: 200 }, | |
| 71 | { kind: "HEAD of a page", path: "/", status: 200, init: { method: "HEAD" } }, | |
| 72 | { kind: "JSON answer", path: "/api/v1/repos/ada/notes", status: 200 }, | |
| 73 | { kind: "JSON answer, signed in", path: "/api/v1/user", status: 200, signedIn: true }, | |
| 74 | { kind: "plain text", path: "/robots.txt", status: 200 }, | |
| 75 | { kind: "stylesheet asset", path: () => `/assets/${stylesheetAsset().name}.css`, status: 200 }, | |
| 76 | { kind: "sprite asset", path: () => `/assets/${spriteAsset().name}.svg`, status: 200 }, | |
| 77 | { kind: "304 of an asset", path: () => `/assets/${stylesheetAsset().name}.css`, status: 304, conditional: true }, | |
| 78 | { kind: "304 of a JSON answer", path: "/api/v1/repos/ada/notes/issues", status: 304, conditional: true }, | |
| 79 | { kind: "404 page", path: "/no/such/path/at/all", status: 404 }, | |
| 80 | { kind: "404 of a repository", path: "/ada/nothing-here", status: 404 }, | |
| 81 | { kind: "404 JSON, no route", path: "/api/v1/no-such-route", status: 404 }, | |
| 82 | { kind: "404 JSON, no resource", path: "/api/v1/repos/ada/nothing-here", status: 404 }, | |
| 83 | { kind: "404 of an asset", path: "/assets/not-a-real-hash.css", status: 404 }, | |
| 84 | { kind: "404 of a stale asset hash", path: "/assets/stale.js", status: 404 }, | |
| 85 | { kind: "405 JSON", path: "/api/v1/repos/ada/notes", status: 405, init: { method: "PUT" } }, | |
| 86 | { kind: "401 JSON", path: "/api/v1/user", status: 401 }, | |
| 87 | { | |
| 88 | kind: "422 JSON, a body that is not JSON", | |
| 89 | path: "/api/v1/repos/ada/notes/issues", | |
| 90 | status: 422, | |
| 91 | signedIn: true, | |
| 92 | init: { method: "POST", headers: { "content-type": "application/json" }, body: "{not json" }, | |
| 93 | }, | |
| 94 | { | |
| 95 | kind: "403 JSON, the origin gate", | |
| 96 | path: "/api/v1/repos/ada/notes/issues", | |
| 97 | status: 403, | |
| 98 | signedIn: true, | |
| 99 | init: { method: "POST", headers: FOREIGN, body: "{}" }, | |
| 100 | }, | |
| 101 | { | |
| 102 | kind: "403 page, the origin gate to a form", | |
| 103 | path: "/new", | |
| 104 | status: 403, | |
| 105 | signedIn: true, | |
| 106 | init: { | |
| 107 | method: "POST", | |
| 108 | headers: { origin: "https://evil.example", accept: "text/html", "content-type": "application/x-www-form-urlencoded" }, | |
| 109 | body: "name=x", | |
| 110 | }, | |
| 111 | }, | |
| 112 | { kind: "redirect", path: "/openapi.json", status: 308 }, | |
| 113 | { kind: "OPTIONS, answered as no route", path: "/api/v1", status: 404, init: { method: "OPTIONS" } }, | |
| 114 | { kind: "liveness probe", path: "/healthz", status: 200 }, | |
| 115 | { kind: "readiness probe", path: "/readyz", status: 200 }, | |
| 116 | ]; | |
| 117 | ||
| 118 | async function ask(sample: Sample, extra: Record<string, string> = {}): Promise<Response> { | |
| 119 | const path = typeof sample.path === "function" ? sample.path() : sample.path; | |
| 120 | const headers = new Headers(sample.init?.headers); | |
| 121 | headers.set("accept-encoding", "identity"); | |
| 122 | if (sample.signedIn === true) headers.set("cookie", cookie); | |
| 123 | for (const [k, v] of Object.entries(extra)) headers.set(k, v); | |
| 124 | return fetch(`${servers.url}${path}`, { | |
| 125 | ...sample.init, | |
| 126 | headers, | |
| 127 | redirect: "manual", | |
| 128 | signal: AbortSignal.timeout(5_000), | |
| 129 | }); | |
| 130 | } | |
| 131 | ||
| 132 | /** What is wrong with one sample's answer: a missing floor header, or the wrong kind of answer. */ | |
| 133 | async function wrongWith(sample: Sample): Promise<string[]> { | |
| 134 | let res = await ask(sample); | |
| 135 | if (sample.conditional === true) { | |
| 136 | const etag = res.headers.get("etag"); | |
| 137 | await res.body?.cancel(); | |
| 138 | if (etag === null) return [`${sample.kind}: no etag to revalidate with`]; | |
| 139 | res = await ask(sample, { "if-none-match": etag }); | |
| 140 | } | |
| 141 | await res.body?.cancel(); | |
| 142 | const wrong = FLOOR.filter((h) => !res.headers.get(h)).map((h) => `${sample.kind} (${res.status}) lacks ${h}`); | |
| 143 | if (res.status !== sample.status) wrong.push(`${sample.kind} answered ${res.status}, not ${sample.status}`); | |
| 144 | return wrong; | |
| 145 | } | |
| 146 | ||
| 147 | test("every kind of response carries the floor", async () => { | |
| 148 | const wrong = (await Promise.all(SAMPLES.map(wrongWith))).flat(); | |
| 149 | expect(wrong).toEqual([]); | |
| 150 | }, 15_000); | |
| 151 | ||
| 152 | test("a handler's throw, answered as a 500, carries the floor", async () => { | |
| 153 | // The session read is the first statement a signed-in request makes; a | |
| 154 | // database that throws there is the fault the 500 path exists for. | |
| 155 | const query = app.db.query.bind(app.db); | |
| 156 | app.db.query = (() => { | |
| 157 | throw new Error("the disk went away"); | |
| 158 | }) as typeof app.db.query; | |
| 159 | let res: Response; | |
| 160 | try { | |
| 161 | res = await ask({ kind: "500", path: "/api/v1/user", status: 500, signedIn: true }); | |
| 162 | } finally { | |
| 163 | app.db.query = query; | |
| 164 | } | |
| 165 | await res.body?.cancel(); | |
| 166 | expect(res.status).toBe(500); | |
| 167 | for (const h of FLOOR) expect(res.headers.get(h)).toBeTruthy(); | |
| 168 | }); | |
| 169 | ||
| 170 | test("the floor's own policy is the inert one on anything that is not a page", async () => { | |
| 171 | const res = await ask({ kind: "JSON", path: "/api/v1/repos/ada/notes", status: 200 }); | |
| 172 | await res.body?.cancel(); | |
| 173 | expect(res.headers.get("content-security-policy")).toContain("default-src 'none'"); | |
| 174 | expect(res.headers.get("x-content-type-options")).toBe("nosniff"); | |
| 175 | }); |