From 020fc1961bbe5ed165ef87aaafb6fab840e94822 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 01:36:46 +0300 Subject: [PATCH] chore(auth): close the password_set contract gap, fix the card's stale form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend shipped, so the debt RUK-289 left behind came due. `me.json` is re-recorded against the live backend and `password_set` is on the wire, so the gap row and its absence assertion are gone. The detector worked as designed: the assertion said the key was ABSENT, so it was green while the gap was open and went red the moment the fixture was re-recorded. That is how this got closed rather than quietly forgotten. The registry row is kept and marked closed, per the file's convention — including a note that the row's first version named a detector that could not fire. Only `me` was re-captured. A full refresh needs a maintenance in the capture window and this database has none, but the recorder already skips context resolution for endpoints with a static path, so the narrow capture is the supported path rather than a workaround. The recorder masked `password_set` into the string `""` — `SENSITIVE_KEY_RE` matches its `password` prefix. It is a boolean saying whether an account HAS a password, not a credential, and masking changed its TYPE, which would fail any test asserting the shape. Added as a narrow allowlist rather than by loosening the pattern: the rule's asymmetry is right, and a false positive is fixed by naming it. Then the live run found a real defect no test could see. `asChange` was seeded with `useState(passwordSet === true)`, which runs only on mount, and this card is never remounted — so setting a password left `password_set` flipping to `true` on the wire while the form still offered "Set password", and the next submit would omit `current_password` and earn a 400 the user did nothing to deserve. It is now derived from the prop, with the one-time flip kept as an override. Every existing test mounted the component fresh, which is exactly why they all passed; the new ones re-render with a changed prop instead, and five tests fail against the old code. Verified end to end against the local stack: reset requested, code redeemed (204), session torn down, confirmation shown, sign-in with the new password works, `password_set` flips to true, and the profile card switches to the change form with no reload. Co-Authored-By: Claude Opus 5 --- docs/contract-gaps.md | 61 ++++++++++--------- scripts/refresh-fixtures.mjs | 25 +++++++- .../settings/__tests__/password-card.test.tsx | 52 +++++++++++++++- src/features/settings/password-card.tsx | 30 ++++++--- tests/contracts/contract-gaps.test.ts | 17 ------ tests/fixtures/wire/manifest.json | 6 +- tests/fixtures/wire/me.json | 3 +- 7 files changed, 131 insertions(+), 63 deletions(-) diff --git a/docs/contract-gaps.md b/docs/contract-gaps.md index bd2b0be..0335518 100644 --- a/docs/contract-gaps.md +++ b/docs/contract-gaps.md @@ -103,35 +103,38 @@ 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. +### `password_set` on /me — CLOSED 2026-09-08 (RUK-289) + +| Field | Where it is needed | What was on the wire | Ticket | Status | +| -------------- | --------------------------------------------------- | ---------------------------------------- | ------- | ----------------------------- | +| `password_set` | profile password card, `/set-password` (RUK-289 UI) | the key was absent from the recorded 200 | RUK-289 | closed — backend now sends it | + +The backend that serves this field was written on a branch while the frontend +was built, so the recorded fixture predated it and the field arrived as +`undefined`. That branch is merged, `me.json` is re-recorded, and the key is on +the wire as a real boolean. + +**The row is kept rather than deleted, per this file's convention for a closed +gap** — a reader who finds `password_set?: boolean` optional in +`src/domain/admin/user.ts` should be able to learn why. The optionality stays +deliberately: `undefined` still means "the deployed backend predates the field", +which is a real state for any instance not yet on this version, and the UI keeps +treating it as "unknown" rather than as `false`. Collapsing the two would draw a +set-password form for every operator on such an instance, whose every save is +then a 400. + +**How it was detected, and why that mattered.** The mechanism was a mirror +assertion — `contract-gaps.test.ts` asserted the key was ABSENT from `me.json`, +so it was green while the gap was open and went red the moment the fixture was +re-recorded. It fired exactly as designed, which is how this row came to be +closed rather than quietly forgotten. Both the assertion and the gap are gone. + +The first version of this row claimed a different detector — the key-set +comparison in `me.contract.test.ts` — and that claim was wrong. That assertion +compares the route's echo against the same fixture that fed its mock: a tautology +on key sets, green whatever the fixture holds. It proves the route narrows +nothing and says nothing about which fields exist. Recorded here because a +detector that cannot fire is worse than an acknowledged absence of one. ## Class B′ — the backend sends it, the frontend does not read it diff --git a/scripts/refresh-fixtures.mjs b/scripts/refresh-fixtures.mjs index 3dd6cf8..270117e 100644 --- a/scripts/refresh-fixtures.mjs +++ b/scripts/refresh-fixtures.mjs @@ -297,6 +297,24 @@ function collectValueDomains(rows) { const SENSITIVE_KEY_RE = /(^|_)(ip|ips|user_agent|useragent|token|secret|password|passwd|authorization|auth|api_key|apikey|key|session|session_id|cookie|phone|telegram_tag|slack_tag|refresh_token|access_token)($|_)/i; +/** + * Keys that LOOK sensitive to the rule above but are not, and whose masking + * would destroy the very thing the fixture exists to pin. + * + * `password_set` is a boolean saying whether an account HAS a password. It is + * not a credential, and it is not derived from one — but it matches + * `(^|_)password($|_)` on its prefix, so it was masked into the string + * `""`, which changes its TYPE. A contract test that + * checks the field is a boolean then fails, and the fixture stops describing + * the wire. + * + * Kept as a narrow allowlist rather than a loosened pattern: the asymmetry + * argued above is right, and the fix for a false positive is to name it, not + * to make the rule leakier. Anything added here needs the same justification — + * that masking it would break a test and that the value carries no secret. + */ +const NOT_SENSITIVE_KEYS = new Set(["password_set"]); + /** * Numeric fields that are identifiers or clocks rather than data. * @@ -366,7 +384,12 @@ function normalize(value, seen = new Map(), key = "", counters = new Map()) { // Key-based masking runs FIRST and ignores value shape entirely — that is the // whole point of having it (see SENSITIVE_KEY_RE). - if (SENSITIVE_KEY_RE.test(key) && value !== null && value !== "") { + if ( + SENSITIVE_KEY_RE.test(key) && + !NOT_SENSITIVE_KEYS.has(key.toLowerCase()) && + value !== null && + value !== "" + ) { return ``; } if (VOLATILE_NUMERIC_KEY_RE.test(key) && typeof value === "number") { diff --git a/src/features/settings/__tests__/password-card.test.tsx b/src/features/settings/__tests__/password-card.test.tsx index d43c77d..71111fe 100644 --- a/src/features/settings/__tests__/password-card.test.tsx +++ b/src/features/settings/__tests__/password-card.test.tsx @@ -29,12 +29,19 @@ 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( + const view = render( , ); - return { invalidate }; + /** Re-renders with a new prop WITHOUT remounting — the case a fresh mount hides. */ + const setPasswordSet = (next: boolean | undefined) => + view.rerender( + + + , + ); + return { invalidate, setPasswordSet }; } function fill(label: string, value: string) { @@ -149,6 +156,47 @@ describe("failures the user must be able to act on", () => { }); }); +describe("the form follows password_set when it changes under the card", () => { + // Found against a live backend, invisible to every test that mounts fresh. + // `password_set` flips to `true` the moment a password is set, and this card + // is not remounted — a `useState` seed would leave it offering "Set password" + // for an account that now HAS one, and the next submit would omit + // `current_password` and earn a 400 the user did nothing to deserve. + it("switches to the change form when the prop flips after a save", () => { + const { setPasswordSet } = renderCard(false); + expect(screen.queryByLabelText("Current password")).toBeNull(); + + setPasswordSet(true); + + expect(screen.getByLabelText("Current password")).toBeTruthy(); + expect(screen.getByRole("button", { name: "Change password" })).toBeTruthy(); + }); + + it("switches back if the account loses its password", () => { + const { setPasswordSet } = renderCard(true); + expect(screen.getByLabelText("Current password")).toBeTruthy(); + + setPasswordSet(false); + + expect(screen.queryByLabelText("Current password")).toBeNull(); + }); + + // A flip earned from a 400 is a correction to what the prop claimed, so it + // must not be undone by the next render of that same stale prop. + it("keeps a flip that a 400 earned, even when the prop re-renders", async () => { + bffFetch.mockRejectedValue(new BffError(400, "validation error")); + const { setPasswordSet } = renderCard(false); + + fill("New password", LONG_ENOUGH); + fireEvent.click(screen.getByRole("button", { name: "Set password" })); + await waitFor(() => expect(screen.getByLabelText("Current password")).toBeTruthy()); + + setPasswordSet(false); + + expect(screen.getByLabelText("Current password")).toBeTruthy(); + }); +}); + 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 diff --git a/src/features/settings/password-card.tsx b/src/features/settings/password-card.tsx index bac89f4..1516430 100644 --- a/src/features/settings/password-card.tsx +++ b/src/features/settings/password-card.tsx @@ -34,23 +34,34 @@ export function PasswordCard({ passwordSet }: PasswordCardProps) { 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. + * is answered with a 400, and the form flips 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. + * + * `null` means "no flip has happened, follow the prop". + */ + const [flipTo, setFlipTo] = useState(null); + + /** + * Which shape the form is in: the prop, unless a flip has overridden it. + * + * DERIVED, not seeded into state. A `useState(passwordSet === true)` + * initializer runs only on mount, and this card is never remounted — so + * setting a password left `password_set` flipping to `true` on the wire while + * the form still offered "Set password". The next submit then omitted + * `current_password` and earned a 400 the user had done nothing to deserve. + * Caught against a live backend; no test could see it, because every test + * mounts the component fresh. */ - const [flipped, setFlipped] = useState(false); + const asChange = flipTo ?? passwordSet === true; + const flipped = flipTo !== null; if (passwordSet === undefined) { return ( @@ -99,8 +110,7 @@ export function PasswordCard({ passwordSet }: PasswordCardProps) { 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); + setFlipTo(!asChange); setCurrent(""); setError( asChange diff --git a/tests/contracts/contract-gaps.test.ts b/tests/contracts/contract-gaps.test.ts index 96b0e3a..f245b10 100644 --- a/tests/contracts/contract-gaps.test.ts +++ b/tests/contracts/contract-gaps.test.ts @@ -353,23 +353,6 @@ 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", () => { diff --git a/tests/fixtures/wire/manifest.json b/tests/fixtures/wire/manifest.json index 8366c33..bc64a8b 100644 --- a/tests/fixtures/wire/manifest.json +++ b/tests/fixtures/wire/manifest.json @@ -73,7 +73,7 @@ "method": "GET", "status": 200, "capturedAt": "2026-08-11T18:37:05.031Z", - "why": "RUK-156: built against an invented shape. Also the counterpart to the calendar gap \u2014 detail DOES carry `resources`, calendar does not.", + "why": "RUK-156: built against an invented shape. Also the counterpart to the calendar gap — detail DOES carry `resources`, calendar does not.", "handEdited": false }, "approvals": { @@ -92,7 +92,7 @@ "url": "http://localhost:9000/auth/api/v1/me", "method": "GET", "status": 200, - "capturedAt": "2026-08-11T18:37:05.031Z", + "capturedAt": "2026-09-07T22:27:19.551Z", "why": "RUK-202 added `timezone`; confirms it is really on the wire.", "handEdited": false }, @@ -101,7 +101,7 @@ "method": "GET", "status": 200, "capturedAt": "2026-08-11T18:37:05.031Z", - "why": "RUK-171: `details` is a flat string, `actor` an email \u2014 FE renders degraded.", + "why": "RUK-171: `details` is a flat string, `actor` an email — FE renders degraded.", "handEdited": false, "sampledRows": { "logs": { diff --git a/tests/fixtures/wire/me.json b/tests/fixtures/wire/me.json index 8a31eb7..b56f09d 100644 --- a/tests/fixtures/wire/me.json +++ b/tests/fixtures/wire/me.json @@ -12,5 +12,6 @@ ], "timezone": null, "telegram_tag": null, - "slack_tag": null + "slack_tag": null, + "password_set": false }