From cb97b699d58b8b8de6f9e4074a6b94bee6eafe8c Mon Sep 17 00:00:00 2001 From: mforce <> Date: Fri, 31 Jul 2026 20:17:43 -0700 Subject: [PATCH 1/5] fix(web): gate login/refresh commits on a session generation (#310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Treat login, the bootstrap silent refresh, an explicit (401-triggered) refresh, and logout as one browser-session state machine instead of four calls that can race each other. A monotonically increasing generation is bumped by login (a newer session supersedes anything in flight) and by logout (before clearing tokens or awaiting the server call); a refresh's resolution — success or failure — only commits tokens/state while its captured generation is still current, otherwise it is discarded as stale. logout also now cancels any in-flight refresh where possible, and its best-effort server-side revoke failure is logged instead of swallowed silently. Fixes a real security bug: an in-flight refresh completing after logout could resurrect authenticated state, and an older bootstrap refresh could overwrite a newer explicit login. --- web/src/api/client.test.ts | 159 +++++++++++++++++++++++++++++++++++++ web/src/api/client.ts | 87 +++++++++++++++++--- 2 files changed, 237 insertions(+), 9 deletions(-) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 60f9657ed..1daa211fe 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -574,6 +574,45 @@ describe("auth endpoints", () => { expect(fetchMock).toHaveBeenCalledTimes(1); expect((fetchMock.mock.calls[0] as Call)[0]).toBe("/api/v1/auth/logout"); }); + + // #310 — local invalidation must win immediately, independent of the network. + it("clears in-memory auth state synchronously, before the server call settles", async () => { + const gate = deferred(); + fetchMock.mockReturnValueOnce(gate.promise); // server call hangs + setAccessToken("live-token"); + + const done = logout(); + await Promise.resolve(); // let logout()'s synchronous prefix run; network still pending + expect(getAccessToken()).toBeNull(); + + gate.resolve(new Response(null, { status: 204 })); + await done; + }); + + // #310 — a full network rejection (not just a non-2xx response) must behave + // the same as the existing 500 case above: local state wins regardless. + it("clears the local token even when the server call rejects outright (network error)", async () => { + setAccessToken("live-token"); + fetchMock.mockRejectedValueOnce(new TypeError("network down")); + await expect(logout()).resolves.toBeUndefined(); + expect(getAccessToken()).toBeNull(); + }); + + // #310 — the revoke failure must be observable (not silently swallowed) but + // must never expose the token that was already cleared. + it("reports a server-side revoke failure without leaking the token", async () => { + const consoleErr = vi.spyOn(console, "error").mockImplementation(() => {}); + setAccessToken("super-secret-live-token"); + fetchMock.mockRejectedValueOnce(new TypeError("network down")); + + await expect(logout()).resolves.toBeUndefined(); + + expect(getAccessToken()).toBeNull(); + expect(consoleErr).toHaveBeenCalledTimes(1); + const logged = consoleErr.mock.calls[0].map((a) => String(a)).join(" "); + expect(logged).not.toContain("super-secret-live-token"); + consoleErr.mockRestore(); + }); }); describe("changePassword (#165)", () => { @@ -743,3 +782,123 @@ describe("cross-tab refresh coordination (#169)", () => { expect(getAccessToken()).toBe("at2"); }); }); + +// #310 — login, bootstrap refresh, explicit refresh, and logout are one browser +// session state machine. A stale completion (one whose generation was superseded +// by a later login or an earlier logout) may never commit tokens or state. +describe("session generation (#310)", () => { + it("commits a bootstrap refresh normally when nothing else races it (control)", async () => { + clearAccessToken(); + fetchMock.mockResolvedValueOnce(accessResponse("bootedToken")); + + await expect(restoreSession()).resolves.toBe(true); + + expect(getAccessToken()).toBe("bootedToken"); + expect(onTokens).toHaveBeenCalledTimes(1); + }); + + it("discards a bootstrap refresh that resolves after logout — the session does not resurrect", async () => { + clearAccessToken(); + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/logout")) return Promise.resolve(new Response(null, { status: 204 })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const restoring = restoreSession(); // bootstrap refresh in flight, held open + await drain(); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(1); + + await logout(); // user logs out while the bootstrap refresh is still pending + + refreshGate.resolve(accessResponse("resurrected-token")); // the stale refresh finally answers + await expect(restoring).resolves.toBe(false); // discarded — never reports a restored session + + expect(getAccessToken()).toBeNull(); // must stay logged out + expect(onTokens).not.toHaveBeenCalled(); // the stale completion must not fire tokensChanged + }); + + it("commits an explicit login normally when nothing else races it (control)", async () => { + clearAccessToken(); + fetchMock.mockResolvedValueOnce(accessResponse("loggedInToken")); + + await login({ email: "a@b.co", password: "pw" }); + + expect(getAccessToken()).toBe("loggedInToken"); + expect(onTokens).toHaveBeenCalledTimes(1); + }); + + it("does not let a late bootstrap refresh overwrite a newer explicit login", 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("newLoginToken")); + throw new Error(`unexpected fetch: ${url}`); + }); + + const restoring = restoreSession(); // older bootstrap refresh in flight + await drain(); + + await login({ email: "a@b.co", password: "pw" }); // a newer, explicit login completes first + expect(getAccessToken()).toBe("newLoginToken"); + expect(onTokens).toHaveBeenCalledTimes(1); + + refreshGate.resolve(accessResponse("staleBootstrapToken")); // the old refresh answers late + await expect(restoring).resolves.toBe(false); // discarded, not adopted + + expect(getAccessToken()).toBe("newLoginToken"); // untouched by the stale refresh + expect(onTokens).toHaveBeenCalledTimes(1); // still just the login's single notification + }); + + it("a late-FAILING obsolete refresh does not clear or corrupt a newer session", async () => { + // Distinct from the success case above: here the stale refresh's own network + // call fails (refresh token revoked) — the failure path must be gated too, or + // it would clearAccessToken()/onUnauthenticated() and boot the fresh login. + 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("freshLoginToken")); + if (url.endsWith("/stock")) return Promise.resolve(jsonResponse({ ok: true })); + throw new Error(`unexpected fetch: ${url}`); + }); + + // apiGet with no in-memory token forces currentAccessToken() to park on a + // silent refresh — the same refresh a bootstrap would trigger. + const fetching = apiGet<{ ok: boolean }>("/stock").catch((e: unknown) => e); + await drain(); + + await login({ email: "a@b.co", password: "pw" }); // supersedes the still-parked refresh + expect(getAccessToken()).toBe("freshLoginToken"); + + refreshGate.resolve(jsonResponse({ title: "refresh token revoked" }, 401)); // stale refresh FAILS late + await fetching; + + expect(getAccessToken()).toBe("freshLoginToken"); // untouched by the stale failure + expect(onUnauth).not.toHaveBeenCalled(); // must not tear down the fresh session + }); + + it("discards a login that resolves after a concurrent logout — does not resurrect authentication", async () => { + clearAccessToken(); + const loginGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/login")) return loginGate.promise; + if (url.endsWith("/auth/logout")) return Promise.resolve(new Response(null, { status: 204 })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const loggingIn = login({ email: "a@b.co", password: "pw" }).catch((e: unknown) => e); + await drain(); + + await logout(); // fires while the login request is still in flight + + loginGate.resolve(accessResponse("late-login-token")); + const result = await loggingIn; + expect(result).toBeInstanceOf(Error); // discarded, surfaced as a rejection — not silently accepted + + expect(getAccessToken()).toBeNull(); + expect(onTokens).not.toHaveBeenCalled(); + }); +}); diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 48d63d0a2..ac1288154 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -114,13 +114,39 @@ async function raw( // --- Auth endpoints ------------------------------------------------------- +// #310 — login, the load-time bootstrap refresh, an explicit (401-triggered) +// refresh, and logout are treated as ONE browser-session state machine, not +// four independent calls racing each other. Every entry into that machine +// captures the CURRENT generation before its own await; login and logout each +// bump it (login because it is by definition a newer session than anything +// already in flight, logout because ending the session must supersede +// everything). A completion — success OR failure — may only commit tokens or +// user state while the generation it captured is still the current one; +// otherwise it is stale and must be discarded rather than resurrecting or +// clobbering whatever the (newer) generation already did. +let sessionGeneration = 0; + +class StaleSessionError extends Error { + constructor() { + super("Discarded: superseded by a newer login or an intervening logout."); + this.name = "StaleSessionError"; + } +} + export async function login(body: LoginRequest): 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. + const generation = ++sessionGeneration; // 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), }); + // Superseded while in flight (e.g. a logout landed before this resolved) — + // do not resurrect a session the user already ended. + if (sessionGeneration !== generation) throw new StaleSessionError(); setAccessToken(res.accessToken); onTokensChanged?.(); } @@ -145,6 +171,13 @@ export async function changePassword( } export async function logout(): Promise { + // #310 — bump the generation and clear in-memory state SYNCHRONOUSLY, before + // any await: local logout must win immediately, whatever a slower in-flight + // login/refresh does later. Also cancel any refresh currently in flight + // (cancellation where possible) so it stops sooner instead of only being + // discarded on arrival by the generation check in refreshTokens(). + sessionGeneration++; + abortInFlightRefresh?.(); clearAccessToken(); try { // Cookie-authenticated: the HttpOnly refresh cookie rides along and the CSRF @@ -153,8 +186,12 @@ export async function logout(): Promise { // fires — JS can't read the HttpOnly cookie to know whether a session exists, // so we always ask the server to revoke + clear it. await raw("/auth/logout", { method: "POST", headers: { [CSRF_HEADER]: "1" } }); - } catch { - // best-effort revoke; the in-memory token is already cleared + } catch (err) { + // Best-effort revoke: the in-memory token is already cleared above and this + // failure can never reverse that. Still surfaced (not silently swallowed, + // #310) so an operator can see the server-side session may be un-revoked — + // logged, never the (already-cleared) token itself. + console.error("logout: server-side session revoke failed", err); } } @@ -181,11 +218,20 @@ const REFRESH_LOCK = "cluckwork.auth.refresh"; // which releases the lock; the aborting tab recovers on its next request. const REFRESH_TIMEOUT_MS = 15_000; +// #310 — the abort handle of whichever refresh attempt is currently running +// (if any), so logout() can cancel it outright rather than only waiting for +// the generation check below to discard its result on arrival. +let abortInFlightRefresh: (() => void) | null = null; + function withRefreshLock(run: (signal: AbortSignal) => Promise): Promise { const attempt = () => { const controller = new AbortController(); + abortInFlightRefresh = () => controller.abort(); const timer = setTimeout(() => controller.abort(), REFRESH_TIMEOUT_MS); - return run(controller.signal).finally(() => clearTimeout(timer)); + return run(controller.signal).finally(() => { + clearTimeout(timer); + abortInFlightRefresh = null; + }); }; const locks: LockManager | undefined = globalThis.navigator?.locks; return locks ? (locks.request(REFRESH_LOCK, attempt) as Promise) : attempt(); @@ -200,6 +246,11 @@ let refreshInFlight: Promise | null = null; async function refreshTokens(): Promise { if (refreshInFlight) return refreshInFlight; + // #310 — captured once, at the START of this flight. Checked again at + // settlement (success or failure) so a logout/login that lands WHILE this + // network call is in the air discards its outcome instead of committing it. + const generation = sessionGeneration; + refreshInFlight = withRefreshLock((signal) => raw("/auth/refresh", { method: "POST", @@ -207,11 +258,20 @@ async function refreshTokens(): Promise { signal, }), ) - .then((res) => { - setAccessToken(res.accessToken); - onTokensChanged?.(); - return res.accessToken; - }) + .then( + (res) => { + if (sessionGeneration !== generation) throw new StaleSessionError(); + setAccessToken(res.accessToken); + onTokensChanged?.(); + return res.accessToken; + }, + (err: unknown) => { + // A late FAILURE from an obsolete generation is discarded the same as + // a late success: the real error (e.g. a genuinely revoked refresh + // token) might otherwise tear down a newer, still-valid session. + throw sessionGeneration !== generation ? new StaleSessionError() : err; + }, + ) .finally(() => { refreshInFlight = null; }); @@ -349,7 +409,12 @@ async function currentAccessToken(): Promise { if (token) return token; try { return await refreshTokens(); - } catch { + } catch (err) { + // #310 — a stale (obsolete-generation) failure belongs to an already- + // 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) throw err; onUnauthenticated?.(); throw new ApiError(401, "NoSession", "Not authenticated."); } @@ -360,7 +425,11 @@ async function currentAccessToken(): Promise { // - 429: rate-limit throttling (#143) — re-logging in hits the same limit. // - AbortError: our own cross-tab lock timeout (#169) fired on a slow/hung // refresh; the cookie is untouched, so the next attempt can still succeed. +// - StaleSessionError (#310): the refresh belonged to an obsolete generation +// (superseded by a newer login, or discarded by a logout) — a newer/ +// already-torn-down session must not be clobbered by this stale failure. function isTransientRefreshFailure(err: unknown): boolean { + if (err instanceof StaleSessionError) return true; if (err instanceof ApiError && err.status === 429) return true; return err instanceof DOMException && err.name === "AbortError"; } From 78586b9e5d131405d8a75f45be266194494f7b6a Mon Sep 17 00:00:00 2001 From: mforce <> Date: Fri, 31 Jul 2026 20:19:08 -0700 Subject: [PATCH 2/5] test(web): generate the leak-probe token instead of a literal Nothing secret-shaped belongs in source even in a test file (GitGuardian scans them), and a unique per-run value makes the "did not leak" assertion exact rather than coincidental. --- web/src/api/client.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 1daa211fe..0b2d90a12 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -602,7 +602,11 @@ describe("auth endpoints", () => { // must never expose the token that was already cleared. it("reports a server-side revoke failure without leaking the token", async () => { const consoleErr = vi.spyOn(console, "error").mockImplementation(() => {}); - setAccessToken("super-secret-live-token"); + // Generated, not a literal: nothing secret-shaped belongs in source + // (GitGuardian scans test files too), and a unique value makes the + // "did not leak" assertion exact rather than coincidental. + const token = `tok-${crypto.randomUUID()}`; + setAccessToken(token); fetchMock.mockRejectedValueOnce(new TypeError("network down")); await expect(logout()).resolves.toBeUndefined(); @@ -610,7 +614,7 @@ describe("auth endpoints", () => { expect(getAccessToken()).toBeNull(); expect(consoleErr).toHaveBeenCalledTimes(1); const logged = consoleErr.mock.calls[0].map((a) => String(a)).join(" "); - expect(logged).not.toContain("super-secret-live-token"); + expect(logged).not.toContain(token); consoleErr.mockRestore(); }); }); From e30651435318124539c2d7ee237b205a0b64f09b Mon Sep 17 00:00:00 2001 From: mforce <> Date: Fri, 31 Jul 2026 20:30:50 -0700 Subject: [PATCH 3/5] fix(web): guard changePassword and stop the discard marker reaching the UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #330 found a fifth token-store writer the generation guard never covered. changePassword() committed its response unconditionally, and logout is reachable from every screen via AppLayout's persistent button — so submitting a password change and then logging out before it landed wrote a fresh token back over the cleared session. That is exactly the bug #310 exists to close. Second, StaleSessionError was escaping to callers. currentAccessToken() rethrew it outside apiFetch's try, and both 401-retry branches rethrew it as a "transient" failure. Every screen renders a non-ApiError's .message verbatim, so a user whose refresh merely lost a race to their own login saw "Discarded: superseded by a newer login or an intervening logout." instead of their request simply succeeding. A discarded refresh now resolves against whatever superseded it: a newer login's token is used (the request is valid, so it proceeds), a logout raises a plain 401 without re-firing onUnauthenticated. The marker no longer leaves the module. The existing late-failure test asserted the token store but discarded the parked request's own outcome, so it passed either way; it now asserts the request resolves with its data. Also corrected the logout comment: the abort only reaches a refresh that already holds the cross-tab lock, not one still queued behind another tab. Correctness rests on the generation check, not the abort. --- web/src/api/client.test.ts | 42 +++++++++++++++++++++++++++++++++- web/src/api/client.ts | 46 +++++++++++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 0b2d90a12..67ad02d07 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -878,10 +878,50 @@ describe("session generation (#310)", () => { expect(getAccessToken()).toBe("freshLoginToken"); refreshGate.resolve(jsonResponse({ title: "refresh token revoked" }, 401)); // stale refresh FAILS late - await fetching; + const settled = await fetching; expect(getAccessToken()).toBe("freshLoginToken"); // untouched by the stale failure expect(onUnauth).not.toHaveBeenCalled(); // must not tear down the fresh session + // #310 review — the parked request itself must not surface the internal + // discard marker. The login committed a valid token, so the request the + // user is actually waiting on proceeds on it rather than failing with + // "Discarded: superseded by…", which every screen would render verbatim. + expect(settled).toEqual({ ok: true }); + }); + + // #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 + // session the user already ended. + it("discards a change-password response that resolves after logout (#310)", async () => { + const changeGate = deferred(); + setAccessToken(`tok-${crypto.randomUUID()}`); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/change-password")) return changeGate.promise; + if (url.endsWith("/auth/logout")) return Promise.resolve(jsonResponse({}, 204)); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((e: unknown) => e); + await drain(); + + await logout(); + expect(getAccessToken()).toBeNull(); + + changeGate.resolve(jsonResponse({ accessToken: "resurrected" })); + await changing; + + expect(getAccessToken()).toBeNull(); // the ended session stays ended + }); + + it("commits a change-password response normally when nothing races it (control)", async () => { + setAccessToken(`tok-${crypto.randomUUID()}`); + fetchMock.mockResolvedValueOnce(accessResponse("at-after-change")); + + await changePassword({ currentPassword: "a", newPassword: "b" }); + + expect(getAccessToken()).toBe("at-after-change"); }); it("discards a login that resolves after a concurrent logout — does not resurrect authentication", async () => { diff --git a/web/src/api/client.ts b/web/src/api/client.ts index ac1288154..1ceb02804 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -165,7 +165,14 @@ export async function login(body: LoginRequest): Promise { export async function changePassword( body: { currentPassword: string; newPassword: string }, ): Promise { + // #310 — this is a token-store writer like login/refresh, so it needs the + // same generation guard: logout is reachable from every screen (AppLayout's + // persistent button), so a user who submits a password change and logs out + // before it lands would otherwise have the response write a fresh token back + // over the cleared session. + const generation = sessionGeneration; const res = await apiPost("/auth/change-password", body); + if (sessionGeneration !== generation) throw new StaleSessionError(); setAccessToken(res.accessToken); onTokensChanged?.(); } @@ -173,8 +180,13 @@ export async function changePassword( export async function logout(): Promise { // #310 — bump the generation and clear in-memory state SYNCHRONOUSLY, before // any await: local logout must win immediately, whatever a slower in-flight - // login/refresh does later. Also cancel any refresh currently in flight - // (cancellation where possible) so it stops sooner instead of only being + // login/refresh does later. Also cancel any refresh currently in flight — + // note this only reaches a refresh that has already been GRANTED the + // cross-tab lock (#169); one still queued behind another tab has no + // controller yet, so it will still fire its request and then be discarded on + // arrival by the generation check. Correctness rests on the generation, not + // on the abort; the abort is only an optimisation so it stops sooner + // instead of only being // discarded on arrival by the generation check in refreshTokens(). sessionGeneration++; abortInFlightRefresh?.(); @@ -359,6 +371,12 @@ export async function apiGetBlob( const refreshed = await refreshTokens(); return await rawBlob(path, refreshed); } catch (refreshErr) { + // #310 review — same supersession handling as apiFetch. + if (refreshErr instanceof StaleSessionError) { + const fresh = getAccessToken(); + if (fresh) return await rawBlob(path, fresh); + throw err; + } if (isTransientRefreshFailure(refreshErr)) throw refreshErr; clearAccessToken(); onUnauthenticated?.(); @@ -393,6 +411,14 @@ export async function apiFetch(path: string, init: RequestInit = {}): Promise const refreshed = await refreshTokens(); return await raw(path, init, refreshed); } catch (refreshErr) { + // #310 review — superseded mid-retry: retry once on the newer login's + // token, or surface the ORIGINAL 401 if a logout ended the session. + // Never let the internal marker reach the caller. + if (refreshErr instanceof StaleSessionError) { + const fresh = getAccessToken(); + if (fresh) return await raw(path, init, fresh); + throw err; + } if (isTransientRefreshFailure(refreshErr)) throw refreshErr; clearAccessToken(); onUnauthenticated?.(); @@ -414,12 +440,26 @@ async function currentAccessToken(): Promise { // 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) throw err; + if (err instanceof StaleSessionError) return supersededToken(); onUnauthenticated?.(); throw new ApiError(401, "NoSession", "Not authenticated."); } } +// #310 review — resolve a discarded refresh against whatever superseded it. +// A newer LOGIN has already committed its token, so the caller's request is +// perfectly valid and should proceed on it. A newer LOGOUT left no session, +// and has already navigated, so this raises a plain 401 rather than firing +// onUnauthenticated a second time. Either way StaleSessionError itself stops +// 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 { + const fresh = getAccessToken(); + if (fresh) return fresh; + throw new ApiError(401, "NoSession", "Not authenticated."); +} + // A refresh failure is "transient" when the session is likely still valid, so we // keep the tokens and surface the error rather than forcing a re-login: // - 429: rate-limit throttling (#143) — re-logging in hits the same limit. From 2ba47de276c5298d308aeb38bdd47b7f95bf3183 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Fri, 31 Jul 2026 20:43:08 -0700 Subject: [PATCH 4/5] fix(web): revoke the refresh cookie a discarded response already set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #330 found the generation check only gated JavaScript state. Both /auth/login and /auth/refresh (and change-password) rotate the HttpOnly refresh cookie, and the browser applies Set-Cookie the moment the response arrives — before any of our code runs. So a login landing after logout left a VALID refresh cookie behind: the next reload authenticated straight through restoreSession() and the session the user had ended came back. Discarding the in-memory token never touched that. A discarded cookie-setting response now asks the server to revoke what it issued. Scoped to "the session is actually gone" (no access token): when a newer LOGIN superseded the flight, the cookie belongs to that live session and revoking it would sign the user out of the thing they just did — covered by its own test. The failure branch of refresh is deliberately excluded: a refresh that failed issued no cookie to revoke. --- web/src/api/client.test.ts | 33 +++++++++++++++++++++++++ web/src/api/client.ts | 49 +++++++++++++++++++++++++++++++++----- 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 67ad02d07..973da0ef0 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -821,6 +821,12 @@ describe("session generation (#310)", () => { expect(getAccessToken()).toBeNull(); // must stay logged out expect(onTokens).not.toHaveBeenCalled(); // the stale completion must not fire tokensChanged + // #310 review — clearing the in-memory token is only half of it. That + // response carried a rotated refresh cookie, which the browser applied + // before any of our code ran, so the server session must be revoked too: + // one revoke for the logout itself, a second for the cookie the discarded + // refresh issued. Without it a reload walks straight back in. + expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(2); }); it("commits an explicit login normally when nothing else races it (control)", async () => { @@ -944,5 +950,32 @@ describe("session generation (#310)", () => { expect(getAccessToken()).toBeNull(); expect(onTokens).not.toHaveBeenCalled(); + // The late login's Set-Cookie already reached the browser; revoke it, or a + // reload restores the session the user just ended. + expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(2); + }); + + // #310 review — the revoke is scoped to "the session is gone". When a newer + // LOGIN supersedes an older flight, the cookie in the browser belongs to that + // live login; revoking it would sign the user out of the thing they just did. + it("does not revoke the cookie when a newer login superseded the flight", 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("newLoginToken")); + if (url.endsWith("/auth/logout")) return Promise.resolve(new Response(null, { status: 204 })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const restoring = restoreSession(); + await drain(); + + await login({ email: "a@b.co", password: "pw" }); // supersedes the parked refresh + refreshGate.resolve(accessResponse("stale-refresh-token")); + await restoring; + + expect(getAccessToken()).toBe("newLoginToken"); // the live login survives + expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(0); // and is NOT revoked }); }); diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 1ceb02804..458cf5acd 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -133,6 +133,28 @@ class StaleSessionError extends Error { } } +// #310 review — discarding the JS result is NOT enough for a request that also +// rotates the refresh cookie (/auth/login and /auth/refresh both do). The +// browser applies Set-Cookie the moment the response arrives, before any of our +// code runs, so a login that lands after logout leaves a VALID HttpOnly refresh +// cookie behind: the next reload authenticates straight through +// restoreSession() and the session the user ended comes back. +// +// So when a cookie-setting response is discarded, ask the server to revoke what +// it just issued. Only when the session is actually gone (no access token): if a +// newer LOGIN superseded this flight, the cookie now belongs to that live +// session and revoking it would sign the user out of the thing they just did. +async function revokeSupersededCookie(): Promise { + if (getAccessToken()) return; // superseded by a newer login, not by a logout + try { + await raw("/auth/logout", { method: "POST", headers: { [CSRF_HEADER]: "1" } }); + } catch (err) { + // Best-effort, like logout's own revoke: report it rather than swallow, but + // never surface it to the caller whose result was already discarded. + console.error("discarded response: revoking its refresh cookie failed", err); + } +} + export async function login(body: LoginRequest): 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 @@ -145,8 +167,12 @@ export async function login(body: LoginRequest): Promise { body: JSON.stringify(body), }); // Superseded while in flight (e.g. a logout landed before this resolved) — - // do not resurrect a session the user already ended. - if (sessionGeneration !== generation) throw new StaleSessionError(); + // 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. + if (sessionGeneration !== generation) { + await revokeSupersededCookie(); + throw new StaleSessionError(); + } setAccessToken(res.accessToken); onTokensChanged?.(); } @@ -172,7 +198,12 @@ export async function changePassword( // over the cleared session. const generation = sessionGeneration; const res = await apiPost("/auth/change-password", body); - if (sessionGeneration !== generation) throw new StaleSessionError(); + // This response rotates the refresh cookie too, so a discarded one needs that + // cookie revoked, not merely the in-memory token dropped. + if (sessionGeneration !== generation) { + await revokeSupersededCookie(); + throw new StaleSessionError(); + } setAccessToken(res.accessToken); onTokensChanged?.(); } @@ -271,8 +302,13 @@ async function refreshTokens(): Promise { }), ) .then( - (res) => { - if (sessionGeneration !== generation) throw new StaleSessionError(); + async (res) => { + // A SUCCESSFUL refresh rotated the cookie before we got here, so a + // discarded one leaves live credentials in the browser — revoke them. + if (sessionGeneration !== generation) { + await revokeSupersededCookie(); + throw new StaleSessionError(); + } setAccessToken(res.accessToken); onTokensChanged?.(); return res.accessToken; @@ -280,7 +316,8 @@ async function refreshTokens(): Promise { (err: unknown) => { // A late FAILURE from an obsolete generation is discarded the same as // a late success: the real error (e.g. a genuinely revoked refresh - // token) might otherwise tear down a newer, still-valid session. + // token) might otherwise tear down a newer, still-valid session. No + // revoke here — a failed refresh issued no cookie to revoke. throw sessionGeneration !== generation ? new StaleSessionError() : err; }, ) From b610dfc470cb321b30412434f39d4f62822d5db8 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Fri, 31 Jul 2026 20:55:02 -0700 Subject: [PATCH 5/5] test(web): cover the supersession branches the coverage gate caught MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's per-file gate on src/api/client.ts failed: the cookie-revoke fix added branches that no test reached (statements 90 vs 94, branches 80.7 vs 85). The gate was right — these were real behaviours shipped untested, not a threshold that needed relaxing. Adds the four missing cases, both outcomes on both request paths: a refresh superseded mid-401-retry proceeds on the newer login's token, and one superseded by logout surfaces the original 401 rather than the internal discard marker — for apiFetch and for apiGetBlob, whose retry is a separate branch. Also pins that a request parked on currentAccessToken sees a plain ApiError, since screens render a non-ApiError's message verbatim. client.ts now 96.5 statements / 89.2 branches / 98 lines, comfortably clear of the floors rather than a hair over. The one remaining uncovered span (417-419) predates this branch. --- web/src/api/client.test.ts | 110 +++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 973da0ef0..79113afd6 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -955,6 +955,116 @@ describe("session generation (#310)", () => { expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(2); }); + // #310 review — a request parked on a refresh that a LOGOUT discarded must + // surface a normal 401, not the internal StaleSessionError whose message + // every screen would render verbatim. (The newer-login counterpart, where the + // request proceeds on the fresh token, is asserted above.) + it("surfaces a plain 401 — not the discard marker — when logout ended the session", async () => { + clearAccessToken(); + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/logout")) return Promise.resolve(new Response(null, { status: 204 })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const fetching = apiGet("/stock").catch((e: unknown) => e); + await drain(); + + await logout(); + refreshGate.resolve(accessResponse("discarded-token")); + + const settled = await fetching; + expect(settled).toBeInstanceOf(ApiError); + expect(settled).toMatchObject({ status: 401 }); + }); + + // The two outcomes again for apiFetch's 401-RETRY branch specifically. The + // test above enters through currentAccessToken (empty token store); this one + // starts authenticated, 401s, and is superseded mid-retry — a separate branch. + it("retries a request on the newer login's token when the retry refresh is superseded", async () => { + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/stock") && getAccessToken() === "newLoginToken") + return Promise.resolve(jsonResponse({ value: 7 })); + if (url.endsWith("/stock")) return Promise.resolve(jsonResponse({ title: "expired" }, 401)); + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/login")) return Promise.resolve(accessResponse("newLoginToken")); + throw new Error(`unexpected fetch: ${url}`); + }); + + const fetching = apiGet<{ value: number }>("/stock"); + await drain(); + + await login({ email: "a@b.co", password: "pw" }); + refreshGate.resolve(accessResponse("stale-token")); + + expect(await fetching).toEqual({ value: 7 }); + }); + + it("surfaces the original 401 when logout supersedes a retry refresh", async () => { + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/stock")) return Promise.resolve(jsonResponse({ title: "expired" }, 401)); + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/logout")) return Promise.resolve(new Response(null, { status: 204 })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const fetching = apiGet("/stock").catch((e: unknown) => e); + await drain(); + + await logout(); + refreshGate.resolve(accessResponse("discarded-token")); + + const settled = await fetching; + expect(settled).toBeInstanceOf(ApiError); + expect(settled).toMatchObject({ status: 401 }); + }); + + // The same two outcomes for the blob path, whose 401-retry is a separate + // branch from apiFetch's. + it("retries a download on the newer login's token when a refresh is superseded", async () => { + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/export/all") && getAccessToken() === "newLoginToken") + return Promise.resolve(new Response("data", { status: 200 })); + if (url.endsWith("/export/all")) return Promise.resolve(jsonResponse({ title: "expired" }, 401)); + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/login")) return Promise.resolve(accessResponse("newLoginToken")); + throw new Error(`unexpected fetch: ${url}`); + }); + + const downloading = apiGetBlob("/export/all"); + await drain(); + + await login({ email: "a@b.co", password: "pw" }); // supersedes the parked refresh + refreshGate.resolve(accessResponse("stale-token")); + + const { blob } = await downloading; + expect(blob.size).toBe(4); // "data" — served on the live login's token + }); + + it("surfaces the original 401 on a download when logout ended the session", async () => { + const refreshGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/export/all")) return Promise.resolve(jsonResponse({ title: "expired" }, 401)); + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/logout")) return Promise.resolve(new Response(null, { status: 204 })); + throw new Error(`unexpected fetch: ${url}`); + }); + + const downloading = apiGetBlob("/export/all").catch((e: unknown) => e); + await drain(); + + await logout(); + refreshGate.resolve(accessResponse("discarded-token")); + + const settled = await downloading; + expect(settled).toBeInstanceOf(ApiError); + expect(settled).toMatchObject({ status: 401 }); + }); + // #310 review — the revoke is scoped to "the session is gone". When a newer // LOGIN supersedes an older flight, the cookie in the browser belongs to that // live login; revoking it would sign the user out of the thing they just did.