From c976cf194ae80b172c359de7d8956a4f680665c7 Mon Sep 17 00:00:00 2001 From: Omar Al Matar Date: Wed, 20 May 2026 16:47:28 +0300 Subject: [PATCH] refactor(auth): remove navigator.locks-based mutex; introduce commit guard + dispose() Removes the `_acquireLock` mutex that wrapped every auth operation and the underlying `navigator.locks` / `processLock` machinery. Replaces it with two lighter primitives that target the specific synchronization needs each operation actually has: - `refreshingDeferred` (already existed) continues to single-flight the refresh path within an instance. - A storage-level commit guard in `_callRefreshToken` re-reads the storage refresh_token between the rotated-tokens response and `_saveSession`. If storage changed under us (e.g. a concurrent `signOut` ran `_removeSession`), the rotated tokens are discarded rather than written back. Returns `AuthRefreshDiscardedError` on the result. Cross-tab refresh races are handled server-side by GoTrue's v1 parent-of-active mechanism at `internal/tokens/service.go:376-385`, so no client-side coordination is needed. New `client.dispose()` tears down the auto-refresh interval, the `visibilitychange` listener, and the BroadcastChannel; clears registered `onAuthStateChange` subscribers. Idempotent. Call from cleanup hooks in React Strict Mode / HMR contexts to prevent stale tickers from outliving the client. The `lock` and `lockAcquireTimeout` constructor options are silently ignored for backwards compatibility; both are marked `@deprecated`. `navigatorLock`, `processLock`, and the `LockAcquireTimeoutError` family remain exported from `./lib/locks` for one major version. Stale lock references in JSDoc on `getSession`, `onAuthStateChange`, `_challengeAndVerify`, and `_listFactors` updated to match the new model. Test branch only, not for merge yet. See RFC `lockless_auth_coordination`. Resolves (test target): supabase/supabase-js#2013, #936, #2111 --- packages/core/auth-js/src/GoTrueClient.ts | 609 ++++++++---------- packages/core/auth-js/src/lib/errors.ts | 32 + packages/core/auth-js/src/lib/locks.ts | 64 +- packages/core/auth-js/src/lib/types.ts | 36 +- .../auth-js/test/GoTrueClient.browser.test.ts | 65 +- .../core/auth-js/test/GoTrueClient.test.ts | 203 +++--- packages/core/supabase-js/src/lib/types.ts | 15 +- 7 files changed, 474 insertions(+), 550 deletions(-) diff --git a/packages/core/auth-js/src/GoTrueClient.ts b/packages/core/auth-js/src/GoTrueClient.ts index a24e2fdc7..eed9ceab4 100644 --- a/packages/core/auth-js/src/GoTrueClient.ts +++ b/packages/core/auth-js/src/GoTrueClient.ts @@ -16,11 +16,13 @@ import { AuthInvalidTokenResponseError, AuthPKCECodeVerifierMissingError, AuthPKCEGrantCodeExchangeError, + AuthRefreshDiscardedError, AuthSessionMissingError, AuthUnknownError, isAuthApiError, isAuthError, isAuthImplicitGrantRedirectError, + isAuthRefreshDiscardedError, isAuthRetryableFetchError, isAuthSessionMissingError, } from './lib/errors' @@ -54,7 +56,6 @@ import { validateExp, } from './lib/helpers' import { memoryLocalStorageAdapter } from './lib/local-storage' -import { LockAcquireTimeoutError, navigatorLock } from './lib/locks' import { polyfillGlobalThis } from './lib/polyfills' import { version } from './lib/version' @@ -91,7 +92,6 @@ import type { JWK, JwtHeader, JwtPayload, - LockFunc, MFAChallengeAndVerifyParams, MFAChallengeParams, MFAChallengePhoneParams, @@ -183,7 +183,7 @@ polyfillGlobalThis() // Make "globalThis" available const DEFAULT_OPTIONS: Omit< Required, - 'fetch' | 'storage' | 'userStorage' | 'lock' + 'fetch' | 'storage' | 'userStorage' | 'lock' | 'lockAcquireTimeout' > = { url: GOTRUE_URL, storageKey: STORAGE_KEY, @@ -195,15 +195,10 @@ const DEFAULT_OPTIONS: Omit< debug: false, hasCustomAuthorizationHeader: false, throwOnError: false, - lockAcquireTimeout: 5000, // 5 seconds skipAutoInitialize: false, experimental: {}, } -async function lockNoOp(name: string, acquireTimeout: number, fn: () => Promise): Promise { - return await fn() -} - /** * Caches JWKS values for all clients created in the same environment. This is * especially useful for shared-memory execution environments such as Vercel's @@ -297,11 +292,7 @@ export default class GoTrueClient { protected hasCustomAuthorizationHeader = false protected suppressGetSessionWarning = false protected fetch: Fetch - protected lock: LockFunc - protected lockAcquired = false - protected pendingInLock: Promise[] = [] protected throwOnError: boolean - protected lockAcquireTimeout: number /** * Opt-in flags for experimental features. Defaults to an empty object. * See `GoTrueClientOptions.experimental`. @@ -371,20 +362,13 @@ export default class GoTrueClient { this.url = settings.url this.headers = settings.headers this.fetch = resolveFetch(settings.fetch) - this.lock = settings.lock || lockNoOp this.detectSessionInUrl = settings.detectSessionInUrl this.flowType = settings.flowType this.hasCustomAuthorizationHeader = settings.hasCustomAuthorizationHeader this.throwOnError = settings.throwOnError - this.lockAcquireTimeout = settings.lockAcquireTimeout - if (settings.lock) { - this.lock = settings.lock - } else if (this.persistSession && isBrowser() && globalThis?.navigator?.locks) { - this.lock = navigatorLock - } else { - this.lock = lockNoOp - } + // settings.lock and settings.lockAcquireTimeout are typed for backwards + // compatibility and have no effect; deliberately not read here. if (!this.jwks) { this.jwks = { keys: [] } @@ -518,9 +502,7 @@ export default class GoTrueClient { } this.initializePromise = (async () => { - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._initialize() - }) + return await this._initialize() })() return await this.initializePromise @@ -1405,9 +1387,7 @@ export default class GoTrueClient { async exchangeCodeForSession(authCode: string): Promise { await this.initializePromise - return this._acquireLock(this.lockAcquireTimeout, async () => { - return this._exchangeCodeForSession(authCode) - }) + return this._exchangeCodeForSession(authCode) } /** @@ -2444,9 +2424,7 @@ export default class GoTrueClient { async reauthenticate(): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._reauthenticate() - }) + return await this._reauthenticate() } private async _reauthenticate(): Promise { @@ -2593,7 +2571,7 @@ export default class GoTrueClient { * - If the session's access token is expired or is about to expire, this method will use the refresh token to refresh the session. * - When using in a browser, or you've called `startAutoRefresh()` in your environment (React Native, etc.) this function always returns a valid access token without refreshing the session itself, as this is done in the background. This function returns very fast. * - **IMPORTANT SECURITY NOTICE:** If using an insecure storage medium, such as cookies or request headers, the user object returned by this function **must not be trusted**. Always verify the JWT using `getClaims()` or your own JWT verification library to securely establish the user's identity and access. You can also use `getUser()` to fetch the user object directly from the Auth server for this purpose. - * - When using in a browser, this function is synchronized across all tabs using the [LockManager](https://developer.mozilla.org/en-US/docs/Web/API/LockManager) API. In other environments make sure you've defined a proper `lock` property, if necessary, to make sure there are no race conditions while the session is being refreshed. + * - Cross-tab refresh races are handled by the GoTrue server (the rotated token from the first tab is returned to subsequent tabs via the parent-of-active mechanism), so no client-side serialization is needed. * * @example Get the session data * ```js @@ -2661,91 +2639,15 @@ export default class GoTrueClient { async getSession() { await this.initializePromise - const result = await this._acquireLock(this.lockAcquireTimeout, async () => { - return this._useSession(async (result) => { - return result - }) + return await this._useSession(async (result) => { + return result }) - - return result - } - - /** - * Acquires a global lock based on the storage key. - */ - private async _acquireLock(acquireTimeout: number, fn: () => Promise): Promise { - this._debug('#_acquireLock', 'begin', acquireTimeout) - - try { - if (this.lockAcquired) { - const last = this.pendingInLock.length - ? this.pendingInLock[this.pendingInLock.length - 1] - : Promise.resolve() - - const result = (async () => { - await last - return await fn() - })() - - this.pendingInLock.push( - (async () => { - try { - await result - } catch (_e) { - // we just care if it finished - } - })() - ) - - return result - } - - return await this.lock(`lock:${this.storageKey}`, acquireTimeout, async () => { - this._debug('#_acquireLock', 'lock acquired for storage key', this.storageKey) - - try { - this.lockAcquired = true - - const result = fn() - - this.pendingInLock.push( - (async () => { - try { - await result - } catch (e: any) { - // we just care if it finished - } - })() - ) - - await result - - // keep draining the queue until there's nothing to wait on - while (this.pendingInLock.length) { - const waitOn = [...this.pendingInLock] - - await Promise.all(waitOn) - - this.pendingInLock.splice(0, waitOn.length) - } - - return await result - } finally { - this._debug('#_acquireLock', 'lock released for storage key', this.storageKey) - - this.lockAcquired = false - } - }) - } finally { - this._debug('#_acquireLock', 'end') - } } /** - * Use instead of {@link #getSession} inside the library. It is - * semantically usually what you want, as getting a session involves some - * processing afterwards that requires only one client operating on the - * session at once across multiple tabs or processes. + * Use instead of {@link #getSession} inside the library. Loads the session + * via `__loadSession` (which may trigger a refresh if the access token is + * within the expiry margin) and runs `fn` with the result. */ private async _useSession( fn: ( @@ -2809,10 +2711,6 @@ export default class GoTrueClient { > { this._debug('#__loadSession()', 'begin') - if (!this.lockAcquired) { - this._debug('#__loadSession()', 'used outside of an acquired lock!', new Error().stack) - } - try { let currentSession: Session | null = null @@ -2976,9 +2874,7 @@ export default class GoTrueClient { await this.initializePromise - const result = await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._getUser() - }) + const result = await this._getUser() if (result.data.user) { this.suppressGetSessionWarning = true @@ -3153,9 +3049,7 @@ export default class GoTrueClient { ): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._updateUser(attributes, options) - }) + return await this._updateUser(attributes, options) } protected async _updateUser( @@ -3342,9 +3236,7 @@ export default class GoTrueClient { }): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._setSession(currentSession) - }) + return await this._setSession(currentSession) } protected async _setSession(currentSession: { @@ -3533,9 +3425,7 @@ export default class GoTrueClient { async refreshSession(currentSession?: { refresh_token: string }): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._refreshSession(currentSession) - }) + return await this._refreshSession(currentSession) } protected async _refreshSession(currentSession?: { @@ -3783,9 +3673,7 @@ export default class GoTrueClient { async signOut(options: SignOut = { scope: 'global' }): Promise<{ error: AuthError | null }> { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._signOut(options) - }) + return await this._signOut(options) } protected async _signOut( @@ -3832,16 +3720,11 @@ export default class GoTrueClient { } /** - * Avoid using an async function inside `onAuthStateChange` as you might end - * up with a deadlock. The callback function runs inside an exclusive lock, - * so calling other Supabase Client APIs that also try to acquire the - * exclusive lock, might cause a deadlock. This behavior is observable across - * tabs. In the next major library version, this behavior will not be supported. - * - * Receive a notification every time an auth event happens. + * Receive a notification every time an auth event happens. Callbacks can be + * `async` and can safely call other Supabase auth methods (`getUser`, + * `setSession`, etc.) from inside the callback. * * @param callback A callback function to be invoked when an auth event happens. - * @deprecated Due to the possibility of deadlocks with async functions as callbacks, use the version without an async function. */ onAuthStateChange(callback: (event: AuthChangeEvent, session: Session | null) => Promise): { data: { subscription: Subscription } @@ -3854,18 +3737,8 @@ export default class GoTrueClient { * - Subscribes to important events occurring on the user's session. * - Use on the frontend/client. It is less useful on the server. * - Events are emitted across tabs to keep your application's UI up-to-date. Some events can fire very frequently, based on the number of tabs open. Use a quick and efficient callback function, and defer or debounce as many operations as you can to be performed outside of the callback. - * - **Important:** A callback can be an `async` function and it runs synchronously during the processing of the changes causing the event. You can easily create a dead-lock by using `await` on a call to another method of the Supabase library. - * - Avoid using `async` functions as callbacks. - * - Limit the number of `await` calls in `async` callbacks. - * - Do not use other Supabase functions in the callback function. If you must, dispatch the functions once the callback has finished executing. Use this as a quick way to achieve this: - * ```js - * supabase.auth.onAuthStateChange((event, session) => { - * setTimeout(async () => { - * // await on other Supabase function here - * // this runs right after the callback has finished - * }, 0) - * }) - * ``` + * - Callbacks can be `async` and can safely call other Supabase auth methods (`getUser`, `setSession`, etc.) from inside the callback. + * - Keep callbacks quick. Events are awaited in order, so a slow callback delays subsequent events to subscribers in this tab. * - Emitted events: * - `INITIAL_SESSION` * - Emitted right after the Supabase client is constructed and the initial session from storage is loaded. @@ -4055,10 +3928,7 @@ export default class GoTrueClient { this.stateChangeEmitters.set(id, subscription) ;(async () => { await this.initializePromise - - await this._acquireLock(this.lockAcquireTimeout, async () => { - this._emitInitialSession(id) - }) + await this._emitInitialSession(id) })() return { data: { subscription } } @@ -4606,15 +4476,22 @@ export default class GoTrueClient { const { error } = await this._callRefreshToken(currentSession.refresh_token) if (error) { - console.error(error) - - if (!isAuthRetryableFetchError(error)) { - this._debug( - debugName, - 'refresh failed with a non-retryable error, removing the session', - error - ) - await this._removeSession() + // AuthRefreshDiscardedError means a concurrent signOut already + // cleared storage and fired SIGNED_OUT. Don't run _removeSession + // again here, or we'll emit a duplicate SIGNED_OUT. + if (isAuthRefreshDiscardedError(error)) { + this._debug(debugName, 'refresh discarded by commit guard', error) + } else { + console.error(error) + + if (!isAuthRetryableFetchError(error)) { + this._debug( + debugName, + 'refresh failed with a non-retryable error, removing the session', + error + ) + await this._removeSession() + } } } } @@ -4678,6 +4555,29 @@ export default class GoTrueClient { if (error) throw error if (!data.session) throw new AuthSessionMissingError() + // Commit guard: storage may have changed under us between fetch start + // and now (usually because a concurrent signOut ran `_removeSession`). + // If the refresh token we just rotated isn't in storage anymore, + // discard the rotated tokens. Writing them back would undo what + // signOut just cleared. + const currentStored = (await getItemAsync(this.storage, this.storageKey)) as Session | null + if (!currentStored || currentStored.refresh_token !== refreshToken) { + this._debug( + debugName, + 'commit guard: storage changed since refresh started, discarding rotated tokens', + { + had: currentStored ? `${currentStored.refresh_token?.substring(0, 5)}...` : null, + expected: `${refreshToken.substring(0, 5)}...`, + } + ) + const discarded: CallRefreshTokenResult = { + data: null, + error: new AuthRefreshDiscardedError(), + } + this.refreshingDeferred.resolve(discarded) + return discarded + } + await this._saveSession(data.session) await this._notifyAllSubscribers('TOKEN_REFRESHED', data.session) @@ -4978,58 +4878,83 @@ export default class GoTrueClient { await this._stopAutoRefresh() } + /** + * Tears down the client's background work: stops the auto-refresh interval, + * removes the `visibilitychange` listener, closes the cross-tab + * `BroadcastChannel`, and clears registered `onAuthStateChange` subscribers. + * + * Call this from cleanup hooks when the client is being replaced before + * its JS realm is destroyed. React Strict Mode and HMR are the common + * cases. After dispose, network operations will reject. Storage reads + * still return whatever is currently persisted; dispose doesn't erase storage. + * + * Safe to call repeatedly. + * + * @category Auth + * + * @example Cleanup on React unmount + * ```ts + * useEffect(() => { + * const client = createClient(...) + * return () => { client.auth.dispose() } + * }, []) + * ``` + */ + async dispose(): Promise { + this._removeVisibilityChangedCallback() + await this._stopAutoRefresh() + this.broadcastChannel?.close() + this.broadcastChannel = null + this.stateChangeEmitters.clear() + } + /** * Runs the auto refresh token tick. */ private async _autoRefreshTokenTick() { this._debug('#_autoRefreshTokenTick()', 'begin') + // Skip if a refresh is already in flight. `_callRefreshToken` also + // dedupes via the same field, so this is just a fast-path skip to + // avoid an unnecessary storage read. + if (this.refreshingDeferred !== null) { + this._debug('#_autoRefreshTokenTick()', 'refresh already in flight, skipping') + return + } + try { - await this._acquireLock(0, async () => { - try { - const now = Date.now() + const now = Date.now() - try { - return await this._useSession(async (result) => { - const { - data: { session }, - } = result + try { + await this._useSession(async (result) => { + const { + data: { session }, + } = result - if (!session || !session.refresh_token || !session.expires_at) { - this._debug('#_autoRefreshTokenTick()', 'no session') - return - } + if (!session || !session.refresh_token || !session.expires_at) { + this._debug('#_autoRefreshTokenTick()', 'no session') + return + } - // session will expire in this many ticks (or has already expired if <= 0) - const expiresInTicks = Math.floor( - (session.expires_at * 1000 - now) / AUTO_REFRESH_TICK_DURATION_MS - ) + // session will expire in this many ticks (or has already expired if <= 0) + const expiresInTicks = Math.floor( + (session.expires_at * 1000 - now) / AUTO_REFRESH_TICK_DURATION_MS + ) - this._debug( - '#_autoRefreshTokenTick()', - `access token expires in ${expiresInTicks} ticks, a tick lasts ${AUTO_REFRESH_TICK_DURATION_MS}ms, refresh threshold is ${AUTO_REFRESH_TICK_THRESHOLD} ticks` - ) + this._debug( + '#_autoRefreshTokenTick()', + `access token expires in ${expiresInTicks} ticks, a tick lasts ${AUTO_REFRESH_TICK_DURATION_MS}ms, refresh threshold is ${AUTO_REFRESH_TICK_THRESHOLD} ticks` + ) - if (expiresInTicks <= AUTO_REFRESH_TICK_THRESHOLD) { - await this._callRefreshToken(session.refresh_token) - } - }) - } catch (e) { - console.error( - 'Auto refresh tick failed with error. This is likely a transient error.', - e - ) + if (expiresInTicks <= AUTO_REFRESH_TICK_THRESHOLD) { + await this._callRefreshToken(session.refresh_token) } - } finally { - this._debug('#_autoRefreshTokenTick()', 'end') - } - }) - } catch (e) { - if (e instanceof LockAcquireTimeoutError) { - this._debug('auto refresh token tick lock not available') - } else { - throw e + }) + } catch (e) { + console.error('Auto refresh tick failed with error. This is likely a transient error.', e) } + } finally { + this._debug('#_autoRefreshTokenTick()', 'end') } } @@ -5086,24 +5011,16 @@ export default class GoTrueClient { if (!calledFromInitialize) { // called when the visibility has changed, i.e. the browser // transitioned from hidden -> visible so we need to see if the session - // should be recovered immediately... but to do that we need to acquire - // the lock first asynchronously + // should be recovered await this.initializePromise - await this._acquireLock(this.lockAcquireTimeout, async () => { - if (document.visibilityState !== 'visible') { - this._debug( - methodName, - 'acquired the lock to recover the session, but the browser visibilityState is no longer visible, aborting' - ) - - // visibility has changed while waiting for the lock, abort - return - } + if (document.visibilityState !== 'visible') { + this._debug(methodName, 'visibilityState is no longer visible, skipping recovery') + return + } - // recover the session - await this._recoverAndRefresh() - }) + // recover the session + await this._recoverAndRefresh() } } else if (document.visibilityState === 'hidden') { if (this.autoRefreshToken) { @@ -5235,78 +5152,76 @@ export default class GoTrueClient { params: MFAVerifyWebauthnParams ): Promise private async _verify(params: MFAVerifyParams): Promise { - return this._acquireLock(this.lockAcquireTimeout, async () => { - try { - return await this._useSession(async (result) => { - const { data: sessionData, error: sessionError } = result - if (sessionError) { - return this._returnResult({ data: null, error: sessionError }) - } + try { + return await this._useSession(async (result) => { + const { data: sessionData, error: sessionError } = result + if (sessionError) { + return this._returnResult({ data: null, error: sessionError }) + } - const body: StrictOmit< - | Exclude - /** Exclude out the webauthn params from here because we're going to need to serialize them in the response */ - | Prettify< - StrictOmit & { - webauthn: Prettify< - StrictOmit & { - credential_response: PublicKeyCredentialJSON - } - > - } - >, - /* Exclude challengeId because the backend expects snake_case, and exclude factorId since it's passed in the path params */ - 'challengeId' | 'factorId' - > & { - challenge_id: string - } = { - challenge_id: params.challengeId, - ...('webauthn' in params - ? { - webauthn: { - ...params.webauthn, - credential_response: - params.webauthn.type === 'create' - ? serializeCredentialCreationResponse( - params.webauthn.credential_response as RegistrationCredential - ) - : serializeCredentialRequestResponse( - params.webauthn.credential_response as AuthenticationCredential - ), - }, - } - : { code: params.code }), - } + const body: StrictOmit< + | Exclude + /** Exclude out the webauthn params from here because we're going to need to serialize them in the response */ + | Prettify< + StrictOmit & { + webauthn: Prettify< + StrictOmit & { + credential_response: PublicKeyCredentialJSON + } + > + } + >, + /* Exclude challengeId because the backend expects snake_case, and exclude factorId since it's passed in the path params */ + 'challengeId' | 'factorId' + > & { + challenge_id: string + } = { + challenge_id: params.challengeId, + ...('webauthn' in params + ? { + webauthn: { + ...params.webauthn, + credential_response: + params.webauthn.type === 'create' + ? serializeCredentialCreationResponse( + params.webauthn.credential_response as RegistrationCredential + ) + : serializeCredentialRequestResponse( + params.webauthn.credential_response as AuthenticationCredential + ), + }, + } + : { code: params.code }), + } - const { data, error } = await _request( - this.fetch, - 'POST', - `${this.url}/factors/${params.factorId}/verify`, - { - body, - headers: this.headers, - jwt: sessionData?.session?.access_token, - } - ) - if (error) { - return this._returnResult({ data: null, error }) + const { data, error } = await _request( + this.fetch, + 'POST', + `${this.url}/factors/${params.factorId}/verify`, + { + body, + headers: this.headers, + jwt: sessionData?.session?.access_token, } - - await this._saveSession({ - expires_at: Math.round(Date.now() / 1000) + data.expires_in, - ...data, - }) - await this._notifyAllSubscribers('MFA_CHALLENGE_VERIFIED', data) - - return this._returnResult({ data, error }) - }) - } catch (error) { - if (isAuthError(error)) { + ) + if (error) { return this._returnResult({ data: null, error }) } - throw error + + await this._saveSession({ + expires_at: Math.round(Date.now() / 1000) + data.expires_in, + ...data, + }) + await this._notifyAllSubscribers('MFA_CHALLENGE_VERIFIED', data) + + return this._returnResult({ data, error }) + }) + } catch (error) { + if (isAuthError(error)) { + return this._returnResult({ data: null, error }) } - }) + throw error + } } /** @@ -5322,80 +5237,78 @@ export default class GoTrueClient { params: MFAChallengeWebauthnParams ): Promise> private async _challenge(params: MFAChallengeParams): Promise { - return this._acquireLock(this.lockAcquireTimeout, async () => { - try { - return await this._useSession(async (result) => { - const { data: sessionData, error: sessionError } = result - if (sessionError) { - return this._returnResult({ data: null, error: sessionError }) - } - - const response = (await _request( - this.fetch, - 'POST', - `${this.url}/factors/${params.factorId}/challenge`, - { - body: params, - headers: this.headers, - jwt: sessionData?.session?.access_token, - } - )) as - | Exclude - /** The server will send `serialized` data, so we assert the serialized response */ - | AuthMFAChallengeWebauthnServerResponse + try { + return await this._useSession(async (result) => { + const { data: sessionData, error: sessionError } = result + if (sessionError) { + return this._returnResult({ data: null, error: sessionError }) + } - if (response.error) { - return response + const response = (await _request( + this.fetch, + 'POST', + `${this.url}/factors/${params.factorId}/challenge`, + { + body: params, + headers: this.headers, + jwt: sessionData?.session?.access_token, } + )) as + | Exclude + /** The server will send `serialized` data, so we assert the serialized response */ + | AuthMFAChallengeWebauthnServerResponse - const { data } = response + if (response.error) { + return response + } - if (data.type !== 'webauthn') { - return { data, error: null } - } + const { data } = response + + if (data.type !== 'webauthn') { + return { data, error: null } + } - switch (data.webauthn.type) { - case 'create': - return { - data: { - ...data, - webauthn: { - ...data.webauthn, - credential_options: { - ...data.webauthn.credential_options, - publicKey: deserializeCredentialCreationOptions( - data.webauthn.credential_options.publicKey - ), - }, + switch (data.webauthn.type) { + case 'create': + return { + data: { + ...data, + webauthn: { + ...data.webauthn, + credential_options: { + ...data.webauthn.credential_options, + publicKey: deserializeCredentialCreationOptions( + data.webauthn.credential_options.publicKey + ), }, }, - error: null, - } - case 'request': - return { - data: { - ...data, - webauthn: { - ...data.webauthn, - credential_options: { - ...data.webauthn.credential_options, - publicKey: deserializeCredentialRequestOptions( - data.webauthn.credential_options.publicKey - ), - }, + }, + error: null, + } + case 'request': + return { + data: { + ...data, + webauthn: { + ...data.webauthn, + credential_options: { + ...data.webauthn.credential_options, + publicKey: deserializeCredentialRequestOptions( + data.webauthn.credential_options.publicKey + ), }, }, - error: null, - } - } - }) - } catch (error) { - if (isAuthError(error)) { - return this._returnResult({ data: null, error }) + }, + error: null, + } } - throw error + }) + } catch (error) { + if (isAuthError(error)) { + return this._returnResult({ data: null, error }) } - }) + throw error + } } /** @@ -5404,9 +5317,6 @@ export default class GoTrueClient { private async _challengeAndVerify( params: MFAChallengeAndVerifyParams ): Promise { - // both _challenge and _verify independently acquire the lock, so no need - // to acquire it here - const { data: challengeData, error: challengeError } = await this._challenge({ factorId: params.factorId, }) @@ -5425,7 +5335,6 @@ export default class GoTrueClient { * {@see GoTrueMFAApi#listFactors} */ private async _listFactors(): Promise { - // use #getUser instead of #_getUser as the former acquires a lock const { data: { user }, error: userError, diff --git a/packages/core/auth-js/src/lib/errors.ts b/packages/core/auth-js/src/lib/errors.ts index 21fb7886f..eb2520dfa 100644 --- a/packages/core/auth-js/src/lib/errors.ts +++ b/packages/core/auth-js/src/lib/errors.ts @@ -297,6 +297,38 @@ export function isAuthRetryableFetchError(error: unknown): error is AuthRetryabl return isAuthError(error) && error.name === 'AuthRetryableFetchError' } +/** + * Returned when the server rotated a refresh token successfully but the + * client chose not to persist the rotated tokens because the local session + * changed mid-flight. Usually means a concurrent `signOut` cleared storage + * between when the refresh started and when it came back. + * + * Set on the `error` field of the refresh result so callers can tell "we + * got rotated tokens but threw them away" apart from "the refresh failed." + * The rotated session on the server will be picked up on the next refresh + * via GoTrue's parent-of-active path. + * + * @example + * ```ts + * import { isAuthRefreshDiscardedError } from '@supabase/auth-js' + * + * if (isAuthRefreshDiscardedError(error)) { + * // Concurrent signOut/sign-in raced our refresh. Treat as a no-op. + * } + * ``` + */ +export class AuthRefreshDiscardedError extends CustomAuthError { + constructor( + message = 'Refresh result discarded: session state changed mid-flight (e.g., concurrent signOut)' + ) { + super(message, 'AuthRefreshDiscardedError', 409, undefined) + } +} + +export function isAuthRefreshDiscardedError(error: unknown): error is AuthRefreshDiscardedError { + return isAuthError(error) && error.name === 'AuthRefreshDiscardedError' +} + /** * This error is thrown on certain methods when the password used is deemed * weak. Inspect the reasons to identify what password strength rules are diff --git a/packages/core/auth-js/src/lib/locks.ts b/packages/core/auth-js/src/lib/locks.ts index ece1610d0..b2f168576 100644 --- a/packages/core/auth-js/src/lib/locks.ts +++ b/packages/core/auth-js/src/lib/locks.ts @@ -1,6 +1,17 @@ +/** + * Lock primitives retained for backwards-compatible imports. The v3 auth + * client coordinates refreshes itself (deduping in-instance callers onto a + * shared in-flight promise) and lets the GoTrue server resolve cross-instance + * races, so it does not invoke any primitive from this module. The functions + * still work for direct callers that need a navigator.locks-backed or + * in-process exclusive lock of their own. Scheduled for removal in v4. + */ + import { supportsLocalStorage } from './helpers' /** + * @deprecated Debug flag for `navigatorLock` / `processLock`. The v3 auth + * client ignores both, so this has no client-side effect. * @experimental */ export const internals = { @@ -18,18 +29,10 @@ export const internals = { /** * An error thrown when a lock cannot be acquired after some amount of time. * - * Use the {@link #isAcquireTimeout} property instead of checking with `instanceof`. - * - * @example - * ```ts - * import { LockAcquireTimeoutError } from '@supabase/auth-js' - * - * class CustomLockError extends LockAcquireTimeoutError { - * constructor() { - * super('Lock timed out') - * } - * } - * ``` + * @deprecated The v3 auth client doesn't acquire locks around auth + * operations, so this error never originates from `supabase.auth.*` calls. + * Direct callers of `navigatorLock` / `processLock` still receive it on + * acquire timeout. */ export abstract class LockAcquireTimeoutError extends Error { public readonly isAcquireTimeout = true @@ -40,25 +43,15 @@ export abstract class LockAcquireTimeoutError extends Error { } /** - * Error thrown when the browser Navigator Lock API fails to acquire a lock. - * - * @example - * ```ts - * import { NavigatorLockAcquireTimeoutError } from '@supabase/auth-js' - * - * throw new NavigatorLockAcquireTimeoutError('Lock timed out') - * ``` + * @deprecated The v3 auth client doesn't call `navigator.locks`, so this + * error never originates from `supabase.auth.*` calls. Direct callers of + * `navigatorLock` still receive it on acquire timeout. */ export class NavigatorLockAcquireTimeoutError extends LockAcquireTimeoutError {} /** - * Error thrown when the process-level lock helper cannot acquire a lock. - * - * @example - * ```ts - * import { ProcessLockAcquireTimeoutError } from '@supabase/auth-js' - * - * throw new ProcessLockAcquireTimeoutError('Lock timed out') - * ``` + * @deprecated The v3 auth client doesn't run `processLock`, so this error + * never originates from `supabase.auth.*` calls. Direct callers of + * `processLock` still receive it on acquire timeout. */ export class ProcessLockAcquireTimeoutError extends LockAcquireTimeoutError {} @@ -86,12 +79,10 @@ export class ProcessLockAcquireTimeoutError extends LockAcquireTimeoutError {} * will time out after so many milliseconds. An error is * a timeout if it has `isAcquireTimeout` set to true. * @param fn The operation to run once the lock is acquired. - * @example - * ```ts - * await navigatorLock('sync-user', 1000, async () => { - * await refreshSession() - * }) - * ``` + * + * @deprecated The v3 auth client coordinates refreshes itself and the + * server resolves concurrent refresh races, so passing `{ lock: navigatorLock }` + * to it has no effect. You can safely drop the import from your client setup. */ export async function navigatorLock( name: string, @@ -320,6 +311,11 @@ const PROCESS_LOCKS: { [name: string]: Promise } = {} * will time out after so many milliseconds. An error is * a timeout if it has `isAcquireTimeout` set to true. * @param fn The operation to run once the lock is acquired. + * + * @deprecated The v3 auth client coordinates refreshes itself and the + * server resolves concurrent refresh races, so passing `{ lock: processLock }` + * to it has no effect. You can safely drop the import from your client setup. + * * @example * ```ts * await processLock('migrate', 5000, async () => { diff --git a/packages/core/auth-js/src/lib/types.ts b/packages/core/auth-js/src/lib/types.ts index 63382a001..ae606c4d9 100644 --- a/packages/core/auth-js/src/lib/types.ts +++ b/packages/core/auth-js/src/lib/types.ts @@ -126,12 +126,9 @@ export type GoTrueClientOptions = { /* If debug messages are emitted. Can be used to inspect the behavior of the library. If set to a function, the provided function will be used instead of `console.log()` to perform the logging. */ debug?: boolean | ((message: string, ...args: any[]) => void) /** - * Provide your own locking mechanism based on the environment. By default, - * `navigatorLock` (Web Locks API) is used in browser environments when - * `persistSession` is true. Falls back to an in-process lock for non-browser - * environments (e.g. React Native). - * - * @experimental + * @deprecated Ignored in v3. The client coordinates refreshes itself and + * the server handles cross-tab races, so you can safely remove this from + * your constructor options. */ lock?: LockFunc /** @@ -145,30 +142,9 @@ export type GoTrueClientOptions = { */ throwOnError?: boolean /** - * The maximum time in milliseconds to wait for acquiring a cross-tab synchronization lock. - * - * When multiple browser tabs or windows use the auth client simultaneously, they coordinate - * via the Web Locks API to prevent race conditions during session refresh and other operations. - * This timeout controls how long to wait before attempting lock recovery. - * - * - **Positive value**: Wait up to this many milliseconds. If the lock is still held, attempt - * automatic recovery by stealing it (the previous holder is evicted, its callback continues - * to completion without exclusive access). This recovers from orphaned locks caused by - * React Strict Mode double-mount, storage API hangs, or aborted operations. - * - **Zero (0)**: Fail immediately if the lock is unavailable; throws `LockAcquireTimeoutError` - * (check `error.isAcquireTimeout === true`). - * - **Negative value**: Wait indefinitely — can cause permanent deadlocks if the lock is orphaned. - * - * @default 5000 - * - * @example - * ```ts - * const client = createClient(url, key, { - * auth: { - * lockAcquireTimeout: 5000, // 5 seconds, then steal orphaned lock - * }, - * }) - * ``` + * @deprecated The v3 client doesn't acquire a lock around auth operations, + * so this timeout has nothing to bound. You can safely remove it from your + * constructor options. */ lockAcquireTimeout?: number diff --git a/packages/core/auth-js/test/GoTrueClient.browser.test.ts b/packages/core/auth-js/test/GoTrueClient.browser.test.ts index 3d1c527c2..22c72147d 100644 --- a/packages/core/auth-js/test/GoTrueClient.browser.test.ts +++ b/packages/core/auth-js/test/GoTrueClient.browser.test.ts @@ -650,51 +650,22 @@ describe('GoTrueClient BroadcastChannel', () => { }) }) -describe('Browser locks functionality', () => { - it('should use navigator locks when available', () => { - // Mock navigator.locks - const mockLock = { name: 'test-lock' } - const mockRequest = jest - .fn() - .mockImplementation((_, __, callback) => Promise.resolve(callback(mockLock))) - +describe('Lockless coordination: navigator.locks should NOT be invoked', () => { + it('initializes without touching navigator.locks', async () => { + const mockRequest = jest.fn() Object.defineProperty(navigator, 'locks', { value: { request: mockRequest }, writable: true, }) - // Test navigator locks usage in GoTrueClient const client = getClientWithSpecificStorage({ getItem: jest.fn(), setItem: jest.fn(), removeItem: jest.fn(), }) - - expect(client).toBeDefined() - }) - - it('should handle _acquireLock with empty pendingInLock', async () => { - const client = getClientWithSpecificStorage({ - getItem: jest.fn(), - setItem: jest.fn(), - removeItem: jest.fn(), - }) - - // Mock navigator.locks - const mockLock = { name: 'test-lock' } - const mockRequest = jest - .fn() - .mockImplementation((_, __, callback) => Promise.resolve(callback(mockLock))) - - Object.defineProperty(navigator, 'locks', { - value: { request: mockRequest }, - writable: true, - }) - - // Initialize client to trigger lock acquisition await client.initialize() - expect(client).toBeDefined() + expect(mockRequest).not.toHaveBeenCalled() }) }) @@ -1225,36 +1196,18 @@ describe('Storage and User Storage Combinations', () => { }) }) -describe('Lock Mechanism Branches', () => { - it('should handle lock acquisition timeout', async () => { - const slowLock = jest - .fn() - .mockImplementation(() => new Promise((resolve) => setTimeout(resolve, 100))) +describe('Lockless backwards-compatibility: deprecated `lock` option', () => { + it('a custom `lock` function passed to the constructor is never invoked', async () => { + const customLock = jest.fn() const client = new (require('../src/GoTrueClient').default)({ url: 'http://localhost:9999', autoRefreshToken: false, - lock: slowLock, + lock: customLock, }) await client.initialize() - expect(client).toBeDefined() - }) - - it('should handle lock release errors', async () => { - const errorLock = jest.fn().mockImplementation(() => ({ - acquire: jest.fn().mockResolvedValue(undefined), - release: jest.fn().mockRejectedValue(new Error('Lock release error')), - })) - - const client = new (require('../src/GoTrueClient').default)({ - url: 'http://localhost:9999', - autoRefreshToken: false, - lock: errorLock, - }) - - await client.initialize() - expect(client).toBeDefined() + expect(customLock).not.toHaveBeenCalled() }) }) diff --git a/packages/core/auth-js/test/GoTrueClient.test.ts b/packages/core/auth-js/test/GoTrueClient.test.ts index f83dba300..32880152e 100644 --- a/packages/core/auth-js/test/GoTrueClient.test.ts +++ b/packages/core/auth-js/test/GoTrueClient.test.ts @@ -56,7 +56,7 @@ describe('GoTrueClient', () => { expect(debugSpy).toHaveBeenCalled() }) - test('should handle custom lock implementation', async () => { + test('accepts a custom lock implementation but never invokes it (deprecated)', async () => { const customLock = jest.fn().mockResolvedValue(undefined) const client = new GoTrueClient({ url: 'http://localhost:9999', @@ -64,7 +64,9 @@ describe('GoTrueClient', () => { }) await client.initialize() - expect(customLock).toHaveBeenCalled() + // The `lock` option is accepted for backwards compatibility but the + // client doesn't invoke it. + expect(customLock).not.toHaveBeenCalled() }) test('should handle userStorage configuration', async () => { @@ -3353,23 +3355,11 @@ describe('SSO Authentication', () => { }) }) -describe('Lock functionality', () => { - test('_acquireLock should execute function when lock is acquired', async () => { - const mockFn = jest.fn().mockResolvedValue('success') - // @ts-expect-error 'Allow access to private _acquireLock' - const result = await authWithSession._acquireLock(1000, mockFn) - expect(result).toBe('success') - expect(mockFn).toHaveBeenCalled() - }) - - test('_acquireLock should throw error when lock acquisition times out', async () => { - let shouldFail = false - const mockLock = jest.fn().mockImplementation(async (name, timeout, fn) => { - if (shouldFail) { - throw new Error('Lock acquisition timeout') - } - return fn() - }) +describe('Lockless coordination', () => { + test('`lock` option is accepted but not invoked by the client', async () => { + const mockLock = jest + .fn() + .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => fn()) const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, @@ -3378,90 +3368,159 @@ describe('Lock functionality', () => { persistSession: false, }) - // Wait for initialization to complete - await new Promise((resolve) => setTimeout(resolve, 100)) + await client.initialize() + await client.getSession() - // Now make the lock fail - shouldFail = true + // The `lock` option is accepted for backwards compatibility; the client + // doesn't route auth operations through it. + expect(mockLock).not.toHaveBeenCalled() + }) - const mockFn = jest.fn() - // @ts-expect-error 'Allow access to private _acquireLock' - await expect(client._acquireLock(1000, mockFn)).rejects.toThrow('Lock acquisition timeout') - expect(mockFn).not.toHaveBeenCalled() + test('`lockAcquireTimeout` option is accepted but ignored', async () => { + expect( + () => + new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + autoRefreshToken: false, + persistSession: false, + lockAcquireTimeout: 12345, + }) + ).not.toThrow() }) - test('should use custom lockAcquireTimeout when provided', async () => { - const capturedTimeouts: number[] = [] - const mockLock = jest - .fn() - .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => { - capturedTimeouts.push(timeout) - return fn() - }) + test('subscriber callback may call other auth methods without deadlock', async () => { + // Auth methods invoked from inside an onAuthStateChange callback complete + // without re-entering a held lock, because notification is not gated by + // one. + const observed: string[] = [] - const customTimeout = 20000 // 20 seconds (different from default 10s) + const { + data: { subscription }, + } = authWithSession.onAuthStateChange(async (event) => { + observed.push(event) + if (event === 'INITIAL_SESSION') { + const { data } = await authWithSession.getSession() + observed.push(`got-session:${data.session ? 'present' : 'null'}`) + } + }) + + // Allow the initial-session emit to flush. + await new Promise((resolve) => setTimeout(resolve, 100)) + + subscription.unsubscribe() + expect(observed).toContain('INITIAL_SESSION') + expect(observed.some((s) => s.startsWith('got-session:'))).toBe(true) + }) +}) + +describe('dispose() lifecycle', () => { + test('idempotent: safe to call repeatedly', async () => { const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, - lock: mockLock, autoRefreshToken: false, persistSession: false, - lockAcquireTimeout: customTimeout, }) - await client.initialize() - // Verify that the custom timeout was passed to the lock function - expect(mockLock).toHaveBeenCalled() - expect(capturedTimeouts).toContain(customTimeout) + await expect(client.dispose()).resolves.toBeUndefined() + await expect(client.dispose()).resolves.toBeUndefined() }) - test('should use default lockAcquireTimeout (10000ms) when not provided', async () => { - const capturedTimeouts: number[] = [] - const mockLock = jest - .fn() - .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => { - capturedTimeouts.push(timeout) - return fn() - }) - + test('clears registered onAuthStateChange subscribers', async () => { const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, - lock: mockLock, autoRefreshToken: false, persistSession: false, - // lockAcquireTimeout not provided, should default to 5000 }) - await client.initialize() - // Verify that the default timeout (5000 = 5 seconds) was used - expect(mockLock).toHaveBeenCalled() - expect(capturedTimeouts).toContain(5000) - }) + const cb = jest.fn() + client.onAuthStateChange(cb) - test('should pass negative timeout to lock for indefinite wait', async () => { - const capturedTimeouts: number[] = [] - const mockLock = jest - .fn() - .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => { - capturedTimeouts.push(timeout) - return fn() - }) + // @ts-expect-error access protected field for verification + expect(client.stateChangeEmitters.size).toBeGreaterThan(0) + + await client.dispose() + // @ts-expect-error access protected field for verification + expect(client.stateChangeEmitters.size).toBe(0) + }) + + test('stops the auto-refresh ticker', async () => { const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, - lock: mockLock, - autoRefreshToken: false, + autoRefreshToken: true, persistSession: false, - lockAcquireTimeout: -1, // Indefinite wait (not recommended) }) + await client.initialize() + await client.startAutoRefresh() + + // @ts-expect-error access protected field for verification + expect(client.autoRefreshTicker).not.toBeNull() + + await client.dispose() + + // @ts-expect-error access protected field for verification + expect(client.autoRefreshTicker).toBeNull() + }) +}) +describe('Refresh commit guard (signOut-during-refresh race)', () => { + test('discards rotated tokens when storage was cleared mid-flight', async () => { + const storage = memoryLocalStorageAdapter() + const client = new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + storage, + autoRefreshToken: false, + persistSession: true, + }) await client.initialize() - // Verify that negative timeout was passed through - expect(mockLock).toHaveBeenCalled() - expect(capturedTimeouts).toContain(-1) + // Plant a session in storage with refresh_token R1. + const R1 = 'refresh-token-r1' + await setItemAsync(storage, STORAGE_KEY, { + access_token: 'jwt.accesstoken.signature', + refresh_token: R1, + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-1', email: 'u@example.com' } as any, + } as Session) + + // Simulate a successful refresh that returns rotated tokens (R2) while + // storage was cleared between fetch start and continuation. + // @ts-expect-error access protected for test + client._refreshAccessToken = jest.fn(async () => { + // Concurrent signOut clears storage before our refresh continuation runs. + // @ts-expect-error access protected for test + await client._removeSession() + return { + data: { + session: { + access_token: 'jwt.new.signature', + refresh_token: 'refresh-token-r2', + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-1', email: 'u@example.com' }, + } as any, + user: { id: 'user-1', email: 'u@example.com' } as any, + }, + error: null, + } as any + }) + + // @ts-expect-error access protected for test + const result = await client._callRefreshToken(R1) + + // Commit guard discards the rotated tokens. + expect(result.error).not.toBeNull() + expect((result.error as Error).name).toBe('AuthRefreshDiscardedError') + + // Storage stays cleared; rotated tokens were NOT written back. + const stored = await storage.getItem(STORAGE_KEY) + expect(stored).toBeNull() }) }) diff --git a/packages/core/supabase-js/src/lib/types.ts b/packages/core/supabase-js/src/lib/types.ts index 3ba0d51b3..fe061c2e2 100644 --- a/packages/core/supabase-js/src/lib/types.ts +++ b/packages/core/supabase-js/src/lib/types.ts @@ -114,9 +114,10 @@ export type SupabaseClientOptions = { */ debug?: SupabaseAuthClientOptions['debug'] /** - * Provide your own locking mechanism based on the environment. By default no locking is done at this time. - * - * @experimental + * @deprecated In v3 the auth client coordinates refreshes itself and the + * server resolves cross-tab races, so any value forwarded through this + * option has no effect. You can safely remove it from your `createClient` + * options. */ lock?: SupabaseAuthClientOptions['lock'] /** @@ -132,11 +133,9 @@ export type SupabaseClientOptions = { */ experimental?: SupabaseAuthClientOptions['experimental'] /** - * Maximum time in milliseconds to wait when acquiring the auth lock before - * stealing it from the previous holder. See `GoTrueClientOptions.lockAcquireTimeout` - * for full semantics (zero fails immediately, negative waits indefinitely). - * - * @default 5000 + * @deprecated The v3 auth client doesn't acquire a lock around auth + * operations, so this timeout has nothing to bound. You can safely + * remove it from your `createClient` options. */ lockAcquireTimeout?: SupabaseAuthClientOptions['lockAcquireTimeout'] /**