Identity comes from the forge: /admin opens for a forge admin and the password sign-in is gone #13

Merged masoncrew opened this 1mo ago

Check · Review · Land · Live done — Landed and live in production.

Conversation Files

Merged

Identity now comes from the forge on every request, and the app's own admin password no longer exists.

shipped production is serving it deploy ab962da

masoncrew opened this 1mo ago

Identity now comes from the forge on every request, and the app's own admin password no longer exists.

src/who.ts verifies x-forge-identity (<userId>|<username>|<role>|<expiresMs>) against x-forge-signature — base64url HMAC-SHA256 with FORGE_APP_IDENTITY_KEY, compared in constant time — and returns { role: "anon" } for anything it cannot prove: no headers, no key, a shape that does not parse, a role that is not user or admin, an empty id or name, an expiry that is not digits or has passed. It reads no cookie. src/server.ts builds ctx.who beside the session and passes it through src/context.ts, so no handler touches those headers; the guard in src/admin.ts serves every /admin path only for role === "admin" and gives everyone else one 403 page whose only action is Sign in at /__forge/signin?to=<the path they asked for>. The site header names whoever is signed in, links /__forge/signout, and shows Admin only to an admin.

Gone with the password: ADMIN_PASSWORD, the generated demo password and its salt, verifyAdminPassword, ensureDemoPassword, usingGeneratedPassword, POST /admin/signin, POST /admin/signout, pages.signIn, the SIGN_IN rate limit and the is_admin read in src/session.ts. The session cookie stays and carries the CSRF token only; CSRF still guards every POST. The admin's look at an order without its token (src/shop.ts) is sourced from ctx.who. The session table and its is_admin column are untouched, and the security floor in src/server.ts is unchanged. One thing beyond the issue: the two setting rows the old password wrote (admin_demo_password, admin_salt) are deleted at boot by forgetPasswordSettings() in src/db.ts, since they are a secret at rest that opens nothing; it has its own unit test. src/tty.ts names the person from FORGE_APP_USER/_ID/_ROLE on its first screen and keeps stock moves and refunds for an admin.

What to look at in the preview: /admin as you are (403 with the sign-in link, or the dashboard if the forge signs you as an admin), and the account corner in the header on /. Tests: src/who.test.ts covers the verifier on its own, and src/server.test.ts signs like the forge — admin opens, user, anon, a tampered signature, an expired stamp and an unset FORGE_APP_IDENTITY_KEY all get 403, including a user's POST carrying a valid CSRF token, so the refusal is the guard's and not the CSRF check's. bun test: 40 pass. No dependencies added.

🤖 Generated with Claude Code

Closes #9

sentinelcrew commented 1mo ago

probe

sentinelcrew Approved 1mo ago

Approved. This moves an authorization boundary, and it moves it the right way: identity is verified rather than asserted, and the app's own password is gone rather than left as a fallback.

What I checked, and what held

src/who.ts matches the spec in #9 line for line: HMAC-SHA256 over the raw identity, compared through equals/timingSafeEqual, and the shape is parsed only after the signature verifies — four fields, non-empty id and name, role in user/admin, expiry digits-only and in the future, identity capped at 512 bytes. No cookie, no query parameter, no environment escape hatch. A missing key returns anon, so the failure mode is closed, not open.
Every one of the 25 /admin handlers calls guard(ctx) as its first statement — I read them all, not only the GETs — and routes.ts has no /admin path pointing anywhere else. signin/signout, SIGN_IN and the is_admin read are gone; CSRF is untouched and still on every POST. The test that posts a valid CSRF token, from a page a user is allowed to see, and gets 403 with stock unchanged, is exactly the right test: it proves the guard rather than the token.
src/shop.ts:224 now reads ctx.who, so an old session row with is_admin = 1 opens nothing. The table and the column are left alone, as the issue asked.
The security floor in src/server.ts is unchanged, and every identity-bearing page is no-store, so the account corner cannot be cached across people; only the CSS and /static are public and neither carries a name. who.name goes through escape.
On the preview: /admin is 403 with the sign-in link and no password field; a request carrying x-forge-identity: u_1|evil|admin|… with a junk signature is 403; /__forge/signin?to=… reaches the forge's door, and the forge reduces an off-site to to /, so the path handed to it is not an open redirect at either end. The forge's screenshot at this head shows trevato named in the header as admin with the Admin link — that is the wire format passing against the real forge, the one thing the unit tests cannot prove.
bun test at 261632d: 40 pass, 0 fail.
forgetPasswordSettings() is beyond the issue but correct: a stored secret that opens nothing is still a stored secret. The unit test pinning tax_default_bp as a survivor is the right guard on a DELETE.

Nits, none blocking

The header's Sign in link drops the query string: render passes ctx.url.pathname while the /admin guard passes pathname + search. Signing in from /search?q=beans returns to an empty /search. One line in src/context.ts.
On desktop the header now wraps to two rows for a signed-in admin — comparing this pull's screenshot with production's, the nav breaks after Gifts and Kitchen Admin fall to a second line. Readable, but it is the first thing people see; a small flex tweak in .top would put it back on one line.
No ledger row names the admin who refunded, moved stock or adjusted a card — the reason is recorded, the actor is not, and there is no column for one. That is #12's work and it now has ctx.who to build on, so nothing to do here.

(Housekeeping: a stray probe comment and an anchored probe note on src/who.ts are mine, from finding this endpoint; the API offers no delete, so please ignore them.)

The README and AGENTS.md changes describe what the code actually does, which is the part that usually drifts. Land it.

sentinelcrew commented 1mo ago

The review is posted: pull #13 is approved, with the body on the pull and the approval registered (approved: 1).

Approved pull #13. src/who.ts implements the signed-identity check exactly as issue #9 specifies — signature verified with timingSafeEqual before the payload is parsed, expiry and role validated, anon on any doubt, and no cookie, query parameter or environment fallback. All 25 /admin handlers call guard(ctx) first, the password path is fully removed, CSRF and the security floor are untouched, and bun test at 261632d is 40 pass / 0 fail. On the preview, /admin is 403 with a sign-in link for anon, spoofed identity headers are rejected, the forge's sign-in door works and sanitizes to, and the forge's screenshot shows a real signed admin named in the header — the wire format works against the live forge. Three non-blocking nits are in the review: the header's Sign in link drops the query string, the desktop header wraps to two rows for a signed-in admin, and no ledger row names the acting admin (that is #12's job).

One piece of housekeeping I could not undo: finding the right endpoint took two probe posts that the forge has no delete route for, so a probe conversation comment and an anchored probe note on src/who.ts:30 remain on the pull. I said so in the review.