feat(auth): add password reset, change and set screens - #2
Merged
Conversation
Both flows write an OTP browser binding, and both wrote it to `__Host-mm.otp_nonce`. Requesting a password-reset code therefore overwrote the sign-in binding, and the guard that should have caught it — the email equality check — passed, because it is the same address. The sign-in code was then verified against the reset nonce and the user was told a correct code was wrong. Reset now writes `__Host-mm.pwreset_nonce`. Sign-in keeps its name: renaming it would invalidate every binding held mid-flow at deploy time. This separates the local binding and nothing else. The backend keeps one OTP record per user, so requesting a reset code still consumes any live sign-in code server-side — the sign-in tab fails either way. What changes is that it now fails as "request a new code" rather than as "that code is wrong". The module comment argued for a single cookie; left standing it would invite the next reader to undo this, so it is corrected in the same change. The names are asserted as literal strings against the real exported constants — not recomputed, not mocked — because a test that re-derives a name passes through exactly the rename it exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e field The password screens have to know whether an account already has a password: with one the profile card asks for the current password, without one it must not, and the endpoint answers a 400 either way round if the client guesses wrong. The field is optional, and `undefined` is deliberately not folded into `false`. They mean different things — "this account has no password" versus "the deployed backend predates the field" — and collapsing them would draw a set-password form for every operator on a pre-RUK-289 backend, whose every save would then 400. `timezone` is typed this way for the same reason. `false` is a hint for choosing a form, not a security assertion: the backend answers `false` when its own read of the credential fails. The registry row is prose in the sense the `updated_at` row is — the Class-B stub-scanner is pinned to the maintenance mapper and cannot reach `/api/me`, which is an unnarrowed pass-through with no mapper and so no stub to cite. It is not unprotected, though: contract-gaps now asserts the key is ABSENT from `me.json`, which is green today and goes red on the day someone re-records the fixture against a backend that sends it — the day the row and the assertion are both owed deletion. That assertion is written in contract-gaps rather than in me.contract.test.ts because the pass-through check there compares the route's echo against the same fixture that fed its mock: a tautology on key sets, green whatever the fixture holds. Proven to bite by adding the key to the fixture and watching it fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two steps mirroring the sign-in code flow, ending in a new password rather than a session. It writes its own `__Host-mm.pwreset_nonce` binding, and the affordance lives inside the password form so an instance not advertising password sign-in does not offer to reset one. Four things here are not obvious from the screens. The length check runs before the request, and in BYTES. The backend measures with Go's `len()`, so an 11-character Cyrillic password is 22 bytes and passes while `.length` would reject it. It matters more than the usual client-side politeness because the backend deliberately hides a policy violation inside the same 401 it uses for a wrong code — without the check, a user with a short password is told the code they just read off their screen is wrong. Success tears down the local session. The backend answers 204 with no tokens and has already revoked every session, so leaving the NextAuth cookie in place means `proxy.ts` still sees a session, `/login` bounces the user to `/`, and every request 401s. The confirmation is returned unconditionally: past the 204 the password IS changed, so a teardown that throws must not swallow the news. Reload is the flow's primary re-entry, not an edge case — it spans an email round-trip, so the user leaves the tab. The server page reads the binding and hands down the step. Only the address crosses; the nonce stays in the httpOnly cookie, which is the whole reason it is httpOnly. The client counts its own submits. The backend collapses "wrong code", "expired" and "attempts exhausted" into one indistinguishable answer, so nothing in a response says the budget is gone; without a local count the binding outlives the code and every reload returns the user to a dead one. Counted for every submit, because the backend claims an attempt before it compares the code. A 404/5xx says the service is unavailable rather than joining the anti-enumeration collapse: an outage is a fact about the service, not about an account, and folding it in tells every user their input was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Proxies `POST /api/v1/me/password`, deliberately bypassing the two defaults every other route here relies on. Both are wrong for this endpoint, and both fail the same way: the user is signed out and their password is unchanged. `authenticatedBackendRequest` refreshes and RETRIES a mutation on a 401. This endpoint carries the `refresh_token` in its body, so the retry would send the token the refresh had just superseded — the backend answers 401 again and nothing is written. The session is therefore read once, sent verbatim, and the call is never retried. `routeErrorResponse` maps every backend 401 to `AUTH_REQUIRED`, which `bffFetch` answers by navigating to `/login` behind a never-resolving promise. A wrong current password is a 401. Left alone, the most common mistake on this form would sign the operator out with no message, indistinguishable from an expired session. So no failure here answers 401 except the one that genuinely means it: no session at all. The two 401s are told apart by what the REQUEST carried, not by anything in the response — a 401 after sending a current password is that password being wrong; a 401 without one is a stale refresh token, which is not the user's fault and did not change their password. The backend's three 400s share a code and differ only in prose, so the message is passed through opaquely and never parsed. `current_password` is omitted rather than empty when the account has none: the backend rejects the field outright in that state, so "" and absent are different requests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sets or changes the account password, drawing whichever form `password_set` calls for — and treating its three values as three, not two. `undefined` means the deployed backend predates the field, so the card offers nothing rather than guessing: folding it into `false` would show every operator a set-password form whose every save is a 400. The card can still be wrong, because the backend answers `false` when its own read of the credential fails. The 400 that follows flips the form to the other shape so the user is not stuck — ONCE. A 400 is not evidence about `password_set`: the same status also carries a length-policy failure and all three of the backend's 400s share one code, so an unbounded rule would oscillate between the two forms forever. The local length check runs first, which keeps a short password from being misattributed to the wrong cause. A 422 renders in the form (wrong current password, correctable in place) and a 409 is a toast (the session went stale and nothing was changed — not the user's fault). Neither is a 401, which `bffFetch` would answer by navigating away. Success invalidates `["me"]` rather than seeding it: the endpoint answers 204 with an empty body, so there is no user object to write, and `staleTime` would otherwise leave the card drawing the form for the state the user just left. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The same form the profile card renders, on its own in a bare centered layout, for someone who came to do this one thing. Reached from the card, which is its only entry point. The redirect for an account that already has a password is client-side, and that is forced rather than preferred. A server component cannot read the session here: `readActiveSession()` refreshes and writes cookies when the access token is near expiry, and Next permits a cookie write only in a Server Action or Route Handler — a page render throws. Doing it server-side would 500 only inside the rotation window, which is the shape of bug that leaves tests green and fails intermittently in production. The cost is a brief render before the navigation, and the skeleton covers the gap so the form does not flash at someone on their way out. `undefined` redirects alongside `true`: the backend cannot say, the card would offer nothing, and the profile is where the rest of the account lives. It sits under `(app)` for the session and the provider tree — `(public)` mounts neither React Query nor sonner — but deliberately outside `AppShell`, since app chrome around a single form is noise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iring A coverage audit found the change's most consequential module untested: `backend-token-exchange.ts`'s three new functions were mocked by every suite that touched them, so five mutations passed all 1391 tests — including inverting the 401 classification, which is the exact failure the change was designed around, and deleting the throw in `confirmPasswordReset`, which reports every reset failure as success and signs the user out of a password that never changed. The route tests covered the switch over `outcome.kind`; nothing covered what produced those kinds. They now run against a stubbed `fetch` and assert on the request that actually goes on the wire. Writing the login-page tests found two real defects, both invisible until then: - `BuiltInMethod` accepted `onForgotPassword` and never passed it to the form, so the reset flow had no entry point in the product at all; - rehydrating into step two started the countdown at zero, which reads as expired and hides the submit button — a user coming back from their email found a form they could not send, which is precisely the case rehydration exists to serve. The attempt-budget suite could not fail: it looped `MAX_CODE_ATTEMPTS` times, so changing the constant from 5 to 50 kept it green while the user burned 45 doomed submits. The value is now pinned to a literal, with a four-failure case that an off-by-one would break. Also adds the double-submit guard test (each stray submit spends one of five attempts against a ~300ms response floor), the 429 case on the confirm path (§3.6 wants it distinct on both endpoints, and only the request path had it), and drops two of the four places the same byte-length fact was proven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found two blockers.
`readJsonBody` throws a `BffValidationError` on malformed JSON, and this route
called it with no catch — so a bad body left the handler as an uncaught throw,
which Next answers with a 500 whose payload is not the `{error, code}` envelope
`bffFetch` parses. The card would have printed framework noise into the form.
The catch is scoped to the parse alone, leaving the deliberate hand-rolled
handling of the backend's own failures outside the generic mapper.
The route also shipped without a contract test, which AGENTS.md makes mandatory
for anything new under `src/app/api/`. SPEC §7.3 argues a fixture cannot be
recorded here — the recorder is GET-only and refuses non-2xx — but that is an
argument about the fixture, not about the test: the policy's four questions are
answerable against a stubbed fetch, and one of them pins the drift §7.3 itself
named as otherwise undetectable. Without it, "contracts are green" was a claim
that did not cover this route.
Four smaller fixes:
`/set-password` rendered the card's "this deployment doesn't report password
state" copy when the `/me` query ERRORED, blaming a version gap for a transient
outage — the same conflation §3.6 forbids elsewhere.
`restart()` now takes the message to display. It previously cleared every field
while the caller set the error afterwards, which worked only because nothing in
`restart` touched `error`; adding the obvious `setError(undefined)` there would
have silently deleted the only explanation a user gets after burning their
attempts.
The reset-request action no longer reports every non-429 failure as an outage,
mirroring the discrimination the confirm path already had.
`codeIs` parses the body and reads the `code` field instead of substring-matching
the raw JSON, which a space in the encoder's output would have defeated —
failing open into the generic collapse and showing sign-in copy on a reset
screen. Also drops a logged `hadBinding` that was structurally always true.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The countdown, resend cooldown and double-submit guard were written twice — once here and once in the password-reset flow, which extracted them into `useCodeTimers` when it was built. The duplication was not stylistic: the TTL equals the backend's `otp_ttl` and the attempt budget is the same per-user budget on the same OTP record, so two copies would drift on facts about one mechanism, and the drifted copy would tell a user their live code had expired. Behaviour is unchanged — the 94 sign-in tests pass untouched. 50 lines out, 22 in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…skeleton Two findings from a performance pass, both with a concrete mechanism. The countdown decremented a counter once per second, which drifts whenever the tab is throttled — backgrounded, woken from sleep, restored from bfcache. It drifts in the direction that OVER-reports: the user is shown "expires in 4:12" for a code the backend discarded minutes ago, then told their correct code is wrong. Deriving from a deadline in a ref survives all three, and the displayed value stays optimistic only by the network delay it was always optimistic by. Worth doing now rather than later: both flows share this hook, so the drift had just doubled its surface. `/set-password`'s loading placeholder was a fixed 160px block standing in for a form that is shorter than that. The page centres its card, so a height change on swap re-centres the whole thing and visibly moves the heading — on every load, since the query cannot start until hydration. The placeholder is now shaped like the form it replaces. Also records, at the import, why the reset flow is NOT lazily loaded: it can be the first component this page paints, because a live binding resumes the flow when the user returns from their email client, and a lazy chunk would turn that re-entry into two sequential round-trips to save a few KB on the other path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dicate Six functions in the token-exchange module repeated the same four lines: read config, resolve the URL, arm an AbortController, clear the timeout in a `finally`. They now go through one `backendFetch`. The helper takes a `handleResponse` callback rather than returning the `Response`, and that shape is load-bearing rather than stylistic. `fetch` resolves on the response HEAD, so a helper that cleared its timeout on return would stop covering the body read — a stalled body would then hang forever, which is the exact case the timeout exists for. The first draft did that, and all 1424 tests passed on it; nothing here covers a stalled body. The callback keeps the timer alive across `.text()`. Every response-handling body is unchanged: `postBackendJson` still shape-checks and throws, `confirmPasswordReset` still throws on non-ok, and `changeBackendPassword` still returns its discriminated outcome with the 401 classified by what the request carried. Deliberately NOT collapsed: `confirmPasswordReset` and `changeBackendPassword` into one 204-aware helper. One throws and the other returns a discriminated result its caller must read three ways, and a shared helper would erase exactly that distinction. Sharing the transport and nothing else is the right depth. In the reset flow, the resend throttle condition appeared twice — on the button's `disabled` and in its label. Named once, with the rule it encodes: a dead code waives the cooldown, because resending is then the only way forward. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The error message carries role="alert", so it is announced once when it appears. Nothing associated it with the field it was about, so a screen-reader user who moved focus back into the code input after a rejected code heard the label and nothing explaining the rejection. The code input now points at the error when there is one and at the countdown otherwise, which is what the sign-in flow already does. The password input adds the error alongside its hint only when the error is about the password. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…words out of state
Three findings from the ship gate's parallel review, all in the seam between
"what the backend said" and "what a person or a log sees".
The post-reset teardown logged the whole error object. `BackendAuthError`
carries `responseBody` as a public field holding the backend's raw response
text, and the refresh path's body is a token pair — so a failed teardown wrote
a live refresh token into the application log. Both auditors reproduced it
rather than inferring it. The two log lines above it already destructure
`{ status }`; this one now matches.
The change-password 400 passed `await response.text()` — the entire envelope —
through to the card, so an operator hitting the length policy read
`{"code":"invalid request","message":"validation error: …"}` out of a form
field, and a proxy's HTML error page would have been pasted in whole. It now
reads the `message` field with a sentence as fallback. This is not a retreat
from the rule against parsing backend prose: reading for display was always
allowed, only branching is forbidden, and the sole behavioural branch remains
the card's flip flag.
`restart()` cleared the code but not the password. Backing out and resetting a
different address left the previous password pre-filled, so a user could submit
for that account a secret they never knowingly re-entered — besides holding a
plaintext password in state long after the flow needed it.
The tests that should have caught the second one asserted against a hand-typed
bare string in a mock, so passing the whole body through satisfied them
identically — the "expectation derived from the thing it checks" hazard, in
mock form. Both the unit and contract versions now use a realistic envelope,
plus a case for a 400 that is not JSON at all. All three fixes were checked by
mutation: reverting each turns its test red.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…code field A mutation-based coverage audit ran 27 mutations against the suite. Three survived, and all three are behavioural. The reset flow had no timer coverage at all — a regression against its own sibling. Disabling expiry, dropping the submit guard, or making the cooldown never throttle all stayed green here while the equivalent tests have existed on the sign-in side since RUK-288. When both flows moved onto the shared `useCodeTimers`, the hook got tests and the reset flow's use of it did not. Those tests are now ported, and the two mutations that motivated them are confirmed caught. `backendFetch`'s abort covering the response BODY was untestable only in appearance. It needs no fake timers: the config floor is 100ms, and the load-bearing assertion is that the signal aborted — without that line the test passes against the broken version, because the rejection then comes from shape validation instead. Verified by mutation: removing the abort makes the test hang until the runner kills it, which is exactly the production failure. `codeIs` reading the code FIELD rather than substring-matching the body was unenforced. Every existing fixture had the two implementations agreeing; the new one has the code inside `message` only, where a substring match would turn a wrong code into a session mismatch and clear a binding that still had attempts left. Also removes two unreachable render branches. `budgetSpent` cannot be true while the code step renders — `setAttempts` and the `restart()` that follows batch into one update, so the flow has already left the step. The submit guard keeps checking it, being cheap and stating the intent; the rendering that could never happen is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the three password screens RUK-289 calls for on the frontend: a
forgot-password flow inside /login, a change/set-password card in the profile,
and a standalone /set-password page. The backend endpoints already exist; this
wires the UI to them.
__Host-mm.otp_nonce, so requesting a reset code overwrote the sign-inbinding and the user was told a correct code was wrong
password_setto the browser as a three-value field;undefined(backend predates it) is deliberately not folded into
falseTextEncoder, matching Go'slen()—an 11-character Cyrillic password is 22 bytes and passes
authenticatedBackendRequest: it retriesmutations on 401 and this endpoint carries a refresh token in its body, and a
401 answer would sign the operator out on a mistyped current password
already revoked everything by then, so a failed teardown must not swallow the
confirmation
condition also traps Google-OAuth admins, and the access token carries no
sign-in method to tell them apart
Ships with 233 new tests (213 contract / 1436 unit, from 203 / 1324).
password_setis registered indocs/contract-gaps.mduntil the/mefixtureis re-recorded against the merged backend.
🤖 Generated with Claude Code