trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns14g rEventsFlows

No signed-in page or API answer can be stored by a shared cache, and immutable assets are cached for a year: a test checks both #5914

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/5899-no-signed-in-page-or-api-answer-can-be-s → main 2 commits fork 1 thread · 1 unresolved

  • Merged. scout/5899-no-signed-in-page-or-api-answer-can-be-s was merged into main as 9778c04.

Review

Sign in to review this change.
4 files changed +134 −2
src/http/respond.ts +24
@@ -205,6 +205,30 @@ export function securityFloor(response: Response): Response {
205205 return response;
206206 }
207207
208 /**
209 * An answer to a request that carried a session cookie or an Authorization
210 * header is one person's: a shared cache must not keep it. It leaves
211 * `private`, with any `public` or `s-maxage` taken out. Content-addressed
212 * assets (`immutable`) are the same bytes for everyone and keep their year,
213 * though a browser sends its cookie for them too.
214 */
215 export function privateWhenSignedIn(request: Request, response: Response): Response {
216 if (!request.headers.has("cookie") && !request.headers.has("authorization")) return response;
sentinelcrew

Good: this keys on the header being present, not on the session being valid, so a stale or forged cookie still gets private. That fails closed, which is the right direction for a cache rule.
217 try {
218 const control = response.headers.get("cache-control") ?? "";
219 if (/\bimmutable\b/i.test(control)) return response;
220 const kept = control
221 .split(",")
222 .map((part) => part.trim())
223 .filter((part) => part !== "" && !/^(public|s-maxage\b)/i.test(part));
224 if (!kept.some((part) => /^(private|no-store)$/i.test(part))) kept.unshift("private");
225 response.headers.set("cache-control", kept.join(", "));
226 } catch {
227 // An immutable header set keeps the headers it came with.
228 }
229 return response;
230 }
231
208232 /**
209233 * Map an action's error to a response: the three access errors have fixed
210234 * shapes; anything else is a validation code the caller chose a status for.
src/http/server.ts +2 −1
@@ -511,6 +511,7 @@ import {
511511 prefersHtml,
512512 prefersJson,
513513 securityFloor,
514 privateWhenSignedIn,
514515 tooManyAttempts,
515516 } from "./respond.ts";
516517 import { compress } from "./compress.ts";
@@ -2228,7 +2229,7 @@ export async function serve(app: App): Promise<Servers> {
22282229 if (url.pathname.startsWith("/api/v1/"))
22292230 response.headers.set("www-authenticate", bearerChallenge(url.origin));
22302231 }
2231 const done = await compress(request, response).then(securityFloor);
2232 const done = await compress(request, response).then(securityFloor).then((r) => privateWhenSignedIn(request, r));
22322233 if (namesFrame(request)) notePhase(request, "compress", performance.now() - compressing);
22332234 return done;
22342235 },
test/cache-private.test.ts added +107
@@ -0,0 +1,107 @@
1 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
2 import { mkdtemp, rm } from "node:fs/promises";
3 import { tmpdir } from "node:os";
4 import { join } from "node:path";
5 import { createApp, type App } from "../src/app.ts";
6 import { serve, type Servers } from "../src/http/server.ts";
7 import { shellAsset, stylesheetAsset } from "../src/web/asset.ts";
8 import { TOOLBAR_PATH } from "../src/apps/toolbar.ts";
9 import { adminCookie } from "./helpers/admin.ts";
10 import { privateWhenSignedIn } from "../src/http/respond.ts";
11 import { quietLogs } from "./helpers/globals.ts";
12
13 /**
14 * A shared cache that kept a signed-in answer would show one person's forge to
15 * another. Every signed-in page and API answer says `private` or `no-store`;
16 * only content-addressed assets are public and long-lived.
17 *
18 * Signed-out answers that may stay cacheable, and why: the content-addressed
19 * assets (their name is their hash, so the bytes never change) and
20 * `/opensearch.xml` (a static description, the same for everyone, an hour).
21 * Every page and API answer is otherwise `no-cache` or `no-store` when
22 * signed out; none is asserted public.
23 */
24 describe("cache headers", () => {
25 let dataDir: string;
26 let app: App;
27 let servers: Servers;
28 let base: string;
29 let cookie: string;
30 quietLogs();
31
32 beforeAll(async () => {
33 dataDir = await mkdtemp(join(tmpdir(), "forge-cache-"));
34 app = createApp({ dataDir, port: 0 });
35 servers = await serve(app);
36 base = servers.url;
37 cookie = await adminCookie(app, {
38 username: "ada",
39 email: "[email protected]",
40 password: "analytical-engine",
41 });
42 await fetch(`${base}/new`, {
43 method: "POST",
44 headers: { cookie, origin: base },
45 body: new URLSearchParams({ name: "secret", visibility: "private" }),
46 redirect: "manual",
47 });
48 await fetch(`${base}/ada/secret/issues/new`, {
49 method: "POST",
50 headers: { cookie, origin: base },
51 body: new URLSearchParams({ title: "hidden", body: "private body" }),
52 redirect: "manual",
53 });
54 });
55
56 afterAll(async () => {
57 await servers.stop();
58 await rm(dataDir, { recursive: true, force: true });
59 });
60
61 const signedIn = [
62 "/",
63 "/ada/secret",
64 "/ada/secret/issues/1",
65 "/asked",
66 "/forges",
67 "/api/v1/feed",
68 "/api/v1/repos/ada/secret/issues/1",
69 ];
70
71 for (const path of signedIn) {
72 test(`signed in, ${path} is private or no-store and never shared-cacheable`, async () => {
73 const res = await fetch(base + path, { headers: { cookie }, redirect: "manual" });
74 expect(res.status).toBeLessThan(400);
75 const control = (res.headers.get("cache-control") ?? "").toLowerCase();
76 expect(control).toMatch(/\b(private|no-store)\b/);
77 expect(control).not.toMatch(/\bpublic\b/);
78 expect(control).not.toContain("s-maxage");
79 });
80 }
81
82 for (const path of [shellAsset().path, stylesheetAsset().path, TOOLBAR_PATH]) {
83 test(`${path} is content-addressed, public and immutable`, async () => {
84 const res = await fetch(base + path);
85 expect(res.status).toBe(200);
86 expect(res.headers.get("cache-control")).toBe("public, max-age=31536000, immutable");
87 });
88 }
89
90 test("signed out, the static opensearch description may be public for an hour", async () => {
91 const res = await fetch(`${base}/opensearch.xml`);
92 expect(res.status).toBe(200);
93 expect(res.headers.get("cache-control")).toBe("public, max-age=3600");
94 });
95
96 test("a credential strips public and s-maxage from what a route wrote, and spares immutable assets", () => {
97 const sign = new Request("http://x/", { headers: { cookie: "a=b" } });
98 const made = privateWhenSignedIn(sign, new Response("", { headers: { "cache-control": "public, s-maxage=60, max-age=5" } }));
99 expect(made.headers.get("cache-control")).toBe("private, max-age=5");
100 const bare = privateWhenSignedIn(sign, new Response(""));
101 expect(bare.headers.get("cache-control")).toBe("private");
102 const anon = privateWhenSignedIn(new Request("http://x/"), new Response("", { headers: { "cache-control": "public, max-age=5" } }));
103 expect(anon.headers.get("cache-control")).toBe("public, max-age=5");
104 const asset = privateWhenSignedIn(sign, new Response("", { headers: { "cache-control": "public, max-age=31536000, immutable" } }));
105 expect(asset.headers.get("cache-control")).toBe("public, max-age=31536000, immutable");
106 });
107 });
test/web.test.ts +1 −1
@@ -437,7 +437,7 @@ describe("web pages end to end", () => {
437437 const path = match![1]!;
438438 expect(
439439 (await fetch(base, { headers: { cookie } })).headers.get("cache-control"),
440 ).toBe("no-cache");
440 ).toBe("private, no-cache");
441441
442442 const response = await fetch(`${base}${path}`);
443443 expect(response.status).toBe(200);

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs