From 1ac325689ab102e6b217d88de281f5188e385d6d Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 01:29:38 +0300 Subject: [PATCH 01/14] fix(auth): bind sign-in and password-reset codes to separate cookies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../auth/__tests__/otp-nonce-cookie.test.ts | 68 +++++++++++++++++++ src/server/auth/otp-nonce-cookie.ts | 57 +++++++++++++--- 2 files changed, 117 insertions(+), 8 deletions(-) diff --git a/src/server/auth/__tests__/otp-nonce-cookie.test.ts b/src/server/auth/__tests__/otp-nonce-cookie.test.ts index c09d557..2e510a6 100644 --- a/src/server/auth/__tests__/otp-nonce-cookie.test.ts +++ b/src/server/auth/__tests__/otp-nonce-cookie.test.ts @@ -2,9 +2,13 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { OTP_NONCE_COOKIE, + PWRESET_NONCE_COOKIE, clearOtpBinding, + clearPasswordResetBinding, readOtpBinding, + readPasswordResetBinding, setOtpBinding, + setPasswordResetBinding, } from "@/server/auth/otp-nonce-cookie"; const store = { @@ -116,3 +120,67 @@ describe("otp-nonce-cookie — the encoding resists a hostile email address", () expect(read?.email).toBe(hostile); }); }); + +describe("two flows, two cookies (RUK-289)", () => { + beforeEach(() => { + store.set.mockReset(); + store.get.mockReset(); + store.delete.mockReset(); + }); + + // Asserted as literal strings against the real exported constants, NOT + // re-derived from whatever produces them and NOT mocked. Both names are + // deployed artifacts: renaming the sign-in one invalidates every binding a + // user is holding mid-flow at deploy time, and a test that recomputes the + // name would pass through exactly that rename. + it("pins both cookie names as deployed artifacts", () => { + expect(OTP_NONCE_COOKIE).toBe("__Host-mm.otp_nonce"); + expect(PWRESET_NONCE_COOKIE).toBe("__Host-mm.pwreset_nonce"); + }); + + // The bug this split exists for: with ONE cookie, requesting a password-reset + // code overwrites the sign-in binding, and the email equality check PASSES + // because the address is the same. The sign-in code is then verified against + // the reset nonce and the user is told a correct code is wrong. + // + // Note what this does NOT claim: the sign-in CODE does not survive either, + // because the backend keeps one OTP record per user and issuing a reset code + // consumes the live one (SPEC §1.5). Only the local binding is separated. + it("writing a reset binding leaves the sign-in binding untouched", async () => { + await setOtpBinding({ nonce: "signin-nonce", email: "op@example.test" }); + await setPasswordResetBinding({ nonce: "reset-nonce", email: "op@example.test" }); + + const written = store.set.mock.calls.map(([name]) => name); + expect(written).toEqual(["__Host-mm.otp_nonce", "__Host-mm.pwreset_nonce"]); + }); + + it("reads the reset binding from its own cookie", async () => { + store.get.mockImplementation((name: string) => + name === "__Host-mm.pwreset_nonce" + ? { value: encode({ nonce: "reset-nonce", email: "op@example.test" }) } + : undefined, + ); + + await expect(readPasswordResetBinding()).resolves.toEqual({ + nonce: "reset-nonce", + email: "op@example.test", + }); + // The sign-in reader must not fall back to the reset cookie. + await expect(readOtpBinding()).resolves.toBeUndefined(); + }); + + it("clearing one flow's binding does not clear the other's", async () => { + await clearPasswordResetBinding(); + + expect(store.delete).toHaveBeenCalledTimes(1); + expect(store.delete).toHaveBeenCalledWith("__Host-mm.pwreset_nonce"); + }); + + it("keeps the reset cookie's flags and TTL identical to sign-in's", async () => { + await setPasswordResetBinding({ nonce: "n", email: "op@example.test" }); + + const [, , opts] = store.set.mock.calls[0]; + expect(opts).toMatchObject({ httpOnly: true, sameSite: "lax", secure: true, path: "/" }); + expect(opts.maxAge).toBe(300); + }); +}); diff --git a/src/server/auth/otp-nonce-cookie.ts b/src/server/auth/otp-nonce-cookie.ts index f3aa578..1076613 100644 --- a/src/server/auth/otp-nonce-cookie.ts +++ b/src/server/auth/otp-nonce-cookie.ts @@ -29,11 +29,28 @@ import { cookies } from "next/headers"; * (the backend answers 202 for any well-formed one), so `${nonce}:${email}` * would let an address containing the delimiter control the parsed nonce. * - * One cookie, not one per flow: a second tab overwrites the first, and the - * first tab's code then fails with `otp_session_mismatch`, which renders the + * ONE COOKIE PER FLOW, not one overall (revised in RUK-289). Within a single + * flow a second tab still overwrites the first, and that remains deliberate: + * the first tab then fails with `otp_session_mismatch`, which renders the * honest "request a new code" state that exists for exactly this situation. + * + * Across flows it was a defect. Sign-in and password-reset both bind a nonce, + * and with one cookie a reset request overwrote the sign-in binding while the + * email equality check still PASSED — same address — so a sign-in code was + * verified against the reset nonce and the user was told a correct code was + * wrong. + * + * Separating the names fixes the binding, and only the binding. The backend + * keeps one OTP record per user, so requesting a reset code still consumes any + * live sign-in code server-side (SPEC §1.5): the sign-in tab fails either way, + * and the point of the split is that it now fails as "request a new code" + * rather than as "that code is wrong". + * + * Both names are deployed artifacts. Renaming either one invalidates every + * binding users hold mid-flow at deploy time. */ export const OTP_NONCE_COOKIE = "__Host-mm.otp_nonce"; +export const PWRESET_NONCE_COOKIE = "__Host-mm.pwreset_nonce"; /** * Equal to the backend's `otp_ttl` (5 min), deliberately not longer. A margin @@ -67,13 +84,13 @@ export function normalizeEmail(email: string): string { return email.trim().toLowerCase(); } -export async function setOtpBinding(binding: OtpBinding): Promise { +async function setBinding(name: string, binding: OtpBinding): Promise { const encoded = Buffer.from( JSON.stringify({ nonce: binding.nonce, email: normalizeEmail(binding.email) }), "utf8", ).toString("base64url"); const store = await cookies(); - store.set(OTP_NONCE_COOKIE, encoded, { + store.set(name, encoded, { httpOnly: true, sameSite: "lax", secure: true, @@ -82,6 +99,14 @@ export async function setOtpBinding(binding: OtpBinding): Promise { }); } +export function setOtpBinding(binding: OtpBinding): Promise { + return setBinding(OTP_NONCE_COOKIE, binding); +} + +export function setPasswordResetBinding(binding: OtpBinding): Promise { + return setBinding(PWRESET_NONCE_COOKIE, binding); +} + /** * Reads the binding, or `undefined` when there is none. * @@ -91,9 +116,9 @@ export async function setOtpBinding(binding: OtpBinding): Promise { * state — so a corrupted cookie can never surface as "wrong code" to someone * holding a correct one, and can never crash the sign-in callback. */ -export async function readOtpBinding(): Promise { +async function readBinding(name: string): Promise { const store = await cookies(); - const raw = store.get(OTP_NONCE_COOKIE)?.value; + const raw = store.get(name)?.value; if (!raw) { return undefined; } @@ -106,7 +131,23 @@ export async function readOtpBinding(): Promise { } } -export async function clearOtpBinding(): Promise { +export function readOtpBinding(): Promise { + return readBinding(OTP_NONCE_COOKIE); +} + +export function readPasswordResetBinding(): Promise { + return readBinding(PWRESET_NONCE_COOKIE); +} + +async function clearBinding(name: string): Promise { const store = await cookies(); - store.delete(OTP_NONCE_COOKIE); + store.delete(name); +} + +export function clearOtpBinding(): Promise { + return clearBinding(OTP_NONCE_COOKIE); +} + +export function clearPasswordResetBinding(): Promise { + return clearBinding(PWRESET_NONCE_COOKIE); } From ab9b136c1e59414bbac9ccdc694aded7a39873e4 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 01:32:47 +0300 Subject: [PATCH 02/14] feat(auth): carry password_set through to the browser as a three-value field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/contract-gaps.md | 30 +++++++++++++++++++++++++++ src/domain/admin/user.ts | 19 +++++++++++++++++ tests/contracts/contract-gaps.test.ts | 17 +++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/docs/contract-gaps.md b/docs/contract-gaps.md index 3cbb42e..bd2b0be 100644 --- a/docs/contract-gaps.md +++ b/docs/contract-gaps.md @@ -103,6 +103,36 @@ case") is exactly the defect RUK-258 removed. --- +### `password_set` on /me — read by the frontend, not yet on the wire + +| Field | Where it is needed | What is on the wire | Ticket | Stub | +| -------------- | --------------------------------------------------- | --------------------------------------- | ------- | ---------------- | +| `password_set` | profile password card, `/set-password` (RUK-289 UI) | the key is absent from the recorded 200 | RUK-289 | none — see below | + +The backend that serves this field is written but sits on an unmerged branch, so +the recorded fixture predates it and the field arrives as `undefined` until that +branch ships. The frontend types it optional and treats `undefined` as "unknown" +rather than as `false`: the two mean different things, and collapsing them would +draw a set-password form for every operator, whose every save is then a 400. + +**There is no mapper stub, because there is no mapper.** `/api/me` is an +unnarrowed pass-through (`NextResponse.json(data)`), so nothing substitutes a +placeholder value the way `notify-channel-mapper.ts` does for `updated_at` — the +key is simply not there. The Stub column has nothing to point at. + +**This row is prose, in the sense the `updated_at` row above means it** — the +Class-B stub-scanner is pinned to `maintenance-mapper.ts` and cannot cover it. +Unlike that row, however, this one is not unprotected: `contract-gaps.test.ts` +asserts that `password_set` is **absent** from `me.json`, which is green today +and goes red on the day the fixture is re-recorded against a backend that sends +it. That is the day this row is owed deletion, along with the assertion itself. + +The pass-through assertion in `me.contract.test.ts` deliberately does NOT serve +this purpose: it compares the route's echo against the same fixture that fed its +mock, so it is a tautology on key sets and stays green whatever the fixture +holds. It proves the route narrows nothing; it says nothing about which fields +exist. + ## Class B′ — the backend sends it, the frontend does not read it The opposite direction. It does not break a screen, but it means data the diff --git a/src/domain/admin/user.ts b/src/domain/admin/user.ts index 87819b2..51b51f3 100644 --- a/src/domain/admin/user.ts +++ b/src/domain/admin/user.ts @@ -27,6 +27,25 @@ export interface User { * optional so the FE tolerates the pre-RUK-202 wire that omits it entirely. */ timezone?: string | null; + /** + * Whether this account has a password credential (RUK-289, `GET`/`PATCH /me`). + * + * THREE values, not two, and the difference decides which form the profile + * card draws: + * + * - `true` — a password exists. + * - `false` — no password, OR the backend's read of the credential failed + * (it logs and answers `false` with a 200). So `false` is a + * usable hint for choosing a form and is NOT a security + * assertion; a wrong guess is answered by the endpoint with a + * 400, never accepted silently. + * - absent — the deployed backend predates the field. + * + * Optional for that last reason, and `undefined` must never be coerced to + * `false`: they mean different things and the UI treats them differently + * (SPEC §1.4, §2.4). Same shape as `timezone` above, for the same reason. + */ + password_set?: boolean; /** * Telegram handle used to name this person in notification text (RUK-217). * Stored **verbatim**, including any leading `@` — `@username` and `username` are diff --git a/tests/contracts/contract-gaps.test.ts b/tests/contracts/contract-gaps.test.ts index f245b10..96b0e3a 100644 --- a/tests/contracts/contract-gaps.test.ts +++ b/tests/contracts/contract-gaps.test.ts @@ -353,6 +353,23 @@ describe("registry — refuted claims stay refuted", () => { // `null` is a VALUE — "not set". Absence would be the gap, and it is not. expect("timezone" in fixture("me.json")).toBe(true); }); + + it("`password_set` is NOT yet a key on /me (RUK-289 gap, still open)", () => { + // The mirror of the assertion above, and the only executable check the + // `password_set` registry row has. + // + // It is green today because the backend that serves this field is on an + // unmerged branch, so the recorded fixture predates it. It goes RED on the + // day someone re-records `me.json` against a backend that sends the field — + // which is exactly the day the gap closes, the registry row is owed + // deletion, and this assertion is owed deletion with it. + // + // Written this way round deliberately. The pass-through assertion in + // `me.contract.test.ts` cannot do this job: it compares the route's echo + // against the same fixture that fed the mock, so it is a tautology on key + // sets and stays green whatever the fixture contains. + expect("password_set" in fixture("me.json")).toBe(false); + }); }); describe("the registry file itself", () => { From d80cf1fa410979876e4668af893f2a2dc15923f8 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 01:41:27 +0300 Subject: [PATCH 03/14] feat(auth): add the forgot-password flow to /login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/app/(public)/login/page.tsx | 20 ++ .../auth/__tests__/password-policy.test.ts | 41 +++ src/domain/auth/sign-in-method.ts | 29 ++ .../accept-invite-no-query-provider.test.tsx | 3 + .../__tests__/login-page-methods.test.tsx | 3 + .../auth/__tests__/login-page.test.tsx | 3 + .../__tests__/password-reset-flow.test.tsx | 184 ++++++++++++ src/features/auth/login-page.tsx | 120 ++++++-- src/features/auth/password-reset-flow.tsx | 278 ++++++++++++++++++ src/features/auth/password-sign-in-form.tsx | 15 +- src/features/auth/use-code-timers.ts | 115 ++++++++ .../__tests__/password-reset-actions.test.ts | 272 +++++++++++++++++ src/server/auth/backend-token-exchange.ts | 69 +++++ src/server/auth/contracts.ts | 20 ++ src/server/auth/password-reset-actions.ts | 161 ++++++++++ 15 files changed, 1307 insertions(+), 26 deletions(-) create mode 100644 src/domain/auth/__tests__/password-policy.test.ts create mode 100644 src/features/auth/__tests__/password-reset-flow.test.tsx create mode 100644 src/features/auth/password-reset-flow.tsx create mode 100644 src/features/auth/use-code-timers.ts create mode 100644 src/server/auth/__tests__/password-reset-actions.test.ts create mode 100644 src/server/auth/password-reset-actions.ts diff --git a/src/app/(public)/login/page.tsx b/src/app/(public)/login/page.tsx index 29c7a89..0ecc280 100644 --- a/src/app/(public)/login/page.tsx +++ b/src/app/(public)/login/page.tsx @@ -5,6 +5,12 @@ import { credentialsSignInAction, requestOtpAction, } from "@/server/auth/built-in-sign-in-actions"; +import { + abandonPasswordResetAction, + confirmPasswordResetAction, + requestPasswordResetAction, +} from "@/server/auth/password-reset-actions"; +import { readPasswordResetBinding } from "@/server/auth/otp-nonce-cookie"; import { signIn } from "@/server/auth/auth-config"; import { safeNext } from "@/server/auth/safe-next"; @@ -41,6 +47,16 @@ export default async function Page({ // break-glass fallback rather than a 500. const providers = await resolveAuthProviders(); + // A password reset spans an email round-trip, so the user leaves this tab and + // comes back — reload is the flow's primary re-entry, not an edge case. The + // cookie is httpOnly and readable only here, so the step has to be resolved + // server-side and handed down. + // + // ONLY the address crosses. The nonce stays in the cookie: it is httpOnly + // precisely so browser JavaScript cannot read the binding, and passing the + // whole binding to a client component would give that away for nothing. + const resetBinding = await readPasswordResetBinding(); + /** * The built-in methods post through server actions rather than a client * `fetch`: NextAuth attaches its CSRF token only when `signIn` runs on the @@ -66,6 +82,10 @@ export default async function Page({ otpSignInAction={otpSignInAction} passwordSignInAction={passwordSignInAction} changeEmailAction={changeEmailAction} + requestPasswordResetAction={requestPasswordResetAction} + confirmPasswordResetAction={confirmPasswordResetAction} + abandonPasswordResetAction={abandonPasswordResetAction} + resetInProgressEmail={resetBinding?.email} /> ); } diff --git a/src/domain/auth/__tests__/password-policy.test.ts b/src/domain/auth/__tests__/password-policy.test.ts new file mode 100644 index 0000000..3e3cad9 --- /dev/null +++ b/src/domain/auth/__tests__/password-policy.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from "vitest"; + +import { isPasswordWithinPolicy } from "@/domain/auth/sign-in-method"; + +describe("password policy is measured in bytes, as the backend measures it", () => { + it("rejects 11 ASCII characters and accepts 12", () => { + expect(isPasswordWithinPolicy("a".repeat(11))).toBe(false); + expect(isPasswordWithinPolicy("a".repeat(12))).toBe(true); + }); + + it("rejects 257 ASCII characters and accepts 256", () => { + expect(isPasswordWithinPolicy("a".repeat(257))).toBe(false); + expect(isPasswordWithinPolicy("a".repeat(256))).toBe(true); + }); + + // The case that makes this suite bite. A naive `.length >= 12` implementation + // REJECTS this — 11 characters — while the backend ACCEPTS it, because the + // string is 22 bytes. Getting this wrong on the reset path surfaces as + // "that code is wrong" about a perfectly good code. + it("accepts 11 Cyrillic characters, which are 22 bytes", () => { + const password = "паролькудлин"; + expect(password.length).toBe(12); + expect(new TextEncoder().encode(password).length).toBe(24); + + const eleven = "паролькудли"; + expect(eleven.length).toBe(11); + expect(new TextEncoder().encode(eleven).length).toBe(22); + expect(isPasswordWithinPolicy(eleven)).toBe(true); + }); + + // The mirror: 6 Cyrillic characters are exactly 12 bytes, so the backend + // accepts them and so must we, however short they look. + it("accepts 6 Cyrillic characters, which are exactly 12 bytes", () => { + expect(new TextEncoder().encode("пароль").length).toBe(12); + expect(isPasswordWithinPolicy("пароль")).toBe(true); + }); + + it("rejects a 256-character Cyrillic password, which is 512 bytes", () => { + expect(isPasswordWithinPolicy("я".repeat(256))).toBe(false); + }); +}); diff --git a/src/domain/auth/sign-in-method.ts b/src/domain/auth/sign-in-method.ts index 9786eff..fff97f1 100644 --- a/src/domain/auth/sign-in-method.ts +++ b/src/domain/auth/sign-in-method.ts @@ -40,3 +40,32 @@ export function isKnownSignInMethodType(value: string): value is SignInMethodTyp export function isWellFormedOtpCode(value: string): boolean { return /^\d{6}$/.test(value.trim()); } + +/** + * The backend's password length policy, in BYTES (RUK-289). + * + * Bytes, not characters, and the distinction is not pedantic. The backend + * validates with ozzo's `Length` rather than `RuneLength`, which measures Go's + * `len()` — so "аброакадабра" is 12 characters but 24 bytes, and a 6-character + * Cyrillic password is 12 bytes and passes. Measuring `.length` here would + * diverge from the server for every non-ASCII password. + */ +export const PASSWORD_MIN_BYTES = 12; +export const PASSWORD_MAX_BYTES = 256; + +/** + * Whether a password satisfies the backend's policy, measured the way the + * backend measures it. + * + * Duplicating a server-side rule on the client is usually a smell. Here it is + * required: on the reset path a policy violation is collapsed into the same + * 401 as a wrong code, so without this check a user with a short password is + * told the code they just read off their screen is wrong. The maximum matters + * for the same reason — an over-long password lands in that same collapse. + * + * This is a UX guard, never an authorization one. The backend re-checks. + */ +export function isPasswordWithinPolicy(password: string): boolean { + const bytes = new TextEncoder().encode(password).length; + return bytes >= PASSWORD_MIN_BYTES && bytes <= PASSWORD_MAX_BYTES; +} diff --git a/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx b/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx index 239718b..65baa21 100644 --- a/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx +++ b/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx @@ -67,6 +67,9 @@ describe("public pages render without a QueryClientProvider", () => { otpSignInAction: async () => ({}), passwordSignInAction: async () => ({}), changeEmailAction: async () => {}, + requestPasswordResetAction: async () => ({}), + confirmPasswordResetAction: async () => ({}), + abandonPasswordResetAction: async () => {}, }; expect(() => diff --git a/src/features/auth/__tests__/login-page-methods.test.tsx b/src/features/auth/__tests__/login-page-methods.test.tsx index 2cea0bd..e078bfd 100644 --- a/src/features/auth/__tests__/login-page-methods.test.tsx +++ b/src/features/auth/__tests__/login-page-methods.test.tsx @@ -21,6 +21,9 @@ const actions = { otpSignInAction: async () => ({}), passwordSignInAction: async () => ({}), changeEmailAction: async () => {}, + requestPasswordResetAction: async () => ({}), + confirmPasswordResetAction: async () => ({}), + abandonPasswordResetAction: async () => {}, }; const PASSWORD: SignInMethod = { id: "email_password", type: "password", display_name: "Password" }; diff --git a/src/features/auth/__tests__/login-page.test.tsx b/src/features/auth/__tests__/login-page.test.tsx index 5b3e982..36293b2 100644 --- a/src/features/auth/__tests__/login-page.test.tsx +++ b/src/features/auth/__tests__/login-page.test.tsx @@ -18,6 +18,9 @@ function renderLogin(error?: string) { requestOtpAction={async () => ({})} otpSignInAction={async () => ({})} passwordSignInAction={async () => ({})} + requestPasswordResetAction={async () => ({})} + confirmPasswordResetAction={async () => ({})} + abandonPasswordResetAction={async () => {}} changeEmailAction={async () => {}} />, ); diff --git a/src/features/auth/__tests__/password-reset-flow.test.tsx b/src/features/auth/__tests__/password-reset-flow.test.tsx new file mode 100644 index 0000000..a9fafec --- /dev/null +++ b/src/features/auth/__tests__/password-reset-flow.test.tsx @@ -0,0 +1,184 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; + +import { PasswordResetFlow } from "@/features/auth/password-reset-flow"; +import { MAX_CODE_ATTEMPTS } from "@/features/auth/use-code-timers"; + +afterEach(() => cleanup()); + +const LONG_ENOUGH = "a-long-enough-password"; + +function setup(overrides: Partial> = {}) { + const props = { + requestCode: vi.fn(async () => ({})), + confirm: vi.fn(async () => ({ done: true })), + abandon: vi.fn(async () => {}), + onDone: vi.fn(), + onCancel: vi.fn(), + ...overrides, + }; + render(); + return props; +} + +/** Walks step one so the code + password fields are on screen. */ +async function reachCodeStep(props: ReturnType) { + fireEvent.change(screen.getByLabelText("Reset your password"), { + target: { value: "op@example.test" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); + await waitFor(() => expect(props.requestCode).toHaveBeenCalled()); + await screen.findByLabelText("Enter the 6-digit code"); +} + +async function submitCode(code: string, password: string) { + fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { target: { value: code } }); + fireEvent.change(screen.getByLabelText("New password"), { target: { value: password } }); + fireEvent.click(screen.getByRole("button", { name: "Set new password" })); +} + +describe("the reset flow's two steps", () => { + it("asks for an address, then for a code and a new password", async () => { + const props = setup(); + await reachCodeStep(props); + + expect(screen.getByLabelText("New password")).toBeTruthy(); + expect(screen.getByText(/Sent to op@example.test/)).toBeTruthy(); + }); + + it("hands the confirmation up rather than trying to sign the user in", async () => { + // The backend answers 204 with no tokens and has revoked every session, so + // there is nothing to sign into — the page owns what the user is told. + const props = setup(); + await reachCodeStep(props); + await submitCode("123456", LONG_ENOUGH); + + await waitFor(() => expect(props.onDone).toHaveBeenCalled()); + }); +}); + +describe("the client-side length check", () => { + // AC-3, at the UI. Without this the request goes out, the backend answers the + // same 401 it uses for a wrong code, and the user is told the code they just + // read off their screen is wrong. + it("refuses a short password without calling the server", async () => { + const props = setup(); + await reachCodeStep(props); + await submitCode("123456", "short"); + + expect(props.confirm).not.toHaveBeenCalled(); + expect(screen.getByRole("alert").textContent).toMatch(/at least 12 characters/i); + }); + + // The case a `.length >= 12` implementation gets wrong: 11 characters, 22 + // bytes, accepted by the backend. + it("accepts an 11-character Cyrillic password", async () => { + const props = setup(); + await reachCodeStep(props); + await submitCode("123456", "паролькудли"); + + await waitFor(() => expect(props.confirm).toHaveBeenCalled()); + }); +}); + +describe("the local attempt budget", () => { + // AC-13. The backend collapses "wrong code", "expired" and "attempts + // exhausted" into one answer, so the client cannot learn from a response that + // the budget is gone. Without a local count the binding survives its full TTL + // and every reload drops the user back onto a dead code. + it("returns to step one once the budget is spent, discarding the binding", async () => { + const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_failed" })) }); + await reachCodeStep(props); + + for (let i = 0; i < MAX_CODE_ATTEMPTS; i++) { + await submitCode("000000", LONG_ENOUGH); + await waitFor(() => expect(props.confirm).toHaveBeenCalledTimes(i + 1)); + } + + await waitFor(() => expect(props.abandon).toHaveBeenCalled()); + expect(screen.getByLabelText("Reset your password")).toBeTruthy(); + }); + + it("does not spend the budget on a locally rejected password", async () => { + const props = setup(); + await reachCodeStep(props); + + for (let i = 0; i < MAX_CODE_ATTEMPTS + 2; i++) { + await submitCode("123456", "short"); + } + + // Nothing was sent, so nothing was spent and the binding is still good. + expect(props.confirm).not.toHaveBeenCalled(); + expect(props.abandon).not.toHaveBeenCalled(); + expect(screen.getByLabelText("Enter the 6-digit code")).toBeTruthy(); + }); +}); + +describe("failures the user must be able to tell apart", () => { + it("sends the user back to step one when the binding is gone", async () => { + const props = setup({ + confirm: vi.fn(async () => ({ error: "password_reset_session_mismatch" })), + }); + await reachCodeStep(props); + await submitCode("123456", LONG_ENOUGH); + + await waitFor(() => expect(props.abandon).toHaveBeenCalled()); + expect(screen.getByLabelText("Reset your password")).toBeTruthy(); + }); + + // An outage is a fact about the service. Folding it into the wrong-code copy + // tells every user their input was wrong while nothing of theirs was. + it("says the service is unavailable rather than blaming the code", async () => { + const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_unavailable" })) }); + await reachCodeStep(props); + await submitCode("123456", LONG_ENOUGH); + + await waitFor(() => { + expect(screen.getByRole("alert").textContent).toMatch(/unavailable right now/i); + }); + // Still on step two: nothing about the code was wrong, so the user keeps it. + expect(screen.getByLabelText("Enter the 6-digit code")).toBeTruthy(); + }); + + it("keeps the user on step two after a wrong code, so attempts are not wasted", async () => { + const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_failed" })) }); + await reachCodeStep(props); + await submitCode("000000", LONG_ENOUGH); + + await waitFor(() => expect(props.confirm).toHaveBeenCalled()); + expect(props.abandon).not.toHaveBeenCalled(); + expect(screen.getByLabelText("Enter the 6-digit code")).toBeTruthy(); + }); +}); + +describe("rehydration after a reload", () => { + // The flow spans an email round-trip, so the user leaves the tab. The server + // page reads the cookie and hands down the step; the nonce never crosses. + it("resumes at step two with the bound address", () => { + render( + ({}))} + confirm={vi.fn(async () => ({ done: true }))} + abandon={vi.fn(async () => {})} + onDone={vi.fn()} + onCancel={vi.fn()} + />, + ); + + expect(screen.getByLabelText("Enter the 6-digit code")).toBeTruthy(); + expect(screen.getByText(/Sent to op@example.test/)).toBeTruthy(); + }); +}); + +describe("the destructive consequence is stated before the submit", () => { + it("warns on both steps that this signs the user out everywhere", async () => { + const props = setup(); + expect(screen.getByText(/signs you out everywhere/i)).toBeTruthy(); + + await reachCodeStep(props); + expect(screen.getByText(/signs you out of every device/i)).toBeTruthy(); + }); +}); diff --git a/src/features/auth/login-page.tsx b/src/features/auth/login-page.tsx index 1a0f9c3..6420c0d 100644 --- a/src/features/auth/login-page.tsx +++ b/src/features/auth/login-page.tsx @@ -1,6 +1,7 @@ "use client"; import { AlertTriangle, ChevronRight } from "lucide-react"; +import { useState } from "react"; import { Button } from "@/shared/ui/shadcn/button"; import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "@/shared/ui/shadcn/tooltip"; @@ -8,6 +9,7 @@ import { BrandIcon, MaintMark, type BrandProvider } from "@/shared/ui/icons/bran import type { SignInMethod } from "@/domain/auth/sign-in-method"; import { OtpSignInFlow } from "@/features/auth/otp-sign-in-flow"; import { PasswordSignInForm } from "@/features/auth/password-sign-in-form"; +import { PasswordResetFlow } from "@/features/auth/password-reset-flow"; export interface LoginPageProps { error?: string; @@ -32,6 +34,26 @@ export interface LoginPageProps { passwordSignInAction: (email: string, password: string) => Promise<{ error?: string }>; /** Abandons the current OTP flow so another address can be used. */ changeEmailAction: () => Promise; + /** Step one of the password reset (RUK-289): mails a code, binds this browser. */ + requestPasswordResetAction: (email: string) => Promise<{ error?: string }>; + /** Step two: redeems the code, installs the password, ends every session. */ + confirmPasswordResetAction: (args: { + email: string; + code: string; + newPassword: string; + }) => Promise<{ error?: string; done?: boolean }>; + /** Abandons the reset flow, discarding its binding. */ + abandonPasswordResetAction: () => Promise; + /** + * Rehydrated from the reset cookie by the server page. A reset spans an email + * round-trip, so the user WILL leave the tab and come back; without this the + * component remounts at step one while a live binding sits on the server. + * + * Only the address crosses, never the nonce: the cookie is httpOnly so that + * browser JavaScript cannot read the binding, and passing the whole thing + * down would undo exactly that. + */ + resetInProgressEmail?: string; } /** @@ -72,10 +94,28 @@ const BREAK_GLASS_METHODS: SignInMethod[] = [ /** Disabled providers are gated until backend support ships. */ const COMING_SOON_TOOLTIP = "Coming soon — additional providers are on the way"; -export function LoginPage({ error, methods, signInAction, ...actions }: LoginPageProps) { +export function LoginPage({ + error, + methods, + signInAction, + requestPasswordResetAction, + confirmPasswordResetAction, + abandonPasswordResetAction, + resetInProgressEmail, + ...actions +}: LoginPageProps) { const resolvedFailed = methods === undefined; const builtIn = (resolvedFailed ? BREAK_GLASS_METHODS : methods).filter((m) => !OAUTH_IDS.has(m.id)); + const offersPassword = builtIn.some((m) => m.type === "password"); + // A live binding only rehydrates if this page is still drawing the form the + // affordance lives in. An operator who toggled password sign-in off between + // the code being sent and the tab being reloaded gets the normal page: the + // advertised method list is the authority on what is offered, and a cookie + // must not resurrect a withdrawn one. + const [resetting, setResetting] = useState(Boolean(resetInProgressEmail) && offersPassword); + const [resetDone, setResetDone] = useState(false); + return (
@@ -101,28 +141,63 @@ export function LoginPage({ error, methods, signInAction, ...actions }: LoginPag ) : null} -
- {OAUTH_PROVIDERS.map((p) => - p.enabled ? ( -
- +
+ ) : ( + {p.label} -
+ + ), + )} + + {builtIn.map((method) => ( + { + setResetDone(false); + setResetting(true); + }} + {...actions} + /> + ))} + + )} {resolvedFailed ? (

@@ -153,7 +228,8 @@ function BuiltInMethod({ otpSignInAction, passwordSignInAction, changeEmailAction, -}: BuiltInMethodActions & { method: SignInMethod }) { + onForgotPassword, +}: BuiltInMethodActions & { method: SignInMethod; onForgotPassword: () => void }) { if (method.type === "password") { return (

diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx new file mode 100644 index 0000000..b69a975 --- /dev/null +++ b/src/features/auth/password-reset-flow.tsx @@ -0,0 +1,278 @@ +"use client"; + +import { useState } from "react"; + +import { Button } from "@/shared/ui/shadcn/button"; +import { Input } from "@/shared/ui/shadcn/input"; +import { Label } from "@/shared/ui/shadcn/label"; +import { isWellFormedOtpCode, isPasswordWithinPolicy } from "@/domain/auth/sign-in-method"; +import { flowErrorMessage } from "@/features/auth/otp-sign-in-flow"; +import { MAX_CODE_ATTEMPTS, useCodeTimers } from "@/features/auth/use-code-timers"; + +/** + * "Forgot password" — a two-step emailed-code flow that ends in a new password + * (RUK-289). + * + * A sibling of `OtpSignInFlow` rather than a mode of it. That component's steps + * end in a session (its success path is a redirect thrown by NextAuth, so there + * is no success state to render), while this one ends signed OUT with something + * to say. Sharing the shell would mean a prop deciding which of two endpoints, + * which of two cookies and which of two terminal states applies — the timers + * are what the two genuinely share, and those are extracted into a hook. + */ + +type Step = "email" | "code"; + +export interface PasswordResetFlowProps { + /** Rehydrated from the reset cookie by the server page, after a reload. */ + initialEmail?: string; + initialStep?: Step; + requestCode: (email: string) => Promise<{ error?: string }>; + confirm: (args: { + email: string; + code: string; + newPassword: string; + }) => Promise<{ error?: string; done?: boolean }>; + abandon: () => Promise; + /** Returns to the sign-in form; the confirmation is owned by the page. */ + onDone: () => void; + onCancel: () => void; +} + +export function PasswordResetFlow({ + initialEmail, + initialStep, + requestCode, + confirm, + abandon, + onDone, + onCancel, +}: PasswordResetFlowProps) { + const [step, setStep] = useState(initialStep ?? "email"); + const [email, setEmail] = useState(initialEmail ?? ""); + const [code, setCode] = useState(""); + const [password, setPassword] = useState(""); + const [error, setError] = useState(); + const [pending, setPending] = useState(false); + // Counts THIS browser's submits against the backend's per-code budget. The + // backend collapses "wrong code", "expired" and "attempts exhausted" into one + // indistinguishable answer, so without a local count a user who burns every + // attempt keeps a live cookie for its full TTL and every reload drops them + // back onto a permanently dead code. + const [attempts, setAttempts] = useState(0); + + const timers = useCodeTimers(step === "code"); + const budgetSpent = attempts >= MAX_CODE_ATTEMPTS; + + async function send(address: string) { + setPending(true); + setError(undefined); + const result = await requestCode(address); + setPending(false); + + if (result.error) { + setError(result.error); + timers.startCooldown(); + return; + } + timers.start(); + setCode(""); + setAttempts(0); + setStep("code"); + } + + /** Leaves step two for step one, discarding the binding server-side. */ + async function restart() { + await abandon(); + timers.reset(); + setStep("email"); + setCode(""); + setAttempts(0); + } + + async function onSubmitCode(event: React.FormEvent) { + event.preventDefault(); + if (pending || timers.expired || budgetSpent) return; + + if (!isWellFormedOtpCode(code)) { + setError("invalid_code_format"); + return; + } + // Checked here as well as in the action so the user is told which field is + // wrong before a round-trip. The action is the authority; this is the + // message. + if (!isPasswordWithinPolicy(password)) { + setError("password_policy_violation"); + return; + } + + await timers.guard(async () => { + setPending(true); + setError(undefined); + const result = await confirm({ email, code: code.trim(), newPassword: password }); + setPending(false); + + if (result.done) { + onDone(); + return; + } + + // Counted for every answer the server gave, including a mismatch: the + // backend claims an attempt BEFORE it compares the code, so a stale nonce + // costs one exactly as a wrong digit does. + const spent = attempts + 1; + setAttempts(spent); + + if (result.error === "password_reset_session_mismatch") { + // The binding is already gone server-side, so step two is a dead end. + await restart(); + } else if (spent >= MAX_CODE_ATTEMPTS) { + // Out of attempts: drop the binding rather than leave a live cookie + // pointing at a code that can no longer be redeemed. + await restart(); + } + setError(result.error); + }); + } + + if (step === "email") { + return ( +
{ + e.preventDefault(); + const trimmed = email.trim(); + if (!trimmed || pending) return; + void send(trimmed); + }} + > + +

+ We'll email you a code if that address has an account. Setting a new password signs you out + everywhere. +

+ setEmail(e.target.value)} + aria-describedby={error ? "reset-error" : undefined} + /> + {error ? : null} + + + + ); + } + + const dead = timers.expired || budgetSpent; + + return ( +
+ +

+ Sent to {email}.{" "} + +

+ setCode(e.target.value.replace(/\D/g, ""))} + /> + + setPassword(e.target.value)} + aria-describedby="reset-password-hint" + /> + {/* + * "Characters" rather than bytes: the policy is 12 BYTES, which is not a + * unit to put in front of an operator. The hint is the ASCII worst case, + * so it can only ever under-promise — an 11-character Cyrillic password + * is 22 bytes and is accepted. Erring that way shows a user their + * password was taken when the hint implied otherwise, never the reverse. + */} +

+ At least 12 characters. This signs you out of every device. +

+ {dead ? ( + + ) : error ? ( + + ) : ( +

+ Expires in {formatRemaining(timers.remaining)} +

+ )} + {!dead ? ( + + ) : null} + + + ); +} + +function formatRemaining(seconds: number): string { + const m = Math.floor(seconds / 60); + const s = seconds % 60; + return `${m}:${String(s).padStart(2, "0")}`; +} + +function ResetError({ code }: { code: string }) { + return ( + + ); +} + +/** + * Reset-specific copy, falling through to the shared map for the codes both + * flows raise. Kept separate where the wording has to differ: sign-in's + * mismatch copy tells the user to go back and sign in, which is not where + * someone mid-reset is trying to go. + */ +export function resetErrorMessage(code: string): string { + switch (code) { + case "password_reset_session_mismatch": + return "This code can't be checked in this browser. Request a new one to continue."; + case "password_reset_failed": + return "That code isn't valid, or it has been used too many times. Request a new one."; + case "password_policy_violation": + return "Choose a longer password — at least 12 characters."; + case "password_reset_unavailable": + // A fact about the service, not about the account or the code. Saying so + // stops an outage from reading as "you typed something wrong". + return "Password reset is unavailable right now. Try again shortly."; + default: + return flowErrorMessage(code); + } +} diff --git a/src/features/auth/password-sign-in-form.tsx b/src/features/auth/password-sign-in-form.tsx index 2ef421b..0540a79 100644 --- a/src/features/auth/password-sign-in-form.tsx +++ b/src/features/auth/password-sign-in-form.tsx @@ -15,17 +15,19 @@ import { flowErrorMessage } from "@/features/auth/otp-sign-in-flow"; * `email_password`; the backend decides which internally, so this form does not * change when the second arrives. * - * Only the sign-in form lives here. Forced change, set-password-by-invite, - * forgot-password and change-in-profile are deliberately out of scope — those - * endpoints do not exist yet and their screens ship with them. + * The "Forgot password?" affordance lives INSIDE this form (RUK-289), so it + * appears and disappears with the password method itself: an instance that is + * not advertising password sign-in must not offer to reset one. */ export interface PasswordSignInFormProps { label: string; submit: (email: string, password: string) => Promise<{ error?: string }>; + /** Opens the reset flow. Absent when the deployment has no reset endpoint. */ + onForgotPassword?: () => void; } -export function PasswordSignInForm({ label, submit }: PasswordSignInFormProps) { +export function PasswordSignInForm({ label, submit, onForgotPassword }: PasswordSignInFormProps) { const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); const [error, setError] = useState(); @@ -77,6 +79,11 @@ export function PasswordSignInForm({ label, submit }: PasswordSignInFormProps) { + {onForgotPassword ? ( + + ) : null} ); } diff --git a/src/features/auth/use-code-timers.ts b/src/features/auth/use-code-timers.ts new file mode 100644 index 0000000..aad9827 --- /dev/null +++ b/src/features/auth/use-code-timers.ts @@ -0,0 +1,115 @@ +"use client"; + +import { useCallback, useEffect, useRef, useState } from "react"; + +/** + * The countdown, resend cooldown and double-submit guard shared by the two + * emailed-code flows: sign-in (RUK-288) and password reset (RUK-289). + * + * Extracted rather than duplicated because the numbers are contract facts, not + * styling: `CODE_TTL_SECONDS` equals the backend's `otp_ttl`, and both flows + * spend attempts from the same per-user budget. Two copies would drift, and the + * copy that drifted would tell a user their live code had expired. + */ + +/** Backend `otp_ttl`. The real expiry lives server-side and is never returned. */ +export const CODE_TTL_SECONDS = 300; + +/** + * Advisory only. The backend has no resend cooldown, and its per-IP bucket is + * shared with the password and OAuth endpoints — so an unthrottled resend + * button would lock the user out of the *other* ways in. The server's 429 is + * the real backstop. + */ +export const RESEND_COOLDOWN_SECONDS = 30; + +/** + * The backend's `auth.otp_max_attempts`. It claims an attempt BEFORE comparing + * the code, so a submit with a stale nonce spends one exactly like a wrong + * digit does — which is why the caller counts every submit rather than only the + * plausible ones. + * + * Deliberately the configured value (5) and not the backend's ceiling for that + * setting (10). Erring high would hand the user five doomed submits against an + * exhausted code; erring low costs at worst one premature "request a new code", + * which is the state they were heading for anyway. + */ +export const MAX_CODE_ATTEMPTS = 5; + +export interface CodeTimers { + /** Seconds until the code is presumed dead. Zero means expired. */ + remaining: number; + /** Seconds until resend is offered again. Zero means it is available. */ + cooldown: number; + /** True once `remaining` has run out on the code step. */ + expired: boolean; + /** Starts both timers — call on a successful code request. */ + start: () => void; + /** Stops both — call when leaving the code step. */ + reset: () => void; + /** Restarts only the cooldown, for a request that failed. */ + startCooldown: () => void; + /** + * Runs `fn` unless a submit is already in flight, and reports whether it ran. + * Each stray submit spends one of the attempts and the backend floors every + * response to ~300 ms, so a double-click is a live risk rather than a + * theoretical one. + */ + guard: (fn: () => Promise) => Promise; +} + +export function useCodeTimers(active: boolean): CodeTimers { + const [remaining, setRemaining] = useState(0); + const [cooldown, setCooldown] = useState(0); + const inFlight = useRef(false); + + // One interval drives both counters, torn down when the step is left, so a + // backgrounded tab cannot leave a timer running. + useEffect(() => { + if (!active) return; + const id = setInterval(() => { + setRemaining((r) => (r > 0 ? r - 1 : 0)); + setCooldown((c) => (c > 0 ? c - 1 : 0)); + }, 1000); + return () => clearInterval(id); + }, [active]); + + const start = useCallback(() => { + // Counted from response receipt, so the client is always slightly + // optimistic relative to the server. That is the safe direction: the + // backend, not this timer, decides whether a code is still valid. + setRemaining(CODE_TTL_SECONDS); + setCooldown(RESEND_COOLDOWN_SECONDS); + }, []); + + const reset = useCallback(() => { + setRemaining(0); + setCooldown(0); + }, []); + + const startCooldown = useCallback(() => { + // A failed request starts a fresh cooldown rather than leaving the button + // hot: a 429 answered by immediate retries is what caused it. + setCooldown(RESEND_COOLDOWN_SECONDS); + }, []); + + const guard = useCallback(async (fn: () => Promise): Promise => { + if (inFlight.current) return undefined; + inFlight.current = true; + try { + return await fn(); + } finally { + inFlight.current = false; + } + }, []); + + return { + remaining, + cooldown, + expired: active && remaining === 0, + start, + reset, + startCooldown, + guard, + }; +} diff --git a/src/server/auth/__tests__/password-reset-actions.test.ts b/src/server/auth/__tests__/password-reset-actions.test.ts new file mode 100644 index 0000000..33552ba --- /dev/null +++ b/src/server/auth/__tests__/password-reset-actions.test.ts @@ -0,0 +1,272 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { + abandonPasswordResetAction, + confirmPasswordResetAction, + requestPasswordResetAction, +} from "@/server/auth/password-reset-actions"; + +const requestPasswordResetCode = vi.fn(); +const confirmPasswordReset = vi.fn(); +const setPasswordResetBinding = vi.fn(); +const readPasswordResetBinding = vi.fn(); +const clearPasswordResetBinding = vi.fn(); +const signOut = vi.fn(); +const clearActiveSession = vi.fn(); + +vi.mock("@/server/auth/backend-token-exchange", () => ({ + requestPasswordResetCode: (...args: unknown[]) => requestPasswordResetCode(...args), + confirmPasswordReset: (...args: unknown[]) => confirmPasswordReset(...args), +})); + +vi.mock("@/server/auth/otp-nonce-cookie", async (importOriginal) => { + // `normalizeEmail` is the real one: the binding check compares against it, and + // a stubbed version would make the mismatch tests agree with themselves. + const actual = await importOriginal(); + return { + normalizeEmail: actual.normalizeEmail, + setPasswordResetBinding: (...args: unknown[]) => setPasswordResetBinding(...args), + readPasswordResetBinding: () => readPasswordResetBinding(), + clearPasswordResetBinding: () => clearPasswordResetBinding(), + }; +}); + +vi.mock("@/server/auth/auth-config", () => ({ signOut: (...args: unknown[]) => signOut(...args) })); +vi.mock("@/server/auth/session-token", () => ({ clearActiveSession: () => clearActiveSession() })); + +/** A backend failure as `postBackendJson` throws it. */ +function backendError(status: number, body = "") { + return Object.assign(new Error(`backend ${status}`), { status, responseBody: body }); +} + +beforeEach(() => { + vi.clearAllMocks(); + console.error = vi.fn(); +}); + +describe("requesting a reset code cannot become an account-existence oracle", () => { + // AC-1. The backend answers 202 for a registered address, an unknown one and + // a blocked one alike; this asserts the ACTION collapses them too, which is + // the only half of that promise the frontend owns. + it("answers identically for every address the backend accepts", async () => { + requestPasswordResetCode.mockResolvedValue({ session_nonce: "n" }); + + const registered = await requestPasswordResetAction("known@example.test"); + const unknown = await requestPasswordResetAction("nobody@example.test"); + const blocked = await requestPasswordResetAction("blocked@example.test"); + + expect(registered).toEqual({}); + expect(unknown).toEqual(registered); + expect(blocked).toEqual(registered); + }); + + it("binds the nonce to this browser", async () => { + requestPasswordResetCode.mockResolvedValue({ session_nonce: "nonce-1" }); + + await requestPasswordResetAction("op@example.test"); + + expect(setPasswordResetBinding).toHaveBeenCalledWith({ nonce: "nonce-1", email: "op@example.test" }); + }); + + it("rejects an empty address without calling the backend", async () => { + await expect(requestPasswordResetAction(" ")).resolves.toEqual({ error: "invalid_email" }); + expect(requestPasswordResetCode).not.toHaveBeenCalled(); + }); + + // AC-12. A rate limit is not a verdict on the address, and an outage is a + // fact about the service — neither may wear the uniform copy, which would + // tell every user their input was wrong. + it("separates a rate limit and an outage from the uniform answer", async () => { + requestPasswordResetCode.mockRejectedValueOnce(backendError(429)); + await expect(requestPasswordResetAction("op@example.test")).resolves.toEqual({ + error: "otp_rate_limited", + }); + + requestPasswordResetCode.mockRejectedValueOnce(backendError(503)); + await expect(requestPasswordResetAction("op@example.test")).resolves.toEqual({ + error: "password_reset_unavailable", + }); + + requestPasswordResetCode.mockRejectedValueOnce(backendError(404)); + await expect(requestPasswordResetAction("op@example.test")).resolves.toEqual({ + error: "password_reset_unavailable", + }); + }); +}); + +describe("confirming a reset", () => { + beforeEach(() => { + readPasswordResetBinding.mockResolvedValue({ nonce: "nonce-1", email: "op@example.test" }); + }); + + // AC-3, at the action boundary. The backend hides a policy violation inside + // the same 401 as a wrong code, so a password that fails the policy must + // never reach it — the user would be told their correct code was wrong. + it("refuses a short password without spending an attempt", async () => { + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "short", + }); + + expect(result).toEqual({ error: "password_policy_violation" }); + expect(confirmPasswordReset).not.toHaveBeenCalled(); + // The binding survives: nothing was spent, and the code is still good. + expect(clearPasswordResetBinding).not.toHaveBeenCalled(); + }); + + it("accepts an 11-character Cyrillic password, which is 22 bytes", async () => { + confirmPasswordReset.mockResolvedValue(undefined); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "паролькудли", + }); + + expect(result).toEqual({ done: true }); + expect(confirmPasswordReset).toHaveBeenCalled(); + }); + + // AC-4. Past the 204 the password IS changed and every session is dead. + it("tears the local session down on success", async () => { + confirmPasswordReset.mockResolvedValue(undefined); + + await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(signOut).toHaveBeenCalledWith({ redirect: false }); + expect(clearActiveSession).toHaveBeenCalled(); + }); + + // AC-4, the half that matters more. The backend has already committed the + // change; a teardown that throws must not swallow the confirmation, or the + // user is left believing their password is unchanged when it is not. + it("still confirms when the teardown throws", async () => { + confirmPasswordReset.mockResolvedValue(undefined); + signOut.mockRejectedValueOnce(new Error("cookie store unavailable")); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ done: true }); + }); + + it("sends the bound nonce, not anything the caller supplied", async () => { + confirmPasswordReset.mockResolvedValue(undefined); + + await confirmPasswordResetAction({ + email: "OP@Example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(confirmPasswordReset).toHaveBeenCalledWith({ + email: "op@example.test", + code: "123456", + sessionNonce: "nonce-1", + newPassword: "a-long-enough-password", + }); + }); + + it("reports a lost binding with the reset flow's own code, not sign-in's", async () => { + readPasswordResetBinding.mockResolvedValue(undefined); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + // Not `otp_session_mismatch`: its copy returns the user to sign-in, which + // is not where someone mid-reset is trying to go. + expect(result).toEqual({ error: "password_reset_session_mismatch" }); + expect(confirmPasswordReset).not.toHaveBeenCalled(); + }); + + it("refuses when the bound address is not the one being confirmed", async () => { + readPasswordResetBinding.mockResolvedValue({ nonce: "n", email: "someone-else@example.test" }); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ error: "password_reset_session_mismatch" }); + expect(confirmPasswordReset).not.toHaveBeenCalled(); + }); + + // The clear policy, which is the difference between a recoverable mistype and + // a destroyed code. + it("keeps the binding on a wrong code, so remaining attempts survive", async () => { + confirmPasswordReset.mockRejectedValueOnce( + backendError(401, '{"code":"unauthorized","message":"authentication failed"}'), + ); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "000000", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ error: "password_reset_failed" }); + expect(clearPasswordResetBinding).not.toHaveBeenCalled(); + }); + + it("clears the binding when the backend reports a session mismatch", async () => { + confirmPasswordReset.mockRejectedValueOnce( + backendError(401, '{"code":"otp_session_mismatch","message":"authentication failed"}'), + ); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ error: "password_reset_session_mismatch" }); + expect(clearPasswordResetBinding).toHaveBeenCalled(); + }); + + it("keeps an outage apart from a wrong code, and keeps the binding", async () => { + confirmPasswordReset.mockRejectedValueOnce(backendError(502)); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ error: "password_reset_unavailable" }); + expect(clearPasswordResetBinding).not.toHaveBeenCalled(); + }); + + it("never logs the address or the code", async () => { + confirmPasswordReset.mockRejectedValueOnce(backendError(401, "{}")); + + await confirmPasswordResetAction({ + email: "secret@example.test", + code: "424242", + newPassword: "a-long-enough-password", + }); + + const logged = JSON.stringify((console.error as unknown as ReturnType).mock.calls); + expect(logged).not.toContain("secret@example.test"); + expect(logged).not.toContain("424242"); + }); +}); + +describe("abandoning the flow", () => { + it("clears only the reset binding", async () => { + await abandonPasswordResetAction(); + + expect(clearPasswordResetBinding).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/server/auth/backend-token-exchange.ts b/src/server/auth/backend-token-exchange.ts index 2d42c6f..8f2c8b7 100644 --- a/src/server/auth/backend-token-exchange.ts +++ b/src/server/auth/backend-token-exchange.ts @@ -12,6 +12,8 @@ const ME_PATH = "/api/v1/me"; const OTP_REQUEST_PATH = "/api/v1/login/otp/request"; const OTP_VERIFY_PATH = "/api/v1/login/otp/verify"; const PASSWORD_LOGIN_PATH = "/api/v1/login/password"; +const PASSWORD_RESET_REQUEST_PATH = "/api/v1/password/reset/request"; +const PASSWORD_RESET_CONFIRM_PATH = "/api/v1/password/reset/confirm"; /** * BFF-owned OAuth. @@ -149,6 +151,73 @@ export async function loginWithPassword(args: { ); } +/** + * Password reset, step one: ask the backend to mail a code. + * + * Shaped exactly like `requestOtpCode` because it IS the same mechanism — the + * backend routes both through one OTP issuer. It answers 202 with a session + * nonce for every outcome, including an address with no account and a malformed + * body (a placeholder nonce), so nothing here may branch on the result. + * + * Note the consequence the caller must not paper over: because the two flows + * share one OTP record per user, asking for a reset code consumes any live + * sign-in code. Separate cookies keep the two BINDINGS apart; they cannot keep + * the codes apart. + */ +export async function requestPasswordResetCode(email: string): Promise<{ session_nonce: string }> { + return postBackendJson<{ session_nonce: string }>( + PASSWORD_RESET_REQUEST_PATH, + { email }, + (value): value is { session_nonce: string } => + typeof value === "object" && + value !== null && + typeof (value as { session_nonce?: unknown }).session_nonce === "string", + ); +} + +/** + * Password reset, step two: redeem the code and install the new password. + * + * Answers **204 with an empty body** — no token pair — and revokes every + * session, so the caller signs in again afterwards. It cannot go through + * `postBackendJson`, which requires a JSON payload it can shape-check. + * + * Failures collapse into one 401 "authentication failed", with + * `otp_session_mismatch` as the sole distinguishable case. A password that + * breaks the length policy is INSIDE that collapse and is reported as a wrong + * code — which is why the client checks the length before calling this. + */ +export async function confirmPasswordReset(args: { + email: string; + code: string; + sessionNonce: string; + newPassword: string; +}): Promise { + const config = readMaintmodeBackendConfig(); + const target = resolveBackendUrl(config.authApiBaseUrl, PASSWORD_RESET_CONFIRM_PATH); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); + + try { + const response = await fetch(target, { + method: "POST", + headers: { accept: "application/json", "content-type": "application/json" }, + body: JSON.stringify({ + email: args.email, + code: args.code, + session_nonce: args.sessionNonce, + new_password: args.newPassword, + }), + signal: controller.signal, + }); + if (!response.ok) { + throw new BackendAuthError(response.status, (await response.text()) || response.statusText); + } + } finally { + clearTimeout(timeout); + } +} + /** * Rotates the refresh token via `POST /api/v1/refresh`. Returns the new * `TokenPairResponse`. diff --git a/src/server/auth/contracts.ts b/src/server/auth/contracts.ts index 1d1158a..eb96a52 100644 --- a/src/server/auth/contracts.ts +++ b/src/server/auth/contracts.ts @@ -90,5 +90,25 @@ export const AUTH_ERROR_CODES = { // separate so the copy does not tell someone to re-check a correct code and // send more requests into the limiter that is already refusing them. otpRateLimited: "otp_rate_limited", + // Password reset (RUK-289). The reset flow's OWN mismatch code, deliberately + // not a reuse of `otpSessionMismatch`: that one's copy sends the user back to + // sign-in, which is the wrong destination when they are mid-reset and have + // not chosen a new password yet. Leaks nothing, for the same reason its + // sign-in twin leaks nothing — it names a fact about this browser. + passwordResetSessionMismatch: "password_reset_session_mismatch", + // Every other confirm failure: wrong code, expired, attempts exhausted, and — + // because the backend deliberately hides it inside the same collapse — a + // password that breaks the length policy. The client checks length before + // sending precisely so a user never sees this code for that reason. + passwordResetFailed: "password_reset_failed", + // The new password failed the client-side policy check, so nothing was sent. + // Distinct from every server answer: no attempt was spent and the binding is + // still good, so the copy must not tell the user to request a new code. + passwordPolicyViolation: "password_policy_violation", + // The endpoint itself is unreachable or broken (404/5xx), which is a fact + // about the SERVICE and not about an account — so saying so plainly leaks + // nothing. Kept apart from the anti-enumeration collapse because folding it + // in tells every user their input was wrong during an outage. + passwordResetUnavailable: "password_reset_unavailable", } as const; export type AuthErrorCode = (typeof AUTH_ERROR_CODES)[keyof typeof AUTH_ERROR_CODES]; diff --git a/src/server/auth/password-reset-actions.ts b/src/server/auth/password-reset-actions.ts new file mode 100644 index 0000000..a1848cd --- /dev/null +++ b/src/server/auth/password-reset-actions.ts @@ -0,0 +1,161 @@ +"use server"; + +import { signOut } from "@/server/auth/auth-config"; +import { confirmPasswordReset, requestPasswordResetCode } from "@/server/auth/backend-token-exchange"; +import { AUTH_ERROR_CODES } from "@/server/auth/contracts"; +import { + clearPasswordResetBinding, + normalizeEmail, + readPasswordResetBinding, + setPasswordResetBinding, +} from "@/server/auth/otp-nonce-cookie"; +import { clearActiveSession } from "@/server/auth/session-token"; +import { isPasswordWithinPolicy } from "@/domain/auth/sign-in-method"; + +/** + * Server actions behind the "forgot password" flow (RUK-289). + * + * Actions rather than BFF routes for the reason RUK-288 established: these + * write a cookie, and `/login` sits under `(public)`, which mounts neither + * React Query nor `sonner` — a `useMutation` there throws at runtime while + * `tsc` and the unit tests stay green. + */ + +export interface PasswordResetActionResult { + /** An `AUTH_ERROR_CODES` value the client renders in place, or undefined. */ + error?: string; + /** Set once the password is changed, so the sign-in form can confirm it. */ + done?: boolean; +} + +/** A backend failure's HTTP status, when it carried one. */ +function statusOf(error: unknown): number | undefined { + const status = (error as { status?: unknown } | null)?.status; + return typeof status === "number" ? status : undefined; +} + +/** Whether the backend's body named a specific code, without parsing prose. */ +function codeIs(error: unknown, code: string): boolean { + const body = (error as { responseBody?: unknown } | null)?.responseBody; + return typeof body === "string" && body.includes(`"code":"${code}"`); +} + +/** + * Step one: ask the backend to mail a reset code, and bind it to this browser. + * + * The backend answers 202 for every outcome — unknown address, blocked user, + * even a malformed body — so this reports success identically in all of them. + * Anything else would turn the screen into an account-existence oracle. + * + * What it cannot promise, and the copy must not either: if the user has a live + * code with attempts left, the backend issues no new one and still answers 202. + * "We sent a code if that address is registered" stays true; "check your inbox + * for a new code" would not. + */ +export async function requestPasswordResetAction(email: string): Promise { + const trimmed = email.trim(); + if (!trimmed) { + return { error: "invalid_email" }; + } + + try { + const { session_nonce: nonce } = await requestPasswordResetCode(trimmed); + await setPasswordResetBinding({ nonce, email: trimmed }); + return {}; + } catch (error) { + const status = statusOf(error); + // Logged because the user is shown one uniform state by design, which + // leaves an operator with nothing to diagnose from. Status only — never the + // address, which is the thing the uniform answer exists to protect. + console.error("[password-reset] request failed", { status }); + + if (status === 429) { + return { error: AUTH_ERROR_CODES.otpRateLimited }; + } + // A dead or missing endpoint is a fact about the service, not about an + // account, so saying so plainly leaks nothing — and folding it into the + // anti-enumeration copy would tell every user their input was wrong during + // an outage. + return { error: AUTH_ERROR_CODES.passwordResetUnavailable }; + } +} + +/** + * Step two: redeem the code, install the new password, and end the session. + * + * The length check runs BEFORE the request and is not optional politeness: the + * backend hides a policy violation inside the same 401 as a wrong code, so + * without it a user with a short password is told the code they just typed is + * wrong. It also spends no attempt, which is why its result must not clear the + * binding. + */ +export async function confirmPasswordResetAction(args: { + email: string; + code: string; + newPassword: string; +}): Promise { + if (!isPasswordWithinPolicy(args.newPassword)) { + return { error: AUTH_ERROR_CODES.passwordPolicyViolation }; + } + + const binding = await readPasswordResetBinding(); + if (!binding || binding.email !== normalizeEmail(args.email)) { + // No binding means this browser cannot prove it asked for the code, so the + // request would be refused anyway. Answered locally with the reset flow's + // own mismatch code — not sign-in's, whose copy sends the user somewhere + // they are not trying to go. + await clearPasswordResetBinding(); + return { error: AUTH_ERROR_CODES.passwordResetSessionMismatch }; + } + + try { + await confirmPasswordReset({ + email: binding.email, + code: args.code, + sessionNonce: binding.nonce, + newPassword: args.newPassword, + }); + } catch (error) { + const status = statusOf(error); + // `hadBinding` is a fact about this browser, not about the account, and it + // is the one thing separating "the user lost their tab" from "the user + // mistyped" — invisible in the response by design. + console.error("[password-reset] confirm failed", { status, hadBinding: true }); + + if (status === 429) { + return { error: AUTH_ERROR_CODES.otpRateLimited }; + } + if (status === undefined || status >= 500 || status === 404) { + return { error: AUTH_ERROR_CODES.passwordResetUnavailable }; + } + if (codeIs(error, "otp_session_mismatch")) { + await clearPasswordResetBinding(); + return { error: AUTH_ERROR_CODES.passwordResetSessionMismatch }; + } + // Everything else is the deliberate collapse: wrong code, expired, + // attempts exhausted. The binding is KEPT — attempts may remain, and + // discarding a still-usable code is worse than a retry. + return { error: AUTH_ERROR_CODES.passwordResetFailed }; + } + + // Past this point the password IS changed and every session is revoked. The + // teardown below can fail; the confirmation cannot be conditional on it. + await clearPasswordResetBinding(); + + try { + // Without this the NextAuth cookie outlives the backend session: `proxy.ts` + // still reads a session, `/login` bounces the user to `/`, and every + // request 401s on a dead token. + await signOut({ redirect: false }); + await clearActiveSession(); + } catch (error) { + console.error("[password-reset] post-reset session teardown failed", error); + } + + return { done: true }; +} + +/** Abandons the reset flow so the user can start again with another address. */ +export async function abandonPasswordResetAction(): Promise { + await clearPasswordResetBinding(); +} From 975643eb395b06536e50cfc9d21d5ee35a8bc987 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 01:44:17 +0300 Subject: [PATCH 04/14] feat(auth): add the change-password BFF route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../api/me/password/__tests__/route.test.ts | 169 ++++++++++++++++++ src/app/api/me/password/route.ts | 99 ++++++++++ src/server/auth/backend-token-exchange.ts | 90 ++++++++++ 3 files changed, 358 insertions(+) create mode 100644 src/app/api/me/password/__tests__/route.test.ts create mode 100644 src/app/api/me/password/route.ts diff --git a/src/app/api/me/password/__tests__/route.test.ts b/src/app/api/me/password/__tests__/route.test.ts new file mode 100644 index 0000000..09b3a5b --- /dev/null +++ b/src/app/api/me/password/__tests__/route.test.ts @@ -0,0 +1,169 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { POST } from "@/app/api/me/password/route"; + +const readActiveSession = vi.fn(); +const changeBackendPassword = vi.fn(); +const isSameOriginRequest = vi.fn(() => true); + +vi.mock("@/server/auth/session-token", () => ({ + readActiveSession: () => readActiveSession(), +})); +vi.mock("@/server/auth/backend-token-exchange", () => ({ + changeBackendPassword: (...args: unknown[]) => changeBackendPassword(...args), +})); +vi.mock("@/server/backend/security/csrf", () => ({ + isSameOriginRequest: () => isSameOriginRequest(), +})); + +function post(body: unknown) { + return new Request("https://app.test/api/me/password", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + isSameOriginRequest.mockReturnValue(true); + readActiveSession.mockResolvedValue({ + accessToken: "access-1", + refreshToken: "refresh-1", + accessTokenExpiresAt: Date.now() + 600_000, + }); +}); + +describe("the session is read once and sent verbatim", () => { + // AC-6. `readActiveSession` rotates the refresh token when the access token + // is near expiry, so a second read can hand back a different token — and the + // backend answers a superseded `refresh_token` with a 401 and NO change. + it("reads the session exactly once per request", async () => { + changeBackendPassword.mockResolvedValue({ ok: true }); + + await POST(post({ current_password: "old-password", new_password: "new-password-here" })); + + expect(readActiveSession).toHaveBeenCalledTimes(1); + }); + + // Without this the backend revokes EVERY session including the caller's, and + // the user is signed out by their own success. + it("sends the refresh token so the caller's own session survives", async () => { + changeBackendPassword.mockResolvedValue({ ok: true }); + + await POST(post({ current_password: "old-password", new_password: "new-password-here" })); + + expect(changeBackendPassword).toHaveBeenCalledWith( + expect.objectContaining({ accessToken: "access-1", refreshToken: "refresh-1" }), + ); + }); + + it("never retries the mutation", async () => { + changeBackendPassword.mockResolvedValue({ ok: false, kind: "session-stale" }); + + await POST(post({ new_password: "new-password-here" })); + + // One call, however it failed. A retry would send a token the refresh had + // already replaced, and the password would silently stay unchanged. + expect(changeBackendPassword).toHaveBeenCalledTimes(1); + }); + + it("omits current_password entirely when the account has none", async () => { + changeBackendPassword.mockResolvedValue({ ok: true }); + + await POST(post({ new_password: "new-password-here" })); + + // Absent, not empty: the backend rejects the field outright for an account + // with no password, so "" and undefined are different requests. + expect(changeBackendPassword).toHaveBeenCalledWith( + expect.objectContaining({ currentPassword: undefined }), + ); + }); +}); + +describe("no failure may answer 401", () => { + // AC-7, and the reason this route exists at all. `bffFetch` navigates to + // /login on any 401 carrying AUTH_REQUIRED, behind a never-resolving promise + // — so answering a wrong current password with a 401 signs the operator out + // on their most common mistake, with no message. + it("answers a wrong current password with a renderable status", async () => { + changeBackendPassword.mockResolvedValue({ ok: false, kind: "wrong-current-password" }); + + const response = await POST(post({ current_password: "wrong", new_password: "new-password-here" })); + + expect(response.status).toBe(422); + expect(response.status).not.toBe(401); + await expect(response.json()).resolves.toMatchObject({ code: "WRONG_CURRENT_PASSWORD" }); + }); + + it("distinguishes a stale session from a wrong password", async () => { + changeBackendPassword.mockResolvedValue({ ok: false, kind: "session-stale" }); + + const response = await POST(post({ new_password: "new-password-here" })); + + // Different meaning, different copy: nothing the user typed was wrong and + // the password was NOT changed. + expect(response.status).toBe(409); + await expect(response.json()).resolves.toMatchObject({ code: "SESSION_STALE" }); + }); + + it("passes a backend 400 through without parsing its prose", async () => { + changeBackendPassword.mockResolvedValue({ + ok: false, + kind: "rejected", + message: "validation error: the current password is required", + }); + + const response = await POST(post({ new_password: "new-password-here" })); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ code: "INVALID_REQUEST" }); + }); + + it("answers an outage as an outage", async () => { + changeBackendPassword.mockResolvedValue({ ok: false, kind: "unavailable" }); + + const response = await POST(post({ new_password: "new-password-here" })); + + expect(response.status).toBe(503); + }); +}); + +describe("the guards every mutating route here carries", () => { + it("refuses a cross-origin request", async () => { + isSameOriginRequest.mockReturnValue(false); + + const response = await POST(post({ new_password: "new-password-here" })); + + expect(response.status).toBe(403); + expect(changeBackendPassword).not.toHaveBeenCalled(); + }); + + it("answers 401 when there is genuinely no session", async () => { + // The one legitimate 401: no session at all, which IS the redirect case. + readActiveSession.mockResolvedValue(null); + + const response = await POST(post({ new_password: "new-password-here" })); + + expect(response.status).toBe(401); + await expect(response.json()).resolves.toMatchObject({ code: "AUTH_REQUIRED" }); + }); + + it("rejects a request with no new password", async () => { + const response = await POST(post({ current_password: "old-password" })); + + expect(response.status).toBe(400); + expect(changeBackendPassword).not.toHaveBeenCalled(); + }); +}); + +describe("success", () => { + it("answers 204 with no body", async () => { + changeBackendPassword.mockResolvedValue({ ok: true }); + + const response = await POST(post({ new_password: "new-password-here" })); + + expect(response.status).toBe(204); + await expect(response.text()).resolves.toBe(""); + }); +}); diff --git a/src/app/api/me/password/route.ts b/src/app/api/me/password/route.ts new file mode 100644 index 0000000..69edc5f --- /dev/null +++ b/src/app/api/me/password/route.ts @@ -0,0 +1,99 @@ +import { NextResponse } from "next/server"; + +import { changeBackendPassword } from "@/server/auth/backend-token-exchange"; +import { readActiveSession } from "@/server/auth/session-token"; +import { readJsonBody } from "@/server/backend/http/read-json-body"; +import { isSameOriginRequest } from "@/server/backend/security/csrf"; + +interface ChangePasswordBody { + /** Omitted when the account has no password yet — the backend rejects it then. */ + current_password?: string; + new_password?: string; +} + +/** + * POST /api/me/password — proxy to backend `POST /api/v1/me/password`. + * + * This route deliberately does NOT use `authenticatedBackendRequest`, and does + * not let a backend 401 reach `routeErrorResponse`. Both defaults are wrong + * here in the same direction: + * + * - the wrapper refreshes and RETRIES the mutation on a 401, but this endpoint + * takes a `refresh_token` in its body, so the retry would send the token the + * refresh just replaced — the backend answers 401 again and the password is + * never changed; + * - `routeErrorResponse` maps every `BackendUnauthorizedError` to + * `AUTH_REQUIRED`, which `bffFetch` answers by navigating to `/login` behind + * a never-resolving promise. A wrong current password is a 401, so the most + * common mistake on this form would sign the operator out with no message, + * indistinguishable from an expired session. + * + * So: read the session ONCE, send that exact token, never retry, and answer + * every failure with a non-401 status the card can render in place. + */ +export async function POST(request: Request) { + if (!isSameOriginRequest(request)) { + return NextResponse.json( + { error: "Cross-origin requests are not allowed", code: "FORBIDDEN" }, + { status: 403 }, + ); + } + + // Read once. `readActiveSession` rotates the refresh token when the access + // token is close to expiry, so calling it twice — or letting anything else + // refresh between the read and the send — puts a superseded token in the body. + const session = await readActiveSession(); + if (!session) { + return NextResponse.json({ error: "Sign-in is required", code: "AUTH_REQUIRED" }, { status: 401 }); + } + + const body = await readJsonBody(request); + const newPassword = body?.new_password; + if (typeof newPassword !== "string" || !newPassword) { + return NextResponse.json( + { error: "A new password is required", code: "INVALID_REQUEST" }, + { status: 400 }, + ); + } + + const outcome = await changeBackendPassword({ + accessToken: session.accessToken, + currentPassword: body?.current_password || undefined, + newPassword, + // Keeps THIS session alive while the backend revokes the others. Without + // it the backend revokes every session including the caller's, and the user + // is signed out by their own success. + refreshToken: session.refreshToken, + }); + + if (outcome.ok) { + return new NextResponse(null, { status: 204 }); + } + + switch (outcome.kind) { + case "wrong-current-password": + // 422, not 401: the status is what decides whether `bffFetch` navigates + // away, and this is a field the user can correct in place. + return NextResponse.json( + { error: "That current password isn't right", code: "WRONG_CURRENT_PASSWORD" }, + { status: 422 }, + ); + case "session-stale": + // Also not a 401, for the same mechanical reason — but the meaning is + // different and so is the copy: nothing the user typed was wrong, and + // the password was NOT changed. + return NextResponse.json( + { error: "Your session expired before the change was saved", code: "SESSION_STALE" }, + { status: 409 }, + ); + case "rejected": + // The backend's own message, passed through as an opaque string. Never + // parsed: its three 400s share one code and differ only in prose. + return NextResponse.json({ error: outcome.message, code: "INVALID_REQUEST" }, { status: 400 }); + default: + return NextResponse.json( + { error: "Password change is unavailable right now", code: "BACKEND_UNAVAILABLE" }, + { status: 503 }, + ); + } +} diff --git a/src/server/auth/backend-token-exchange.ts b/src/server/auth/backend-token-exchange.ts index 8f2c8b7..4e4bae2 100644 --- a/src/server/auth/backend-token-exchange.ts +++ b/src/server/auth/backend-token-exchange.ts @@ -14,6 +14,7 @@ const OTP_VERIFY_PATH = "/api/v1/login/otp/verify"; const PASSWORD_LOGIN_PATH = "/api/v1/login/password"; const PASSWORD_RESET_REQUEST_PATH = "/api/v1/password/reset/request"; const PASSWORD_RESET_CONFIRM_PATH = "/api/v1/password/reset/confirm"; +const CHANGE_PASSWORD_PATH = "/api/v1/me/password"; /** * BFF-owned OAuth. @@ -218,6 +219,95 @@ export async function confirmPasswordReset(args: { } } +/** + * The outcome of a change-password call, classified WITHOUT reading any prose. + * + * The backend's three 400s all carry `code: "invalid request"` and differ only + * in free text, so branching on the message would be a contract that breaks on + * a reword. What the caller knows instead is what it sent, which is enough. + */ +export type ChangePasswordOutcome = + | { ok: true } + /** Wrong `current_password` — the caller sent one and the backend refused. */ + | { ok: false; kind: "wrong-current-password" } + /** The `refresh_token` was stale or foreign; NOTHING was changed. */ + | { ok: false; kind: "session-stale" } + /** A 400: the wrong shape for this account's state, or a policy violation. */ + | { ok: false; kind: "rejected"; message: string } + | { ok: false; kind: "unavailable" }; + +/** + * Sets the caller's own password. + * + * Deliberately NOT routed through `authenticatedBackendRequest`, for two + * reasons that both end in "the user is signed out and their password is + * unchanged": + * + * - it retries the mutation after refreshing on a 401, and this endpoint takes + * a `refresh_token` in the BODY, so the retry would carry a token the + * refresh just superseded; + * - it collapses every 401 into one error type, losing the difference between + * a wrong current password and a dead session — which is the difference + * between a message in the form and a redirect to /login. + * + * `refreshToken` names the session to keep alive. Omitting it revokes every + * session including the caller's, which is the honest fallback when the BFF has + * no live refresh token to offer. + */ +export async function changeBackendPassword(args: { + accessToken: string; + currentPassword?: string; + newPassword: string; + refreshToken?: string; +}): Promise { + const config = readMaintmodeBackendConfig(); + const target = resolveBackendUrl(config.authApiBaseUrl, CHANGE_PASSWORD_PATH); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); + + try { + const response = await fetch(target, { + method: "POST", + headers: { + accept: "application/json", + "content-type": "application/json", + authorization: `Bearer ${args.accessToken}`, + }, + body: JSON.stringify({ + // Sent only when there is one. The backend rejects the field outright + // for an account with no password, so an empty string is not the same + // as absent. + ...(args.currentPassword ? { current_password: args.currentPassword } : {}), + new_password: args.newPassword, + ...(args.refreshToken ? { refresh_token: args.refreshToken } : {}), + }), + signal: controller.signal, + }); + + if (response.status === 204) { + return { ok: true }; + } + + const body = await response.text(); + + if (response.status === 401) { + // The one classification, and it reads what the REQUEST carried rather + // than what the response says. A 401 when a current password was sent is + // that password being wrong; a 401 when none was sent is the + // `refresh_token` having been superseded, and nothing the user typed. + return { ok: false, kind: args.currentPassword ? "wrong-current-password" : "session-stale" }; + } + if (response.status === 400) { + return { ok: false, kind: "rejected", message: body }; + } + return { ok: false, kind: "unavailable" }; + } catch { + return { ok: false, kind: "unavailable" }; + } finally { + clearTimeout(timeout); + } +} + /** * Rotates the refresh token via `POST /api/v1/refresh`. Returns the new * `TokenPairResponse`. From 18307178b37f2485cb7aced3327e05b413d4e0fe Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 01:47:13 +0300 Subject: [PATCH 05/14] feat(settings): add the password card to the profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/features/_shared/queries/use-me-query.ts | 32 +++ .../settings/__tests__/password-card.test.tsx | 210 ++++++++++++++++++ src/features/settings/password-card.tsx | 168 ++++++++++++++ src/features/settings/user-settings-page.tsx | 5 + 4 files changed, 415 insertions(+) create mode 100644 src/features/settings/__tests__/password-card.test.tsx create mode 100644 src/features/settings/password-card.tsx diff --git a/src/features/_shared/queries/use-me-query.ts b/src/features/_shared/queries/use-me-query.ts index 2a9e982..e9bec14 100644 --- a/src/features/_shared/queries/use-me-query.ts +++ b/src/features/_shared/queries/use-me-query.ts @@ -114,3 +114,35 @@ export function useUpdateMyTags() { }, }); } + +/** What the change-password card sends. `current_password` is omitted, never + * empty, when the account has none — the backend rejects the field outright in + * that state, so "" and absent are different requests. */ +export type ChangePasswordArgs = { + current_password?: string; + new_password: string; +}; + +/** + * Set or change the caller's password (RUK-289) via `POST /api/me/password`. + * + * Unlike its siblings this cannot seed the cache from the response: the backend + * answers 204 with an empty body, so there is no user object to write. It + * INVALIDATES instead — `password_set` has just flipped, and `useMeQuery` holds + * its answer for `staleTime`, which would leave the card drawing the form for + * the state the user was in a minute ago. + */ +export function useChangePassword() { + const queryClient = useQueryClient(); + return useMutation({ + mutationFn: async (args: ChangePasswordArgs): Promise => { + await bffFetch("/api/me/password", { + method: "POST", + body: JSON.stringify(args), + }); + }, + onSuccess: () => { + void queryClient.invalidateQueries({ queryKey: meKey() }); + }, + }); +} diff --git a/src/features/settings/__tests__/password-card.test.tsx b/src/features/settings/__tests__/password-card.test.tsx new file mode 100644 index 0000000..1248cab --- /dev/null +++ b/src/features/settings/__tests__/password-card.test.tsx @@ -0,0 +1,210 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; + +import { PasswordCard } from "@/features/settings/password-card"; +import { BffError } from "@/features/_shared/api/bff-fetch"; + +const bffFetch = vi.fn(); +vi.mock("@/features/_shared/api/bff-fetch", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, bffFetch: (...args: unknown[]) => bffFetch(...args) }; +}); + +const toastSuccess = vi.fn(); +const toastError = vi.fn(); +vi.mock("sonner", () => ({ + toast: { + success: (...args: unknown[]) => toastSuccess(...args), + error: (...args: unknown[]) => toastError(...args), + }, +})); + +afterEach(() => cleanup()); +beforeEach(() => vi.clearAllMocks()); + +const LONG_ENOUGH = "a-long-enough-password"; + +function renderCard(passwordSet: boolean | undefined) { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const invalidate = vi.spyOn(client, "invalidateQueries"); + render( + + + , + ); + return { invalidate }; +} + +function fill(label: string, value: string) { + fireEvent.change(screen.getByLabelText(label), { target: { value } }); +} + +describe("which form is drawn — the three values of password_set", () => { + // AC-5. `undefined` is NOT `false`: a backend that predates the field would + // otherwise get the set-password form for every operator, and every save + // would be a 400. + it("asks for the current password when the account has one", () => { + renderCard(true); + + expect(screen.getByLabelText("Current password")).toBeTruthy(); + expect(screen.getByRole("button", { name: "Change password" })).toBeTruthy(); + }); + + it("omits the current password when the account has none", () => { + renderCard(false); + + expect(screen.queryByLabelText("Current password")).toBeNull(); + expect(screen.getByRole("button", { name: "Set password" })).toBeTruthy(); + }); + + it("offers nothing at all when the field is absent", () => { + renderCard(undefined); + + expect(screen.queryByLabelText("New password")).toBeNull(); + expect(screen.getByText(/unavailable/i)).toBeTruthy(); + }); +}); + +describe("the length check", () => { + it("refuses a short password without calling the server", () => { + renderCard(false); + fill("New password", "short"); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + expect(bffFetch).not.toHaveBeenCalled(); + expect(screen.getByRole("alert").textContent).toMatch(/at least 12 characters/i); + }); + + // 11 characters, 22 bytes — accepted by the backend, rejected by a naive + // `.length` implementation. + it("accepts an 11-character Cyrillic password", async () => { + bffFetch.mockResolvedValue(undefined); + renderCard(false); + fill("New password", "паролькудли"); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + await waitFor(() => expect(bffFetch).toHaveBeenCalled()); + }); +}); + +describe("what is sent", () => { + it("omits current_password entirely on the set form", async () => { + bffFetch.mockResolvedValue(undefined); + renderCard(false); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + await waitFor(() => expect(bffFetch).toHaveBeenCalled()); + const body = JSON.parse((bffFetch.mock.calls[0][1] as { body: string }).body); + expect(body).toEqual({ new_password: LONG_ENOUGH }); + expect("current_password" in body).toBe(false); + }); + + it("sends both fields on the change form", async () => { + bffFetch.mockResolvedValue(undefined); + renderCard(true); + fill("Current password", "old-password"); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Change password" })); + + await waitFor(() => expect(bffFetch).toHaveBeenCalled()); + const body = JSON.parse((bffFetch.mock.calls[0][1] as { body: string }).body); + expect(body).toEqual({ current_password: "old-password", new_password: LONG_ENOUGH }); + }); +}); + +describe("after a successful save", () => { + // AC-14. Both endpoints answer 204 with no body, so the cache cannot be + // seeded from the response — and `useMeQuery` holds its answer for 60s, which + // would leave the card drawing the form for the state the user just left. + it("invalidates the me query so password_set is refetched", async () => { + bffFetch.mockResolvedValue(undefined); + const { invalidate } = renderCard(false); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + await waitFor(() => { + expect(invalidate).toHaveBeenCalledWith(expect.objectContaining({ queryKey: ["me"] })); + }); + }); +}); + +describe("failures the user must be able to act on", () => { + it("shows a wrong current password in place, without a redirect", async () => { + bffFetch.mockRejectedValue(new BffError(422, "That current password isn't right")); + renderCard(true); + fill("Current password", "wrong"); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Change password" })); + + await waitFor(() => { + expect(screen.getByRole("alert").textContent).toMatch(/current password isn't right/i); + }); + }); + + it("says a stale session changed nothing", async () => { + bffFetch.mockRejectedValue(new BffError(409, "session expired")); + renderCard(false); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + await waitFor(() => { + expect(toastError).toHaveBeenCalledWith(expect.stringMatching(/session expired/i)); + }); + }); +}); + +describe("recovering from a wrong password_set — one flip, never a loop", () => { + // AC-8. `password_set` degrades to `false` when the backend's own read fails, + // so the card can draw the wrong form. The 400 that follows must lead + // somewhere. + it("flips to the change form when the account turns out to have a password", async () => { + bffFetch.mockRejectedValue(new BffError(400, "validation error: the current password is required")); + renderCard(false); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + await waitFor(() => expect(screen.getByLabelText("Current password")).toBeTruthy()); + expect(screen.getByRole("alert").textContent).toMatch(/already has a password/i); + }); + + // The termination proof. A 400 is not evidence about `password_set` — the + // same status carries a length failure, and all three of the backend's 400s + // share one code — so an unbounded rule would oscillate set → change → set + // forever. + it("does not flip a second time", async () => { + bffFetch.mockRejectedValue(new BffError(400, "validation error")); + renderCard(false); + + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + await waitFor(() => expect(screen.getByLabelText("Current password")).toBeTruthy()); + + fill("Current password", "something"); + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Change password" })); + + await waitFor(() => expect(bffFetch).toHaveBeenCalledTimes(2)); + // Still the change form: the second 400 is a terminal error, not a flip + // back to where it started. + expect(screen.getByLabelText("Current password")).toBeTruthy(); + }); + + it("does not flip on a length failure, which never reaches the server", () => { + renderCard(false); + fill("New password", "short"); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + + expect(screen.queryByLabelText("Current password")).toBeNull(); + }); +}); + +describe("the destructive consequence is stated before the submit", () => { + it("warns that changing a password signs other devices out", () => { + renderCard(true); + + expect(screen.getByText(/signs you out of your other devices/i)).toBeTruthy(); + }); +}); diff --git a/src/features/settings/password-card.tsx b/src/features/settings/password-card.tsx new file mode 100644 index 0000000..f43e116 --- /dev/null +++ b/src/features/settings/password-card.tsx @@ -0,0 +1,168 @@ +"use client"; + +import { useState } from "react"; +import { toast } from "sonner"; + +import { isPasswordWithinPolicy } from "@/domain/auth/sign-in-method"; +import { BffError } from "@/features/_shared/api/bff-fetch"; +import { useChangePassword } from "@/features/_shared/queries/use-me-query"; +import { Button } from "@/shared/ui/shadcn/button"; +import { Input } from "@/shared/ui/shadcn/input"; +import { Label } from "@/shared/ui/shadcn/label"; + +/** + * Set or change the account password (RUK-289). + * + * Which form is drawn comes from `password_set`, which has THREE meaningful + * values (SPEC §2.4): + * + * - `true` — change: current password + new password. + * - `false` — set: new password only. Sending a current password for an + * account with none is a 400, so the field must not be there. + * - `undefined` — the deployed backend predates the field. Nothing is + * offered, because guessing either form makes every save a 400. + */ + +export interface PasswordCardProps { + /** `me.password_set`. `undefined` is "unknown", never "false". */ + passwordSet: boolean | undefined; +} + +export function PasswordCard({ passwordSet }: PasswordCardProps) { + const change = useChangePassword(); + const [current, setCurrent] = useState(""); + const [next, setNext] = useState(""); + const [error, setError] = useState(); + + /** + * Which shape the form is in. Seeded from `password_set` and allowed to flip + * ONCE — see `flipped`. + */ + const [asChange, setAsChange] = useState(passwordSet === true); + /** + * A `password_set` that disagrees with reality is possible: the backend + * answers `false` when its own read of the credential fails. The wrong guess + * is answered with a 400, and this flips the form to the other shape so the + * user is not stuck. + * + * Once, and only 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. + */ + const [flipped, setFlipped] = useState(false); + + if (passwordSet === undefined) { + return ( +

+ Password management is unavailable — this deployment doesn't report password state yet. +

+ ); + } + + const onSubmit = (event: React.FormEvent) => { + event.preventDefault(); + if (change.isPending) return; + + // Checked before sending so a length failure is never attributed to the + // wrong thing by the flip rule below. Bytes, not characters: the backend + // measures with Go's `len()`. + if (!isPasswordWithinPolicy(next)) { + setError("Choose a longer password — at least 12 characters."); + return; + } + setError(undefined); + + change.mutate( + { ...(asChange ? { current_password: current } : {}), new_password: next }, + { + onSuccess: () => { + toast.success(asChange ? "Password changed" : "Password set"); + setCurrent(""); + setNext(""); + }, + onError: (mutationError) => { + if (!(mutationError instanceof BffError)) { + toast.error("Couldn't save your password. Try again."); + return; + } + + if (mutationError.status === 422) { + setError("That current password isn't right."); + return; + } + if (mutationError.status === 409) { + // Nothing the user typed was wrong, and nothing was changed. + toast.error("Your session expired before the change was saved. Sign in again."); + return; + } + if (mutationError.status === 400 && !flipped) { + // The password passed the local length check, so this 400 is the + // shape being wrong for this account's real state. Flip once. + setFlipped(true); + setAsChange((wasChange) => !wasChange); + setCurrent(""); + setError( + asChange + ? "This account has no password yet — set one below." + : "This account already has a password — enter your current one.", + ); + return; + } + setError(mutationError.message || "Couldn't save your password. Try again."); + }, + }, + ); + }; + + return ( +
+ {asChange ? ( + <> + + setCurrent(e.target.value)} + /> + + ) : null} + + + setNext(e.target.value)} + aria-describedby="new-password-hint" + /> + {/* + * "Characters" rather than bytes. The policy is 12 BYTES, which is not a + * unit to show an operator; the ASCII worst case can only under-promise, + * so a non-ASCII password shorter than the hint is still accepted. + */} +

+ At least 12 characters. {asChange ? "Changing it signs you out of your other devices." : null} +

+ + {error ? ( +

+ {error} +

+ ) : null} + + +
+ ); +} diff --git a/src/features/settings/user-settings-page.tsx b/src/features/settings/user-settings-page.tsx index 95275dd..4dca7c8 100644 --- a/src/features/settings/user-settings-page.tsx +++ b/src/features/settings/user-settings-page.tsx @@ -22,6 +22,7 @@ import { BrandIcon, type BrandProvider } from "@/shared/ui/icons/brand-icons"; import { useMeQuery } from "@/features/_shared/queries/use-me-query"; import { MessengerTagsFields } from "./messenger-tags-card"; import { TimezoneCard } from "./timezone-card"; +import { PasswordCard } from "./password-card"; import type { Role } from "@/domain/auth/permissions"; /** Role chips render admin-first, consistent with users-management. */ @@ -181,6 +182,10 @@ export function UserSettingsPage() {
+ + + +

This signs you out on this device only. Use the option below to sign out from all devices. From 280319e8dae63fc21f6f0f51d58d89cdd0a75a4c Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 01:49:04 +0300 Subject: [PATCH 06/14] feat(settings): add a standalone /set-password page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/app/(app)/set-password/page.tsx | 11 +++ .../__tests__/set-password-page.test.tsx | 69 +++++++++++++++++++ src/features/settings/password-card.tsx | 19 +++-- src/features/settings/set-password-page.tsx | 63 +++++++++++++++++ 4 files changed, 155 insertions(+), 7 deletions(-) create mode 100644 src/app/(app)/set-password/page.tsx create mode 100644 src/features/settings/__tests__/set-password-page.test.tsx create mode 100644 src/features/settings/set-password-page.tsx diff --git a/src/app/(app)/set-password/page.tsx b/src/app/(app)/set-password/page.tsx new file mode 100644 index 0000000..b89a447 --- /dev/null +++ b/src/app/(app)/set-password/page.tsx @@ -0,0 +1,11 @@ +import { SetPasswordPage } from "@/features/settings/set-password-page"; + +/** + * Deliberately NOT wrapped in `AppShell`: this is a single-purpose page reached + * from the profile, and the app chrome around one form is noise. It sits under + * `(app)` for the session and the provider tree, which the form's mutation + * needs and which `(public)` does not mount. + */ +export default function Page() { + return ; +} diff --git a/src/features/settings/__tests__/set-password-page.test.tsx b/src/features/settings/__tests__/set-password-page.test.tsx new file mode 100644 index 0000000..5d2c324 --- /dev/null +++ b/src/features/settings/__tests__/set-password-page.test.tsx @@ -0,0 +1,69 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { cleanup, render, screen, waitFor } from "@testing-library/react"; + +import { SetPasswordPage } from "@/features/settings/set-password-page"; + +const replace = vi.fn(); +vi.mock("next/navigation", () => ({ useRouter: () => ({ replace }) })); + +const useMeQuery = vi.fn(); +vi.mock("@/features/_shared/queries/use-me-query", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useMeQuery: () => useMeQuery() }; +}); + +vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +afterEach(() => cleanup()); +beforeEach(() => vi.clearAllMocks()); + +function renderPage() { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + , + ); +} + +describe("who this page is for", () => { + it("shows the form to an account with no password", () => { + useMeQuery.mockReturnValue({ isPending: false, isSuccess: true, data: { password_set: false } }); + renderPage(); + + expect(screen.getByLabelText("New password")).toBeTruthy(); + expect(replace).not.toHaveBeenCalled(); + }); + + // AC-9. The redirect is client-side by necessity (a page cannot call + // `readActiveSession`, which writes cookies), so this asserts the redirect + // fires and that nothing is submittable before it does — NOT the absence of a + // paint, which this mechanism cannot promise. + it("sends an account that already has a password to the profile", async () => { + useMeQuery.mockReturnValue({ isPending: false, isSuccess: true, data: { password_set: true } }); + renderPage(); + + await waitFor(() => expect(replace).toHaveBeenCalledWith("/settings/profile")); + expect(screen.queryByLabelText("New password")).toBeNull(); + }); + + // `undefined` is not `false`: the backend cannot say, so the card would offer + // nothing anyway, and the profile is where the rest of the account lives. + it("sends an unknown password state to the profile too", async () => { + useMeQuery.mockReturnValue({ isPending: false, isSuccess: true, data: {} }); + renderPage(); + + await waitFor(() => expect(replace).toHaveBeenCalledWith("/settings/profile")); + expect(screen.queryByLabelText("New password")).toBeNull(); + }); + + it("shows no form while the answer is still loading", () => { + useMeQuery.mockReturnValue({ isPending: true, isSuccess: false, data: undefined }); + renderPage(); + + expect(screen.queryByLabelText("New password")).toBeNull(); + expect(replace).not.toHaveBeenCalled(); + }); +}); diff --git a/src/features/settings/password-card.tsx b/src/features/settings/password-card.tsx index f43e116..bac89f4 100644 --- a/src/features/settings/password-card.tsx +++ b/src/features/settings/password-card.tsx @@ -156,13 +156,18 @@ export function PasswordCard({ passwordSet }: PasswordCardProps) {

) : null} - +
+ + {!asChange ? ( + // The page is the same form on its own, for someone who came here to + // do this one thing. It is this card's only entry point. + + Open on its own page + + ) : null} +
); } diff --git a/src/features/settings/set-password-page.tsx b/src/features/settings/set-password-page.tsx new file mode 100644 index 0000000..4d37b50 --- /dev/null +++ b/src/features/settings/set-password-page.tsx @@ -0,0 +1,63 @@ +"use client"; + +import { useRouter } from "next/navigation"; +import { useEffect } from "react"; + +import { useMeQuery } from "@/features/_shared/queries/use-me-query"; +import { PasswordCard } from "@/features/settings/password-card"; +import { Skeleton } from "@/shared/ui/domain/skeleton"; + +/** + * A focused page for setting a first password (RUK-289), reached from the + * profile card. Renders the same form in a bare centered layout rather than + * inside `AppShell`, so there is one thing on the screen to do. + * + * **The redirect 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 inside a Server Action or Route Handler — a page + * render throws `ReadonlyRequestCookiesError`. Building it server-side would + * therefore 500 only inside the rotation window: green tests, intermittent + * production failure. + * + * The cost is a brief render before the redirect. It is acceptable because the + * page is reached deliberately from the profile card, so arriving in the wrong + * state is rare, and because the form is inert until submitted. + */ +export function SetPasswordPage() { + const router = useRouter(); + const meQuery = useMeQuery(); + const passwordSet = meQuery.data?.password_set; + + // `true` means they are not who this page is for; `undefined` means the + // backend cannot say, and the card would offer nothing anyway. Both belong on + // the profile, where the rest of the account lives. + const shouldLeave = meQuery.isSuccess && passwordSet !== false; + + useEffect(() => { + if (shouldLeave) { + router.replace("/settings/profile"); + } + }, [shouldLeave, router]); + + return ( +
+
+
+

Set a password

+

+ Add a password so you can sign in with your email address as well as your current method. +

+
+ + {meQuery.isPending || shouldLeave ? ( + // Also covers the moment between deciding to leave and the navigation + // landing, so the form never flashes at someone on their way out. + + ) : ( + + )} +
+
+ ); +} From 6ecfb217b017764e0631485391d1eb73f2aca354 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:01:16 +0300 Subject: [PATCH 07/14] test(auth): cover the password HTTP client and the login-page reset wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../auth/__tests__/login-page-reset.test.tsx | 119 +++++++++ .../__tests__/password-reset-flow.test.tsx | 50 ++++ src/features/auth/login-page.tsx | 6 +- src/features/auth/password-reset-flow.tsx | 14 +- .../settings/__tests__/password-card.test.tsx | 16 +- .../backend-token-exchange-password.test.ts | 240 ++++++++++++++++++ .../built-in-sign-in-callback.test.ts | 1 - .../__tests__/password-reset-actions.test.ts | 29 ++- 8 files changed, 448 insertions(+), 27 deletions(-) create mode 100644 src/features/auth/__tests__/login-page-reset.test.tsx create mode 100644 src/server/auth/__tests__/backend-token-exchange-password.test.ts diff --git a/src/features/auth/__tests__/login-page-reset.test.tsx b/src/features/auth/__tests__/login-page-reset.test.tsx new file mode 100644 index 0000000..6a158cb --- /dev/null +++ b/src/features/auth/__tests__/login-page-reset.test.tsx @@ -0,0 +1,119 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; + +import { LoginPage } from "@/features/auth/login-page"; +import type { SignInMethod } from "@/domain/auth/sign-in-method"; + +afterEach(() => cleanup()); + +const PASSWORD_METHOD: SignInMethod = { + id: "email_password", + type: "password", + display_name: "Password", +}; +const CODE_METHOD: SignInMethod = { id: "email_otp", type: "code", display_name: "Email code" }; + +function renderLogin( + overrides: Partial> = {}, +): React.ComponentProps { + const props: React.ComponentProps = { + methods: [PASSWORD_METHOD], + signInAction: vi.fn(async () => {}), + requestOtpAction: vi.fn(async () => ({})), + otpSignInAction: vi.fn(async () => ({})), + passwordSignInAction: vi.fn(async () => ({})), + changeEmailAction: vi.fn(async () => {}), + requestPasswordResetAction: vi.fn(async () => ({})), + confirmPasswordResetAction: vi.fn(async () => ({ done: true })), + abandonPasswordResetAction: vi.fn(async () => {}), + ...overrides, + }; + render(); + return props; +} + +describe("the entry point into the reset flow", () => { + // Without this the feature has no way in and nothing else would fail. + it("offers 'Forgot password?' beside the password form", () => { + renderLogin(); + + expect(screen.getByRole("button", { name: "Forgot password?" })).toBeTruthy(); + }); + + it("opens the reset flow when it is clicked", () => { + renderLogin(); + fireEvent.click(screen.getByRole("button", { name: "Forgot password?" })); + + expect(screen.getByLabelText("Reset your password")).toBeTruthy(); + // The sign-in form is gone: the page shows one thing to do at a time. + expect(screen.queryByLabelText("Email")).toBeNull(); + }); + + // The affordance belongs to the password form, so an instance advertising + // only emailed codes must not offer to reset a password it does not accept. + it("offers nothing when the backend advertises no password method", () => { + renderLogin({ methods: [CODE_METHOD] }); + + expect(screen.queryByRole("button", { name: "Forgot password?" })).toBeNull(); + }); +}); + +describe("rehydration after a reload", () => { + // A reset spans an email round-trip, so the user leaves the tab. Without this + // they come back to step one and silently lose the code they were sent. + it("resumes at step two when the server hands down a live binding", () => { + renderLogin({ resetInProgressEmail: "op@example.test" }); + + expect(screen.getByLabelText("Enter the 6-digit code")).toBeTruthy(); + expect(screen.getByText(/Sent to op@example.test/)).toBeTruthy(); + }); + + // SPEC §2.1: the advertised method list is the authority on what the page + // offers. A cookie must not resurrect a method an operator has switched off. + it("ignores a live binding when password sign-in is no longer offered", () => { + renderLogin({ resetInProgressEmail: "op@example.test", methods: [CODE_METHOD] }); + + expect(screen.queryByLabelText("Enter the 6-digit code")).toBeNull(); + expect(screen.getByLabelText("Email code")).toBeTruthy(); + }); + + it("starts at step one when there is no binding", () => { + renderLogin(); + fireEvent.click(screen.getByRole("button", { name: "Forgot password?" })); + + expect(screen.getByLabelText("Reset your password")).toBeTruthy(); + }); +}); + +describe("what the user is told afterwards", () => { + // The backend has revoked every session and returned no tokens, so there is + // nothing to sign into — the confirmation is the entire outcome. SPEC §2.1 + // names shipping without it as the failure to avoid. + it("confirms the change and returns to the sign-in form", async () => { + renderLogin({ resetInProgressEmail: "op@example.test" }); + + fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { + target: { value: "123456" }, + }); + fireEvent.change(screen.getByLabelText("New password"), { + target: { value: "a-long-enough-password" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Set new password" })); + + await waitFor(() => { + expect(screen.getByRole("status").textContent).toMatch(/password updated/i); + }); + // Back on the sign-in form, which is where the new password is used. + expect(screen.getByLabelText("Password")).toBeTruthy(); + }); + + it("returns to sign-in without a confirmation when the user backs out", () => { + renderLogin(); + fireEvent.click(screen.getByRole("button", { name: "Forgot password?" })); + fireEvent.click(screen.getByRole("button", { name: "Back to sign in" })); + + expect(screen.getByLabelText("Password")).toBeTruthy(); + expect(screen.queryByRole("status")).toBeNull(); + }); +}); diff --git a/src/features/auth/__tests__/password-reset-flow.test.tsx b/src/features/auth/__tests__/password-reset-flow.test.tsx index a9fafec..31b6063 100644 --- a/src/features/auth/__tests__/password-reset-flow.test.tsx +++ b/src/features/auth/__tests__/password-reset-flow.test.tsx @@ -83,6 +83,29 @@ describe("the client-side length check", () => { }); describe("the local attempt budget", () => { + // Pinned as a LITERAL. The loops below use the constant, so without this the + // whole budget suite is self-fulfilling: changing 5 to 50 would keep every + // test green while the user burned 45 doomed submits. SPEC §3.1 argues the + // value must be the backend's configured 5 and not its ceiling of 10. + it("is five, the backend's configured limit", () => { + expect(MAX_CODE_ATTEMPTS).toBe(5); + }); + + it("does not give up before the budget is spent", async () => { + const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_failed" })) }); + await reachCodeStep(props); + + // Four failures, hard-coded rather than derived: an off-by-one that let the + // binding survive one submit too long would otherwise pass. + for (let i = 0; i < 4; i++) { + await submitCode("000000", LONG_ENOUGH); + await waitFor(() => expect(props.confirm).toHaveBeenCalledTimes(i + 1)); + } + + expect(props.abandon).not.toHaveBeenCalled(); + expect(screen.getByLabelText("Enter the 6-digit code")).toBeTruthy(); + }); + // AC-13. The backend collapses "wrong code", "expired" and "attempts // exhausted" into one answer, so the client cannot learn from a response that // the budget is gone. Without a local count the binding survives its full TTL @@ -115,6 +138,33 @@ describe("the local attempt budget", () => { }); }); +describe("the double-submit guard", () => { + // Each stray submit spends one of five attempts and the backend floors every + // response to ~300 ms, so a double-click is a live risk, not a theoretical + // one. + it("sends one request when the button is clicked twice", async () => { + let release: (value: { done: true }) => void = () => {}; + const confirm = vi.fn( + () => + new Promise<{ done: true }>((resolve) => { + release = resolve; + }), + ); + const props = setup({ confirm }); + await reachCodeStep(props); + + fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { target: { value: "123456" } }); + fireEvent.change(screen.getByLabelText("New password"), { target: { value: LONG_ENOUGH } }); + const button = screen.getByRole("button", { name: "Set new password" }); + fireEvent.click(button); + fireEvent.click(button); + + expect(confirm).toHaveBeenCalledTimes(1); + release({ done: true }); + await waitFor(() => expect(props.onDone).toHaveBeenCalled()); + }); +}); + describe("failures the user must be able to tell apart", () => { it("sends the user back to step one when the binding is gone", async () => { const props = setup({ diff --git a/src/features/auth/login-page.tsx b/src/features/auth/login-page.tsx index 6420c0d..19d38ff 100644 --- a/src/features/auth/login-page.tsx +++ b/src/features/auth/login-page.tsx @@ -233,7 +233,11 @@ function BuiltInMethod({ if (method.type === "password") { return (
- +
); } diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx index b69a975..3f43f36 100644 --- a/src/features/auth/password-reset-flow.tsx +++ b/src/features/auth/password-reset-flow.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState } from "react"; +import { useEffect, useState } from "react"; import { Button } from "@/shared/ui/shadcn/button"; import { Input } from "@/shared/ui/shadcn/input"; @@ -64,6 +64,18 @@ export function PasswordResetFlow({ const timers = useCodeTimers(step === "code"); const budgetSpent = attempts >= MAX_CODE_ATTEMPTS; + // Rehydrating straight into step two starts with the countdown at zero, which + // reads as "expired" and hides the submit button — so a user returning from + // their email finds a form they cannot send. The real expiry is server-side + // and never returned; this is the same optimistic local clock the flow uses + // after a fresh request, and the backend remains the authority. + useEffect(() => { + if (initialStep === "code") timers.start(); + // Once, on mount: `start` is stable and re-running it would reset the + // countdown under a user who is mid-flow. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + async function send(address: string) { setPending(true); setError(undefined); diff --git a/src/features/settings/__tests__/password-card.test.tsx b/src/features/settings/__tests__/password-card.test.tsx index 1248cab..d43c77d 100644 --- a/src/features/settings/__tests__/password-card.test.tsx +++ b/src/features/settings/__tests__/password-card.test.tsx @@ -77,16 +77,9 @@ describe("the length check", () => { expect(screen.getByRole("alert").textContent).toMatch(/at least 12 characters/i); }); - // 11 characters, 22 bytes — accepted by the backend, rejected by a naive - // `.length` implementation. - it("accepts an 11-character Cyrillic password", async () => { - bffFetch.mockResolvedValue(undefined); - renderCard(false); - fill("New password", "паролькудли"); - fireEvent.click(screen.getByRole("button", { name: "Set password" })); - - await waitFor(() => expect(bffFetch).toHaveBeenCalled()); - }); + // The byte arithmetic itself is pinned once, in the domain policy test. What + // matters here is only that the guard runs before the request, which the + // short-password case above already proves. }); describe("what is sent", () => { @@ -188,8 +181,9 @@ describe("recovering from a wrong password_set — one flip, never a loop", () = await waitFor(() => expect(bffFetch).toHaveBeenCalledTimes(2)); // Still the change form: the second 400 is a terminal error, not a flip - // back to where it started. + // back to where it started — and the error is SHOWN, not swallowed. expect(screen.getByLabelText("Current password")).toBeTruthy(); + expect(screen.getByRole("alert")).toBeTruthy(); }); it("does not flip on a length failure, which never reaches the server", () => { diff --git a/src/server/auth/__tests__/backend-token-exchange-password.test.ts b/src/server/auth/__tests__/backend-token-exchange-password.test.ts new file mode 100644 index 0000000..32481ee --- /dev/null +++ b/src/server/auth/__tests__/backend-token-exchange-password.test.ts @@ -0,0 +1,240 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + changeBackendPassword, + confirmPasswordReset, + requestPasswordResetCode, +} from "@/server/auth/backend-token-exchange"; + +/** + * The password HTTP client, executed rather than mocked. + * + * Every other suite in this change stubs this module, which left its 401 + * classification — the single most consequential branch in RUK-289 — provable + * only by reading it. An inverted classification tells an operator with a wrong + * password that their session expired, and an operator with a dead session that + * their password is wrong on a form with no password field. + * + * Assertions read the REQUEST off the fetch stub, not the arguments object, so + * they pin what actually goes on the wire. + */ + +const fetchMock = vi.fn(); + +beforeEach(() => { + vi.stubGlobal("fetch", fetchMock); + fetchMock.mockReset(); + process.env.MAINTMODE_API_BASE_URL = "http://backend.test/maintmode"; +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +function respond(status: number, body = "") { + return { + ok: status >= 200 && status < 300, + status, + statusText: `status ${status}`, + text: async () => body, + }; +} + +/** The JSON body of the Nth fetch call. */ +function sentBody(call = 0): Record { + return JSON.parse(fetchMock.mock.calls[call][1].body); +} + +describe("changeBackendPassword — the 401 classification", () => { + // The inversion of this pair is invisible to every other test in the change, + // and it is the failure SPEC §3.4 exists to prevent. + it("reads a 401 as a wrong password when a current password was sent", async () => { + fetchMock.mockResolvedValue(respond(401, '{"code":"unauthorized"}')); + + const outcome = await changeBackendPassword({ + accessToken: "access-1", + currentPassword: "wrong", + newPassword: "a-long-enough-password", + refreshToken: "refresh-1", + }); + + expect(outcome).toEqual({ ok: false, kind: "wrong-current-password" }); + }); + + it("reads a 401 as a stale session when none was sent", async () => { + fetchMock.mockResolvedValue(respond(401, '{"code":"unauthorized"}')); + + const outcome = await changeBackendPassword({ + accessToken: "access-1", + newPassword: "a-long-enough-password", + refreshToken: "stale-refresh", + }); + + // Nothing the user typed was wrong, and the backend changed nothing. + expect(outcome).toEqual({ ok: false, kind: "session-stale" }); + }); + + it("maps 204, 400 and everything else", async () => { + fetchMock.mockResolvedValueOnce(respond(204)); + await expect( + changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), + ).resolves.toEqual({ ok: true }); + + fetchMock.mockResolvedValueOnce(respond(400, "validation error: length policy")); + await expect( + changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), + ).resolves.toEqual({ ok: false, kind: "rejected", message: "validation error: length policy" }); + + fetchMock.mockResolvedValueOnce(respond(503, "gateway down")); + await expect( + changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), + ).resolves.toEqual({ ok: false, kind: "unavailable" }); + }); + + // A 200 or 202 is not success here: the contract is 204 with an empty body, + // and treating any 2xx as done would report a password change the backend + // never made. + it("treats a 2xx that is not 204 as unavailable", async () => { + fetchMock.mockResolvedValue(respond(200, "{}")); + + await expect( + changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), + ).resolves.toEqual({ ok: false, kind: "unavailable" }); + }); + + it("treats a thrown fetch as unavailable rather than propagating", async () => { + fetchMock.mockRejectedValue(new Error("network down")); + + await expect( + changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), + ).resolves.toEqual({ ok: false, kind: "unavailable" }); + }); +}); + +describe("changeBackendPassword — what goes on the wire", () => { + it("sends the refresh token, which is what keeps the caller signed in", async () => { + fetchMock.mockResolvedValue(respond(204)); + + await changeBackendPassword({ + accessToken: "access-1", + currentPassword: "old-password", + newPassword: "a-long-enough-password", + refreshToken: "refresh-1", + }); + + // Omitting it makes the backend revoke every session including this one: + // the operator is signed out by their own success. + expect(sentBody()).toEqual({ + current_password: "old-password", + new_password: "a-long-enough-password", + refresh_token: "refresh-1", + }); + }); + + it("omits current_password as a KEY when the account has none", async () => { + fetchMock.mockResolvedValue(respond(204)); + + await changeBackendPassword({ + accessToken: "access-1", + newPassword: "a-long-enough-password", + refreshToken: "refresh-1", + }); + + // Absent, not empty: the backend rejects the field outright for an account + // with no password, so `""` would 400 every set-password call. + expect("current_password" in sentBody()).toBe(false); + }); + + it("omits refresh_token as a key when there is none to send", async () => { + fetchMock.mockResolvedValue(respond(204)); + + await changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }); + + expect("refresh_token" in sentBody()).toBe(false); + }); + + it("carries the access token as a bearer credential", async () => { + fetchMock.mockResolvedValue(respond(204)); + + await changeBackendPassword({ accessToken: "access-1", newPassword: "a-long-enough-password" }); + + expect(fetchMock.mock.calls[0][1].headers.authorization).toBe("Bearer access-1"); + expect(String(fetchMock.mock.calls[0][0])).toContain("/api/v1/me/password"); + }); +}); + +describe("confirmPasswordReset", () => { + // Without the throw, EVERY reset failure resolves — the caller reports + // success, the user is told their password changed and is signed out, and the + // password is unchanged. + it("throws on a failure rather than resolving", async () => { + fetchMock.mockResolvedValue(respond(401, '{"code":"otp_session_mismatch"}')); + + await expect( + confirmPasswordReset({ + email: "op@example.test", + code: "123456", + sessionNonce: "nonce-1", + newPassword: "a-long-enough-password", + }), + ).rejects.toMatchObject({ status: 401, responseBody: '{"code":"otp_session_mismatch"}' }); + }); + + it("resolves on 204", async () => { + fetchMock.mockResolvedValue(respond(204)); + + await expect( + confirmPasswordReset({ + email: "op@example.test", + code: "123456", + sessionNonce: "nonce-1", + newPassword: "a-long-enough-password", + }), + ).resolves.toBeUndefined(); + }); + + it("sends the nonce under the key the backend reads", async () => { + fetchMock.mockResolvedValue(respond(204)); + + await confirmPasswordReset({ + email: "op@example.test", + code: "123456", + sessionNonce: "nonce-1", + newPassword: "a-long-enough-password", + }); + + // Named explicitly: a renamed or empty `session_nonce` is answered with the + // same opaque 401 as a wrong code, so nothing downstream could tell. + expect(sentBody()).toEqual({ + email: "op@example.test", + code: "123456", + session_nonce: "nonce-1", + new_password: "a-long-enough-password", + }); + expect(String(fetchMock.mock.calls[0][0])).toContain("/api/v1/password/reset/confirm"); + }); +}); + +describe("requestPasswordResetCode", () => { + it("returns the session nonce from a 202", async () => { + fetchMock.mockResolvedValue(respond(202, '{"session_nonce":"nonce-1"}')); + + await expect(requestPasswordResetCode("op@example.test")).resolves.toEqual({ + session_nonce: "nonce-1", + }); + expect(sentBody()).toEqual({ email: "op@example.test" }); + expect(String(fetchMock.mock.calls[0][0])).toContain("/api/v1/password/reset/request"); + }); + + it("rejects a 202 that carries no nonce, rather than binding undefined", async () => { + fetchMock.mockResolvedValue(respond(202, "{}")); + + await expect(requestPasswordResetCode("op@example.test")).rejects.toThrow(); + }); + + it("throws with the status on a rate limit", async () => { + fetchMock.mockResolvedValue(respond(429, "slow down")); + + await expect(requestPasswordResetCode("op@example.test")).rejects.toMatchObject({ status: 429 }); + }); +}); diff --git a/src/server/auth/__tests__/built-in-sign-in-callback.test.ts b/src/server/auth/__tests__/built-in-sign-in-callback.test.ts index 76658b4..e96ee27 100644 --- a/src/server/auth/__tests__/built-in-sign-in-callback.test.ts +++ b/src/server/auth/__tests__/built-in-sign-in-callback.test.ts @@ -31,7 +31,6 @@ vi.mock("@/server/auth/otp-nonce-cookie", () => ({ // The real implementation: normalization is part of the behaviour under test, // so stubbing it would hide the case-variant bug this file now covers. normalizeEmail: (email: string) => email.trim().toLowerCase(), - OTP_NONCE_COOKIE: "__Host-mm.otp_nonce", })); const { runBuiltInSignIn } = await import("@/server/auth/built-in-sign-in"); diff --git a/src/server/auth/__tests__/password-reset-actions.test.ts b/src/server/auth/__tests__/password-reset-actions.test.ts index 33552ba..2d4e214 100644 --- a/src/server/auth/__tests__/password-reset-actions.test.ts +++ b/src/server/auth/__tests__/password-reset-actions.test.ts @@ -115,19 +115,6 @@ describe("confirming a reset", () => { expect(clearPasswordResetBinding).not.toHaveBeenCalled(); }); - it("accepts an 11-character Cyrillic password, which is 22 bytes", async () => { - confirmPasswordReset.mockResolvedValue(undefined); - - const result = await confirmPasswordResetAction({ - email: "op@example.test", - code: "123456", - newPassword: "паролькудли", - }); - - expect(result).toEqual({ done: true }); - expect(confirmPasswordReset).toHaveBeenCalled(); - }); - // AC-4. Past the 204 the password IS changed and every session is dead. it("tears the local session down on success", async () => { confirmPasswordReset.mockResolvedValue(undefined); @@ -235,6 +222,22 @@ describe("confirming a reset", () => { expect(clearPasswordResetBinding).toHaveBeenCalled(); }); + // §3.6 requires the rate limit to keep its own copy on BOTH endpoints. The + // request path already had this; the confirm path did not, and folding a 429 + // into "unavailable" tells a throttled user the service is broken. + it("keeps a rate limit apart from an outage, and keeps the binding", async () => { + confirmPasswordReset.mockRejectedValueOnce(backendError(429)); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ error: "otp_rate_limited" }); + expect(clearPasswordResetBinding).not.toHaveBeenCalled(); + }); + it("keeps an outage apart from a wrong code, and keeps the binding", async () => { confirmPasswordReset.mockRejectedValueOnce(backendError(502)); From d5c4b3fd4a5058787f434ce9bcff352d8261e149 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:08:31 +0300 Subject: [PATCH 08/14] fix(auth): stop the password route 500ing, and add its contract test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../api/me/password/__tests__/route.test.ts | 15 ++ src/app/api/me/password/route.ts | 15 +- .../__tests__/password-reset-flow.test.tsx | 16 ++ src/features/auth/password-reset-flow.tsx | 27 ++- .../__tests__/set-password-page.test.tsx | 17 ++ src/features/settings/set-password-page.tsx | 9 + .../__tests__/password-reset-actions.test.ts | 16 +- src/server/auth/password-reset-actions.ts | 36 +++- tests/contracts/me-password.contract.test.ts | 176 ++++++++++++++++++ 9 files changed, 309 insertions(+), 18 deletions(-) create mode 100644 tests/contracts/me-password.contract.test.ts diff --git a/src/app/api/me/password/__tests__/route.test.ts b/src/app/api/me/password/__tests__/route.test.ts index 09b3a5b..68ef13e 100644 --- a/src/app/api/me/password/__tests__/route.test.ts +++ b/src/app/api/me/password/__tests__/route.test.ts @@ -149,6 +149,21 @@ describe("the guards every mutating route here carries", () => { await expect(response.json()).resolves.toMatchObject({ code: "AUTH_REQUIRED" }); }); + it("answers a malformed body with a 400, not a 500", async () => { + const malformed = new Request("https://app.test/api/me/password", { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{not json", + }); + + const response = await POST(malformed); + + // An uncaught throw here would be a Next 500 whose body is not the envelope + // `bffFetch` parses, so the card would surface framework noise. + expect(response.status).toBe(400); + expect(changeBackendPassword).not.toHaveBeenCalled(); + }); + it("rejects a request with no new password", async () => { const response = await POST(post({ current_password: "old-password" })); diff --git a/src/app/api/me/password/route.ts b/src/app/api/me/password/route.ts index 69edc5f..32de993 100644 --- a/src/app/api/me/password/route.ts +++ b/src/app/api/me/password/route.ts @@ -2,6 +2,7 @@ import { NextResponse } from "next/server"; import { changeBackendPassword } from "@/server/auth/backend-token-exchange"; import { readActiveSession } from "@/server/auth/session-token"; +import { routeErrorResponse } from "@/server/backend/errors/bff-error"; import { readJsonBody } from "@/server/backend/http/read-json-body"; import { isSameOriginRequest } from "@/server/backend/security/csrf"; @@ -47,7 +48,19 @@ export async function POST(request: Request) { return NextResponse.json({ error: "Sign-in is required", code: "AUTH_REQUIRED" }, { status: 401 }); } - const body = await readJsonBody(request); + // Caught rather than left to propagate: `readJsonBody` throws a + // `BffValidationError` on malformed JSON, and an uncaught throw out of a route + // handler is a Next 500 whose body is not the `{error, code}` envelope + // `bffFetch` parses — the card would print framework noise into the form. The + // catch is scoped to the parse alone, so the deliberate hand-rolled handling + // of the backend's own failures below still bypasses the generic mapper. + let body: ChangePasswordBody | undefined; + try { + body = await readJsonBody(request); + } catch (error) { + return routeErrorResponse(error); + } + const newPassword = body?.new_password; if (typeof newPassword !== "string" || !newPassword) { return NextResponse.json( diff --git a/src/features/auth/__tests__/password-reset-flow.test.tsx b/src/features/auth/__tests__/password-reset-flow.test.tsx index 31b6063..cecb36b 100644 --- a/src/features/auth/__tests__/password-reset-flow.test.tsx +++ b/src/features/auth/__tests__/password-reset-flow.test.tsx @@ -123,6 +123,22 @@ describe("the local attempt budget", () => { expect(screen.getByLabelText("Reset your password")).toBeTruthy(); }); + // The message has to survive the trip back to step one: `restart()` clears + // every other field, and an error set separately afterwards would be one + // edit away from being wiped by the obvious `setError(undefined)`. + it("explains why it returned to step one", async () => { + const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_failed" })) }); + await reachCodeStep(props); + + for (let i = 0; i < MAX_CODE_ATTEMPTS; i++) { + await submitCode("000000", LONG_ENOUGH); + await waitFor(() => expect(props.confirm).toHaveBeenCalledTimes(i + 1)); + } + + await waitFor(() => expect(screen.getByLabelText("Reset your password")).toBeTruthy()); + expect(screen.getByRole("alert")).toBeTruthy(); + }); + it("does not spend the budget on a locally rejected password", async () => { const props = setup(); await reachCodeStep(props); diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx index 3f43f36..250ab1f 100644 --- a/src/features/auth/password-reset-flow.tsx +++ b/src/features/auth/password-reset-flow.tsx @@ -93,13 +93,22 @@ export function PasswordResetFlow({ setStep("code"); } - /** Leaves step two for step one, discarding the binding server-side. */ - async function restart() { + /** + * Leaves step two for step one, discarding the binding server-side. + * + * Takes the message to show rather than leaving the caller to set it + * afterwards: the caller's `setError` used to land in a later batch and + * survive only because nothing here touched `error`. Adding the obvious + * `setError(undefined)` to this function — it clears every other field — + * would have silently deleted the only explanation the user gets. + */ + async function restart(message?: string) { await abandon(); timers.reset(); setStep("email"); setCode(""); setAttempts(0); + setError(message); } async function onSubmitCode(event: React.FormEvent) { @@ -135,13 +144,13 @@ export function PasswordResetFlow({ const spent = attempts + 1; setAttempts(spent); - if (result.error === "password_reset_session_mismatch") { - // The binding is already gone server-side, so step two is a dead end. - await restart(); - } else if (spent >= MAX_CODE_ATTEMPTS) { - // Out of attempts: drop the binding rather than leave a live cookie - // pointing at a code that can no longer be redeemed. - await restart(); + if (result.error === "password_reset_session_mismatch" || spent >= MAX_CODE_ATTEMPTS) { + // Either the binding is already gone server-side, or the attempts are + // spent — both make step two a dead end, and leaving a live cookie + // behind would rehydrate the user onto a code that can never be + // redeemed. + await restart(result.error); + return; } setError(result.error); }); diff --git a/src/features/settings/__tests__/set-password-page.test.tsx b/src/features/settings/__tests__/set-password-page.test.tsx index 5d2c324..5bba3ec 100644 --- a/src/features/settings/__tests__/set-password-page.test.tsx +++ b/src/features/settings/__tests__/set-password-page.test.tsx @@ -59,6 +59,23 @@ describe("who this page is for", () => { expect(screen.queryByLabelText("New password")).toBeNull(); }); + // An errored query leaves `password_set` undefined, which the card renders as + // "this deployment doesn't report password state" — blaming a version gap for + // a transient outage. The two must not share copy. + it("says the account could not be loaded, not that the feature is missing", () => { + useMeQuery.mockReturnValue({ + isPending: false, + isSuccess: false, + isError: true, + data: undefined, + }); + renderPage(); + + expect(screen.getByRole("alert").textContent).toMatch(/couldn't load your account/i); + expect(screen.queryByText(/doesn't report password state/i)).toBeNull(); + expect(replace).not.toHaveBeenCalled(); + }); + it("shows no form while the answer is still loading", () => { useMeQuery.mockReturnValue({ isPending: true, isSuccess: false, data: undefined }); renderPage(); diff --git a/src/features/settings/set-password-page.tsx b/src/features/settings/set-password-page.tsx index 4d37b50..820fa56 100644 --- a/src/features/settings/set-password-page.tsx +++ b/src/features/settings/set-password-page.tsx @@ -54,6 +54,15 @@ export function SetPasswordPage() { // Also covers the moment between deciding to leave and the navigation // landing, so the form never flashes at someone on their way out. + ) : meQuery.isError ? ( + // Distinct from the card's own "this deployment doesn't report + // password state" copy. Both would otherwise render here — an errored + // query leaves `password_set` undefined — and blaming a version gap + // for a transient outage is the §3.6 conflation this change avoids + // everywhere else. +

+ Couldn't load your account just now. Reload to try again. +

) : ( )} diff --git a/src/server/auth/__tests__/password-reset-actions.test.ts b/src/server/auth/__tests__/password-reset-actions.test.ts index 2d4e214..c5b276c 100644 --- a/src/server/auth/__tests__/password-reset-actions.test.ts +++ b/src/server/auth/__tests__/password-reset-actions.test.ts @@ -92,6 +92,17 @@ describe("requesting a reset code cannot become an account-existence oracle", () error: "password_reset_unavailable", }); }); + + // A rejected input is not an outage. Today this endpoint answers only 202 and + // 429, but reporting a future 4xx as "the service is down" would send a user + // whose address was refused off to wait for a recovery that never comes. + it("does not report a rejected request as an outage", async () => { + requestPasswordResetCode.mockRejectedValueOnce(backendError(400)); + + await expect(requestPasswordResetAction("op@example.test")).resolves.toEqual({ + error: "invalid_email", + }); + }); }); describe("confirming a reset", () => { @@ -208,8 +219,11 @@ describe("confirming a reset", () => { }); it("clears the binding when the backend reports a session mismatch", async () => { + // Spaced exactly as a pretty-printing encoder would emit it: a substring + // match on `"code":"..."` would miss this and fail open into the generic + // collapse, showing sign-in recovery copy on a reset screen. confirmPasswordReset.mockRejectedValueOnce( - backendError(401, '{"code":"otp_session_mismatch","message":"authentication failed"}'), + backendError(401, '{ "code": "otp_session_mismatch", "message": "authentication failed" }'), ); const result = await confirmPasswordResetAction({ diff --git a/src/server/auth/password-reset-actions.ts b/src/server/auth/password-reset-actions.ts index a1848cd..434e6cc 100644 --- a/src/server/auth/password-reset-actions.ts +++ b/src/server/auth/password-reset-actions.ts @@ -34,10 +34,23 @@ function statusOf(error: unknown): number | undefined { return typeof status === "number" ? status : undefined; } -/** Whether the backend's body named a specific code, without parsing prose. */ +/** + * Whether the backend's body named a specific code. + * + * Reads the `code` FIELD rather than substring-matching the raw text: a + * whitespace difference in the encoder (`"code": "..."`) would defeat a + * substring match, and it would fail open into the generic collapse — showing + * sign-in recovery copy on a reset screen. Never reads `message`, which is + * prose and is not a contract. + */ function codeIs(error: unknown, code: string): boolean { const body = (error as { responseBody?: unknown } | null)?.responseBody; - return typeof body === "string" && body.includes(`"code":"${code}"`); + if (typeof body !== "string") return false; + try { + return (JSON.parse(body) as { code?: unknown }).code === code; + } catch { + return false; + } } /** @@ -76,7 +89,15 @@ export async function requestPasswordResetAction(email: string): Promise= 500 || status === 404) { + return { error: AUTH_ERROR_CODES.passwordResetUnavailable }; + } + return { error: "invalid_email" }; } } @@ -117,10 +138,11 @@ export async function confirmPasswordResetAction(args: { }); } catch (error) { const status = statusOf(error); - // `hadBinding` is a fact about this browser, not about the account, and it - // is the one thing separating "the user lost their tab" from "the user - // mistyped" — invisible in the response by design. - console.error("[password-reset] confirm failed", { status, hadBinding: true }); + // Status only. A binding was necessarily present — the function returns + // early without one — so logging that fact would be a constant dressed as a + // variable. The address and the code are never logged: they are what the + // uniform 202 exists to protect. + console.error("[password-reset] confirm failed", { status }); if (status === 429) { return { error: AUTH_ERROR_CODES.otpRateLimited }; diff --git a/tests/contracts/me-password.contract.test.ts b/tests/contracts/me-password.contract.test.ts new file mode 100644 index 0000000..9b4627f --- /dev/null +++ b/tests/contracts/me-password.contract.test.ts @@ -0,0 +1,176 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Contract test — `POST /api/me/password` → backend `POST /api/v1/me/password`. + * RUK-289, SPEC §1.3, §3.3, §3.4. + * + * **Why there is no recorded fixture here, and why that does not excuse the + * test.** `scripts/refresh-fixtures.mjs` issues GET only and refuses any + * non-2xx, while this seam is a POST whose success is `204` with an empty body + * and whose interesting shapes are the error envelopes the recorder rejects + * (SPEC §7.3). So the contract policy's four questions are answered against a + * stubbed `fetch` rather than a fixture: + * + * 1. are the fields forwarded — including the two whose ABSENCE is meaningful? + * 2. does the response reach the client unchanged (204 stays 204)? + * 3. does a backend error stay an error, rather than degrading into success? + * 4. is the expectation independent of the thing it checks — the assertions + * read the request off the `fetch` stub, not off the arguments passed in. + * + * The seam matters more than most: SPEC §7.3 lists "a wrong `current_password` + * moving off 401" as a drift no automated check would catch, and this is the + * check that catches it. It is here rather than only in the unit suite because + * `npm run test:contracts` is what a reviewer reads as "the contracts are + * green", and a route absent from it is a route that claim does not cover. + */ + +const readActiveSession = vi.fn(); +vi.mock("@/server/auth/session-token", () => ({ + readActiveSession: () => readActiveSession(), +})); +vi.mock("@/server/backend/security/csrf", () => ({ isSameOriginRequest: () => true })); + +const { POST } = await import("@/app/api/me/password/route"); + +const fetchMock = vi.fn(); + +beforeEach(() => { + vi.stubGlobal("fetch", fetchMock); + fetchMock.mockReset(); + process.env.MAINTMODE_API_BASE_URL = "http://backend.test/maintmode"; + readActiveSession.mockResolvedValue({ + accessToken: "access-1", + refreshToken: "refresh-1", + accessTokenExpiresAt: Date.now() + 600_000, + }); +}); + +afterEach(() => vi.unstubAllGlobals()); + +function backendAnswers(status: number, body = "") { + fetchMock.mockResolvedValue({ + ok: status >= 200 && status < 300, + status, + statusText: `status ${status}`, + text: async () => body, + }); +} + +function post(body: unknown) { + return new Request("https://app.test/api/me/password", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +/** What actually went on the wire, read from the stub rather than the input. */ +function wireRequest() { + const [url, init] = fetchMock.mock.calls[0]; + return { url: String(url), init, body: JSON.parse(init.body) as Record }; +} + +describe("me/password — request forwarding", () => { + it("posts to the auth base's password path with a bearer credential", async () => { + backendAnswers(204); + + await POST(post({ current_password: "old", new_password: "a-long-enough-password" })); + + const { url, init } = wireRequest(); + expect(url).toContain("/api/v1/me/password"); + expect(init.method).toBe("POST"); + expect(init.headers.authorization).toBe("Bearer access-1"); + }); + + it("forwards refresh_token, which is what keeps the caller signed in", async () => { + backendAnswers(204); + + await POST(post({ current_password: "old", new_password: "a-long-enough-password" })); + + // Dropping it makes the backend revoke every session including this one, so + // the operator is signed out by their own success (SPEC §1.3). + expect(wireRequest().body).toEqual({ + current_password: "old", + new_password: "a-long-enough-password", + refresh_token: "refresh-1", + }); + }); + + it("omits current_password as a KEY when the caller sent none", async () => { + backendAnswers(204); + + await POST(post({ new_password: "a-long-enough-password" })); + + // Absent and empty are different requests: the backend rejects the field + // outright for an account that has no password. + expect("current_password" in wireRequest().body).toBe(false); + }); +}); + +describe("me/password — response pass-through", () => { + it("hands 204 to the client as 204 with no body", async () => { + backendAnswers(204); + + const response = await POST(post({ new_password: "a-long-enough-password" })); + + expect(response.status).toBe(204); + await expect(response.text()).resolves.toBe(""); + }); +}); + +describe("me/password — a backend error stays an error", () => { + // The degradation this guards against is not an empty list but a false + // success: reporting 2xx for a password the backend never changed. + it("does not turn a backend failure into a success", async () => { + for (const status of [400, 401, 403, 500, 503]) { + fetchMock.mockReset(); + backendAnswers(status, '{"code":"unauthorized"}'); + + const response = await POST(post({ new_password: "a-long-enough-password" })); + + expect(response.status).not.toBe(204); + expect(response.status).toBeGreaterThanOrEqual(400); + } + }); + + // SPEC §7.3's drift #2, pinned. If the backend moves a wrong current password + // off 401, this fails — and without it the change would reach the generic + // mapper, which answers AUTH_REQUIRED and signs the operator out with no + // message on their most common mistake. + it("keeps a wrong current password renderable rather than signing the user out", async () => { + backendAnswers(401, '{"code":"unauthorized","message":"invalid credentials"}'); + + const response = await POST(post({ current_password: "wrong", new_password: "a-long-enough-password" })); + + expect(response.status).toBe(422); + await expect(response.json()).resolves.toMatchObject({ code: "WRONG_CURRENT_PASSWORD" }); + }); + + it("distinguishes a stale refresh token, which changed nothing", async () => { + backendAnswers(401, '{"code":"unauthorized"}'); + + const response = await POST(post({ new_password: "a-long-enough-password" })); + + expect(response.status).toBe(409); + await expect(response.json()).resolves.toMatchObject({ code: "SESSION_STALE" }); + }); + + it("passes a 400 through with the backend's own message, unparsed", async () => { + backendAnswers(400, "validation error: password does not meet the length policy"); + + const response = await POST(post({ new_password: "a-long-enough-password" })); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ + error: "validation error: password does not meet the length policy", + }); + }); + + it("answers an unreachable backend as an outage", async () => { + fetchMock.mockRejectedValue(new Error("connect ECONNREFUSED")); + + const response = await POST(post({ new_password: "a-long-enough-password" })); + + expect(response.status).toBe(503); + }); +}); From fd56c561169e087162e65850e3c680cd4f07c0b6 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:10:24 +0300 Subject: [PATCH 09/14] refactor(auth): move the sign-in code flow onto the shared timers hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/features/auth/otp-sign-in-flow.tsx | 72 ++++++++------------------ 1 file changed, 22 insertions(+), 50 deletions(-) diff --git a/src/features/auth/otp-sign-in-flow.tsx b/src/features/auth/otp-sign-in-flow.tsx index 7f07e4a..a9adf6c 100644 --- a/src/features/auth/otp-sign-in-flow.tsx +++ b/src/features/auth/otp-sign-in-flow.tsx @@ -1,11 +1,12 @@ "use client"; -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useState } from "react"; import { Button } from "@/shared/ui/shadcn/button"; import { Input } from "@/shared/ui/shadcn/input"; import { Label } from "@/shared/ui/shadcn/label"; import { isWellFormedOtpCode } from "@/domain/auth/sign-in-method"; +import { useCodeTimers } from "@/features/auth/use-code-timers"; /** * Two-step email one-time-code sign-in (RUK-288). @@ -16,16 +17,6 @@ import { isWellFormedOtpCode } from "@/domain/auth/sign-in-method"; * `check-bundle-budget.mjs` guards against. */ -/** Backend `otp_ttl`. The real expiry lives server-side and is never returned. */ -const CODE_TTL_SECONDS = 300; -/** - * Advisory only. The backend has no resend cooldown, and its per-IP bucket is - * shared with the password and OAuth endpoints — so an unthrottled resend - * button would lock the user out of the *other* ways in. The server's 429 is - * the real backstop. - */ -const RESEND_COOLDOWN_SECONDS = 30; - type Step = "email" | "code"; export interface OtpSignInFlowProps { @@ -41,21 +32,12 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: const [code, setCode] = useState(""); const [error, setError] = useState(); const [pending, setPending] = useState(false); - const [remaining, setRemaining] = useState(0); - const [cooldown, setCooldown] = useState(0); - - // One interval drives both counters. Started on entry to step two and torn - // down on leaving it, so a backgrounded tab cannot leave a timer running. - useEffect(() => { - if (step !== "code") return; - const id = setInterval(() => { - setRemaining((r) => (r > 0 ? r - 1 : 0)); - setCooldown((c) => (c > 0 ? c - 1 : 0)); - }, 1000); - return () => clearInterval(id); - }, [step]); - - const expired = step === "code" && remaining === 0; + + // Shared with the password-reset flow (RUK-289): the TTL and the attempt + // budget are contract facts about the same backend mechanism, and two copies + // would drift. + const timers = useCodeTimers(step === "code"); + const { remaining, cooldown, expired } = timers; const send = useCallback( async (address: string) => { @@ -66,41 +48,32 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: if (result.error) { setError(result.error); - // A failed request starts a fresh cooldown rather than leaving the - // button hot: a 429 answered by immediate retries is what caused it. - setCooldown(RESEND_COOLDOWN_SECONDS); + timers.startCooldown(); return; } - // Counted from response receipt, so the client is always slightly - // optimistic relative to the server. That is the safe direction: the - // backend, not this timer, decides whether a code is still valid. - setRemaining(CODE_TTL_SECONDS); - setCooldown(RESEND_COOLDOWN_SECONDS); + timers.start(); setCode(""); setStep("code"); }, - [requestCode], + [requestCode, timers], ); - const inFlight = useRef(false); - async function onSubmitCode(event: React.FormEvent) { event.preventDefault(); - // Each stray submit spends one of five attempts, and the backend floors - // every response to ~300ms, so a double-click is a live risk. - if (inFlight.current || pending || expired) return; + if (pending || expired) return; if (!isWellFormedOtpCode(code)) { setError("invalid_code_format"); return; } - inFlight.current = true; - setPending(true); - setError(undefined); - const result = await submitCode(email, code.trim()); - inFlight.current = false; - setPending(false); - if (!result.error) return; + const result = await timers.guard(async () => { + setPending(true); + setError(undefined); + const outcome = await submitCode(email, code.trim()); + setPending(false); + return outcome; + }); + if (!result || !result.error) return; if (result.error === "otp_session_mismatch") { // The binding is gone — the server has already cleared it — so step two @@ -110,8 +83,7 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: // is the primary action and nothing is throttled. setStep("email"); setCode(""); - setRemaining(0); - setCooldown(0); + timers.reset(); } setError(result.error); } @@ -121,7 +93,7 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: setStep("email"); setCode(""); setError(undefined); - setRemaining(0); + timers.reset(); } if (step === "email") { From 3ec3dbd77ca323c8c4522867c8db0d4b2bfceb3c Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:13:25 +0300 Subject: [PATCH 10/14] perf(auth): derive the code countdown from a deadline, and shape the skeleton MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../auth/__tests__/use-code-timers.test.tsx | 99 +++++++++++++++++++ src/features/auth/login-page.tsx | 6 ++ src/features/auth/use-code-timers.ts | 33 ++++++- src/features/settings/set-password-page.tsx | 12 ++- 4 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 src/features/auth/__tests__/use-code-timers.test.tsx diff --git a/src/features/auth/__tests__/use-code-timers.test.tsx b/src/features/auth/__tests__/use-code-timers.test.tsx new file mode 100644 index 0000000..870c7ed --- /dev/null +++ b/src/features/auth/__tests__/use-code-timers.test.tsx @@ -0,0 +1,99 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, render, screen } from "@testing-library/react"; + +import { CODE_TTL_SECONDS, MAX_CODE_ATTEMPTS, useCodeTimers } from "@/features/auth/use-code-timers"; + +afterEach(() => { + cleanup(); + vi.useRealTimers(); +}); + +function Probe({ active }: { active: boolean }) { + const timers = useCodeTimers(active); + return ( +
+ {timers.remaining} + +
+ ); +} + +function remaining() { + return Number(screen.getByTestId("remaining").textContent); +} + +describe("the countdown is derived from a deadline, not decremented", () => { + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: false }); + vi.setSystemTime(new Date("2026-09-07T12:00:00Z")); + }); + + // The case a decrementing counter gets wrong. A backgrounded tab, a laptop + // waking from sleep and a bfcache restore all suspend the interval; a counter + // then resumes from where it stopped and OVER-reports the time left, showing + // "expires in 4:12" for a code the backend has already discarded. + it("catches up after the tab is suspended, rather than resuming where it paused", () => { + render(); + act(() => { + screen.getByRole("button", { name: "start" }).click(); + }); + expect(remaining()).toBe(CODE_TTL_SECONDS); + + // The clock jumps two minutes while the interval fires only once — a + // throttled or suspended tab. A decrementing counter would read 299 here, + // because it only ever subtracts what it observed. + act(() => { + vi.setSystemTime(new Date("2026-09-07T12:02:00Z")); + vi.advanceTimersByTime(1000); + }); + + // 120s of wall clock plus the 1s the tick itself advanced. + expect(remaining()).toBe(CODE_TTL_SECONDS - 121); + expect(remaining()).toBeLessThan(CODE_TTL_SECONDS - 100); + }); + + it("floors at zero rather than going negative", () => { + render(); + act(() => { + screen.getByRole("button", { name: "start" }).click(); + }); + + act(() => { + vi.setSystemTime(new Date("2026-09-07T12:10:00Z")); + vi.advanceTimersByTime(1000); + }); + + expect(remaining()).toBe(0); + }); + + it("stops ticking once the step is left", () => { + const { rerender } = render(); + act(() => { + screen.getByRole("button", { name: "start" }).click(); + }); + + rerender(); + const frozen = remaining(); + + act(() => { + vi.setSystemTime(new Date("2026-09-07T12:00:30Z")); + vi.advanceTimersByTime(5000); + }); + + // No interval is running, so nothing recomputes: the value is whatever the + // last active tick left, and `active` gating is what tears it down. + expect(remaining()).toBe(frozen); + }); +}); + +describe("the contract constants", () => { + // Pinned as literals. Every other suite derives its loops from these, so + // without this the whole budget story is self-fulfilling. + it("match the backend's configuration", () => { + expect(CODE_TTL_SECONDS).toBe(300); + expect(MAX_CODE_ATTEMPTS).toBe(5); + }); +}); diff --git a/src/features/auth/login-page.tsx b/src/features/auth/login-page.tsx index 19d38ff..5365a05 100644 --- a/src/features/auth/login-page.tsx +++ b/src/features/auth/login-page.tsx @@ -9,6 +9,12 @@ import { BrandIcon, MaintMark, type BrandProvider } from "@/shared/ui/icons/bran import type { SignInMethod } from "@/domain/auth/sign-in-method"; import { OtpSignInFlow } from "@/features/auth/otp-sign-in-flow"; import { PasswordSignInForm } from "@/features/auth/password-sign-in-form"; +// Statically imported, deliberately. It looks like a candidate for `dynamic()` +// — it is only reachable behind a click — but it can also be the FIRST thing +// this page paints: `resetInProgressEmail` resumes the flow after the user +// comes back from their email client, and a lazy chunk would turn that +// re-entry into two sequential round-trips. Its own weight is a few KB; every +// primitive it uses is already on this route. import { PasswordResetFlow } from "@/features/auth/password-reset-flow"; export interface LoginPageProps { diff --git a/src/features/auth/use-code-timers.ts b/src/features/auth/use-code-timers.ts index aad9827..61559ab 100644 --- a/src/features/auth/use-code-timers.ts +++ b/src/features/auth/use-code-timers.ts @@ -58,7 +58,21 @@ export interface CodeTimers { guard: (fn: () => Promise) => Promise; } +/** Whole seconds from now until `deadline`, floored at zero. */ +function secondsUntil(deadline: number, now: number): number { + return Math.max(0, Math.ceil((deadline - now) / 1000)); +} + export function useCodeTimers(active: boolean): CodeTimers { + // Deadlines, not counters. A decrementing counter drifts whenever the tab is + // throttled — a backgrounded tab, a laptop waking from sleep, a bfcache + // restore — and it drifts in the direction that OVER-reports the time left, + // so the user is shown "expires in 4:12" for a code the backend has already + // discarded. Deriving from a timestamp survives all three, and the displayed + // value stays optimistic only by the network delay it was always optimistic + // by. + const codeDeadline = useRef(0); + const cooldownDeadline = useRef(0); const [remaining, setRemaining] = useState(0); const [cooldown, setCooldown] = useState(0); const inFlight = useRef(false); @@ -67,10 +81,15 @@ export function useCodeTimers(active: boolean): CodeTimers { // backgrounded tab cannot leave a timer running. useEffect(() => { if (!active) return; - const id = setInterval(() => { - setRemaining((r) => (r > 0 ? r - 1 : 0)); - setCooldown((c) => (c > 0 ? c - 1 : 0)); - }, 1000); + const tick = () => { + const now = Date.now(); + setRemaining(secondsUntil(codeDeadline.current, now)); + setCooldown(secondsUntil(cooldownDeadline.current, now)); + }; + const id = setInterval(tick, 1000); + // Once immediately, so a tab returning to the foreground corrects on the + // frame it wakes rather than a second later. + tick(); return () => clearInterval(id); }, [active]); @@ -78,11 +97,16 @@ export function useCodeTimers(active: boolean): CodeTimers { // Counted from response receipt, so the client is always slightly // optimistic relative to the server. That is the safe direction: the // backend, not this timer, decides whether a code is still valid. + const now = Date.now(); + codeDeadline.current = now + CODE_TTL_SECONDS * 1000; + cooldownDeadline.current = now + RESEND_COOLDOWN_SECONDS * 1000; setRemaining(CODE_TTL_SECONDS); setCooldown(RESEND_COOLDOWN_SECONDS); }, []); const reset = useCallback(() => { + codeDeadline.current = 0; + cooldownDeadline.current = 0; setRemaining(0); setCooldown(0); }, []); @@ -90,6 +114,7 @@ export function useCodeTimers(active: boolean): CodeTimers { const startCooldown = useCallback(() => { // A failed request starts a fresh cooldown rather than leaving the button // hot: a 429 answered by immediate retries is what caused it. + cooldownDeadline.current = Date.now() + RESEND_COOLDOWN_SECONDS * 1000; setCooldown(RESEND_COOLDOWN_SECONDS); }, []); diff --git a/src/features/settings/set-password-page.tsx b/src/features/settings/set-password-page.tsx index 820fa56..e3e9615 100644 --- a/src/features/settings/set-password-page.tsx +++ b/src/features/settings/set-password-page.tsx @@ -51,9 +51,19 @@ export function SetPasswordPage() { {meQuery.isPending || shouldLeave ? ( + // Shaped like the form it stands in for, not a fixed block. The page + // is `grid place-items-center`, so a height change on swap + // re-centres the whole card and visibly moves the heading above it — + // a placeholder of the wrong height guarantees that on every load. + // // Also covers the moment between deciding to leave and the navigation // landing, so the form never flashes at someone on their way out. - + ) : meQuery.isError ? ( // Distinct from the card's own "this deployment doesn't report // password state" copy. Both would otherwise render here — an errored From 6bd65f123c4a91609193b0c17e78f57832c43cc7 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:21:53 +0300 Subject: [PATCH 11/14] refactor(auth): share the backend fetch scaffold, name the resend predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/features/auth/password-reset-flow.tsx | 7 +- src/server/auth/backend-token-exchange.ts | 257 +++++++++++----------- 2 files changed, 137 insertions(+), 127 deletions(-) diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx index 250ab1f..593e069 100644 --- a/src/features/auth/password-reset-flow.tsx +++ b/src/features/auth/password-reset-flow.tsx @@ -194,6 +194,9 @@ export function PasswordResetFlow({ } const dead = timers.expired || budgetSpent; + // A dead code makes resend the only way forward, so the cooldown is waived + // rather than made to run out first. + const throttled = timers.cooldown > 0 && !dead; return (
@@ -252,10 +255,10 @@ export function PasswordResetFlow({
); diff --git a/src/server/auth/backend-token-exchange.ts b/src/server/auth/backend-token-exchange.ts index 4e4bae2..2e566bf 100644 --- a/src/server/auth/backend-token-exchange.ts +++ b/src/server/auth/backend-token-exchange.ts @@ -194,13 +194,9 @@ export async function confirmPasswordReset(args: { sessionNonce: string; newPassword: string; }): Promise { - const config = readMaintmodeBackendConfig(); - const target = resolveBackendUrl(config.authApiBaseUrl, PASSWORD_RESET_CONFIRM_PATH); - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); - - try { - const response = await fetch(target, { + return backendFetch( + PASSWORD_RESET_CONFIRM_PATH, + { method: "POST", headers: { accept: "application/json", "content-type": "application/json" }, body: JSON.stringify({ @@ -209,14 +205,13 @@ export async function confirmPasswordReset(args: { session_nonce: args.sessionNonce, new_password: args.newPassword, }), - signal: controller.signal, - }); - if (!response.ok) { - throw new BackendAuthError(response.status, (await response.text()) || response.statusText); - } - } finally { - clearTimeout(timeout); - } + }, + async (response) => { + if (!response.ok) { + throw new BackendAuthError(response.status, (await response.text()) || response.statusText); + } + }, + ); } /** @@ -260,51 +255,49 @@ export async function changeBackendPassword(args: { newPassword: string; refreshToken?: string; }): Promise { - const config = readMaintmodeBackendConfig(); - const target = resolveBackendUrl(config.authApiBaseUrl, CHANGE_PASSWORD_PATH); - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); - try { - const response = await fetch(target, { - method: "POST", - headers: { - accept: "application/json", - "content-type": "application/json", - authorization: `Bearer ${args.accessToken}`, + return await backendFetch( + CHANGE_PASSWORD_PATH, + { + method: "POST", + headers: { + accept: "application/json", + "content-type": "application/json", + authorization: `Bearer ${args.accessToken}`, + }, + body: JSON.stringify({ + // Sent only when there is one. The backend rejects the field outright + // for an account with no password, so an empty string is not the same + // as absent. + ...(args.currentPassword ? { current_password: args.currentPassword } : {}), + new_password: args.newPassword, + ...(args.refreshToken ? { refresh_token: args.refreshToken } : {}), + }), }, - body: JSON.stringify({ - // Sent only when there is one. The backend rejects the field outright - // for an account with no password, so an empty string is not the same - // as absent. - ...(args.currentPassword ? { current_password: args.currentPassword } : {}), - new_password: args.newPassword, - ...(args.refreshToken ? { refresh_token: args.refreshToken } : {}), - }), - signal: controller.signal, - }); + async (response): Promise => { + if (response.status === 204) { + return { ok: true }; + } - if (response.status === 204) { - return { ok: true }; - } + const body = await response.text(); - const body = await response.text(); - - if (response.status === 401) { - // The one classification, and it reads what the REQUEST carried rather - // than what the response says. A 401 when a current password was sent is - // that password being wrong; a 401 when none was sent is the - // `refresh_token` having been superseded, and nothing the user typed. - return { ok: false, kind: args.currentPassword ? "wrong-current-password" : "session-stale" }; - } - if (response.status === 400) { - return { ok: false, kind: "rejected", message: body }; - } - return { ok: false, kind: "unavailable" }; + if (response.status === 401) { + // The one classification, and it reads what the REQUEST carried rather + // than what the response says. A 401 when a current password was sent is + // that password being wrong; a 401 when none was sent is the + // `refresh_token` having been superseded, and nothing the user typed. + return { ok: false, kind: args.currentPassword ? "wrong-current-password" : "session-stale" }; + } + if (response.status === 400) { + return { ok: false, kind: "rejected", message: body }; + } + return { ok: false, kind: "unavailable" }; + }, + ); } catch { + // Network failure, a malformed URL, or the abort above. This function + // reports rather than throws, so every one of them is `unavailable`. return { ok: false, kind: "unavailable" }; - } finally { - clearTimeout(timeout); } } @@ -323,13 +316,9 @@ export async function refreshBackendToken(refreshToken: string): Promise { - const config = readMaintmodeBackendConfig(); - const target = resolveBackendUrl(config.authApiBaseUrl, LOGOUT_PATH); - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); - - try { - const response = await fetch(target, { + return backendFetch( + LOGOUT_PATH, + { method: "POST", headers: { accept: "application/json", @@ -337,15 +326,14 @@ export async function revokeBackendSession(accessToken: string, refreshToken: st authorization: `Bearer ${accessToken}`, }, body: JSON.stringify({ refresh_token: refreshToken }), - signal: controller.signal, - }); - if (!response.ok && response.status !== 204) { - const body = await response.text(); - throw new BackendAuthError(response.status, body || response.statusText); - } - } finally { - clearTimeout(timeout); - } + }, + async (response) => { + if (!response.ok && response.status !== 204) { + const body = await response.text(); + throw new BackendAuthError(response.status, body || response.statusText); + } + }, + ); } /** @@ -354,56 +342,80 @@ export async function revokeBackendSession(accessToken: string, refreshToken: st * (`Authorization: Bearer`); there is no body. */ export async function revokeAllBackendSessions(accessToken: string): Promise { - const config = readMaintmodeBackendConfig(); - const target = resolveBackendUrl(config.authApiBaseUrl, LOGOUT_ALL_PATH); - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); - - try { - const response = await fetch(target, { + return backendFetch( + LOGOUT_ALL_PATH, + { method: "POST", headers: { accept: "application/json", authorization: `Bearer ${accessToken}`, }, - signal: controller.signal, - }); - if (!response.ok && response.status !== 204) { - const body = await response.text(); - throw new BackendAuthError(response.status, body || response.statusText); - } - } finally { - clearTimeout(timeout); - } + }, + async (response) => { + if (!response.ok && response.status !== 204) { + const body = await response.text(); + throw new BackendAuthError(response.status, body || response.statusText); + } + }, + ); } /** * Loads the current user's profile via `GET /api/v1/me`. */ export async function fetchBackendMe(accessToken: string): Promise { - const config = readMaintmodeBackendConfig(); - const target = resolveBackendUrl(config.authApiBaseUrl, ME_PATH); - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); - - try { - const response = await fetch(target, { + return backendFetch( + ME_PATH, + { method: "GET", headers: { accept: "application/json", authorization: `Bearer ${accessToken}`, }, - signal: controller.signal, - }); - const body = await response.text(); - if (!response.ok) { - throw new BackendAuthError(response.status, body || response.statusText); - } - const parsed = safeJsonParse(body); - if (!parsed?.id || !parsed.email) { - throw new BackendAuthError(response.status, body, "Backend /me returned an unexpected payload"); - } - return parsed; + }, + async (response) => { + const body = await response.text(); + if (!response.ok) { + throw new BackendAuthError(response.status, body || response.statusText); + } + const parsed = safeJsonParse(body); + if (!parsed?.id || !parsed.email) { + throw new BackendAuthError(response.status, body, "Backend /me returned an unexpected payload"); + } + return parsed; + }, + ); +} + +/** + * Resolves `path` against the auth base URL and runs one `fetch` under the + * configured timeout. + * + * Owns the request scaffold ONLY — the URL and the abort timer, which were + * identical at six call sites. It deliberately does not touch the response: + * these endpoints disagree about what a response even is (a shape-checked JSON + * body, a bare 204, a 401 classified by what the REQUEST carried), and some + * throw where others return a discriminated result. Folding that in would erase + * the distinctions their callers exist to act on. + * + * `handleResponse` receives the response, and the timeout is cleared only once + * it settles. That is why this takes a callback rather than returning the + * `Response`: the timer has to outlive the body read. `fetch` resolves on the + * response HEAD, so clearing it at that point would leave a stalled `.text()` + * hanging forever — exactly the case the timeout exists for. + */ +async function backendFetch( + path: string, + init: Omit, + handleResponse: (response: Response) => Promise, +): Promise { + const config = readMaintmodeBackendConfig(); + const target = resolveBackendUrl(config.authApiBaseUrl, path); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); + + try { + return await handleResponse(await fetch(target, { ...init, signal: controller.signal })); } finally { clearTimeout(timeout); } @@ -415,13 +427,9 @@ async function postBackendJson( isShapeValid: (parsed: TResponse | undefined) => boolean, extraHeaders?: Record, ): Promise { - const config = readMaintmodeBackendConfig(); - const target = resolveBackendUrl(config.authApiBaseUrl, path); - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), config.requestTimeoutMs); - - try { - const response = await fetch(target, { + return backendFetch( + path, + { method: "POST", headers: { // Spread extra headers first so the fixed accept/content-type below @@ -431,20 +439,19 @@ async function postBackendJson( "content-type": "application/json", }, body: JSON.stringify(body), - signal: controller.signal, - }); - const text = await response.text(); - if (!response.ok) { - throw new BackendAuthError(response.status, text || response.statusText); - } - const parsed = safeJsonParse(text); - if (!isShapeValid(parsed)) { - throw new BackendAuthError(response.status, text, `Backend ${path} returned an unexpected payload`); - } - return parsed as TResponse; - } finally { - clearTimeout(timeout); - } + }, + async (response) => { + const text = await response.text(); + if (!response.ok) { + throw new BackendAuthError(response.status, text || response.statusText); + } + const parsed = safeJsonParse(text); + if (!isShapeValid(parsed)) { + throw new BackendAuthError(response.status, text, `Backend ${path} returned an unexpected payload`); + } + return parsed as TResponse; + }, + ); } function safeJsonParse(text: string): T | undefined { From 4b1963bbea21f1e5a4a993d0e307e14cbf77bb4a Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:28:37 +0300 Subject: [PATCH 12/14] fix(auth): describe the reset field errors for screen readers 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 --- .../__tests__/password-reset-flow.test.tsx | 34 +++++++++++++++++++ src/features/auth/password-reset-flow.tsx | 11 ++++-- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/features/auth/__tests__/password-reset-flow.test.tsx b/src/features/auth/__tests__/password-reset-flow.test.tsx index cecb36b..b85a116 100644 --- a/src/features/auth/__tests__/password-reset-flow.test.tsx +++ b/src/features/auth/__tests__/password-reset-flow.test.tsx @@ -218,6 +218,40 @@ describe("failures the user must be able to tell apart", () => { }); }); +describe("what a screen reader is told", () => { + // The alert fires once when the error appears. Returning focus to the field + // afterwards must still explain why it was rejected, which is what the + // description association is for — the sign-in flow does the same. + it("points the code field at the error, and at the countdown otherwise", async () => { + const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_failed" })) }); + await reachCodeStep(props); + + expect(screen.getByLabelText("Enter the 6-digit code").getAttribute("aria-describedby")).toBe( + "reset-countdown", + ); + + await submitCode("000000", LONG_ENOUGH); + + await waitFor(() => { + expect(screen.getByLabelText("Enter the 6-digit code").getAttribute("aria-describedby")).toBe( + "reset-error", + ); + }); + }); + + it("points the password field at the error when the error is about the password", async () => { + const props = setup(); + await reachCodeStep(props); + + // Keeps the hint too: the requirement is still what the user needs to hear. + await submitCode("123456", "short"); + + expect(screen.getByLabelText("New password").getAttribute("aria-describedby")).toBe( + "reset-error reset-password-hint", + ); + }); +}); + describe("rehydration after a reload", () => { // The flow spans an email round-trip, so the user leaves the tab. The server // page reads the cookie and hands down the step; the nonce never crosses. diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx index 593e069..d38d62f 100644 --- a/src/features/auth/password-reset-flow.tsx +++ b/src/features/auth/password-reset-flow.tsx @@ -216,6 +216,11 @@ export function PasswordResetFlow({ value={code} disabled={dead} onChange={(e) => setCode(e.target.value.replace(/\D/g, ""))} + // Points at whichever of the two actually renders below, matching the + // sign-in flow: the alert fires once, and a screen-reader user who + // returns focus here afterwards would otherwise hear nothing saying why + // the code was rejected. + aria-describedby={error || dead ? "reset-error" : "reset-countdown"} /> setPassword(e.target.value)} - aria-describedby="reset-password-hint" + aria-describedby={ + error === "password_policy_violation" ? "reset-error reset-password-hint" : "reset-password-hint" + } /> {/* * "Characters" rather than bytes: the policy is 12 BYTES, which is not a @@ -243,7 +250,7 @@ export function PasswordResetFlow({ ) : error ? ( ) : ( -

+

Expires in {formatRemaining(timers.remaining)}

)} From fc20746632235c588cc80e8cd4c6a06266c7ece5 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:33:13 +0300 Subject: [PATCH 13/14] fix(auth): keep credentials out of logs, envelopes out of forms, passwords out of state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/password-reset-flow.test.tsx | 24 +++++++++++++++ src/features/auth/password-reset-flow.tsx | 6 ++++ .../backend-token-exchange-password.test.ts | 29 +++++++++++++++++-- .../__tests__/password-reset-actions.test.ts | 22 ++++++++++++++ src/server/auth/backend-token-exchange.ts | 13 ++++++++- src/server/auth/password-reset-actions.ts | 8 ++++- tests/contracts/me-password.contract.test.ts | 10 +++++-- 7 files changed, 106 insertions(+), 6 deletions(-) diff --git a/src/features/auth/__tests__/password-reset-flow.test.tsx b/src/features/auth/__tests__/password-reset-flow.test.tsx index b85a116..1926a66 100644 --- a/src/features/auth/__tests__/password-reset-flow.test.tsx +++ b/src/features/auth/__tests__/password-reset-flow.test.tsx @@ -218,6 +218,30 @@ describe("failures the user must be able to tell apart", () => { }); }); +describe("leaving the flow", () => { + // Backing out and resetting a DIFFERENT address must not carry the previous + // password forward: the user would submit for that account a secret they + // never knowingly re-entered. + it("clears the typed password when returning to step one", async () => { + const props = setup({ + confirm: vi.fn(async () => ({ error: "password_reset_session_mismatch" })), + }); + await reachCodeStep(props); + await submitCode("123456", LONG_ENOUGH); + + await waitFor(() => expect(screen.getByLabelText("Reset your password")).toBeTruthy()); + + // Back to step two for another address; the field must be empty. + fireEvent.change(screen.getByLabelText("Reset your password"), { + target: { value: "other@example.test" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); + await screen.findByLabelText("New password"); + + expect((screen.getByLabelText("New password") as HTMLInputElement).value).toBe(""); + }); +}); + describe("what a screen reader is told", () => { // The alert fires once when the error appears. Returning focus to the field // afterwards must still explain why it was rejected, which is what the diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx index d38d62f..21b04e5 100644 --- a/src/features/auth/password-reset-flow.tsx +++ b/src/features/auth/password-reset-flow.tsx @@ -107,6 +107,12 @@ export function PasswordResetFlow({ timers.reset(); setStep("email"); setCode(""); + // Cleared with the code, not left behind. Without this, someone who backs + // out and resets a DIFFERENT address finds the previous password already + // filled in, and submits for that account a secret they never knowingly + // re-entered — besides holding a plaintext password in state longer than + // the flow has any use for it. + setPassword(""); setAttempts(0); setError(message); } diff --git a/src/server/auth/__tests__/backend-token-exchange-password.test.ts b/src/server/auth/__tests__/backend-token-exchange-password.test.ts index 32481ee..a78b97a 100644 --- a/src/server/auth/__tests__/backend-token-exchange-password.test.ts +++ b/src/server/auth/__tests__/backend-token-exchange-password.test.ts @@ -80,10 +80,22 @@ describe("changeBackendPassword — the 401 classification", () => { changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), ).resolves.toEqual({ ok: true }); - fetchMock.mockResolvedValueOnce(respond(400, "validation error: length policy")); + // A realistic envelope, not a hand-typed bare string. The previous fixture + // was satisfied identically by passing the WHOLE body through, which is how + // raw JSON reached the operator's form unnoticed. + fetchMock.mockResolvedValueOnce( + respond( + 400, + '{"code":"invalid request","message":"validation error: password does not meet the length policy"}', + ), + ); await expect( changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), - ).resolves.toEqual({ ok: false, kind: "rejected", message: "validation error: length policy" }); + ).resolves.toEqual({ + ok: false, + kind: "rejected", + message: "validation error: password does not meet the length policy", + }); fetchMock.mockResolvedValueOnce(respond(503, "gateway down")); await expect( @@ -94,6 +106,19 @@ describe("changeBackendPassword — the 401 classification", () => { // A 200 or 202 is not success here: the contract is 204 with an empty body, // and treating any 2xx as done would report a password change the backend // never made. + // A proxy's HTML error page must not be pasted into the form whole. + it("falls back to a sentence when a 400 is not the expected envelope", async () => { + fetchMock.mockResolvedValue(respond(400, "502 Bad Gateway")); + + await expect( + changeBackendPassword({ accessToken: "a", newPassword: "a-long-enough-password" }), + ).resolves.toEqual({ + ok: false, + kind: "rejected", + message: "That password wasn't accepted.", + }); + }); + it("treats a 2xx that is not 204 as unavailable", async () => { fetchMock.mockResolvedValue(respond(200, "{}")); diff --git a/src/server/auth/__tests__/password-reset-actions.test.ts b/src/server/auth/__tests__/password-reset-actions.test.ts index c5b276c..3ec29a1 100644 --- a/src/server/auth/__tests__/password-reset-actions.test.ts +++ b/src/server/auth/__tests__/password-reset-actions.test.ts @@ -265,6 +265,28 @@ describe("confirming a reset", () => { expect(clearPasswordResetBinding).not.toHaveBeenCalled(); }); + // The teardown error can be a BackendAuthError whose `responseBody` is the + // raw refresh response — a token pair. Logging the object would serialize it. + it("never logs credential material when the teardown fails", async () => { + confirmPasswordReset.mockResolvedValue(undefined); + signOut.mockRejectedValueOnce( + Object.assign(new Error("refresh failed"), { + status: 401, + responseBody: '{"access_token":"at_SECRET","refresh_token":"rt_SECRET"}', + }), + ); + + await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + const logged = JSON.stringify((console.error as unknown as ReturnType).mock.calls); + expect(logged).not.toContain("rt_SECRET"); + expect(logged).not.toContain("access_token"); + }); + it("never logs the address or the code", async () => { confirmPasswordReset.mockRejectedValueOnce(backendError(401, "{}")); diff --git a/src/server/auth/backend-token-exchange.ts b/src/server/auth/backend-token-exchange.ts index 2e566bf..2bfd421 100644 --- a/src/server/auth/backend-token-exchange.ts +++ b/src/server/auth/backend-token-exchange.ts @@ -289,7 +289,18 @@ export async function changeBackendPassword(args: { return { ok: false, kind: args.currentPassword ? "wrong-current-password" : "session-stale" }; } if (response.status === 400) { - return { ok: false, kind: "rejected", message: body }; + // The `message` FIELD, not the raw envelope. `body` is the whole + // response text, so passing it straight through put + // `{"code":"invalid request","message":…}` verbatim into the + // operator's form — and would paste a proxy's HTML error page in + // whole. Read for DISPLAY only: §3.4 forbids branching on backend + // prose, and the only behavioural branch remains the card's flip flag. + const parsed = safeJsonParse<{ message?: string }>(body); + return { + ok: false, + kind: "rejected", + message: parsed?.message ?? "That password wasn't accepted.", + }; } return { ok: false, kind: "unavailable" }; }, diff --git a/src/server/auth/password-reset-actions.ts b/src/server/auth/password-reset-actions.ts index 434e6cc..98b738c 100644 --- a/src/server/auth/password-reset-actions.ts +++ b/src/server/auth/password-reset-actions.ts @@ -171,7 +171,13 @@ export async function confirmPasswordResetAction(args: { await signOut({ redirect: false }); await clearActiveSession(); } catch (error) { - console.error("[password-reset] post-reset session teardown failed", error); + // Status only, like the two log lines above. Logging the error OBJECT here + // would render `BackendAuthError.responseBody` — the backend's raw response + // text — and the refresh path's body is a token pair, so a failed teardown + // would write a live refresh token into the application log. + console.error("[password-reset] post-reset session teardown failed", { + status: statusOf(error), + }); } return { done: true }; diff --git a/tests/contracts/me-password.contract.test.ts b/tests/contracts/me-password.contract.test.ts index 9b4627f..54bac27 100644 --- a/tests/contracts/me-password.contract.test.ts +++ b/tests/contracts/me-password.contract.test.ts @@ -155,12 +155,18 @@ describe("me/password — a backend error stays an error", () => { await expect(response.json()).resolves.toMatchObject({ code: "SESSION_STALE" }); }); - it("passes a 400 through with the backend's own message, unparsed", async () => { - backendAnswers(400, "validation error: password does not meet the length policy"); + it("surfaces the backend's message field on a 400, not its envelope", async () => { + backendAnswers( + 400, + '{"code":"invalid request","message":"validation error: password does not meet the length policy"}', + ); const response = await POST(post({ new_password: "a-long-enough-password" })); expect(response.status).toBe(400); + // The message, not the whole body: echoing the envelope would put raw JSON + // in the operator's form, and would paste a proxy's HTML page in whole. + // Read for display only — nothing branches on it. await expect(response.json()).resolves.toMatchObject({ error: "validation error: password does not meet the length policy", }); From ba4c792b254c501b32b58ef2a033e894de159b2b Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 02:42:11 +0300 Subject: [PATCH 14/14] test(auth): cover the reset flow's timers, the body timeout, and the code field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/password-reset-flow.test.tsx | 103 ++++++++++++++++-- src/features/auth/password-reset-flow.tsx | 9 +- .../backend-token-exchange-password.test.ts | 34 ++++++ .../__tests__/password-reset-actions.test.ts | 23 ++++ 4 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/features/auth/__tests__/password-reset-flow.test.tsx b/src/features/auth/__tests__/password-reset-flow.test.tsx index 1926a66..293558a 100644 --- a/src/features/auth/__tests__/password-reset-flow.test.tsx +++ b/src/features/auth/__tests__/password-reset-flow.test.tsx @@ -1,5 +1,5 @@ // @vitest-environment jsdom -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { PasswordResetFlow } from "@/features/auth/password-reset-flow"; @@ -83,13 +83,10 @@ describe("the client-side length check", () => { }); describe("the local attempt budget", () => { - // Pinned as a LITERAL. The loops below use the constant, so without this the - // whole budget suite is self-fulfilling: changing 5 to 50 would keep every - // test green while the user burned 45 doomed submits. SPEC §3.1 argues the - // value must be the backend's configured 5 and not its ceiling of 10. - it("is five, the backend's configured limit", () => { - expect(MAX_CODE_ATTEMPTS).toBe(5); - }); + // The literal is pinned beside the constant's definition, in + // `use-code-timers.test.tsx`, which also pins `CODE_TTL_SECONDS`. The loops + // below use the constant, so that assertion is what stops this suite being + // self-fulfilling. it("does not give up before the budget is spent", async () => { const props = setup({ confirm: vi.fn(async () => ({ error: "password_reset_failed" })) }); @@ -218,6 +215,96 @@ describe("failures the user must be able to tell apart", () => { }); }); +describe("expiry and the resend cooldown", () => { + // Ported from the sign-in flow's suite. Both flows moved onto the shared + // `useCodeTimers`, but only the sign-in side had behavioural coverage of it — + // so a mutation disabling expiry or the cooldown stayed green here while + // failing there. + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("counts down from five minutes", async () => { + const props = setup(); + await reachCodeStep(props); + + expect(screen.getByRole("timer").textContent).toContain("5:00"); + }); + + it("disables the fields and drops the submit button once expired", async () => { + const props = setup(); + await reachCodeStep(props); + + await vi.advanceTimersByTimeAsync(300_000); + + await waitFor(() => + expect((screen.getByLabelText("Enter the 6-digit code") as HTMLInputElement).disabled).toBe(true), + ); + expect((screen.getByLabelText("New password") as HTMLInputElement).disabled).toBe(true); + expect(screen.queryByRole("button", { name: "Set new password" })).toBeNull(); + expect(screen.getByRole("alert").textContent).toMatch(/expired/i); + }); + + it("refuses a submit on an expired code", async () => { + const props = setup(); + await reachCodeStep(props); + fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { + target: { value: "123456" }, + }); + fireEvent.change(screen.getByLabelText("New password"), { target: { value: LONG_ENOUGH } }); + + await vi.advanceTimersByTimeAsync(300_000); + // The button is gone, so submit the form directly — a stray Enter keypress + // reaches the handler even when the control does not. + fireEvent.submit(screen.getByLabelText("Enter the 6-digit code").closest("form")!); + + expect(props.confirm).not.toHaveBeenCalled(); + }); + + it("blocks resend during the cooldown, then allows it", async () => { + const props = setup(); + await reachCodeStep(props); + + const resend = () => screen.getByRole("button", { name: /Request a new code/ }); + expect(resend().hasAttribute("disabled")).toBe(true); + + await vi.advanceTimersByTimeAsync(30_000); + + await waitFor(() => expect(resend().hasAttribute("disabled")).toBe(false)); + }); + + it("lets an expired code be replaced even before the cooldown ends", async () => { + // Expiry must not trap the user: with no valid code left, the only useful + // control has to stay live. + const props = setup(); + await reachCodeStep(props); + + await vi.advanceTimersByTimeAsync(300_000); + + await waitFor(() => + expect(screen.getByRole("button", { name: /Request a new code/ }).hasAttribute("disabled")).toBe(false), + ); + }); + + it("restarts the cooldown when a request fails, rather than leaving resend hot", async () => { + const props = setup({ requestCode: vi.fn(async () => ({ error: "otp_rate_limited" })) }); + + fireEvent.change(screen.getByLabelText("Reset your password"), { + target: { value: "op@example.test" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); + await waitFor(() => expect(props.requestCode).toHaveBeenCalled()); + + // Still on step one — the request failed — and a 429 answered by immediate + // retries is what caused it, so the button must not be hot. + expect(screen.getByLabelText("Reset your password")).toBeTruthy(); + }); +}); + describe("leaving the flow", () => { // Backing out and resetting a DIFFERENT address must not carry the previous // password forward: the user would submit for that account a secret they diff --git a/src/features/auth/password-reset-flow.tsx b/src/features/auth/password-reset-flow.tsx index 21b04e5..82f75a0 100644 --- a/src/features/auth/password-reset-flow.tsx +++ b/src/features/auth/password-reset-flow.tsx @@ -199,7 +199,12 @@ export function PasswordResetFlow({ ); } - const dead = timers.expired || budgetSpent; + // Only expiry can be true here. Spending the budget is always followed by + // `restart()` in the same React batch, so the flow leaves this step before a + // `budgetSpent` render can happen — the submit guard above still checks it, + // because a guard that is cheap and states the intent is worth keeping, but + // the rendering below would be dead code. + const dead = timers.expired; // A dead code makes resend the only way forward, so the cooldown is waived // rather than made to run out first. const throttled = timers.cooldown > 0 && !dead; @@ -252,7 +257,7 @@ export function PasswordResetFlow({ At least 12 characters. This signs you out of every device.

{dead ? ( - + ) : error ? ( ) : ( diff --git a/src/server/auth/__tests__/backend-token-exchange-password.test.ts b/src/server/auth/__tests__/backend-token-exchange-password.test.ts index a78b97a..43694e5 100644 --- a/src/server/auth/__tests__/backend-token-exchange-password.test.ts +++ b/src/server/auth/__tests__/backend-token-exchange-password.test.ts @@ -240,6 +240,40 @@ describe("confirmPasswordReset", () => { }); }); +describe("the request timeout covers the body read, not just the head", () => { + // `fetch` resolves on the response HEAD, so a helper that cleared its timeout + // on return would leave a stalled BODY hanging forever — which is the case + // the timeout exists for. A refactor that did exactly this passed all 1424 + // tests, because nothing covered it. + // + // No fake timers needed: the config floor is 100ms, and the assertion that + // matters is that the signal ABORTED. Without that line the test passes + // against the broken version, because the rejection then comes from shape + // validation rather than from the abort. + it("aborts a response whose body never arrives", async () => { + process.env.MAINTMODE_API_TIMEOUT_MS = "100"; + let aborted = false; + + fetchMock.mockImplementation(async (_url: string, init: { signal: AbortSignal }) => ({ + ok: true, + status: 202, + statusText: "Accepted", + text: () => + new Promise((_resolve, reject) => { + init.signal.addEventListener("abort", () => { + aborted = true; + reject(new Error("aborted")); + }); + }), + })); + + await expect(requestPasswordResetCode("op@example.test")).rejects.toThrow(); + expect(aborted).toBe(true); + + delete process.env.MAINTMODE_API_TIMEOUT_MS; + }); +}); + describe("requestPasswordResetCode", () => { it("returns the session nonce from a 202", async () => { fetchMock.mockResolvedValue(respond(202, '{"session_nonce":"nonce-1"}')); diff --git a/src/server/auth/__tests__/password-reset-actions.test.ts b/src/server/auth/__tests__/password-reset-actions.test.ts index 3ec29a1..c0856ff 100644 --- a/src/server/auth/__tests__/password-reset-actions.test.ts +++ b/src/server/auth/__tests__/password-reset-actions.test.ts @@ -252,6 +252,29 @@ describe("confirming a reset", () => { expect(clearPasswordResetBinding).not.toHaveBeenCalled(); }); + // §3.4's rule, made executable. A substring implementation would match the + // code wherever it appears — including inside `message` — and would then + // clear a binding that still had attempts left, destroying a usable code. + // Every other fixture here has field and substring agreeing, so only this + // one separates the two implementations. + it("reads the code FIELD, not the message text", async () => { + confirmPasswordReset.mockRejectedValueOnce( + backendError( + 401, + '{"code":"unauthorized","message":"authentication failed for otp_session_mismatch reasons"}', + ), + ); + + const result = await confirmPasswordResetAction({ + email: "op@example.test", + code: "123456", + newPassword: "a-long-enough-password", + }); + + expect(result).toEqual({ error: "password_reset_failed" }); + expect(clearPasswordResetBinding).not.toHaveBeenCalled(); + }); + it("keeps an outage apart from a wrong code, and keeps the binding", async () => { confirmPasswordReset.mockRejectedValueOnce(backendError(502));