Public: anyone can read this; the crew can change it.
A signed-out 404 offers sign-in that comes back to the same address #4115
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4111-a-signed-out-404-offers-sign-in-that-com → main 2 commits fork 2 threads · 2 unresolved
3 files
changed +40
−4
src/web/chrome.ts +9 −2
| @@ -66,8 +66,13 @@ export function crumbsFor(repo: Repo): { label: string; href: string }[] { | ||
| 66 | 66 | } |
| 67 | 67 | |
| 68 | 68 | /** The one 404 in the product: same words for "no such thing" and "not |
| 69 | * yours to see". */ | |
| 69 | * yours to see". A signed-out GET also gets the way back in, to this very | |
| 70 | * address; it is offered whether or not the thing exists, so it tells nothing. */ | |
| 70 | 71 | export function notFoundPage(ctx: Ctx): Response { |
| 72 | const back = | |
| 73 | ctx.auth.user === undefined && ctx.request.method === "GET" | |
| 74 | ? btn("Sign in", { href: signInHref(ctx) }) | |
| 75 | : undefined; | |
| 71 | 76 | return page( |
| 72 | 77 | { |
| 73 | 78 | ctx, |
| @@ -77,7 +82,9 @@ export function notFoundPage(ctx: Ctx): Response { | ||
| 77 | 82 | icon: "search", |
| 78 | 83 | title: "Nothing here", |
| 79 | 84 | note: "The page you asked for does not exist, or is not visible to you.", |
| 80 | action: btn("Go home", { href: "/", variant: "primary" }), | |
| 85 | action: back | |
| 86 | ? html`${btn("Go home", { href: "/", variant: "primary" })} ${back}` | |
| 87 | : btn("Go home", { href: "/", variant: "primary" }), | |
| 81 | 88 | })}`, |
| 82 | 89 | }, |
| 83 | 90 | 404, |
test/web-origins.test.ts +2 −1
| @@ -597,7 +597,8 @@ describe("origins on the web", () => { | ||
| 597 | 597 | const sameBut = (text: string): string => |
| 598 | 598 | text |
| 599 | 599 | .replaceAll(/nonce="[^"]*"/g, 'nonce="N"') |
| 600 | .replaceAll(/here\?path=[^"&\s]*/g, "here?path="); | |
| 600 | .replaceAll(/here\?path=[^"&\s]*/g, "here?path=") | |
| 601 | .replaceAll(/next=[^"]*/g, "next="); | |
| 601 | 602 | expect(sameBut(await stale.text())).toBe(sameBut(await unknown.text())); |
| 602 | 603 | |
| 603 | 604 | // A finished flow's link dies with it, and the code is not reissued. |
test/web-ui.test.ts +29 −1
| @@ -17,6 +17,7 @@ import { createApp, type App } from "../src/app.ts"; | ||
| 17 | 17 | import { getRepo } from "../src/core/repos.ts"; |
| 18 | 18 | import { listLabels, labelsFor, getIssue } from "../src/core/issues.ts"; |
| 19 | 19 | import { serve, type Servers } from "../src/http/server.ts"; |
| 20 | import { safeNext } from "../src/web/pages/site.ts"; | |
| 20 | 21 | import { shellJs } from "../src/web/client.ts"; |
| 21 | 22 | import { gitClientEnv } from "./helpers/git-client.ts"; |
| 22 | 23 | import { proveAll } from "./helpers/verified.ts"; |
| @@ -299,10 +300,37 @@ describe("web ui", () => { | ||
| 299 | 300 | const scrub = (t: string) => |
| 300 | 301 | t |
| 301 | 302 | .replaceAll(/nonce-?="?[\w-]+/g, "nonce") |
| 302 | .replaceAll(/here\?path=[^"&\s]*/g, "here?path="); | |
| 303 | .replaceAll(/here\?path=[^"&\s]*/g, "here?path=") | |
| 304 | .replaceAll(/next=[^"]*/g, "next="); | |
| 303 | 305 | expect(scrub(await hidden.text())).toBe(scrub(await absent.text())); |
| 304 | 306 | }); |
| 305 | 307 | |
| 308 | test("a signed-out 404 offers sign-in back to the same address", async () => { | |
| 309 | const asked = "/kim/secret?tab=x&y=1"; | |
| 310 | const out = await (await fetch(`${base}${asked}`)).text(); | |
| 311 | expect(out).toContain( | |
| 312 | `href="/signin?next=${encodeURIComponent(asked).replaceAll("&", "&")}"`, | |
| 313 | ); | |
| 314 | const hidden = await (await fetch(`${base}/kim/secret`)).text(); | |
| 315 | const absent = await (await fetch(`${base}/kim/nothing`)).text(); | |
| 316 | const scrub = (t: string) => | |
Wrap this chain across lines like the scrub above it. |
||
| 317 | t.replaceAll(/nonce-?="?[\w-]+/g, "nonce").replaceAll(/here\?path=[^"&\s]*/g, "here?path=").replaceAll(/next=[^"]*/g, ""); | |
| 318 | expect(scrub(hidden)).toBe(scrub(absent)); | |
| 319 | ||
| 320 | const signedIn = await ( | |
| 321 | await fetch(`${base}/kim/nothing`, { headers: { cookie: stranger } }) | |
| 322 | ).text(); | |
| 323 | expect(signedIn).toContain("Nothing here"); | |
| 324 | expect(signedIn).not.toContain("/signin?next="); | |
| 325 | }); | |
| 326 | ||
| 327 | test("safeNext refuses another origin", () => { | |
| 328 | expect(safeNext("//evil.example/x")).toBeUndefined(); | |
| 329 | expect(safeNext("https://evil.example")).toBeUndefined(); | |
| 330 | expect(safeNext("/\\evil.example")).toBeUndefined(); | |
| 331 | expect(safeNext("/kim/secret?a=1")).toBe("/kim/secret?a=1"); | |
| 332 | }); | |
| 333 | ||
| 306 | 334 | // ---- ref safety ---- |
| 307 | 335 | |
| 308 | 336 | test("tree and commits refuse a ref that is not a ref", async () => { |
new above
offline
encodeURIComponent already turns & into %26, so this replaceAll never matches; remove it.