Public: anyone can read this; the crew can change it.
A look picture asked for without its version answers 304 when unchanged, behind the same access check #3747
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3745-a-look-picture-asked-for-without-its-ver → main 1 commit fork
2 files
changed +60
−12
src/web/pages/looks.ts +17 −12
| @@ -80,18 +80,23 @@ export function lookImage(ctx: Ctx): Response { | ||
| 80 | 80 | if (!mayFetchLook(ctx, repo, record, look)) return notFound(); |
| 81 | 81 | const path = join(ctx.app.config.dataDir, look.path); |
| 82 | 82 | if (!existsSync(path)) return notFound(); |
| 83 | return new Response(Bun.file(path), { | |
| 84 | headers: { | |
| 85 | "content-type": path.endsWith(".jpg") ? "image/jpeg" : "image/png", | |
| 86 | // Kept for good only under the name of this taking (`lookVersion`); | |
| 87 | // asked for by its bare name, it is checked again every time, so a | |
| 88 | // retake at the same commit is never hidden behind a cached old one. | |
| 89 | "cache-control": | |
| 90 | ctx.url.searchParams.get("v") === lookVersion(look) | |
| 91 | ? "private, max-age=31536000, immutable" | |
| 92 | : "private, no-cache", | |
| 93 | }, | |
| 94 | }); | |
| 83 | const headers = { | |
| 84 | "content-type": path.endsWith(".jpg") ? "image/jpeg" : "image/png", | |
| 85 | // Named by this taking (`lookVersion`), so a retake at the same commit | |
| 86 | // is a new tag as well as a new URL. | |
| 87 | etag: `"${lookVersion(look)}"`, | |
| 88 | // Kept for good only under the name of this taking; asked for by its | |
| 89 | // bare name, it is checked again every time, so a retake at the same | |
| 90 | // commit is never hidden behind a cached old one. | |
| 91 | "cache-control": | |
| 92 | ctx.url.searchParams.get("v") === lookVersion(look) | |
| 93 | ? "private, max-age=31536000, immutable" | |
| 94 | : "private, no-cache", | |
| 95 | }; | |
| 96 | // Only after `mayFetchLook`: a 304 must not say a private picture exists. | |
| 97 | if (ctx.request.headers.get("if-none-match") === headers.etag) | |
| 98 | return new Response(null, { status: 304, headers }); | |
| 99 | return new Response(Bun.file(path), { headers }); | |
| 95 | 100 | } |
| 96 | 101 | |
| 97 | 102 | /** |
test/looks.test.ts +43
| @@ -382,6 +382,49 @@ describe("the look", () => { | ||
| 382 | 382 | ).toBe(404); |
| 383 | 383 | }); |
| 384 | 384 | |
| 385 | test("a picture asked for by its bare name carries an ETag, answers 304 to a match after the access check, and a retake changes the tag", async () => { | |
| 386 | const env = `pull-${pull.number}`; | |
| 387 | const url = `/alice/hello/looks/${env}/${second}/phone.png`; | |
| 388 | const ask = (cookie: string, match?: string) => | |
| 389 | fetch(`${servers.url}${url}`, { | |
| 390 | headers: { cookie, ...(match === undefined ? {} : { "if-none-match": match }) }, | |
| 391 | redirect: "manual", | |
| 392 | }); | |
| 393 | const first200 = await ask(aliceCookie); | |
| 394 | expect(first200.status).toBe(200); | |
| 395 | const etag = first200.headers.get("etag")!; | |
| 396 | expect(etag).toMatch(/^"\d+"$/); | |
| 397 | expect(first200.headers.get("cache-control")).toBe("private, no-cache"); | |
| 398 | ||
| 399 | const same = await ask(aliceCookie, etag); | |
| 400 | expect(same.status).toBe(304); | |
| 401 | expect((await same.arrayBuffer()).byteLength).toBe(0); | |
| 402 | expect(same.headers.get("cache-control")).toBe("private, no-cache"); | |
| 403 | expect(same.headers.get("etag")).toBe(etag); | |
| 404 | ||
| 405 | // Not allowed to fetch it: the one 404, a matching tag or not. | |
| 406 | expect((await ask(bobCookie, etag)).status).toBe(404); | |
| 407 | expect((await ask("", etag)).status).toBe(404); | |
| 408 | ||
| 409 | // A retake at the same commit is a new tag; the old one gets the picture. | |
| 410 | const look = looksForEnv(app.db, record.id, env, "any").find( | |
| 411 | (l) => l.commitOid === second && lookSlot(l.page, l.kind, l.theme) === "phone", | |
| 412 | )!; | |
| 413 | await Bun.sleep(5); | |
| 414 | recordLook(app.db, { | |
| 415 | appId: record.id, | |
| 416 | env, | |
| 417 | commitOid: second, | |
| 418 | kind: look.kind, | |
| 419 | page: look.page, | |
| 420 | theme: look.theme, | |
| 421 | path: look.path, | |
| 422 | }); | |
| 423 | const retaken = await ask(aliceCookie, etag); | |
| 424 | expect(retaken.status).toBe(200); | |
| 425 | expect(retaken.headers.get("etag")).not.toBe(etag); | |
| 426 | }); | |
| 427 | ||
| 385 | 428 | let unpictured: Pull; |
| 386 | 429 | |
| 387 | 430 | test("a picture that could not be taken is recorded with its reason, and the page says so", async () => { |
new above
offline