From 975c8ddb23393add6865d2de81a4ddbf7fd8eb5c Mon Sep 17 00:00:00 2001 From: Brian Cooper Date: Wed, 8 Jul 2026 16:21:15 -0700 Subject: [PATCH] fix(auth): stop refresh-token rotation race from logging users out Concurrent SSR requests for one session each refreshed the single-use OAuth token independently; all but the winner presented a just-rotated token, and getAuth force-signed-out the still-valid session on the first failure, logging users out in bursts on every app re-open. Single-flight the refresh per session so a page-load burst triggers one rotation instead of many, and degrade instead of destroy on a refresh failure: serve the session without a fresh access token and let the natural 401 -> re-auth path recover a genuinely dead session rather than punishing a benign rotation race. --- src/auth/getAuth.test.ts | 94 ++++++++++++++++++++++++-- src/auth/getAuth.ts | 140 +++++++++++++++++++++++---------------- 2 files changed, 173 insertions(+), 61 deletions(-) diff --git a/src/auth/getAuth.test.ts b/src/auth/getAuth.test.ts index 52b3997..d43657c 100644 --- a/src/auth/getAuth.test.ts +++ b/src/auth/getAuth.test.ts @@ -28,17 +28,30 @@ const makeConfig = (overrides: { idTokenClaims?: Record; /** Claims returned by the mocked OIDC userinfo fetch */ userInfoClaims?: Record; + /** Override the mocked Better Auth getAccessToken behavior */ + getAccessTokenImpl?: () => Promise<{ + accessToken?: string; + idToken?: string | null; + } | null>; + /** Override the mocked Better Auth refreshToken behavior */ + refreshTokenImpl?: () => Promise<{ + accessToken?: string; + idToken?: string | null; + } | null>; }) => { const setCookieCalls: Array<{ name: string; value: string }> = []; const encryptCalls: CachedAuthData[] = []; const authApi = { getSession: mock(async () => overrides.session), - getAccessToken: mock(async () => ({ - accessToken: "access-token", - idToken: "id-token", - })), - refreshToken: mock(async () => null), + getAccessToken: mock( + overrides.getAccessTokenImpl ?? + (async () => ({ + accessToken: "access-token", + idToken: "id-token", + })), + ), + refreshToken: mock(overrides.refreshTokenImpl ?? (async () => null)), signOut: mock(async () => undefined), } as unknown as BetterAuthApi; @@ -275,3 +288,74 @@ describe("createGetAuth userinfo org cache", () => { expect(fetchSpy(cfg)).toHaveBeenCalledTimes(2); }); }); + +describe("createGetAuth concurrent refresh handling", () => { + /** APIError wrapper Better Auth throws when a rotated refresh token races */ + const failedTokenError = () => + Object.assign(new Error("Failed to get a valid access token"), { + body: { code: "FAILED_TO_GET_ACCESS_TOKEN" }, + }); + + it("single-flights the token fetch across a concurrent request burst", async () => { + // An SSR page fires many parallel requests for the SAME session. Each + // triggers getAuth, but only ONE token fetch may hit Better Auth/Gatekeeper. + // Firing N refreshes is exactly what races refresh-token rotation and logs + // users out, so concurrent getAuth calls for one session must share a fetch. + let resolveGate: () => void = () => {}; + const gate = new Promise((r) => { + resolveGate = r; + }); + + const cfg = makeConfig({ + session: makeSession({ identityProviderId: "idp-sub-123" }), + getAccessTokenImpl: async () => { + await gate; + return { accessToken: "access-token", idToken: "id-token" }; + }, + }); + + const getAuth = createGetAuth(cfg); + const inflight = [getAuth(request), getAuth(request), getAuth(request)]; + // let all three reach the shared token fetch before it settles + await Promise.resolve(); + resolveGate(); + const results = await Promise.all(inflight); + + expect(results.every((r) => r?.accessToken === "access-token")).toBe(true); + const getAccessToken = ( + cfg.authApi as unknown as { getAccessToken: ReturnType } + ).getAccessToken; + expect(getAccessToken).toHaveBeenCalledTimes(1); + }); + + it("degrades instead of signing out when a refresh loses the rotation race", async () => { + // The BA session itself is valid (getSession succeeded); only the OAuth + // token refresh failed because a sibling request already rotated it. This + // must NOT destroy the session: return it token-less and let the natural + // 401 -> re-auth path recover, rather than force-logging-out a live user. + const cfg = makeConfig({ + session: makeSession({ identityProviderId: "idp-sub-123" }), + getAccessTokenImpl: async () => { + throw failedTokenError(); + }, + refreshTokenImpl: async () => { + throw failedTokenError(); + }, + }); + + const getAuth = createGetAuth(cfg); + const result = await getAuth(request); + + // session preserved, just without a fresh access token + expect(result).not.toBeNull(); + expect(result?.accessToken).toBeUndefined(); + expect(result?.user.identityProviderId).toBe("idp-sub-123"); + // never tears the session down on a single transient failure + const signOut = ( + cfg.authApi as unknown as { signOut: ReturnType } + ).signOut; + expect(signOut).not.toHaveBeenCalled(); + // and never clears the auth cache cookie + expect(cfg.setCookieCalls.some((c) => c.value === "")).toBe(false); + }); +}); diff --git a/src/auth/getAuth.ts b/src/auth/getAuth.ts index e64538a..2e6f2ff 100644 --- a/src/auth/getAuth.ts +++ b/src/auth/getAuth.ts @@ -3,6 +3,7 @@ import { ensureFreshAccessToken, isInvalidGrant } from "./token"; import type { AuthCache } from "./cache"; import type { OidcClient } from "./oidc"; +import type { TokenResult } from "./token"; import type { OrganizationClaim } from "./types"; /** @@ -136,9 +137,12 @@ type GetAuthSession = { * via `ensureFreshAccessToken`, OIDC-verified ID token decoding, * organization claim extraction, and encrypted cookie caching. * - * On `isInvalidGrant`, clears both the session and cache cookie to - * force re-authentication. On other token errors, returns the session - * with whatever organizations are cached (graceful degradation). + * The token refresh is single-flighted per session so a page-load request + * burst never races refresh-token rotation against itself. A refresh failure + * (including `isInvalidGrant`) never tears down the still-valid session: it + * degrades to a session without a fresh access token and lets the app's + * natural 401 -> re-auth path recover a genuinely dead session, rather than + * force-logging-out a user who merely lost a benign rotation race. * @param config - Auth configuration * @returns `getAuth(request)` function that resolves to session or null */ @@ -165,6 +169,15 @@ function createGetAuth(config: GetAuthConfig) { { organizations: OrganizationClaim[]; expiry: number } >(); + // Single-flight the token refresh per session. An SSR page load fires many + // parallel requests carrying the SAME session cookie; without deduping, each + // one independently refreshes the single-use OAuth token, and all but the + // winner present a just-rotated token -> the IDP treats it as reuse and the + // user is logged out in a burst. Sharing one in-flight refresh per session + // collapses the burst to a single rotation. Keyed by the BA session token; + // entries are removed as soon as the refresh settles. + const refreshInFlight = new Map>(); + return async function getAuth( request: Request, ): Promise { @@ -194,53 +207,67 @@ function createGetAuth(config: GetAuthConfig) { // resolve. Organizations are intentionally not part of the cache. const hasCachedData = !!identityProviderId; - // Get tokens from Gatekeeper via Better Auth + // Get tokens from Gatekeeper via Better Auth. Concurrent requests for the + // same session share one refresh (single-flight) so a page-load burst + // never races refresh-token rotation against itself. + const sessionKey = + (session.session?.token as string | undefined) ?? session.user.id; try { - const tokenResult = await ensureFreshAccessToken({ - getAccessToken: async () => { - try { - const result = await authApi.getAccessToken({ - body: { providerId }, - headers: request.headers, - }); + let inFlight = refreshInFlight.get(sessionKey); + if (!inFlight) { + inFlight = ensureFreshAccessToken({ + getAccessToken: async () => { + try { + const result = await authApi.getAccessToken({ + body: { providerId }, + headers: request.headers, + }); + + if (!result?.accessToken) { + console.warn( + `${logPrefix} getAccessToken returned no token`, + { + hasResult: !!result, + }, + ); + } - if (!result?.accessToken) { - console.warn(`${logPrefix} getAccessToken returned no token`, { - hasResult: !!result, + return result; + } catch (err) { + const body = + err && typeof err === "object" && "body" in err + ? (err as { body: unknown }).body + : undefined; + console.error(`${logPrefix} getAccessToken failed:`, { + code: + body && typeof body === "object" && "code" in body + ? (body as { code: string }).code + : "unknown", + message: err instanceof Error ? err.message : String(err), }); + throw err; } - - return result; - } catch (err) { - const body = - err && typeof err === "object" && "body" in err - ? (err as { body: unknown }).body - : undefined; - console.error(`${logPrefix} getAccessToken failed:`, { - code: - body && typeof body === "object" && "code" in body - ? (body as { code: string }).code - : "unknown", - message: err instanceof Error ? err.message : String(err), - }); - throw err; - } - }, - refreshToken: async () => { - try { - return await authApi.refreshToken({ - body: { providerId }, - headers: request.headers, - }); - } catch (err) { - console.error( - `${logPrefix} refreshToken failed:`, - err instanceof Error ? err.message : String(err), - ); - throw err; - } - }, - }); + }, + refreshToken: async () => { + try { + return await authApi.refreshToken({ + body: { providerId }, + headers: request.headers, + }); + } catch (err) { + console.error( + `${logPrefix} refreshToken failed:`, + err instanceof Error ? err.message : String(err), + ); + throw err; + } + }, + }).finally(() => { + refreshInFlight.delete(sessionKey); + }); + refreshInFlight.set(sessionKey, inFlight); + } + const tokenResult = await inFlight; accessToken = tokenResult?.accessToken; if (!accessToken) { @@ -364,19 +391,20 @@ function createGetAuth(config: GetAuthConfig) { }); } } catch (err) { - console.error(`${logPrefix} Token fetch error:`, err); - + // A failed token refresh must NOT tear down a still-valid session. The + // BA session (getSession above) is intact; the refresh most often fails + // only because a concurrent SSR request already rotated the single-use + // refresh token. Signing out here logged users out in bursts on every + // app re-open. Degrade instead: serve the session without a fresh + // access token. A genuinely dead session self-heals on the next request + // via the app's 401 -> re-auth redirect, without punishing the benign + // rotation race. if (isInvalidGrant(err)) { console.warn( - `${logPrefix} Stale OAuth tokens, clearing session for re-auth`, + `${logPrefix} Token refresh failed (likely a rotation race); serving session without a fresh access token`, ); - try { - await authApi.signOut({ headers: request.headers }); - } catch { - // Sign-out may fail if session is already corrupt - } - setCookie(authCache.cookieName, "", { maxAge: 0, path: "/" }); - return null; + } else { + console.error(`${logPrefix} Token fetch error:`, err); } }