From 38e4b32cd5e0ac246e80dae28bdc0cf0a3e0cf94 Mon Sep 17 00:00:00 2001 From: mforce Date: Tue, 1 Sep 2026 23:25:49 -0700 Subject: [PATCH 1/4] fix(web): make login take the cross-tab cookie lock so a racing refresh cannot restore the wrong session sessionGeneration and the access-token store are per-tab module state, so a login in Tab B is invisible to a refresh already in flight in Tab A: Tab A's completion sees its own untouched counter, looks uncontested, and #393's stale branch never fires. Real network arrival order then decides whose Set-Cookie survives, so reloading Tab B can restore the previous user's session. login now runs inside AUTH_COOKIE_LOCK with a pre-send generation check, so any in-flight refresh settles - cookie applied - before login sends a byte, and a logout that lands while the login is queued makes it inert before credentials go out. Login keeps the refresh timeout because it is replayable, unlike a password change. That alone would trade the cross-tab hole for a same-tab timing bug: the synchronous getAccessToken peek a parked request makes when its own refresh returns StaleSessionError assumed the superseding login had already committed. Queued behind the same lock, it has not. A loginInFlight handle now lets supersededToken, apiFetch and apiGetBlob await that login instead of peeking once and 401ing the user out of a session about to exist. Awaiting it from inside a held lock deadlocks - changePassword's nested refresh owns the lock the login is queued behind - so that caller skips the wait and keeps failing closed, which is what it must do on supersession anyway. Tests: three new cases pin ordering, the parked-request wait, and the logout-drops-a-queued-login path; ten existing session-generation and cross-tab cases were restructured to release the parked gate before awaiting login. Six mutants killed on their named assertions, baseline green both sides. Closes #438 --- web/src/api/client.test.ts | 155 +++++++++++++++++++++++++++++++++---- web/src/api/client.ts | 100 +++++++++++++++++++----- 2 files changed, 224 insertions(+), 31 deletions(-) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 2ad045d85..298493eba 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -1319,6 +1319,71 @@ describe("cross-tab refresh coordination (#169)", () => { expect(getAccessToken()).toBe(claimfulToken("at2")); }); + // #438 — login did NOT take this lock. sessionGeneration and the token store + // are per-tab module state, so a login in Tab B is invisible to a refresh + // already in flight in Tab A: Tab A's completion sees its own untouched + // counter, looks uncontested, and its Set-Cookie can land AFTER Tab B's + // login's. Reloading Tab B then restores the PREVIOUS user. Ordering is the + // only fix a page-owned lock can give: make login queue behind any in-flight + // refresh so login's Set-Cookie is provably the last write. + it("login waits for another tab's in-flight refresh before sending, so its Set-Cookie is the last write (#438)", async () => { + const locks = fakeLockManager(); + vi.stubGlobal("navigator", { locks }); + + // Another tab is mid-refresh: its rotated cookie has not arrived yet. + const otherTab = deferred(); + locks.request("cluckwork.auth.refresh", () => otherTab.promise); + + fetchMock.mockImplementation(async (url: string) => + url.endsWith("/auth/login") + ? accessResponse(claimfulToken("login-b")) + : jsonResponse({ ok: true }), + ); + + const loggingIn = login({ farmCode: "default-farm", email: "b@b.co", password: "pw" }); + await drain(); + + // Blocked behind the other tab: login has NOT reached the server yet, so + // its Set-Cookie cannot be overtaken by the refresh already in the air. + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); + + otherTab.resolve(); + await loggingIn; + + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(1); + // The SHARED lock name — a per-tab name would serialize nothing cross-tab. + expect(sutLockName(locks)).toBe("cluckwork.auth.refresh"); + expect(getAccessToken()).toBe(claimfulToken("login-b")); + }); + + // #438 — the pre-send generation check that comes with joining the lock. A + // login can now sit queued for as long as another tab's refresh takes, so a + // logout in between must make it inert BEFORE the credentials are sent, the + // way an already-queued password change is. + it("drops a queued login before any request when logout supersedes it (#438)", async () => { + const locks = fakeLockManager(); + vi.stubGlobal("navigator", { locks }); + + const otherTab = deferred(); + locks.request("cluckwork.auth.refresh", () => otherTab.promise); + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/logout")) return new Response(null, { status: 204 }); + if (url.endsWith("/auth/login")) return accessResponse(claimfulToken("post-logout-login")); + throw new Error(`unexpected fetch: ${url}`); + }); + + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }) + .catch((err: unknown) => err); + await drain(); + await logout(); + otherTab.resolve(); + await loggingIn; + + expect(getAccessToken()).toBeNull(); + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); + expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(1); + }); + it("serializes password change after an in-flight refresh so stale credentials cannot win last", async () => { const locks = fakeLockManager(); vi.stubGlobal("navigator", { locks }); @@ -1397,8 +1462,13 @@ describe("cross-tab refresh coordination (#169)", () => { const changing = changePassword({ currentPassword: "a", newPassword: "b" }) .catch((err: unknown) => err); await drain(); - await login({ farmCode: "default-farm", email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + // #438 — login now queues behind the in-flight auth-cookie operation, so + // it is started (bumping the session generation synchronously) and awaited + // AFTER the parked operation is released, not before. + const loggingIn = login({ farmCode: "default-farm", email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + await drain(); otherTab.resolve(); + await loggingIn; await changing; expect(getAccessToken()).toBe(claimfulToken("new-login-token")); @@ -1422,8 +1492,10 @@ describe("cross-tab refresh coordination (#169)", () => { // The form is already inside the cookie lock, obtaining an access token. // A newer login must prevent that old form from borrowing the new bearer // when the now-stale refresh finishes. - await login({ farmCode: "default-farm", email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + const loggingIn = login({ farmCode: "default-farm", email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + await drain(); refreshGate.resolve(accessResponse(claimfulToken("stale-refresh-token"))); + await loggingIn; await changing; expect(getAccessToken()).toBe(claimfulToken("new-login-token")); @@ -1452,8 +1524,10 @@ describe("cross-tab refresh coordination (#169)", () => { // Supersede the form while its first request is already on the wire, then // make that old request answer 401. Generic apiFetch behavior would refresh // the newer session and resend the old form body against it. - await login({ farmCode: "default-farm", email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + const loggingIn = login({ farmCode: "default-farm", email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + await drain(); firstChange.resolve(jsonResponse({ title: "Unauthorized" }, 401)); + await loggingIn; await changing; expect(getAccessToken()).toBe(claimfulToken("new-login-token")); @@ -1704,11 +1778,17 @@ describe("session generation (#310)", () => { const restoring = restoreSession(); // older bootstrap refresh in flight await drain(); - await login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // a newer, explicit login completes first - expect(getAccessToken()).toBe(claimfulToken("newLoginToken")); - expect(onTokens).toHaveBeenCalledTimes(1); + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // a newer, explicit login + await drain(); + // #438 — the newer login no longer overtakes the bootstrap refresh on the + // wire; it queues behind it, which is exactly what makes its own + // Set-Cookie the last one the browser applies. + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); refreshGate.resolve(accessResponse(claimfulToken("staleBootstrapToken"))); // the old refresh answers late + await loggingIn; + expect(getAccessToken()).toBe(claimfulToken("newLoginToken")); + expect(onTokens).toHaveBeenCalledTimes(1); await expect(restoring).resolves.toBe(false); // discarded, not adopted expect(getAccessToken()).toBe(claimfulToken("newLoginToken")); // untouched by the stale refresh @@ -1733,10 +1813,12 @@ describe("session generation (#310)", () => { const fetching = apiGet<{ ok: boolean }>("/stock").catch((e: unknown) => e); await drain(); - await login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // supersedes the still-parked refresh - expect(getAccessToken()).toBe("freshLoginToken"); + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // supersedes the still-parked refresh + await drain(); refreshGate.resolve(jsonResponse({ title: "refresh token revoked" }, 401)); // stale refresh FAILS late + await loggingIn; + expect(getAccessToken()).toBe("freshLoginToken"); const settled = await fetching; expect(getAccessToken()).toBe("freshLoginToken"); // untouched by the stale failure @@ -1748,6 +1830,43 @@ describe("session generation (#310)", () => { expect(settled).toEqual({ ok: true }); }); + // #438 — the second half of the fix. Once login queues behind the same lock, + // it no longer completes "essentially immediately", so the synchronous + // getAccessToken() peek a parked request makes when its own refresh comes + // back StaleSessionError can run BEFORE the login that superseded it has + // committed anything. The parked request must AWAIT the in-flight login, not + // peek once and 401 the user out of a session that is about to exist. + it("a request parked on a superseded refresh awaits the QUEUED login's token instead of 401ing (#438)", async () => { + clearAccessToken(); + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/login")) return Promise.resolve(accessResponse(claimfulToken("queued-login"))); + if (url.endsWith("/stock")) return Promise.resolve(jsonResponse({ ok: true })); + throw new Error(`unexpected fetch: ${url}`); + }); + + // Parked on a silent refresh, which now holds the auth cookie lock. + const fetching = apiGet<{ ok: boolean }>("/stock").catch((e: unknown) => e); + await drain(); + + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); + await drain(); + // Precondition: the login is QUEUED behind that refresh, not sent. This is + // the state the peek was never written for. + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); + expect(getAccessToken()).toBeNull(); + + refreshGate.resolve(jsonResponse({ title: "refresh token revoked" }, 401)); + await loggingIn; + + expect(getAccessToken()).toBe(claimfulToken("queued-login")); + // The parked request rides the newer login rather than surfacing a 401 for + // a session that was superseded, not ended. + expect(await fetching).toEqual({ ok: true }); + expect(onUnauth).not.toHaveBeenCalled(); + }); + // #310 review — changePassword is a token-store writer too, and logout is // reachable from every screen, so it needs the same generation guard as // login/refresh; without it the response writes a fresh token back over a @@ -1793,8 +1912,10 @@ describe("session generation (#310)", () => { .catch((e: unknown) => e); await drain(); - await login({ farmCode: "farm-c", email: "c@example.test", password: "pw" }); + const loggingIn = login({ farmCode: "farm-c", email: "c@example.test", password: "pw" }); + await drain(); changeGate.resolve(accessResponse(jwtWithAccountId("acct-A", "changed"))); + await loggingIn; await changing; const revokes = callsTo(fetchMock, "/auth/logout"); @@ -1857,8 +1978,10 @@ describe("session generation (#310)", () => { .catch((e: unknown) => e); await drain(); - await login({ farmCode: "farm-c", email: "c@example.test", password: "pw" }); + const loggingIn = login({ farmCode: "farm-c", email: "c@example.test", password: "pw" }); + await drain(); loginGate.resolve(accessResponse(jwtWithAccountId("acct-B", "stale-login"))); + await loggingIn; await staleLogin; const revokes = callsTo(fetchMock, "/auth/logout"); @@ -1935,8 +2058,10 @@ describe("session generation (#310)", () => { const fetching = apiGet<{ value: number }>("/stock"); await drain(); - await login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); + await drain(); refreshGate.resolve(accessResponse(claimfulToken("stale-token"))); + await loggingIn; expect(await fetching).toEqual({ value: 7 }); }); @@ -1977,8 +2102,10 @@ describe("session generation (#310)", () => { const downloading = apiGetBlob("/export/all"); await drain(); - await login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // supersedes the parked refresh + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // supersedes the parked refresh + await drain(); refreshGate.resolve(accessResponse(claimfulToken("stale-token"))); + await loggingIn; const { blob } = await downloading; expect(blob.size).toBe(4); // "data" — served on the live login's token @@ -2030,8 +2157,10 @@ describe("session generation (#310)", () => { const restoring = restoreSession(); await drain(); - await login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // supersedes the parked refresh + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }); // supersedes the parked refresh + await drain(); refreshGate.resolve(accessResponse(jwtWithAccountId("acct-A", "stale-refresh"))); + await loggingIn; await restoring; expect(getAccessToken()).toBe(jwtWithAccountId("acct-C", "new-login")); // the live login survives diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 4782f5eaf..7824b4147 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -222,25 +222,71 @@ async function revokeSupersededCookie(discardedAccessToken: string): Promise { - // #310 — bump first: login is a NEWER session than anything already in - // flight (a bootstrap refresh, say), so its resolution must be the one that - // wins, and any earlier flight must see its captured generation go stale. +// #438 — the settlement handle of the login currently in flight, or null. A +// request parked on a refresh that came back superseded cannot simply peek at +// the token store any more: since login joins the cross-tab cookie lock below, +// the login that superseded that refresh may still be QUEUED behind it and +// therefore hold nothing yet. The parked caller awaits this instead, the same +// way concurrent 401s share refreshInFlight. +let loginInFlight: Promise | null = null; + +// #438 — resolve once the in-flight login (if any) has settled. Its FAILURE is +// the login caller's to report; a parked request only needs to know the login +// is no longer pending before it decides whether a session exists. +function settledLogin(): Promise { + return loginInFlight ? loginInFlight.catch(() => undefined) : Promise.resolve(); +} + +export function login(body: LoginRequest): Promise { + // #310 — bump first, and SYNCHRONOUSLY, before this login queues for the + // lock: login is a NEWER session than anything already in flight (a + // bootstrap refresh, say), so its resolution must be the one that wins, and + // any earlier flight must see its captured generation go stale. const generation = ++sessionGeneration; + // #438 — published before the first await, so a request that parks while + // this login is still queued can find it. + const flight = performLogin(body, generation).finally(() => { + if (loginInFlight === flight) loginInFlight = null; + }); + loginInFlight = flight; + return flight; +} + +// #438 — login takes the SAME cross-tab cookie lock as refresh/changePassword. +// sessionGeneration and the token store are per-tab module state, so a refresh +// in another tab cannot see this login's generation bump: its completion looks +// uncontested there and #393's stale branch never fires, leaving real network +// arrival order to decide whose Set-Cookie survives. Ordering is the fix a +// page-owned lock can actually give — any in-flight refresh settles, cookie +// applied, before this login sends a byte, so login's cookie is provably last. +async function performLogin(body: LoginRequest, generation: number): Promise { // The server sets the HttpOnly refresh cookie; the body returns only the // access token, which lives in memory for this tab's lifetime. - const res = await raw( - "/auth/login", - { - method: "POST", - body: JSON.stringify(body), - }, - // #532 — login's response token is decoded to bind this tab to its farm. - // The REAL server token always carries the account_id claim (the - // cross-farm guard depends on it), so this is an attribution of a known - // token, not adoption of an unknown one — unlike a refresh, which must - // fail closed on an unattributable token. - "", + const res = await withAuthCookieLock( + (signal) => + // Superseded while queued (a logout, or a newer login on another form) — + // never send the credentials at all, exactly as changePassword does. + sessionGeneration !== generation + ? Promise.reject(new StaleSessionError()) + : raw( + "/auth/login", + { + method: "POST", + body: JSON.stringify(body), + signal, + }, + // #532 — login's response token is decoded to bind this tab to + // its farm. The REAL server token always carries the account_id + // claim (the cross-farm guard depends on it), so this is an + // attribution of a known token, not adoption of an unknown one — + // unlike a refresh, which must fail closed on an unattributable + // token. + "", + ), + // Login is replayable (the user simply signs in again), so it must not hold + // the cross-tab lock indefinitely on a hung request, unlike a password + // change. Same cap as refresh. + REFRESH_TIMEOUT_MS, ); // Superseded while in flight (e.g. a logout landed before this resolved) — // do not resurrect a session the user already ended. The response already @@ -756,6 +802,9 @@ export async function apiGetBlob( } catch (refreshErr) { // #310 review — same supersession handling as apiFetch. if (refreshErr instanceof StaleSessionError) { + // #438 — as in apiFetch: await the queued login before concluding + // there is no session to download on. + await settledLogin(); const fresh = getAccessToken(); if (fresh) return await rawBlob(path, fresh); throw err; @@ -815,6 +864,9 @@ export async function apiFetch( // Never let the internal marker reach the caller. if (refreshErr instanceof StaleSessionError) { if (heldAuthCookieLock) throw refreshErr; + // #438 — same wait as supersededToken: the login that superseded this + // refresh can still be queued behind it. + await settledLogin(); const fresh = getAccessToken(); if (fresh) return await raw(path, init, fresh); throw err; @@ -842,7 +894,7 @@ async function currentAccessToken( // superseded session: a newer login already committed its own token, or a // logout already tore everything down. Either way, this failure must not // touch the CURRENT session — no clearing, no onUnauthenticated navigate. - if (err instanceof StaleSessionError) return supersededToken(); + if (err instanceof StaleSessionError) return await supersededToken(heldAuthCookieLock); onUnauthenticated?.(err instanceof ApiError ? err.title : undefined); throw new ApiError(401, "NoSession", "Not authenticated."); } @@ -856,7 +908,19 @@ async function currentAccessToken( // here: it is an internal control marker, and every screen renders a non- // ApiError's `.message` verbatim, so letting it escape would show the user // "Discarded: superseded by..." in place of a real error. -function supersededToken(): string { +async function supersededToken( + heldAuthCookieLock?: HeldAuthCookieLock, +): Promise { + // #438 — the superseding login may still be QUEUED behind the very refresh + // that just failed, so a synchronous peek here would see an empty store and + // 401 the user out of a session that is about to exist. Wait for it first. + // + // NEVER wait when the caller HOLDS the auth cookie lock: that login is queued + // BEHIND the lock this caller owns, so waiting for it here deadlocks both. + // The only such caller is the non-replayable password change, which must fail + // closed on supersession anyway — apiFetch re-checks the generation the + // moment this returns and throws rather than borrowing the newer session. + if (!heldAuthCookieLock) await settledLogin(); const fresh = getAccessToken(); if (fresh) return fresh; throw new ApiError(401, "NoSession", "Not authenticated."); From 2603a739e93cc9b5524a01b5d24dcc1214a96dc4 Mon Sep 17 00:00:00 2001 From: mforce Date: Tue, 1 Sep 2026 23:52:45 -0700 Subject: [PATCH 2/4] fix(web): bound a queued login and re-read the login handle after every await Three review findings on the cookie-lock change, from CodeRabbit and an independent review pass. settledLogin snapshotted the in-flight login once. Logins replace one another, so a caller could resume when the login it happened to observe settled while a newer one was still queued, read an empty token store, and 401 - the same premature 401 the helper exists to prevent, one layer out. It now loops on identity: re-read after the await, compare the promise, never truthiness. A rejection settles a flight exactly as a success does and is exactly as stale, so the loop treats both alike. The lock's own timeout starts when a turn arrives, so it capped login's request and not its wait for the turn. changePassword holds that same globally named lock deliberately unbounded, so one stalled password change parked every later sign-in, in this tab and in any other tab on any farm, with no timeout and no error. The clock now starts at the call, and a turn arriving past it sends nothing: an abandoned sign-in must never put credentials on the wire or a cookie in the jar afterwards. Three tests: only the newest of several queued logins reaches the server, a parked request gets a plain 401 when the login it waited for is rejected, and a login behind an unbounded password change is bounded and then stays silent. Reported honestly rather than papered over: the two-login interleaving CodeRabbit described is NOT reachable, because the pre-send generation check drops a superseded login before it can produce a settlement anyone could read. Both that loop and the identity guard on the handle's clear survive mutation today. They are defensive, and the test above pins the check that makes them so. --- web/src/api/client.test.ts | 135 +++++++++++++++++++++++++++++++++++++ web/src/api/client.ts | 62 +++++++++++++++-- 2 files changed, 190 insertions(+), 7 deletions(-) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 298493eba..f74b0feee 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -1535,6 +1535,59 @@ describe("cross-tab refresh coordination (#169)", () => { expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(0); }); + // #648 review — `withAuthCookieLock`'s timeout starts when the turn arrives, + // so it bounds the request and not the wait for the turn. changePassword holds + // this lock deliberately unbounded, and the lock name is global, so before + // this bound a stalled password change parked every later sign-in — in this + // tab and in any other tab on any farm — with no timeout and no error. + it("bounds a login waiting for its turn behind an unbounded password change, and the abandoned login never sends (#438)", async () => { + vi.useFakeTimers(); + try { + const changeGate = deferred(); + const tokenBefore = `tok-${crypto.randomUUID()}`; + setAccessToken(tokenBefore); + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/change-password")) return changeGate.promise; + if (url.endsWith("/auth/login")) return accessResponse(claimfulToken("late-login")); + if (url.endsWith("/auth/logout")) return new Response(null, { status: 204 }); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(1); + + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }) + .catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); // queued behind it + + // Past the cap, with the password change still hung: the caller is freed + // rather than left waiting on a request that may never settle. + await vi.advanceTimersByTimeAsync(15_000); + const settled = await loggingIn; + expect(settled).toBeInstanceOf(DOMException); + expect((settled as DOMException).name).toBe("AbortError"); + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); + + // And the turn it eventually gets sends nothing: an abandoned sign-in must + // never put credentials on the wire or a cookie in the jar afterwards. + changeGate.resolve(accessResponse(claimfulToken("changed"))); + await changing; + await vi.advanceTimersByTimeAsync(0); + expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); + // The password change is discarded rather than committed: the abandoned + // login still bumped the session generation before it queued, and #310 + // discards a token-store write whose generation moved under it. Failing + // closed is the right side to err on here — the user re-signs in — but it + // is a real consequence of the bump happening before the queue, not after. + expect(getAccessToken()).toBe(tokenBefore); + } finally { + vi.useRealTimers(); + } + }); + it("does not apply the refresh timeout to a non-replayable password change", async () => { vi.useFakeTimers(); try { @@ -1867,6 +1920,88 @@ describe("session generation (#310)", () => { expect(onUnauth).not.toHaveBeenCalled(); }); + // #648 review — CodeRabbit raised that `settledLogin()` snapshotted + // `loginInFlight` once, so an older login settling as stale could release a + // parked caller while a newer login was still queued. The helper now loops on + // identity instead, which is cheap and correct. But the scenario it describes + // is NOT reachable as described, and this test is why: the pre-send generation + // check means a login superseded while queued never sends and never produces a + // settlement a parked caller could mistake for an answer. At most one login is + // ever on the wire. If that check is ever relaxed, the loop stops being + // defensive and starts being load-bearing. + it("only the newest of several queued logins ever puts credentials on the wire (#438)", async () => { + clearAccessToken(); + const refreshGate = deferred(); + const sent: string[] = []; + fetchMock.mockImplementation((url: string, init: RequestInit) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/login")) { + sent.push(JSON.parse(String(init.body ?? "{}")).email); + return Promise.resolve(accessResponse(claimfulToken("newest-login"))); + } + if (url.endsWith("/stock")) return Promise.resolve(jsonResponse({ ok: true })); + throw new Error(`unexpected fetch: ${url}`); + }); + + // A silent refresh holds the auth cookie lock; both sign-ins queue behind it. + const fetching = apiGet<{ ok: boolean }>("/stock").catch((e: unknown) => e); + await drain(); + const supersededLogin = login({ farmCode: "farm-a", email: "first@b.co", password: "pw" }) + .catch((e: unknown) => e); + await drain(); + const newestLogin = login({ farmCode: "farm-a", email: "second@b.co", password: "pw" }) + .catch((e: unknown) => e); + await drain(); + + refreshGate.resolve(jsonResponse({ title: "refresh token revoked" }, 401)); + await supersededLogin; + await newestLogin; + + // The superseded one never reached the server: its credentials are not on + // the wire, and it left no settled response for a parked caller to read. + expect(sent).toEqual(["second@b.co"]); + expect(getAccessToken()).toBe(claimfulToken("newest-login")); + // And the parked request still rides the login that actually committed. + expect(await fetching).toEqual({ ok: true }); + expect(authOf(callsTo(fetchMock, "/stock").at(-1)!)).toBe( + `Bearer ${claimfulToken("newest-login")}`, + ); + }); + + // The failure twin of the wait: a rejection settles the flight exactly as a + // success does, so the parked caller must re-read the token store rather than + // treat the rejection as its own answer — and must not hang on it either. + it("surfaces a plain 401 when the login a parked request waited for is REJECTED (#438)", async () => { + clearAccessToken(); + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/login")) + return Promise.resolve(jsonResponse({ title: "Invalid email or password" }, 401)); + if (url.endsWith("/stock")) return Promise.resolve(jsonResponse({ ok: true })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const fetching = apiGet<{ ok: boolean }>("/stock").catch((e: unknown) => e); + await drain(); + const failedLogin = login({ farmCode: "farm-a", email: "a@b.co", password: "wrong" }) + .catch((e: unknown) => e); + await drain(); + + refreshGate.resolve(jsonResponse({ title: "refresh token revoked" }, 401)); + const rejected = await failedLogin; + + // The credential failure belongs to the sign-in form, unchanged. + expect(rejected).toBeInstanceOf(ApiError); + expect(rejected).toMatchObject({ status: 401, title: "Invalid email or password" }); + // The parked request gets the honest answer — no session exists — and never + // the login's credential error or the internal discard marker. + const settled = await fetching; + expect(settled).toBeInstanceOf(ApiError); + expect(settled).toMatchObject({ status: 401, title: "NoSession" }); + expect(getAccessToken()).toBeNull(); + }); + // #310 review — changePassword is a token-store writer too, and logout is // reachable from every screen, so it needs the same generation guard as // login/refresh; without it the response writes a fresh token back over a diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 7824b4147..3e4eff76d 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -230,11 +230,29 @@ async function revokeSupersededCookie(discardedAccessToken: string): Promise | null = null; -// #438 — resolve once the in-flight login (if any) has settled. Its FAILURE is -// the login caller's to report; a parked request only needs to know the login -// is no longer pending before it decides whether a session exists. -function settledLogin(): Promise { - return loginInFlight ? loginInFlight.catch(() => undefined) : Promise.resolve(); +// #438 — resolve once there is no login still pending. Its FAILURE is the login +// caller's to report; a parked request only needs to know no login is on its way +// before it decides whether a session exists. +// +// #648 review — LOOP ON IDENTITY, not on a single snapshot. A snapshot returns +// as soon as the login it happened to observe settles, and logins replace one +// another: with A pending, B replacing it, and A then settling as stale, the +// parked caller would resume while B is still queued, read no token, and 401 — +// the same premature 401 this function exists to prevent, one layer out. Each +// pass consumes exactly one settled flight, so this terminates; re-reading after +// the await and comparing IDENTITY (never truthiness) is the same rule the +// generation check follows. The identity guard on `loginInFlight`'s clear below +// is what makes the re-read meaningful: without it a stale settle nulls a newer +// flight's handle and this loop exits early on the `!flight` branch. +async function settledLogin(): Promise { + for (;;) { + const flight = loginInFlight; + if (!flight) return; + // A rejection is as much a settlement as a success, and just as stale: the + // caller must re-read rather than conclude anything from it. + await flight.catch(() => undefined); + if (loginInFlight === flight) return; + } } export function login(body: LoginRequest): Promise { @@ -260,15 +278,39 @@ export function login(body: LoginRequest): Promise { // page-owned lock can actually give — any in-flight refresh settles, cookie // applied, before this login sends a byte, so login's cookie is provably last. async function performLogin(body: LoginRequest, generation: number): Promise { + // #648 review — `withAuthCookieLock`'s own timeout starts when this login's + // TURN arrives, so it caps the request and not the wait for the turn. That was + // enough while every holder of this lock was itself bounded, but + // changePassword holds it deliberately unbounded (it cannot be replayed after + // an ambiguous commit), and the lock name is global — one stalled password + // change would otherwise park a sign-in forever, including one in another tab + // on another farm. So the clock starts HERE, at the call, and a turn that + // arrives past it sends nothing: an abandoned login must never put credentials + // on the wire or a cookie in the jar behind the user's back. + let abandoned = false; + let abandon!: () => void; + const abandonedAtDeadline = new Promise((_, reject) => { + abandon = () => { + abandoned = true; + reject( + new DOMException("Timed out waiting for the auth cookie lock.", "AbortError"), + ); + }; + }); + const deadline = setTimeout(abandon, REFRESH_TIMEOUT_MS); // The server sets the HttpOnly refresh cookie; the body returns only the // access token, which lives in memory for this tab's lifetime. - const res = await withAuthCookieLock( + const queued = withAuthCookieLock( (signal) => // Superseded while queued (a logout, or a newer login on another form) — // never send the credentials at all, exactly as changePassword does. sessionGeneration !== generation ? Promise.reject(new StaleSessionError()) - : raw( + : abandoned + ? Promise.reject( + new DOMException("Abandoned before its turn on the auth cookie lock.", "AbortError"), + ) + : raw( "/auth/login", { method: "POST", @@ -288,6 +330,12 @@ async function performLogin(body: LoginRequest, generation: number): Promise undefined); + const res = await Promise.race([queued, abandonedAtDeadline]).finally(() => + clearTimeout(deadline), + ); // Superseded while in flight (e.g. a logout landed before this resolved) — // do not resurrect a session the user already ended. The response already // set a refresh cookie, so revoke it too, not just the in-memory token. From 3203d3167c4eac8831ea803b1cce052f3a627578 Mon Sep 17 00:00:00 2001 From: mforce Date: Tue, 1 Sep 2026 23:57:40 -0700 Subject: [PATCH 3/4] test(e2e): restate the login-vs-bootstrap-refresh race for the cookie lock The spec held a bootstrap refresh open and then awaited the login response. That is unreachable now: the login queues on the same cross-tab cookie lock, so awaiting it before releasing the hold deadlocks the spec against the ordering it exists to assert. It timed out at 45s on this branch's first CI run, which is how the contract change was caught. The release now happens immediately after the click, and the ordering the test is named for is asserted directly from recorded auth traffic: the held refresh's response is observed before the login request is issued. That is the #438 guarantee in a real browser with the real Web Locks API, and it is stronger evidence than the old release point, which only implied it. The durable half changed with it. Before, the two Set-Cookie writes raced and the revoke could land after the login's cookie, so the safe documented outcome was a forced fresh sign-in. The ordering is now decided rather than raced: the stale refresh settles, is discarded and revoked inside the lock, all before the login is allowed to send, so the login's cookie is written last and the reload restores the session the user signed into. The security half is unchanged and still asserted - the restored session is the Sales one, never the Owner one the stale refresh carried. Verified by build-time source mutation, which is what this guarantee needs: the suite's own mutants are network interceptions and cannot reach a client-side ordering property. With login's lock join removed and the image rebuilt, the spec dies on its named assertion - "the login request went out before the held bootstrap refresh settled". Restored, rebuilt (never --no-build, which re-runs the mutant), full smoke green at 41 passed 1 skipped. --- .../simulation/ui/specs/session-races.spec.ts | 118 +++++++++++------- 1 file changed, 70 insertions(+), 48 deletions(-) diff --git a/tools/simulation/ui/specs/session-races.spec.ts b/tools/simulation/ui/specs/session-races.spec.ts index a07509aea..77845a98a 100644 --- a/tools/simulation/ui/specs/session-races.spec.ts +++ b/tools/simulation/ui/specs/session-races.spec.ts @@ -246,7 +246,17 @@ test.describe("#310 session races", () => { ).toHaveURL(/\/login/); }); - test("an explicit login beats a pending bootstrap refresh, and the newer login wins", async ({ + // #438 CHANGED WHAT "BEATS" MEANS HERE, and the change is the point of this + // test now. `login()` used to be issued immediately, overtaking a bootstrap + // refresh already in the air, and which `Set-Cookie` the browser kept was + // then real network arrival order — unobservable from JS, and cross-tab + // invisible (the other tab's generation counter is a different module + // instance). Login now QUEUES on the same cross-tab cookie lock as refresh, + // so the pending refresh settles — cookie applied — before the login request + // is sent, and the login's cookie is provably written last. The user-visible + // outcome is unchanged and still asserted below: the newer login's session is + // the one on screen. + test("an explicit login queues behind a pending bootstrap refresh, and the newer login still wins (#438)", async ({ page, signIn, nav, @@ -290,41 +300,46 @@ test.describe("#310 session races", () => { // sessions are then distinguishable on screen, so the assertion can name // which one won instead of inferring it. const sales = castMember("Sales"); + // #438 — the ordering evidence. Recording the auth traffic in arrival order + // lets the assertion below say WHICH came first without a sleep: an + // absence-of-request check needs a window to be absent in, and a fixed + // window is exactly the timer this file's header refuses. + const authTraffic: string[] = []; + page.on("request", (r) => { + if (r.url().includes("/api/v1/auth/login")) authTraffic.push("login:request"); + }); + page.on("response", (r) => { + if (r.url().includes("/api/v1/auth/refresh")) authTraffic.push("refresh:response"); + }); await page.getByLabel(tEn("auth:farmCode")).fill("default-farm"); await page.getByLabel(tEn("auth:email")).fill(sales.email); await page.getByLabel(tEn("auth:password")).fill(sales.password); const loginLanded = page.waitForResponse( (r) => r.url().includes("/api/v1/auth/login") && r.request().method() === "POST", ); - // RELEASE THE INSTANT THE LOGIN RESPONSE LANDS — before any assertion, and - // in a `finally`. Both halves are load-bearing, and each fixes a separate - // defect found in round 3's own first attempt: + // #438 — RELEASE WITHOUT WAITING FOR THE LOGIN RESPONSE. It cannot land + // while this refresh is held: the login now queues on the same cross-tab + // cookie lock, so awaiting it before releasing deadlocks the spec against + // the very ordering it is asserting. (That deadlock is how this contract + // change was caught — the pre-#438 form of this test timed out at 45s on + // the first CI run of the fix.) The ordering the test is named for is + // asserted directly from the recorded traffic instead of being implied by + // the release point. + // + // Still in a `finally`, and still BEFORE the nav assertion, for the two + // reasons round 3 of #390 established: // // * BEFORE the nav assertion, and this is REQUIRED, not a preference. // Nothing on the login path cancels the held bootstrap refresh, so while // it is held AuthContext's `isLoading` stays set and `Login.tsx` never // navigates — the nav simply is not there to assert on. Releasing here - // is what lets the shell render at all. - // - // (Precisely, on the timing: the refresh does eventually self-abort on - // `REFRESH_TIMEOUT_MS = 15_000`, so `isLoading` is not stuck forever — - // but the `expect` timeout is 10s, so the assertion loses that race. An - // earlier comment here said `isLoading` "never clears", which is wrong - // and is the kind of detail that gets re-derived incorrectly.) - // - // The first version released three lines later, after asserting the - // shell had rendered. Against a build carrying the since-reverted login - // abort, that ordering made a mutant die on the nav timeout rather than - // on the guarantee, and "the mutant failed" was read as "the guarantee - // is covered" — a red for the wrong reason, which is exactly what the - // mutation harness exists to refuse (PR #390 review rounds 3 and 4). + // is what lets the shell render at all. (On the timing: the refresh does + // eventually self-abort on `REFRESH_TIMEOUT_MS = 15_000`, so `isLoading` + // is not stuck forever — but the `expect` timeout is 10s, so the + // assertion loses that race.) // * In a `finally`. The route handler awaits this promise and nothing else // resolves it, so an assertion throwing before the release strands the // interception and buries the real error under a timeout. - // - // Releasing here still satisfies the ordering the test is named for: the - // login response has landed, so its `Set-Cookie` is already applied, and the - // freed refresh can only write on top of it. // Registered BEFORE the click (so before the release in the finally // below): the freed stale refresh settles, and the client's reaction to // discarding it is the unconditional #393/#433 cookie revoke — a POST to @@ -341,10 +356,22 @@ test.describe("#310 session races", () => { ); try { await page.getByRole("button", { name: tEn("auth:signIn") }).click(); - await loginLanded; } finally { releaseHeldRefresh(); } + await loginLanded; + + // #438 THE ORDERING GUARANTEE, in a real browser with the real Web Locks + // API: the held refresh's response was observed BEFORE the login request + // was issued. Without the lock, login is sent immediately on click and this + // reverses — which is the whole cross-tab hazard, since the other tab's + // refresh would then be free to write its cookie after this login's. + expect(authTraffic, "no login request was observed at all").toContain("login:request"); + expect(authTraffic, "the held refresh never settled").toContain("refresh:response"); + expect( + authTraffic.indexOf("refresh:response"), + "the login request went out before the held bootstrap refresh settled — login is not queueing on the cross-tab cookie lock (#438)", + ).toBeLessThan(authTraffic.indexOf("login:request")); await expect(page.getByRole("navigation", { name: tEn("nav:primaryNavAriaLabel") })) .toBeVisible(); @@ -371,33 +398,28 @@ test.describe("#310 session races", () => { await expect(nav.link("nav:sales")).toBeVisible(); await expect(nav.link("nav:customers")).toBeVisible(); - // Reload to pin the DURABLE half of the contract — which CHANGED in - // #393/#433. A discarded stale flight's cookie is now revoked - // UNCONDITIONALLY: network arrival order decides which Set-Cookie the - // browser kept, JS cannot observe it, and the HttpOnly cookie can't be - // read back — so the revoke deliberately accepts catching the newer - // session's own cookie rather than ever leaving the PREVIOUS user's - // cookie live (the wrong-session hole #393 closed). In the race this - // test manufactures, that worst case is certain: the freed Owner - // refresh is always discarded as stale, so the jar's cookie — whichever - // of the two it is — is dead by the time of this reload. + // The DURABLE half of the contract, and #438 CHANGED IT — for the better. // - // The documented outcome (Help → Signing in, multi-tab resync; #455) is - // a forced fresh sign-in, and that is the SAFE outcome this asserts: the - // reload restores NOBODY — not the old Owner session, and not a Sales - // session riding a cookie the client no longer trusts. Before #433 this - // block asserted the Sales session survived the reload; that assertion - // described the pre-#433 contract and broke the day the fix merged - // (silently — this suite is workflow_dispatch-only, the #370 trap). + // Before, the two Set-Cookie writes genuinely raced. JS cannot observe + // which one the browser kept and cannot read the HttpOnly cookie back, so + // #393/#433 revoked the discarded flight's cookie UNCONDITIONALLY, accepting + // that it might catch the newer session's own. In this manufactured race + // that worst case was certain, and the documented outcome was a forced + // fresh sign-in: the reload restored nobody. + // + // Now the ordering is decided rather than raced. The freed bootstrap refresh + // settles, is discarded as stale, and its revoke completes INSIDE the cookie + // lock — all before the queued login is allowed to send. The login's + // Set-Cookie is therefore written last and is the only live one, so the + // reload restores the session the user actually signed into. + // + // The security guarantee is unchanged and still asserted: the restored + // session is Sales's, never the Owner's that the stale refresh carried. await page.reload(); - await expect(page.getByLabel(tEn("auth:email"))).toBeVisible(); - - // And the forced re-auth is a plain re-auth, not a wedged account: - // Sales signs straight back in and lands in a Sales-shaped shell. - await page.getByLabel(tEn("auth:farmCode")).fill("default-farm"); - await page.getByLabel(tEn("auth:email")).fill(sales.email); - await page.getByLabel(tEn("auth:password")).fill(sales.password); - await page.getByRole("button", { name: tEn("auth:signIn") }).click(); + await expect( + page.getByLabel(tEn("auth:email")), + "the reload stranded the user at sign-in — the login's cookie did not win", + ).toBeHidden(); await expect(nav.link("nav:sales")).toBeVisible(); await expect(nav.link("nav:audit")).toBeHidden(); }); From 913dbd679ba03446b112042c5c80af6ef18f72cb Mon Sep 17 00:00:00 2001 From: mforce Date: Wed, 2 Sep 2026 00:11:47 -0700 Subject: [PATCH 4/4] fix(web): let an abandoned login give its session generation back Round-2 review, and the first finding is a real user-visible defect rather than a hardening nicety. login() bumps the session generation eagerly, which is what makes the newest sign-in win. A login that is abandoned at the queue deadline never sends, so it has superseded nothing - but the bump stayed, and an in-flight password change settling afterwards saw its generation move and discarded itself: the server had already committed the new password, the freshly issued cookie was revoked, and the user was told the change failed. "Fails closed" is the right instinct for a write that might not have landed and the wrong one for a write that has. The PR text called that trade-off acceptable and a test pinned it as intended behaviour, which is how a defect becomes a specification; both are corrected. The rollback is conditional and both conditions carry their own test. Nothing may have bumped after us, so the restored value is exactly what earlier flights captured; and no refresh may be in flight, because one that had captured our value would be discarded by the rollback and have its own valid cookie revoked - trading this bug for a forced re-auth. The E2E release now synchronises on the pending Web Lock entry rather than on the click. Locator.click() resolves when the click is dispatched, not when the page's fetch is issued, so releasing on the click let a lock-bypass build win the ordering race by luck - the assertion could pass against the defect it exists to catch. The suite asks navigator.locks.query() instead: while the bootstrap refresh HOLDS the shared name, a queued sign-in is a PENDING entry on it. The wait is best-effort so a bypass build dies on the ordering assertion rather than on this wait's timeout. Re-verified by build-time source mutation with that synchronisation in place. The first attempt killed the mutant on "the held refresh never settled" - a red for arrival rather than for order, and not the assertion the test is named for. Both events are now awaited before their order is judged, and the mutant dies on the named message. --- .../simulation/ui/specs/session-races.spec.ts | 39 +++++++++++- web/src/api/client.test.ts | 63 +++++++++++++++++-- web/src/api/client.ts | 17 +++++ 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/tools/simulation/ui/specs/session-races.spec.ts b/tools/simulation/ui/specs/session-races.spec.ts index 77845a98a..3396678ac 100644 --- a/tools/simulation/ui/specs/session-races.spec.ts +++ b/tools/simulation/ui/specs/session-races.spec.ts @@ -356,10 +356,44 @@ test.describe("#310 session races", () => { ); try { await page.getByRole("button", { name: tEn("auth:signIn") }).click(); + // #648 review — SYNCHRONISE ON THE PENDING LOCK, NOT ON THE CLICK. + // `Locator.click()` resolves when the click is dispatched; it does not + // wait for the page's own fetch, still less for a request deliberately + // not being sent yet. Releasing on the click alone lets a lock-bypass + // build win the ordering race by luck on a fast machine, so the assertion + // below would pass against the very defect it exists to catch. The real + // Web Locks API answers this directly: while the bootstrap refresh HOLDS + // the shared name, a queued sign-in is a PENDING entry on it. + // + // Best-effort on purpose. Against a bypass build no pending entry ever + // appears, and this wait must not be what fails — that would kill the + // mutant on a timeout instead of on the ordering assertion, a red for the + // wrong reason, which is the mistake PR #390 round 3 made here already. + await page + .waitForFunction( + async () => { + const snapshot = await navigator.locks.query(); + const held = snapshot.held ?? []; + const pending = snapshot.pending ?? []; + return ( + held.some((lock) => lock.name === "cluckwork.auth.refresh") + && pending.some((lock) => lock.name === "cluckwork.auth.refresh") + ); + }, + undefined, + { timeout: 5_000 }, + ) + .catch(() => undefined); } finally { releaseHeldRefresh(); } await loginLanded; + // #648 review — wait for BOTH events before judging their order. Against a + // lock-bypass build the login lands first and the freed refresh answers + // after it, so asserting immediately kills the mutant on "the held refresh + // never settled" — a red, but for arrival rather than for ORDER, which is + // the guarantee under test. + await staleRefreshSettled; // #438 THE ORDERING GUARANTEE, in a real browser with the real Web Locks // API: the held refresh's response was observed BEFORE the login request @@ -376,9 +410,8 @@ test.describe("#310 session races", () => { await expect(page.getByRole("navigation", { name: tEn("nav:primaryNavAriaLabel") })) .toBeVisible(); - // The freed refresh has arrived, been discarded as stale, and its cookie - // revoke has completed — everything the reload's outcome depends on. - await staleRefreshSettled; + // The freed refresh has been discarded as stale and its cookie revoke has + // completed — everything the reload's outcome depends on. await revokeSettled; // THE GUARANTEE: this is the Sales session, and the Owner's has not been diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index f74b0feee..78a8c8d81 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -1577,12 +1577,16 @@ describe("cross-tab refresh coordination (#169)", () => { await changing; await vi.advanceTimersByTimeAsync(0); expect(callsTo(fetchMock, "/auth/login")).toHaveLength(0); - // The password change is discarded rather than committed: the abandoned - // login still bumped the session generation before it queued, and #310 - // discards a token-store write whose generation moved under it. Failing - // closed is the right side to err on here — the user re-signs in — but it - // is a real consequence of the bump happening before the queue, not after. - expect(getAccessToken()).toBe(tokenBefore); + // #648 review — AND THE PASSWORD CHANGE STILL COMMITS. The abandoned login + // gives its generation back, because a login that never sent has not + // superseded anything. Leaving the bump in place discarded a response the + // server had already committed: the password was changed, the freshly + // issued cookie was revoked, and the user was told it failed. "Fails + // closed" is the right instinct for a write that might not have landed, + // and the wrong one for a write that has. + expect(getAccessToken()).toBe(claimfulToken("changed")); + expect(tokenBefore).not.toBe(claimfulToken("changed")); + expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(0); // nothing revoked } finally { vi.useRealTimers(); } @@ -1689,6 +1693,53 @@ describe("cross-tab refresh coordination (#169)", () => { expect(getAccessToken()).toBeNull(); }); + // #648 review — the other half of the rollback: it is conditional, and this + // pins the condition. A refresh started while the login waited may have + // captured the login's generation, so rolling back would discard that + // refresh's own valid result and revoke its cookie — trading a discarded + // password change for a forced re-auth. With a refresh in flight the + // conservative behaviour stands. + it("does not give the generation back when a refresh may have captured it (#438)", async () => { + vi.useFakeTimers(); + try { + const changeGate = deferred(); + const refreshGate = deferred(); + setAccessToken(`tok-${crypto.randomUUID()}`); + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/change-password")) return changeGate.promise; + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/logout")) return new Response(null, { status: 204 }); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + + const loggingIn = login({ farmCode: "default-farm", email: "a@b.co", password: "pw" }) + .catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + // A refresh enters the queue behind the login and captures the CURRENT + // generation — the one the login's bump produced. + const restoring = restoreSession().catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + + await vi.advanceTimersByTimeAsync(15_000); // the login is abandoned + expect(await loggingIn).toBeInstanceOf(DOMException); + + changeGate.resolve(accessResponse(claimfulToken("changed"))); + await changing; + refreshGate.resolve(accessResponse(claimfulToken("refreshed"))); + await restoring; + + // The refresh keeps the generation it captured, so it is not discarded + // out from under itself by the rollback. + expect(getAccessToken()).toBe(claimfulToken("refreshed")); + } finally { + vi.useRealTimers(); + } + }); + it("a lock held under a DIFFERENT name does not block refresh (fake is name-scoped, like the real API)", async () => { const locks = fakeLockManager(); vi.stubGlobal("navigator", { locks }); diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 3e4eff76d..12c0d84ab 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -292,6 +292,23 @@ async function performLogin(body: LoginRequest, generation: number): Promise((_, reject) => { abandon = () => { abandoned = true; + // #648 review — GIVE THE GENERATION BACK. The eager bump in `login()` is + // what makes the newest sign-in win, but a login that never sends has not + // superseded anything: leaving the bump in place makes an in-flight + // password change discard a response the SERVER ALREADY COMMITTED, revoke + // the cookie it just issued, and report failure to a user whose password + // did change. Failing closed is the right instinct for a write that might + // not have landed; this one has landed. + // + // Two conditions, and both are load-bearing. `sessionGeneration === + // generation` means nothing bumped after us, so this rollback restores + // exactly the value every earlier flight captured. `refreshInFlight === + // null` means no refresh can have captured OUR value while we waited — + // one that had would be discarded by the rollback and have its own valid + // cookie revoked, trading this bug for a forced re-auth. When either + // fails we keep the conservative behaviour rather than guess. + if (sessionGeneration === generation && refreshInFlight === null) + sessionGeneration = generation - 1; reject( new DOMException("Timed out waiting for the auth cookie lock.", "AbortError"), );