Public: anyone can read this; the crew can change it.
A signed-out 404 offers sign-in back to the same address, and /go/:code's dead-code 404 stays one page #4183
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4139-a-signed-out-404-offers-sign-in-back-to → main 5 commits fork 1 thread · 1 unresolved
4 files
changed +22
−12
src/http/server.ts +4 −2
| @@ -960,7 +960,7 @@ export function buildRouter(): Router<Handler> { | ||
| 960 | 960 | ctx.app.config.dataDir, |
| 961 | 961 | ctx.params.code ?? "", |
| 962 | 962 | ); |
| 963 | if (target === undefined) return notFoundPage(ctx); | |
| 963 | if (target === undefined) return notFoundPage(ctx, { plain: true }); | |
| 964 | 964 | return new Response(null, { |
| 965 | 965 | status: 303, |
| 966 | 966 | headers: { location: target, "cache-control": "no-store" }, |
| @@ -2459,7 +2459,9 @@ async function answerGet( | ||
| 2459 | 2459 | auth: { user: undefined, scope: undefined, via: "none" }, |
| 2460 | 2460 | theme: isThemeName(themeCookie) ? themeCookie : undefined, |
| 2461 | 2461 | }; |
| 2462 | return notFoundPage(anonCtx); | |
| 2462 | // No session was read, so it cannot tell signed-out from signed-in: | |
| 2463 | // it offers no sign-in rather than one to someone who has signed in. | |
| 2464 | return notFoundPage(anonCtx, { plain: true }); | |
| 2463 | 2465 | } |
| 2464 | 2466 | return noRoute(request.method, url.pathname); |
| 2465 | 2467 | } |
src/web/chrome.ts +8 −9
| @@ -66,13 +66,12 @@ 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". 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. */ | |
| 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; | |
| 69 | * yours to see". With no session, on a GET, it also offers sign-in back to | |
| 70 | * this address; `plain` leaves that out for a miss that must reveal nothing | |
| 71 | * (a dead `/go/:code`). */ | |
| 72 | export function notFoundPage(ctx: Ctx, opts: { plain?: boolean } = {}): Response { | |
| 73 | const offerSignIn = | |
| 74 | !opts.plain && ctx.auth.user === undefined && ctx.request.method === "GET"; | |
| 76 | 75 | return page( |
| 77 | 76 | { |
| 78 | 77 | ctx, |
| @@ -82,8 +81,8 @@ export function notFoundPage(ctx: Ctx): Response { | ||
| 82 | 81 | icon: "search", |
| 83 | 82 | title: "Nothing here", |
| 84 | 83 | note: "The page you asked for does not exist, or is not visible to you.", |
| 85 | action: back | |
| 86 | ? html`${btn("Go home", { href: "/", variant: "primary" })} ${back}` | |
| 84 | action: offerSignIn | |
| 85 | ? btn("Sign in", { href: signInHref(ctx), variant: "primary" }) | |
| 87 | 86 | : btn("Go home", { href: "/", variant: "primary" }), |
| 88 | 87 | })}`, |
| 89 | 88 | }, |
test/web-origins.test.ts +8 −1
| @@ -599,7 +599,14 @@ describe("origins on the web", () => { | ||
| 599 | 599 | .replaceAll(/nonce="[^"]*"/g, 'nonce="N"') |
| 600 | 600 | .replaceAll(/here\?path=[^"&\s]*/g, "here?path=") |
| 601 | 601 | .replaceAll(/next=[^"]*/g, "next="); |
| 602 | expect(sameBut(await stale.text())).toBe(sameBut(await unknown.text())); | |
| 602 | const staleText = await stale.text(); | |
| 603 | const unknownText = await unknown.text(); | |
| 604 | expect(sameBut(staleText)).toBe(sameBut(unknownText)); | |
| 605 | // A dead code offers no sign-in back to itself: it reveals nothing. | |
| 606 | for (const text of [staleText, unknownText]) { | |
| 607 | expect(text).toContain("Go home"); | |
| 608 | expect(text).not.toContain("/signin?next="); | |
| 609 | } | |
| 603 | 610 | |
| 604 | 611 | // A finished flow's link dies with it, and the code is not reissued. |
| 605 | 612 | const fresh = await asUser(carol, "/settings/origins/oauth", { |
test/web-ui.test.ts +2
| @@ -311,6 +311,7 @@ describe("web ui", () => { | ||
| 311 | 311 | expect(out).toContain( |
| 312 | 312 | `href="/signin?next=${encodeURIComponent(asked).replaceAll("&", "&")}"`, |
| 313 | 313 | ); |
| 314 | expect(out).not.toContain("Go home"); | |
| 314 | 315 | const hidden = await (await fetch(`${base}/kim/secret`)).text(); |
| 315 | 316 | const absent = await (await fetch(`${base}/kim/nothing`)).text(); |
| 316 | 317 | const scrub = (t: string) => |
| @@ -322,6 +323,7 @@ describe("web ui", () => { | ||
| 322 | 323 | ).text(); |
| 323 | 324 | expect(signedIn).toContain("Nothing here"); |
| 324 | 325 | expect(signedIn).not.toContain("/signin?next="); |
| 326 | expect(signedIn).toContain("Go home"); | |
| 325 | 327 | }); |
| 326 | 328 | |
| 327 | 329 | test("safeNext refuses another origin", () => { |
new above
offline
No test covers this. The dead-code test in
test/web-origins.test.tsremovesnext=before comparing, so it passes with or withoutplain. Assert that the signed-out dead-code page has "Go home" and no/signin?next=, or removeplainhere.