diff --git a/.changeset/fix-429-unauthenticated.md b/.changeset/fix-429-unauthenticated.md new file mode 100644 index 00000000000..cf500a5ef1c --- /dev/null +++ b/.changeset/fix-429-unauthenticated.md @@ -0,0 +1,6 @@ +--- +'@clerk/clerk-js': patch +'@clerk/shared': patch +--- + +Narrow the error conditions that trigger the unauthenticated flow (sign-out) to only high-confidence authentication failures (401, 422). Previously, all 4xx errors — including 429 rate limits — were treated as auth failures, which could sign users out during transient rate limiting. Non-auth errors from `setActive` now propagate to the caller instead of being silently swallowed. diff --git a/integration/tests/resiliency.test.ts b/integration/tests/resiliency.test.ts index db031e34a09..dbb3dab9ceb 100644 --- a/integration/tests/resiliency.test.ts +++ b/integration/tests/resiliency.test.ts @@ -4,6 +4,21 @@ import { appConfigs } from '../presets'; import type { FakeUser } from '../testUtils'; import { createTestUtils, testAgainstRunningApps } from '../testUtils'; +const make429ClerkResponse = () => ({ + status: 429, + headers: { 'retry-after': '1' }, + body: JSON.stringify({ + errors: [ + { + message: 'Too many requests', + long_message: 'Too many requests. Please retry later.', + code: 'rate_limit_exceeded', + }, + ], + clerk_trace_id: 'some-trace-id', + }), +}); + const make500ClerkResponse = () => ({ status: 500, body: JSON.stringify({ @@ -296,6 +311,73 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('resilienc }); }); + test.describe('429 rate limit resiliency', () => { + test('setActive surfaces 429 error to the developer instead of silently swallowing', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await u.po.signIn.goTo(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + await u.po.expect.toBeSignedIn(); + + // Intercept touch requests to return 429 + await page.route('**/v1/client/sessions/*/touch**', route => { + return route.fulfill(make429ClerkResponse()); + }); + + // setActive should surface the 429 error so the developer can handle it + const error = await page.evaluate(async () => { + const session = window.Clerk?.session; + if (!session) { + return null; + } + try { + await window.Clerk?.setActive({ session }); + return null; + } catch (e: any) { + return { status: e.status, message: e.message }; + } + }); + + expect(error).not.toBeNull(); + expect(error!.status).toBe(429); + + await page.unrouteAll(); + + // The user must still be signed in — 429 should not trigger handleUnauthenticated + await u.po.expect.toBeSignedIn(); + }); + + test('429 on /tokens does not cause recursive handleUnauthenticated calls', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await u.po.signIn.goTo(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + await u.po.expect.toBeSignedIn(); + + let clientRequestCount = 0; + await page.route('**/v1/client?**', route => { + clientRequestCount++; + return route.continue(); + }); + + // Intercept token requests to return 429 + await page.route('**/v1/client/sessions/*/tokens**', route => { + return route.fulfill(make429ClerkResponse()); + }); + + // Clear the token cache so the next poller tick makes a fresh network request + await page.evaluate(() => window.Clerk?.session?.clearCache()); + + await page.waitForTimeout(3_000); + + await page.unrouteAll(); + + // Without the fix, 429 on /tokens would trigger handleUnauthenticated → Client.fetch loop. + // With the fix, /v1/client should only see normal poller activity (not hundreds of requests). + expect(clientRequestCount).toBeLessThan(5); + }); + }); + test.describe('clerk-js script loading', () => { test('recovers from transient network failure on clerk-js script load', async ({ page, context }) => { const u = createTestUtils({ app, page, context }); diff --git a/packages/clerk-js/src/core/auth/AuthCookieService.ts b/packages/clerk-js/src/core/auth/AuthCookieService.ts index a20a3cd4635..7a384a70866 100644 --- a/packages/clerk-js/src/core/auth/AuthCookieService.ts +++ b/packages/clerk-js/src/core/auth/AuthCookieService.ts @@ -3,7 +3,12 @@ import type { createClerkEventBus } from '@clerk/shared/clerkEventBus'; import { clerkEvents } from '@clerk/shared/clerkEventBus'; import type { createCookieHandler } from '@clerk/shared/cookie'; import { setDevBrowserInURL } from '@clerk/shared/devBrowser'; -import { is4xxError, isClerkAPIResponseError, isClerkRuntimeError, isNetworkError } from '@clerk/shared/error'; +import { + isClerkAPIResponseError, + isClerkRuntimeError, + isNetworkError, + isUnauthenticatedError, +} from '@clerk/shared/error'; import type { Clerk, InstanceType } from '@clerk/shared/types'; import { noop } from '@clerk/shared/utils'; @@ -215,8 +220,7 @@ export class AuthCookieService { return; } - //sign user out if a 4XX error - if (is4xxError(e)) { + if (isUnauthenticatedError(e)) { void this.clerk.handleUnauthenticated().catch(noop); return; } diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index b6ac5321a41..1a362b2edac 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -8,6 +8,7 @@ import { is4xxError, isClerkAPIResponseError, isClerkRuntimeError, + isUnauthenticatedError, } from '@clerk/shared/error'; import { disabledAllAPIKeysFeatures, @@ -1595,7 +1596,7 @@ export class Clerk implements ClerkInterface { this.updateClient(updatedClient, { __internal_dangerouslySkipEmit: true }); } } catch (e) { - if (is4xxError(e)) { + if (isUnauthenticatedError(e)) { void this.handleUnauthenticated(); } else { throw e; @@ -3174,8 +3175,10 @@ export class Clerk implements ClerkInterface { } await session.touch().catch(e => { - if (is4xxError(e)) { + if (isUnauthenticatedError(e)) { void this.handleUnauthenticated(); + } else { + throw e; } }); }; diff --git a/packages/clerk-js/src/core/resources/Session.ts b/packages/clerk-js/src/core/resources/Session.ts index f839def8c18..303a4e428fe 100644 --- a/packages/clerk-js/src/core/resources/Session.ts +++ b/packages/clerk-js/src/core/resources/Session.ts @@ -1,6 +1,12 @@ import { createCheckAuthorization } from '@clerk/shared/authorization'; import { isValidBrowserOnline } from '@clerk/shared/browser'; -import { ClerkOfflineError, ClerkWebAuthnError, is4xxError, MissingExpiredTokenError } from '@clerk/shared/error'; +import { + ClerkOfflineError, + ClerkWebAuthnError, + is4xxError, + is429Error, + MissingExpiredTokenError, +} from '@clerk/shared/error'; import { convertJSONToPublicKeyRequestOptions, serializePublicKeyCredentialAssertion, @@ -146,9 +152,10 @@ export class Session extends BaseResource implements SessionResource { }; getToken: GetToken = async (options?: GetTokenOptions): Promise => { - // This will retry the getToken call if it fails with a non-4xx error - // For offline state, we use shorter retries (~15s total) before throwing ClerkOfflineError - // For other errors, we retry up to 8 times over ~3 minutes + // Retry on transient failures (5xx, network errors, 429 rate limits). + // Do not retry on deterministic client errors (4xx) — they won't resolve on their own. + // For offline state, we use shorter retries (~15s total) before throwing ClerkOfflineError. + // For other errors, we retry up to 8 times over ~3 minutes. try { const result = await retry(() => this._getToken(options), { factor: 1.55, @@ -156,7 +163,7 @@ export class Session extends BaseResource implements SessionResource { maxDelayBetweenRetries: 50 * 1_000, jitter: false, shouldRetry: (error, iterationsCount) => { - if (is4xxError(error)) { + if (is4xxError(error) && !is429Error(error)) { return false; } diff --git a/packages/shared/src/__tests__/error.spec.ts b/packages/shared/src/__tests__/error.spec.ts index 98fe6a85324..b4d21e8e45d 100644 --- a/packages/shared/src/__tests__/error.spec.ts +++ b/packages/shared/src/__tests__/error.spec.ts @@ -1,7 +1,15 @@ import { describe, expect, it } from 'vitest'; import type { ErrorThrowerOptions } from '../error'; -import { buildErrorThrower, ClerkOfflineError, ClerkRuntimeError, isClerkRuntimeError } from '../error'; +import { + buildErrorThrower, + ClerkOfflineError, + ClerkRuntimeError, + is4xxError, + is429Error, + isClerkRuntimeError, + isUnauthenticatedError, +} from '../error'; describe('ErrorThrower', () => { const errorThrower = buildErrorThrower({ packageName: '@clerk/test-package' }); @@ -63,6 +71,58 @@ describe('ClerkRuntimeError', () => { }); }); +describe('is4xxError', () => { + it('returns true for 4xx status codes', () => { + expect(is4xxError({ status: 400 })).toBe(true); + expect(is4xxError({ status: 401 })).toBe(true); + expect(is4xxError({ status: 429 })).toBe(true); + expect(is4xxError({ status: 499 })).toBe(true); + }); + + it('returns false for non-4xx status codes', () => { + expect(is4xxError({ status: 200 })).toBe(false); + expect(is4xxError({ status: 500 })).toBe(false); + expect(is4xxError({})).toBe(false); + expect(is4xxError(null)).toBe(false); + }); +}); + +describe('is429Error', () => { + it('returns true for 429 status', () => { + expect(is429Error({ status: 429 })).toBe(true); + }); + + it('returns false for other status codes', () => { + expect(is429Error({ status: 400 })).toBe(false); + expect(is429Error({ status: 401 })).toBe(false); + expect(is429Error({ status: 500 })).toBe(false); + expect(is429Error({})).toBe(false); + expect(is429Error(null)).toBe(false); + expect(is429Error(undefined)).toBe(false); + }); +}); + +describe('isUnauthenticatedError', () => { + it('returns true for authentication failure status codes', () => { + expect(isUnauthenticatedError({ status: 401 })).toBe(true); + expect(isUnauthenticatedError({ status: 422 })).toBe(true); + }); + + it('returns false for other 4xx status codes', () => { + expect(isUnauthenticatedError({ status: 400 })).toBe(false); + expect(isUnauthenticatedError({ status: 403 })).toBe(false); + expect(isUnauthenticatedError({ status: 404 })).toBe(false); + expect(isUnauthenticatedError({ status: 429 })).toBe(false); + }); + + it('returns false for non-4xx errors', () => { + expect(isUnauthenticatedError({ status: 200 })).toBe(false); + expect(isUnauthenticatedError({ status: 500 })).toBe(false); + expect(isUnauthenticatedError({})).toBe(false); + expect(isUnauthenticatedError(null)).toBe(false); + }); +}); + describe('ClerkOfflineError', () => { it('is an instance of ClerkRuntimeError', () => { const error = new ClerkOfflineError('Network request failed'); diff --git a/packages/shared/src/error.ts b/packages/shared/src/error.ts index b75e569a5db..37eb5e41bb7 100644 --- a/packages/shared/src/error.ts +++ b/packages/shared/src/error.ts @@ -17,6 +17,7 @@ export { ClerkRuntimeError, isClerkRuntimeError } from './errors/clerkRuntimeErr export { ClerkWebAuthnError } from './errors/webAuthNError'; export { + is429Error, is4xxError, isCaptchaError, isEmailLinkError, @@ -26,6 +27,7 @@ export { isPasswordPwnedError, isPasswordCompromisedError, isReverificationCancelledError, + isUnauthenticatedError, isUnauthorizedError, isUserLockedError, } from './errors/helpers'; diff --git a/packages/shared/src/errors/helpers.ts b/packages/shared/src/errors/helpers.ts index b95f55d5a8c..03566580db3 100644 --- a/packages/shared/src/errors/helpers.ts +++ b/packages/shared/src/errors/helpers.ts @@ -37,6 +37,36 @@ export function is4xxError(e: any): boolean { return !!status && status >= 400 && status < 500; } +/** + * Checks if the provided error is a 429 (Too Many Requests) error. + * + * @internal + */ +export function is429Error(e: any): boolean { + return e?.status === 429; +} + +/** + * Checks if the provided error indicates the user's session is no longer valid + * and should trigger the unauthenticated flow (e.g. sign-out / redirect to sign-in). + * + * Only matches explicit authentication failure status codes: + * - 401: session is invalid or expired + * - 422: invalid session state (e.g. missing_expired_token) + * + * 404 is intentionally excluded despite being returned for "session not found", + * because it's also returned for unrelated resources (org not found, JWT template + * not found) and shares the same `resource_not_found` error code, making it + * impossible to distinguish. Session-not-found 401s are already handled directly + * by Base._fetch. + * + * @internal + */ +export function isUnauthenticatedError(e: any): boolean { + const status = e?.status; + return status === 401 || status === 422; +} + /** * Checks if the provided error is a network error. *