diff --git a/.changeset/fix-token-refresh-race-condition.md b/.changeset/fix-token-refresh-race-condition.md index 3b7dd6746a8..ba56f1c3de4 100644 --- a/.changeset/fix-token-refresh-race-condition.md +++ b/.changeset/fix-token-refresh-race-condition.md @@ -1,4 +1,5 @@ --- "@clerk/clerk-js": patch --- -Fix race condition where multiple browser tabs could fetch session tokens simultaneously. + +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/SessionCookiePoller.ts b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts index 32d3f894faf..ea1d25b1d4a 100644 --- a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts +++ b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts @@ -5,7 +5,7 @@ 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, + * Cross-tab coordination is handled within Session.getToken() itself, * so this poller simply triggers the refresh callback without additional locking. */ export class SessionCookiePoller { diff --git a/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts b/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts index e78d6ba9b19..f3b806b6f28 100644 --- a/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts +++ b/packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts @@ -1,9 +1,22 @@ -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { SafeLockReturn } from '../safeLock'; -import { SafeLock } from '../safeLock'; describe('SafeLock', () => { + let SafeLock: (key: string) => SafeLockReturn; + let addEventListenerSpy: ReturnType; + + beforeEach(async () => { + vi.resetModules(); + const module = await import('../safeLock'); + SafeLock = module.SafeLock; + addEventListenerSpy = vi.spyOn(window, 'addEventListener'); + }); + + afterEach(() => { + addEventListenerSpy.mockRestore(); + }); + describe('interface contract', () => { it('returns SafeLockReturn interface with acquireLockAndRun method', () => { const lock = SafeLock('test-interface'); @@ -24,8 +37,8 @@ describe('SafeLock', () => { 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) { + // Skip if Web Locks not available or not in secure context (e.g. jsdom without polyfill) + if (!('locks' in navigator) || !navigator.locks || !isSecureContext) { return; } @@ -103,4 +116,91 @@ describe('SafeLock', () => { expect(sharedLock.acquireLockAndRun).toHaveBeenCalledTimes(2); }); }); + + describe('error handling', () => { + it('propagates callback errors without double-invocation', async () => { + const originalLocks = navigator.locks; + const callbackError = new Error('Callback failed'); + const callback = vi.fn().mockRejectedValue(callbackError); + + const mockRequest = vi.fn().mockImplementation(async (_name, _options, cb) => { + return await cb(); + }); + + Object.defineProperty(navigator, 'locks', { + value: { request: mockRequest }, + configurable: true, + }); + + try { + const lock = SafeLock('test-error-propagation'); + await expect(lock.acquireLockAndRun(callback)).rejects.toThrow('Callback failed'); + expect(callback).toHaveBeenCalledTimes(1); + } finally { + Object.defineProperty(navigator, 'locks', { + value: originalLocks, + configurable: true, + }); + } + }); + + it('invokes callback in degraded mode on AbortError (timeout)', async () => { + const originalLocks = navigator.locks; + const callback = vi.fn().mockResolvedValue('success'); + + const abortError = new Error('Lock request aborted'); + abortError.name = 'AbortError'; + + const mockRequest = vi.fn().mockRejectedValue(abortError); + + Object.defineProperty(navigator, 'locks', { + value: { request: mockRequest }, + configurable: true, + }); + + try { + const lock = SafeLock('test-abort-fallback'); + const result = await lock.acquireLockAndRun(callback); + + expect(callback).toHaveBeenCalledTimes(1); + expect(result).toBe('success'); + } finally { + Object.defineProperty(navigator, 'locks', { + value: originalLocks, + configurable: true, + }); + } + }); + }); + + describe('beforeunload listener consolidation', () => { + it('registers only one beforeunload listener regardless of lock count', () => { + const beforeUnloadCalls = addEventListenerSpy.mock.calls.filter(call => call[0] === 'beforeunload'); + expect(beforeUnloadCalls).toHaveLength(0); + + SafeLock('lock-1'); + SafeLock('lock-2'); + SafeLock('lock-3'); + + const afterCalls = addEventListenerSpy.mock.calls.filter(call => call[0] === 'beforeunload'); + expect(afterCalls).toHaveLength(1); + }); + + it('fresh module import starts with clean state', async () => { + // First module instance + SafeLock('lock-a'); + expect(addEventListenerSpy.mock.calls.filter(call => call[0] === 'beforeunload')).toHaveLength(1); + + // Reset and get fresh module + vi.resetModules(); + addEventListenerSpy.mockRestore(); + addEventListenerSpy = vi.spyOn(window, 'addEventListener'); + + const freshModule = await import('../safeLock'); + + // Fresh module should register its own listener + freshModule.SafeLock('lock-b'); + expect(addEventListenerSpy.mock.calls.filter(call => call[0] === 'beforeunload')).toHaveLength(1); + }); + }); }); diff --git a/packages/clerk-js/src/core/auth/safeLock.ts b/packages/clerk-js/src/core/auth/safeLock.ts index 33a5d58f4f8..f26a4d3d30d 100644 --- a/packages/clerk-js/src/core/auth/safeLock.ts +++ b/packages/clerk-js/src/core/auth/safeLock.ts @@ -4,6 +4,26 @@ import { debugLogger } from '@/utils/debug'; const LOCK_TIMEOUT_MS = 4999; +const activeLocks = new Map(); +let cleanupListenerRegistered = false; + +function registerCleanupListener() { + if (cleanupListenerRegistered) { + return; + } + cleanupListenerRegistered = true; + + window.addEventListener('beforeunload', () => { + activeLocks.forEach((lock, key) => { + void lock.releaseLock(key); + }); + }); +} + +export interface SafeLockReturn { + acquireLockAndRun: (cb: () => Promise) => Promise; +} + /** * Creates a cross-tab lock for coordinating exclusive operations across browser tabs. * @@ -12,14 +32,11 @@ const LOCK_TIMEOUT_MS = 4999; * * @param key - Unique identifier for the lock (same key = same lock across all tabs) */ -export function SafeLock(key: string) { +export function SafeLock(key: string): SafeLockReturn { const lock = new Lock(); - // 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); - }); + activeLocks.set(key, lock); + registerCleanupListener(); /** * Acquires the cross-tab lock and executes the callback while holding it. @@ -36,11 +53,14 @@ export function SafeLock(key: string) { clearTimeout(lockTimeout); return await cb(); }); - } 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(); + } catch (error) { + clearTimeout(lockTimeout); + if (error instanceof Error && error.name === 'AbortError') { + // Lock request was aborted (timeout) - execute callback anyway in degraded mode + debugLogger.warn('Lock acquisition timed out, proceeding without lock (degraded mode)', { key }, 'safeLock'); + return await cb(); + } + throw error; } } diff --git a/packages/clerk-js/src/core/resources/Session.ts b/packages/clerk-js/src/core/resources/Session.ts index ef167be7c15..240ecb26b17 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, @@ -46,8 +47,11 @@ 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. + * + * LruMap is used to limit the maximum number of lock instances to prevent + * unbounded memory growth in long-lived browser sessions. */ -const tokenLocks = new Map>(); +const tokenLocks = new LruMap>(50); /** * Gets or creates a cross-tab lock for a specific tokenId. @@ -387,7 +391,6 @@ export class Session extends BaseResource implements SessionResource { // 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; @@ -396,7 +399,6 @@ export class Session extends BaseResource implements SessionResource { if (shouldDispatchTokenUpdate) { eventBus.emit(events.TokenUpdate, { token: cachedToken }); } - // Return null when raw string is empty to indicate that there it's signed-out return cachedToken.getRawString() || null; } @@ -454,7 +456,7 @@ export class Session extends BaseResource implements SessionResource { } } - // Return null when raw string is empty to indicate that there it's signed-out + // Return null when raw string is empty to indicate that it's signed-out return token.getRawString() || null; }); }); diff --git a/packages/clerk-js/src/core/resources/__tests__/Session.test.ts b/packages/clerk-js/src/core/resources/__tests__/Session.test.ts index 4de20208046..61a3519e779 100644 --- a/packages/clerk-js/src/core/resources/__tests__/Session.test.ts +++ b/packages/clerk-js/src/core/resources/__tests__/Session.test.ts @@ -426,6 +426,60 @@ describe('Session', () => { expect(requestSpy).toHaveBeenCalledTimes(2); }); + + it('uses cache populated by another source during lock wait (simulates cross-tab)', async () => { + BaseResource.clerk = clerkMock(); + + const session = new Session({ + status: 'active', + id: 'session_1', + object: 'session', + user: createUser({}), + last_active_organization_id: null, + actor: null, + created_at: new Date().getTime(), + updated_at: new Date().getTime(), + } as SessionJSON); + + const requestSpy = BaseResource.clerk.getFapiClient().request as Mock; + requestSpy.mockClear(); + + const token1 = await session.getToken(); + expect(requestSpy).toHaveBeenCalledTimes(1); + expect(token1).toEqual(mockJwt); + + requestSpy.mockClear(); + + const token2 = await session.getToken(); + expect(requestSpy).toHaveBeenCalledTimes(0); + expect(token2).toEqual(mockJwt); + }); + + it('makes API call when skipCache is true even if cache is populated', async () => { + BaseResource.clerk = clerkMock(); + + const session = new Session({ + status: 'active', + id: 'session_1', + object: 'session', + user: createUser({}), + last_active_organization_id: null, + last_active_token: { object: 'token', jwt: mockJwt }, + actor: null, + created_at: new Date().getTime(), + updated_at: new Date().getTime(), + } as SessionJSON); + + expect(SessionTokenCache.size()).toBe(1); + + const requestSpy = BaseResource.clerk.getFapiClient().request as Mock; + requestSpy.mockClear(); + + const token = await session.getToken({ skipCache: true }); + + expect(requestSpy).toHaveBeenCalledTimes(1); + expect(token).toEqual(mockJwt); + }); }); describe('touch()', () => { 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); + } +}