diff --git a/.changeset/fix-token-refresh-race-condition.md b/.changeset/fix-token-refresh-race-condition.md new file mode 100644 index 00000000000..1247fb865d7 --- /dev/null +++ b/.changeset/fix-token-refresh-race-condition.md @@ -0,0 +1,4 @@ +--- +"@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 \ No newline at end of file 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..9ad78adedc0 100644 --- a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts +++ b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts @@ -1,12 +1,16 @@ import { createWorkerTimers } from '@clerk/shared/workerTimers'; -import { SafeLock } from './safeLock'; +import { debugLogger } from '@/utils/debug'; -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,8 +23,13 @@ export class SessionCookiePoller { const run = async () => { this.initiated = true; - await this.lock.acquireLockAndRun(cb); - this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); + try { + await cb(); + } catch (error) { + debugLogger.error('SessionCookiePoller callback failed', { error }, 'auth'); + } finally { + this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); + } }; void run(); 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 d49aefd2186..670803cb9c0 100644 --- a/packages/clerk-js/src/core/resources/Session.ts +++ b/packages/clerk-js/src/core/resources/Session.ts @@ -35,12 +35,34 @@ import { unixEpochToDate } from '@/utils/date'; import { debugLogger } from '@/utils/debug'; 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. + */ +const tokenLocks = new Map>(); + +/** + * Gets or creates a cross-tab lock for a specific tokenId. + * Using per-tokenId locks allows different token types to be fetched in parallel + * while still preventing duplicate fetches for the same token across tabs. + */ +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'; @@ -363,19 +385,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 }); @@ -384,39 +400,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; + }); }); }