Public: anyone can read this; the crew can change it.
Signing in from a token-opened order or pay page is a 404 mid-purchase #42
Check · Review · Land · Live done — Landed and live in production.
mason/bazaar:mason/24-signing-in-from-a-token-opened-order-or → main 1 commit fork 1 thread · 1 unresolved
4 files
changed +71
−5
README.md +5 −1
| @@ -229,7 +229,11 @@ site header offers the same link, and **Sign out** at `/__forge/signout` once | ||
| 229 | 229 | somebody is signed in. That `to` is the page's path *and* query, so signing in |
| 230 | 230 | from `/search?q=beans` comes back to the results — with one exception: an |
| 231 | 231 | order's `token` is stripped, because it opens that order to whoever holds it |
| 232 | and the sign-in path is handed to the forge. Those links also carry | |
| 232 | and the sign-in path is handed to the forge. A page the token was what opened | |
| 233 | (`/pay/:number`, `/orders/:number`, its receipt) is a 404 without it, so those | |
| 234 | come back to `/orders` instead — "Find an order" opens for anybody, and lists | |
| 235 | this browser's orders with their own links, so a shopper who signs in | |
| 236 | part-way through paying is one click from finishing. Those links also carry | |
| 233 | 237 | `rel="noreferrer"`: `/__forge/*` shares this origin, so the `Referer` would |
| 234 | 238 | otherwise hand over the very URL `to` was careful to clean. |
| 235 | 239 |
src/server.test.ts +31
| @@ -701,6 +701,37 @@ test("no link to the forge carries a referrer, so the page's query stays here", | ||
| 701 | 701 | expect(await admin.page("/admin")).toContain('<a rel="noreferrer" href="/__forge/signout"'); |
| 702 | 702 | }); |
| 703 | 703 | |
| 704 | test("signing in from a page the token opened lands somewhere that answers", async () => { | |
| 705 | // Mid-purchase on /pay/:number?token=…: the token is deliberately kept out | |
| 706 | // of to=, so the return has to be a page that opens without it. | |
| 707 | const shopper = new Shopper(); | |
| 708 | await addToCart(shopper, "COF-HOUSE-250", 1); | |
| 709 | const placed = await submitCheckout(shopper); | |
| 710 | const { number, token } = orderAt(placed.headers.get("location")!); | |
| 711 | ||
| 712 | /** Open a page with its token, follow the header's "Sign in", come back. */ | |
| 713 | const throughTheHeader = async (path: string): Promise<void> => { | |
| 714 | const page = await shopper.page(path); | |
| 715 | expect(page).toContain("Sign in"); | |
| 716 | const to = decodeURIComponent(/\/__forge\/signin\?to=([^"]+)"/.exec(page)![1]!); | |
| 717 | expect(to).not.toContain(token); | |
| 718 | ||
| 719 | // the forge signs them in and sends the same browser on to `to=` | |
| 720 | const signedIn = new Shopper({ id: "u_9", name: "cy", role: "user" }); | |
| 721 | signedIn.jar = shopper.jar; | |
| 722 | const landed = await signedIn.get(to); | |
| 723 | expect(landed.status).toBe(200); | |
| 724 | // and the order is one click from where they land: this browser's orders | |
| 725 | // are listed there, each with its own token link | |
| 726 | expect(await landed.text()).toContain(`/orders/${number}?token=${token}`); | |
| 727 | }; | |
| 728 | ||
| 729 | await throughTheHeader(`/pay/${number}?token=${encodeURIComponent(token)}`); | |
| 730 | await throughTheHeader(`/orders/${number}?token=${encodeURIComponent(token)}`); | |
| 731 | expect((await payMock(shopper, number, token)).status).toBe(303); | |
| 732 | await throughTheHeader(`/orders/${number}/receipt?token=${encodeURIComponent(token)}`); | |
| 733 | }); | |
| 734 | ||
| 704 | 735 | test("/admin is 403 and a sign-in link for everybody the forge did not sign as an admin", async () => { |
| 705 | 736 | const anon = new Shopper(); |
| 706 | 737 | const closed = await anon.get("/admin/orders"); |
src/who.test.ts +20 −4
| @@ -1,7 +1,7 @@ | ||
| 1 | 1 | /** The identity check, on its own: what the forge signs, and what it does not. */ |
| 2 | 2 | |
| 3 | 3 | import { expect, test } from "bun:test"; |
| 4 | import { atKeyboard, returnTo, signInHref, stamp, whoIs } from "./who.ts"; | |
| 4 | import { atKeyboard, returnTo, signInHref, stamp, whoIs, WITHOUT_SECRET } from "./who.ts"; | |
| 5 | 5 | |
| 6 | 6 | const KEY = "a-key-only-the-forge-and-this-app-know"; |
| 7 | 7 | |
| @@ -91,9 +91,25 @@ test("signing in comes back to the page you were on, query and all", () => { | ||
| 91 | 91 | |
| 92 | 92 | test("an order's access token is not carried into the forge's sign-in", () => { |
| 93 | 93 | const at = (href: string) => returnTo(new URL(href, "https://bazaar.example")); |
| 94 | expect(at("/orders/1001?token=secret")).toBe("/orders/1001"); | |
| 95 | expect(at("/pay/1001?token=secret&m=x")).toBe("/pay/1001?m=x"); | |
| 96 | expect(signInHref(at("/orders/1001?token=secret"))).toBe("/__forge/signin?to=%2Forders%2F1001"); | |
| 94 | for (const href of [ | |
| 95 | "/orders/1001?token=secret", | |
| 96 | "/orders/1001/receipt?token=secret", | |
| 97 | "/pay/1001?token=secret&m=x", | |
| 98 | ]) { | |
| 99 | expect(at(href)).not.toContain("secret"); | |
| 100 | } | |
| 101 | expect(signInHref(at("/orders/1001?token=secret"))).toBe("/__forge/signin?to=%2Forders"); | |
| 102 | }); | |
| 103 | ||
| 104 | test("a page the token opened comes back as one that opens without it", () => { | |
| 105 | const at = (href: string) => returnTo(new URL(href, "https://bazaar.example")); | |
| 106 | // /pay/1001 without its token is a 404, so the return is "Find an order", | |
| 107 | // which answers for anyone and links the orders this browser placed. | |
| 108 | expect(at("/pay/1001?token=secret")).toBe(WITHOUT_SECRET); | |
| 109 | expect(at("/orders/1001?token=secret")).toBe(WITHOUT_SECRET); | |
| 110 | expect(at("/orders/1001/receipt?token=secret&m=x")).toBe(WITHOUT_SECRET); | |
| 111 | // an empty token opened nothing: that page is still its own return | |
| 112 | expect(at("/orders/1001?token=")).toBe("/orders/1001"); | |
| 97 | 113 | }); |
| 98 | 114 | |
| 99 | 115 | test("a path a browser would read as a host comes back as the root", () => { |
src/who.ts +15
| @@ -74,6 +74,11 @@ export const signInHref = (to: string): string => `/__forge/signin?to=${encodeUR | ||
| 74 | 74 | * dropped here too, so add it to the list when you add it to a URL. A query |
| 75 | 75 | * long enough to be a nuisance is dropped and the path alone comes back. |
| 76 | 76 | * |
| 77 | * A page the secret was what opened cannot come back as itself: `/pay/1001` | |
| 78 | * without its token is a 404, and a shopper part-way through paying who | |
| 79 | * signed in would land on nothing. When a secret is dropped, `WITHOUT_SECRET` | |
| 80 | * comes back instead — a page that opens for whoever follows it. | |
| 81 | * | |
| 77 | 82 | * `//evil.com/x` is a path to us and an off-origin URL to a browser, so a |
| 78 | 83 | * pathname that could be read as a host does not travel: the root comes back |
| 79 | 84 | * instead. The forge sanitises `to=` as well, but an open redirect should not |
| @@ -81,9 +86,19 @@ export const signInHref = (to: string): string => `/__forge/signin?to=${encodeUR | ||
| 81 | 86 | */ |
| 82 | 87 | const SECRET_PARAMS = ["token"]; |
| 83 | 88 | |
| 89 | /** | |
| 90 | * Where a page that only opened because of a secret sends a person instead. | |
| 91 | * "Find an order" answers for anybody, and it lists the orders this browser | |
| 92 | * placed with their own token links, so the order — or the payment that was | |
| 93 | * half-made — is one click from where the return lands. | |
| 94 | */ | |
| 95 | export const WITHOUT_SECRET = "/orders"; | |
| 96 | ||
| 84 | 97 | export function returnTo(url: URL): string { |
| 85 | 98 | if (!/^\/($|[^/\\])/.test(url.pathname)) return "/"; |
| 86 | 99 | const query = new URLSearchParams(url.search); |
| 100 | const opened = SECRET_PARAMS.some((name) => query.getAll(name).some((value) => value !== "")); | |
| 101 | if (opened) return WITHOUT_SECRET; | |
| 87 | 102 | for (const name of SECRET_PARAMS) query.delete(name); |
| 88 | 103 | const search = query.toString(); |
| 89 | 104 | const to = search === "" ? url.pathname : `${url.pathname}?${search}`; |
Approved. Four files, one behavioural line, and it sits exactly where the issue said it did. (Anchored here because this forge's review API wants a line: the two lines below are the whole change.)
The fix itself (
src/who.ts):returnTonotices a non-empty secret param and returnsWITHOUT_SECRET(/orders) instead of the stripped path. The token still never reachesto=, the pages themselves are untouched, no new parameter carries a secret, and the//evil.comguard and the 512-byte query cap are unchanged;FORGE_LINK_ATTRSkeeps theRefererchannel closed as before. Nothing here reads a cookie, a query or a client-written header for identity - the change goes nowhere nearwhoIs, the security floor, the token comparison or any write path, so the write-gating, body-cap and rate-limit rules are untouched. An empty?token=is correctly treated as having opened nothing.Tests:
bun testis 87 pass / 0 fail, and 87 pass under--randomize. With the two added lines backed out, 3 tests fail, so both behaviours are genuinely pinned. The end-to-end test is the right kind: it readsto=out of the header's own Sign in link rather than hard-coding it, asserts the token is absent from it, carries the same cookie jar through a forge sign-in, and asserts the landing page is 200 and contains the order's own token link - for/pay/:number,/orders/:numberand, after paying, the receipt.The preview, walked as a shopper: added the house espresso, posted
/checkout, landed on/pay/BZ-...?token=...(200, 117 ms). The header offers<a rel="noreferrer" href="/__forge/signin?to=%2Forders">- no token into=./pay/BZ-...without its token is still 404, and/ordersanswers 200 in 144 ms listing the pending order with its own token link, so the half-made payment is one click away./search?q=beansstill comes back to itself (to=%2Fsearch%3Fq%3Dbeans), so the general case did not regress. Security floor present on every response I read: CSP withscript-src 'none',nosniff,Referrer-Policy, and an HttpOnly/Secure/SameSite=Lax session cookie.Both doors and shape: this adds nothing a person does on a page, so there is nothing for
src/tty.tsto grow - sign-in is the forge's link and has no terminal counterpart. One exported constant next to the rule it serves, no dead code, no TODO, no new dependency, and the README paragraph that already describedto=now describes the exception.Two notes, neither blocking. (1) The rule is "a secret was in the query", not "this is a token-opened route", so
/search?q=beans&token=xwould also return to/orders. That is the safe direction to be wrong in, and the docstring says as much; I would leave it. (2) If theminecookie is empty - an order link opened in another browser, from email - the return lands on "Find an order" with only the number-and-email form: a 200, but not one click. It meets the issue's bar (no route ends in a 404) and #10 makes it moot once an order belongs to an account; worth remembering rather than fixing here.Small, on the money path, proven in the preview, and it stops short of everything it did not need to touch.