Public: anyone can read this; the crew can change it.
small: an unknown path that looks like a file answers a tiny plain 404, not the 10 KB page #10750
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10739-small-an-unknown-path-that-looks-like-a → main 4 commits fork 2 threads · 2 unresolved
4 files
changed +114
−1
src/http/asset_miss.ts added +21
| @@ -0,0 +1,21 @@ | ||
| 1 | /** | |
| 2 | * A path that names a file (`/feed.xml`, `/x.png`) and matches no route is a | |
| 3 | * crawler or a tool, not a person: it gets a 404 in a few dozen bytes instead | |
| 4 | * of the full not-found page. Extensionless and `.html` paths keep the page. | |
| 5 | */ | |
| 6 | ||
| 7 | const ASSET_EXTENSION = /\.(?:xml|json|txt|js|css|map|png|ico)$/; | |
| 8 | /** Machine surfaces that keep their own JSON miss; the one list server.ts shares. */ | |
| 9 | export const MACHINE_PREFIX = /^\/(?:api|contract|mcp|oauth|\.well-known|metrics|healthz)(?:\/|$)/; | |
| 10 | ||
| 11 | export function isAssetMiss(request: Request, pathname: string): boolean { | |
| 12 | if (request.method !== "GET" && request.method !== "HEAD") return false; | |
| 13 | return ASSET_EXTENSION.test(pathname) && !MACHINE_PREFIX.test(pathname); | |
| 14 | } | |
| 15 | ||
| 16 | export function assetNotFound(): Response { | |
| 17 | return new Response("not found\n", { | |
| 18 | status: 404, | |
| 19 | headers: { "content-type": "text/plain; charset=utf-8", "cache-control": "no-store" }, | |
| 20 | }); | |
| 21 | } |
src/http/server.ts +3 −1
| @@ -459,6 +459,7 @@ import { | ||
| 459 | 459 | } from "../web/pages/browse.ts"; |
| 460 | 460 | import { bringConnect, bringEnvSubmit, bringJobPage, bringPage, bringRun, bringSubmit } from "../web/pages/bring.ts"; |
| 461 | 461 | import { pushThrough } from "../origins/push-through.ts"; |
| 462 | import { assetNotFound, isAssetMiss, MACHINE_PREFIX } from "./asset_miss.ts"; | |
| 462 | 463 | import { CAME_FROM_ELSEWHERE, forbiddenPage, notFoundPage, SENT_FROM_ELSEWHERE, signInHref } from "../web/chrome.ts"; |
| 463 | 464 | import { targetForShortCode } from "../origins/oauth.ts"; |
| 464 | 465 | import { apiRoutes } from "./api.ts"; |
| @@ -2809,7 +2810,8 @@ async function answerGet( | ||
| 2809 | 2810 | // even for someone who is. |
| 2810 | 2811 | // A plain GET (curl, a crawler, a link unfurler) names no type and gets the page too; |
| 2811 | 2812 | // machine paths and anyone who asks for JSON keep the JSON body. |
| 2812 | const machinePath = /^\/(?:contract|mcp|oauth|\.well-known|metrics|healthz)(?:\/|$)|\.(?:json|git)(?:\/|$)|\.git\//.test(url.pathname); | |
| 2813 | if (!isApiPath && isAssetMiss(request, url.pathname)) return assetNotFound(); | |
| 2814 | const machinePath = MACHINE_PREFIX.test(url.pathname) || /\.(?:json|git)(?:\/|$)|\.git\//.test(url.pathname); | |
| 2813 | 2815 | const plainGet = (request.method === "GET" || request.method === "HEAD") && !machinePath && !prefersJson(request); |
| 2814 | 2816 | if (!isApiPath && (prefersHtml(request, url.pathname) || plainGet)) { |
| 2815 | 2817 | const themeCookie = cookieValue(request, "forge_theme") ?? ""; |
src/web/chrome.ts +2
| @@ -8,6 +8,7 @@ | ||
| 8 | 8 | * `ui.ts` may not. That is the line between the two files. |
| 9 | 9 | */ |
| 10 | 10 | |
| 11 | import { assetNotFound, isAssetMiss } from "../http/asset_miss.ts"; | |
| 11 | 12 | import { audienceFacts, audienceOf } from "../core/audience.ts"; |
| 12 | 13 | import { activeEdgeToken, edgeLoginUrl, edgeStrangerLogin } from "../http/edge.ts"; |
| 13 | 14 | import { |
| @@ -65,6 +66,7 @@ export function crumbsFor(repo: Repo): { label: string; href: string }[] { | ||
| 65 | 66 | * this address; `plain` leaves that out for a miss that must reveal nothing |
| 66 | 67 | * (a dead `/go/:code`). */ |
| 67 | 68 | export function notFoundPage(ctx: Ctx, opts: { plain?: boolean } = {}): Response { |
| 69 | if (!opts.plain && isAssetMiss(ctx.request, ctx.url.pathname)) return assetNotFound(); | |
| 68 | 70 | const get = !opts.plain && ctx.request.method === "GET"; |
| 69 | 71 | const offerSignIn = get && ctx.auth.user === undefined; |
| 70 | 72 | const asked = get ? askedFor(ctx.url.pathname) : ""; |
test/not-found-asset.test.ts added +88
| @@ -0,0 +1,88 @@ | ||
| 1 | /** An unknown path that names a file answers a tiny plain 404; pages keep the full one. */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 8 | ||
| 9 | let dataDir = ""; | |
| 10 | let app: App, servers: Servers; | |
| 11 | ||
| 12 | beforeAll(async () => { | |
| 13 | dataDir = await mkdtemp(join(tmpdir(), "forge-asset404-")); | |
| 14 | app = createApp({ dataDir, port: 0 }); | |
| 15 | servers = await serve(app); | |
| 16 | }); | |
| 17 | ||
| 18 | afterAll(async () => { | |
| 19 | servers.stop(); | |
| 20 | app.close(); | |
| 21 | await rm(dataDir, { recursive: true, force: true }); | |
| 22 | }); | |
| 23 | ||
| 24 | for (const ext of ["xml", "json", "txt", "js", "css", "map", "png", "ico"]) { | |
Every case here is a one-segment path, which goes through /:owner and notFoundPage in chrome.ts. The new line in server.ts (the no-route branch) is never exercised, so deleting it keeps this suite green. Add a deeper unknown path (/a/b/c.png, /a/b/c/d.xml) for each branch, and say in the test name which branch it covers. Also add the cases that define the rule's edges: HEAD on /nothing-here.xml gets the 404 with no body, a POST to such a path does not get the plain 404, and /contract/x.json, /mcp/x.txt, /.well-known/x.json and /api/v1/nope.json keep their JSON miss. |
||
| 25 | test(`GET /nothing-here.${ext} is a one-line plain 404 with the floor`, async () => { | |
| 26 | const res = await fetch(`${servers.url}/nothing-here.${ext}`); | |
| 27 | const body = await res.text(); | |
| 28 | expect(res.status).toBe(404); | |
| 29 | expect(res.headers.get("content-type")).toContain("text/plain"); | |
| 30 | expect(body).toBe("not found\n"); | |
| 31 | expect(res.headers.get("cache-control")).toBe("no-store"); | |
| 32 | expect(res.headers.get("x-content-type-options")).toBe("nosniff"); | |
| 33 | expect(res.headers.get("referrer-policy")).toBeTruthy(); | |
| 34 | expect(res.headers.get("content-security-policy")).toBeTruthy(); | |
| 35 | }); | |
| 36 | } | |
| 37 | ||
| 38 | test("extensionless and .html unknown paths still render the full page", async () => { | |
| 39 | for (const path of ["/nothing-here", "/nothing-here.html"]) { | |
| 40 | const res = await fetch(`${servers.url}${path}`); | |
| 41 | const body = await res.text(); | |
| 42 | expect(res.status).toBe(404); | |
| 43 | expect(res.headers.get("content-type")).toContain("text/html"); | |
| 44 | expect(body.length).toBeGreaterThan(2000); | |
| 45 | } | |
| 46 | }); | |
| 47 | ||
| 48 | test("a route that really ends in .json still matches first", async () => { | |
| 49 | const res = await fetch(`${servers.url}/api/v1/openapi.json`); | |
| 50 | expect(res.status).toBe(200); | |
| 51 | await res.arrayBuffer(); | |
| 52 | }); | |
| 53 | ||
| 54 | const miss = (path: string, init?: RequestInit) => fetch(`${servers.url}${path}`, init); | |
| 55 | ||
| 56 | test("no-route branch (server.ts): deep unknown file paths are the tiny 404", async () => { | |
| 57 | for (const path of ["/a/b/c.png", "/a/b/c/d.xml"]) { | |
| 58 | const res = await miss(path); | |
| 59 | expect([path, res.status, await res.text()]).toEqual([path, 404, "not found\n"]); | |
| 60 | } | |
| 61 | }); | |
| 62 | ||
| 63 | test("owner-page branch (notFoundPage in chrome.ts): one-segment unknown file paths are the tiny 404", async () => { | |
| 64 | for (const path of ["/nothing-here.png", "/nothing-here.xml"]) { | |
| 65 | const res = await miss(path); | |
| 66 | expect([path, res.status, await res.text()]).toEqual([path, 404, "not found\n"]); | |
| 67 | } | |
| 68 | }); | |
| 69 | ||
| 70 | test("HEAD gets the 404 with no body", async () => { | |
| 71 | const res = await miss("/nothing-here.xml", { method: "HEAD" }); | |
| 72 | expect(res.status).toBe(404); | |
| 73 | expect(res.headers.get("content-type")).toContain("text/plain"); | |
| 74 | expect(await res.text()).toBe(""); | |
| 75 | }); | |
| 76 | ||
| 77 | test("a POST to a file-shaped path does not get the plain 404", async () => { | |
| 78 | const res = await miss("/nothing-here.xml", { method: "POST", body: "x" }); | |
| 79 | expect(await res.text()).not.toBe("not found\n"); | |
| 80 | }); | |
| 81 | ||
| 82 | test("machine surfaces keep their own miss, not the plain text one", async () => { | |
| 83 | for (const path of ["/contract/x.json", "/mcp/x.txt", "/.well-known/x.json", "/api/v1/nope.json", "/api/nope.png"]) { | |
| 84 | const res = await miss(path); | |
| 85 | expect([path, res.headers.get("content-type") ?? ""]).not.toEqual([path, expect.stringContaining("text/plain")]); | |
| 86 | expect([path, await res.text()]).not.toEqual([path, "not found\n"]); | |
| 87 | } | |
| 88 | }); |
MACHINE_PREFIX repeats the first half of the machinePath regex in server.ts (contract|mcp|oauth|.well-known|metrics|healthz). Two copies will drift, and a drift here makes a machine client get text instead of JSON. Export one pattern from this module and use it in server.ts's machinePath. Also, isAssetMiss does not exclude /api, and relies on the caller's !isApiPath in server.ts; notFoundPage has no such guard. Add /api to the prefix so the rule holds in both callers, and cover it with a test.