diff --git a/.changeset/fix-token-refresh-race-condition.md b/.changeset/fix-token-refresh-race-condition.md new file mode 100644 index 00000000000..ba56f1c3de4 --- /dev/null +++ b/.changeset/fix-token-refresh-race-condition.md @@ -0,0 +1,5 @@ +--- +"@clerk/clerk-js": patch +--- + +Fix race condition where multiple browser tabs could fetch session tokens simultaneously. `getToken()` now uses a cross-tab lock to coordinate token refresh operations diff --git a/packages/clerk-js/src/core/auth/AuthCookieService.ts b/packages/clerk-js/src/core/auth/AuthCookieService.ts index 51268dc6bcd..a801e096aaf 100644 --- a/packages/clerk-js/src/core/auth/AuthCookieService.ts +++ b/packages/clerk-js/src/core/auth/AuthCookieService.ts @@ -41,11 +41,11 @@ import { SessionCookiePoller } from './SessionCookiePoller'; * - handleUnauthenticatedDevBrowser(): resets dev browser in case of invalid dev browser */ export class AuthCookieService { - private poller: SessionCookiePoller | null = null; - private clientUat: ClientUatCookieHandler; - private sessionCookie: SessionCookieHandler; private activeCookie: ReturnType; + private clientUat: ClientUatCookieHandler; private devBrowser: DevBrowser; + private poller: SessionCookiePoller | null = null; + private sessionCookie: SessionCookieHandler; public static async create( clerk: Clerk, @@ -77,14 +77,14 @@ export class AuthCookieService { this.refreshTokenOnFocus(); this.startPollingForToken(); - this.clientUat = createClientUatCookie(cookieSuffix); - this.sessionCookie = createSessionCookie(cookieSuffix); this.activeCookie = createActiveContextCookie(); + this.clientUat = createClientUatCookie(cookieSuffix); this.devBrowser = createDevBrowser({ - frontendApi: clerk.frontendApi, - fapiClient, cookieSuffix, + fapiClient, + frontendApi: clerk.frontendApi, }); + this.sessionCookie = createSessionCookie(cookieSuffix); } public async setup() { diff --git a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts index 91e8040f79d..32d3f894faf 100644 --- a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts +++ b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts @@ -1,12 +1,14 @@ import { createWorkerTimers } from '@clerk/shared/workerTimers'; -import { SafeLock } from './safeLock'; - -const REFRESH_SESSION_TOKEN_LOCK_KEY = 'clerk.lock.refreshSessionToken'; const INTERVAL_IN_MS = 5 * 1_000; +/** + * Polls for session token refresh at regular intervals. + * + * Note: Cross-tab coordination is handled within Session.getToken() itself, + * so this poller simply triggers the refresh callback without additional locking. + */ export class SessionCookiePoller { - private lock = SafeLock(REFRESH_SESSION_TOKEN_LOCK_KEY); private workerTimers = createWorkerTimers(); private timerId: ReturnType | null = null; // Disallows for multiple `startPollingForSessionToken()` calls before `callback` is executed. @@ -19,7 +21,7 @@ export class SessionCookiePoller { const run = async () => { this.initiated = true; - await this.lock.acquireLockAndRun(cb); + await cb(); this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); }; diff --git a/packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts b/packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts new file mode 100644 index 00000000000..71b7dbfc022 --- /dev/null +++ b/packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts @@ -0,0 +1,142 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { SessionCookiePoller } from '../SessionCookiePoller'; + +describe('SessionCookiePoller', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + describe('startPollingForSessionToken', () => { + it('executes callback immediately on start', async () => { + const poller = new SessionCookiePoller(); + const callback = vi.fn().mockResolvedValue(undefined); + + poller.startPollingForSessionToken(callback); + + // Flush microtasks to let the async run() execute + await Promise.resolve(); + + expect(callback).toHaveBeenCalledTimes(1); + + poller.stopPollingForSessionToken(); + }); + + it('prevents multiple concurrent polling sessions', async () => { + const poller = new SessionCookiePoller(); + const callback = vi.fn().mockResolvedValue(undefined); + + poller.startPollingForSessionToken(callback); + poller.startPollingForSessionToken(callback); // Second call should be ignored + + await Promise.resolve(); + + expect(callback).toHaveBeenCalledTimes(1); + + poller.stopPollingForSessionToken(); + }); + }); + + describe('stopPollingForSessionToken', () => { + it('stops polling when called', async () => { + const poller = new SessionCookiePoller(); + const callback = vi.fn().mockResolvedValue(undefined); + + poller.startPollingForSessionToken(callback); + await Promise.resolve(); + + expect(callback).toHaveBeenCalledTimes(1); + + poller.stopPollingForSessionToken(); + + // Advance time - callback should not be called again + await vi.advanceTimersByTimeAsync(10000); + + expect(callback).toHaveBeenCalledTimes(1); + }); + + it('allows restart after stop', async () => { + const poller = new SessionCookiePoller(); + const callback = vi.fn().mockResolvedValue(undefined); + + // Start and stop + poller.startPollingForSessionToken(callback); + await Promise.resolve(); + poller.stopPollingForSessionToken(); + + expect(callback).toHaveBeenCalledTimes(1); + + // Should be able to start again + poller.startPollingForSessionToken(callback); + await Promise.resolve(); + + expect(callback).toHaveBeenCalledTimes(2); + + poller.stopPollingForSessionToken(); + }); + }); + + describe('polling interval', () => { + it('schedules next poll after callback completes', async () => { + const poller = new SessionCookiePoller(); + const callback = vi.fn().mockResolvedValue(undefined); + + poller.startPollingForSessionToken(callback); + + // Initial call + await Promise.resolve(); + expect(callback).toHaveBeenCalledTimes(1); + + // Wait for first interval (5 seconds) + await vi.advanceTimersByTimeAsync(5000); + + // Should have scheduled another call + expect(callback).toHaveBeenCalledTimes(2); + + // Another interval + await vi.advanceTimersByTimeAsync(5000); + expect(callback).toHaveBeenCalledTimes(3); + + poller.stopPollingForSessionToken(); + }); + + it('waits for callback to complete before scheduling next poll', async () => { + const poller = new SessionCookiePoller(); + + let resolveCallback: () => void; + const callbackPromise = new Promise(resolve => { + resolveCallback = resolve; + }); + const callback = vi.fn().mockReturnValue(callbackPromise); + + poller.startPollingForSessionToken(callback); + + // Let the first call start + await Promise.resolve(); + expect(callback).toHaveBeenCalledTimes(1); + + // Advance time while callback is still running - should NOT schedule next poll + // because the callback promise hasn't resolved yet + await vi.advanceTimersByTimeAsync(5000); + + // Should still only be 1 call since previous call hasn't completed + expect(callback).toHaveBeenCalledTimes(1); + + // Complete the callback + resolveCallback!(); + await Promise.resolve(); + + // Now advance time for the next interval + await vi.advanceTimersByTimeAsync(5000); + + expect(callback).toHaveBeenCalledTimes(2); + + poller.stopPollingForSessionToken(); + }); + }); +}); diff --git a/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts b/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts new file mode 100644 index 00000000000..e78d6ba9b19 --- /dev/null +++ b/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it, vi } from 'vitest'; + +import type { SafeLockReturn } from '../safeLock'; +import { SafeLock } from '../safeLock'; + +describe('SafeLock', () => { + describe('interface contract', () => { + it('returns SafeLockReturn interface with acquireLockAndRun method', () => { + const lock = SafeLock('test-interface'); + + expect(lock).toHaveProperty('acquireLockAndRun'); + expect(typeof lock.acquireLockAndRun).toBe('function'); + }); + + it('SafeLockReturn type allows creating mock implementations', () => { + // This test verifies the type interface works correctly for mocking + const mockLock: SafeLockReturn = { + acquireLockAndRun: vi.fn().mockResolvedValue('mock-result'), + }; + + expect(mockLock.acquireLockAndRun).toBeDefined(); + }); + }); + + describe('Web Locks API path', () => { + it('uses Web Locks API when available in secure context', async () => { + // Skip if Web Locks not available (like in jsdom without polyfill) + if (!('locks' in navigator) || !navigator.locks) { + return; + } + + const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout'); + const lock = SafeLock('test-weblocks-' + Date.now()); + const callback = vi.fn().mockResolvedValue('web-locks-result'); + + const result = await lock.acquireLockAndRun(callback); + + expect(callback).toHaveBeenCalled(); + expect(result).toBe('web-locks-result'); + // Verify cleanup happened + expect(clearTimeoutSpy).toHaveBeenCalled(); + + clearTimeoutSpy.mockRestore(); + }); + }); + + describe('shared lock pattern', () => { + it('allows multiple components to share a lock via SafeLockReturn interface', async () => { + // This demonstrates how AuthCookieService shares a lock between poller and focus handler + const executionLog: string[] = []; + + const sharedLock: SafeLockReturn = { + acquireLockAndRun: vi.fn().mockImplementation(async (cb: () => Promise) => { + executionLog.push('lock-acquired'); + const result = await cb(); + executionLog.push('lock-released'); + return result; + }), + }; + + // Simulate poller using the lock + await sharedLock.acquireLockAndRun(() => { + executionLog.push('poller-callback'); + return Promise.resolve('poller-done'); + }); + + // Simulate focus handler using the same lock + await sharedLock.acquireLockAndRun(() => { + executionLog.push('focus-callback'); + return Promise.resolve('focus-done'); + }); + + expect(executionLog).toEqual([ + 'lock-acquired', + 'poller-callback', + 'lock-released', + 'lock-acquired', + 'focus-callback', + 'lock-released', + ]); + }); + + it('mock lock can simulate sequential execution', async () => { + const results: string[] = []; + + // Create a mock that simulates sequential lock behavior + const sharedLock: SafeLockReturn = { + acquireLockAndRun: vi.fn().mockImplementation(async (cb: () => Promise) => { + const result = await cb(); + results.push(result as string); + return result; + }), + }; + + // Both "tabs" try to refresh + const promise1 = sharedLock.acquireLockAndRun(() => Promise.resolve('tab1-result')); + const promise2 = sharedLock.acquireLockAndRun(() => Promise.resolve('tab2-result')); + + await Promise.all([promise1, promise2]); + + expect(results).toContain('tab1-result'); + expect(results).toContain('tab2-result'); + expect(sharedLock.acquireLockAndRun).toHaveBeenCalledTimes(2); + }); + }); +}); diff --git a/packages/clerk-js/src/core/auth/safeLock.ts b/packages/clerk-js/src/core/auth/safeLock.ts index 405190a73ff..33a5d58f4f8 100644 --- a/packages/clerk-js/src/core/auth/safeLock.ts +++ b/packages/clerk-js/src/core/auth/safeLock.ts @@ -1,36 +1,61 @@ import Lock from 'browser-tabs-lock'; +import { debugLogger } from '@/utils/debug'; + +const LOCK_TIMEOUT_MS = 4999; + +/** + * Creates a cross-tab lock for coordinating exclusive operations across browser tabs. + * + * Uses Web Locks API in secure contexts (HTTPS), falling back to browser-tabs-lock + * (localStorage-based) in non-secure contexts. + * + * @param key - Unique identifier for the lock (same key = same lock across all tabs) + */ export function SafeLock(key: string) { const lock = new Lock(); - // TODO: Figure out how to fix this linting error + // Release any held locks when the tab is closing to prevent deadlocks // eslint-disable-next-line @typescript-eslint/no-misused-promises window.addEventListener('beforeunload', async () => { await lock.releaseLock(key); }); - const acquireLockAndRun = async (cb: () => Promise) => { + /** + * Acquires the cross-tab lock and executes the callback while holding it. + * If lock acquisition fails or times out, executes the callback anyway (degraded mode) + * to ensure the operation completes rather than failing. + */ + const acquireLockAndRun = async (cb: () => Promise): Promise => { if ('locks' in navigator && isSecureContext) { const controller = new AbortController(); - const lockTimeout = setTimeout(() => controller.abort(), 4999); - return await navigator.locks - .request(key, { signal: controller.signal }, async () => { + const lockTimeout = setTimeout(() => controller.abort(), LOCK_TIMEOUT_MS); + + try { + return await navigator.locks.request(key, { signal: controller.signal }, async () => { clearTimeout(lockTimeout); return await cb(); - }) - .catch(() => { - // browser-tabs-lock never seems to throw, so we are mirroring the behavior here - return false; }); + } catch { + // Lock request was aborted (timeout) or failed + // Execute callback anyway in degraded mode to ensure operation completes + debugLogger.warn('Lock acquisition timed out, proceeding without lock (degraded mode)', { key }, 'safeLock'); + return await cb(); + } } - if (await lock.acquireLock(key, 5000)) { + // Fallback for non-secure contexts using localStorage-based locking + if (await lock.acquireLock(key, LOCK_TIMEOUT_MS + 1)) { try { return await cb(); } finally { await lock.releaseLock(key); } } + + // Lock acquisition timed out - execute callback anyway in degraded mode + debugLogger.warn('Lock acquisition timed out, proceeding without lock (degraded mode)', { key }, 'safeLock'); + return await cb(); }; return { acquireLockAndRun }; diff --git a/packages/clerk-js/src/core/resources/Session.ts b/packages/clerk-js/src/core/resources/Session.ts index 7ac829ca520..bcacee1cea9 100644 --- a/packages/clerk-js/src/core/resources/Session.ts +++ b/packages/clerk-js/src/core/resources/Session.ts @@ -28,6 +28,7 @@ import { isWebAuthnSupported as isWebAuthnSupportedOnWindow } from '@clerk/share import { unixEpochToDate } from '@/utils/date'; import { debugLogger } from '@/utils/debug'; +import { LruMap } from '@/utils/lru-map'; import { convertJSONToPublicKeyRequestOptions, serializePublicKeyCredentialAssertion, @@ -35,12 +36,33 @@ import { } from '@/utils/passkeys'; import { TokenId } from '@/utils/tokenId'; +import { SafeLock } from '../auth/safeLock'; import { clerkInvalidStrategy, clerkMissingWebAuthnPublicKeyOptions } from '../errors'; import { eventBus, events } from '../events'; import { SessionTokenCache } from '../tokenCache'; import { BaseResource, PublicUserData, Token, User } from './internal'; import { SessionVerification } from './SessionVerification'; +/** + * Cache of per-tokenId locks for cross-tab coordination. + * Each unique tokenId gets its own lock, allowing different token types + * (e.g., different orgs, JWT templates) to be fetched in parallel. + * Uses LRU eviction to prevent unbounded growth. + */ +const tokenLocks = new LruMap>(50); + +/** + * Gets or creates a cross-tab lock for a specific tokenId. + */ +function getTokenLock(tokenId: string) { + let lock = tokenLocks.get(tokenId); + if (!lock) { + lock = SafeLock(`clerk.lock.getToken.${tokenId}`); + tokenLocks.set(tokenId, lock); + } + return lock; +} + export class Session extends BaseResource implements SessionResource { pathRoot = '/client/sessions'; @@ -369,19 +391,13 @@ export class Session extends BaseResource implements SessionResource { const tokenId = this.#getCacheId(template, organizationId); + // Fast path: check cache without lock for immediate hits const cachedEntry = skipCache ? undefined : SessionTokenCache.get({ tokenId }, leewayInSeconds); // Dispatch tokenUpdate only for __session tokens with the session's active organization ID, and not JWT templates const shouldDispatchTokenUpdate = !template && organizationId === this.lastActiveOrganizationId; if (cachedEntry) { - debugLogger.debug( - 'Using cached token (no fetch needed)', - { - tokenId, - }, - 'session', - ); const cachedToken = await cachedEntry.tokenResolver; if (shouldDispatchTokenUpdate) { eventBus.emit(events.TokenUpdate, { token: cachedToken }); @@ -390,39 +406,63 @@ export class Session extends BaseResource implements SessionResource { return cachedToken.getRawString() || null; } - debugLogger.info( - 'Fetching new token from API', - { - organizationId, - template, - tokenId, - }, - 'session', - ); + // Cache miss: acquire cross-tab lock before fetching to prevent duplicate API calls + // when multiple tabs try to refresh the token simultaneously. + // Using per-tokenId locks allows different token types to be fetched in parallel. + const tokenLock = getTokenLock(tokenId); + return tokenLock.acquireLockAndRun(async () => { + // Double-check cache after acquiring lock - another tab may have populated it + const cachedEntryAfterLock = skipCache ? undefined : SessionTokenCache.get({ tokenId }, leewayInSeconds); + + if (cachedEntryAfterLock) { + debugLogger.debug( + 'Using cached token after lock (populated by another tab)', + { + tokenId, + }, + 'session', + ); + const cachedToken = await cachedEntryAfterLock.tokenResolver; + if (shouldDispatchTokenUpdate) { + eventBus.emit(events.TokenUpdate, { token: cachedToken }); + } + return cachedToken.getRawString() || null; + } - const path = template ? `${this.path()}/tokens/${template}` : `${this.path()}/tokens`; + debugLogger.info( + 'Fetching new token from API', + { + organizationId, + template, + tokenId, + }, + 'session', + ); - // TODO: update template endpoint to accept organizationId - const params: Record = template ? {} : { organizationId }; + const path = template ? `${this.path()}/tokens/${template}` : `${this.path()}/tokens`; - const tokenResolver = Token.create(path, params, skipCache); + // TODO: update template endpoint to accept organizationId + const params: Record = template ? {} : { organizationId }; - // Cache the promise immediately to prevent concurrent calls from triggering duplicate requests - SessionTokenCache.set({ tokenId, tokenResolver }); + const tokenResolver = Token.create(path, params, skipCache); - return tokenResolver.then(token => { - if (shouldDispatchTokenUpdate) { - eventBus.emit(events.TokenUpdate, { token }); + // Cache the promise immediately to prevent concurrent calls from triggering duplicate requests + SessionTokenCache.set({ tokenId, tokenResolver }); + + return tokenResolver.then(token => { + if (shouldDispatchTokenUpdate) { + eventBus.emit(events.TokenUpdate, { token }); - if (token.jwt) { - this.lastActiveToken = token; - // Emits the updated session with the new token to the state listeners - eventBus.emit(events.SessionTokenResolved, null); + if (token.jwt) { + this.lastActiveToken = token; + // Emits the updated session with the new token to the state listeners + eventBus.emit(events.SessionTokenResolved, null); + } } - } - // Return null when raw string is empty to indicate that there it's signed-out - return token.getRawString() || null; + // Return null when raw string is empty to indicate that there it's signed-out + return token.getRawString() || null; + }); }); } diff --git a/packages/clerk-js/src/utils/__tests__/lru-map.test.ts b/packages/clerk-js/src/utils/__tests__/lru-map.test.ts new file mode 100644 index 00000000000..a798fc740c2 --- /dev/null +++ b/packages/clerk-js/src/utils/__tests__/lru-map.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from 'vitest'; + +import { LruMap } from '../lru-map'; + +describe('LruMap', () => { + it('stores and retrieves values', () => { + const map = new LruMap(3); + map.set('a', 1); + map.set('b', 2); + + expect(map.get('a')).toBe(1); + expect(map.get('b')).toBe(2); + expect(map.get('c')).toBeUndefined(); + }); + + it('evicts oldest entry when exceeding max size', () => { + const map = new LruMap(3); + map.set('a', 1); + map.set('b', 2); + map.set('c', 3); + map.set('d', 4); + + expect(map.get('a')).toBeUndefined(); + expect(map.get('b')).toBe(2); + expect(map.get('c')).toBe(3); + expect(map.get('d')).toBe(4); + expect(map.size).toBe(3); + }); + + it('moves accessed entry to most recent position', () => { + const map = new LruMap(3); + map.set('a', 1); + map.set('b', 2); + map.set('c', 3); + + map.get('a'); + + map.set('d', 4); + + expect(map.get('a')).toBe(1); + expect(map.get('b')).toBeUndefined(); + expect(map.get('c')).toBe(3); + expect(map.get('d')).toBe(4); + }); + + it('updates existing entry without eviction', () => { + const map = new LruMap(3); + map.set('a', 1); + map.set('b', 2); + map.set('c', 3); + + map.set('a', 100); + + expect(map.get('a')).toBe(100); + expect(map.size).toBe(3); + + map.set('d', 4); + + expect(map.get('a')).toBe(100); + expect(map.get('b')).toBeUndefined(); + expect(map.size).toBe(3); + }); + + it('handles max size of 1', () => { + const map = new LruMap(1); + map.set('a', 1); + map.set('b', 2); + + expect(map.get('a')).toBeUndefined(); + expect(map.get('b')).toBe(2); + expect(map.size).toBe(1); + }); +}); diff --git a/packages/clerk-js/src/utils/lru-map.ts b/packages/clerk-js/src/utils/lru-map.ts new file mode 100644 index 00000000000..c890fbc69a3 --- /dev/null +++ b/packages/clerk-js/src/utils/lru-map.ts @@ -0,0 +1,34 @@ +/** + * A simple Map with LRU (Least Recently Used) eviction. + * When the map exceeds maxSize, the oldest entries are removed. + */ +export class LruMap extends Map { + constructor(private maxSize: number) { + super(); + } + + override get(key: K): V | undefined { + const value = super.get(key); + if (value !== undefined) { + super.delete(key); + super.set(key, value); + } + return value; + } + + override set(key: K, value: V): this { + if (super.has(key)) { + super.delete(key); + } else { + while (this.size >= this.maxSize) { + const oldest = super.keys().next().value; + if (oldest !== undefined) { + super.delete(oldest); + } else { + break; + } + } + } + return super.set(key, value); + } +}