From 3c9c71defa31fbb9eb0991dfd06d44c2f1ee0a82 Mon Sep 17 00:00:00 2001 From: Theo Zourzouvillys Date: Fri, 21 Aug 2026 17:10:03 -0800 Subject: [PATCH 1/3] feat(clerk-js,shared,ui): bound loading a verification, not running one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SDK raced the whole verification against a fixed 60s wall: loading the module, running it, and the round-trip that follows. That is a duration the SDK is not in a position to judge. Which verification runs is decided by the backend per request, long after the bundle shipped, and what it does varies from waiting on a person to transferring data over an unknown connection, so no single number is right for all of them. When the wall tripped, a verification that was progressing normally was aborted and reported to the user as a timeout, and because retrying restarts the work from the beginning, any connection slow enough to trip it once would trip it again. The bound now covers only loading the module, and stops there: once the module takes control it governs its own duration and the SDK imposes no limit. The default is 60s, deliberately generous, because nothing legitimate waits on this timer — its only job is a connection that is accepted and then never answered, since every other load failure already rejects on its own, a blocking Content-Security-Policy included. challenge_load_timeout_ms overrides that default, on a loader or on the instance, resolved loader first. Per loader because loaders roll out gradually: while a new one ramps two are live for the same instance at once, and the new one may need a different value from the one it replaces. Which loader a browser was assigned is a random draw per page load and cannot be recomputed from the config, so Protect reports what the applied loader asked for. Absent means "inherit" at both levels, so the default stays one number under our control rather than being frozen into stored settings. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/protect-check-load-timeout.md | 7 ++ .../protectChallengeLoadTimeout.test.ts | 77 ++++++++++++++++ packages/clerk-js/src/core/clerk.ts | 10 +++ packages/clerk-js/src/core/protect.ts | 22 +++++ .../src/core/resources/ProtectConfig.ts | 3 + .../clerk-js/__tests__/protectCheck.test.ts | 88 ++++++++++++++++++- .../src/internal/clerk-js/protectCheck.ts | 48 +++++++++- packages/shared/src/types/clerk.ts | 10 +++ packages/shared/src/types/protectConfig.ts | 20 +++++ .../ui/src/hooks/useProtectCheckRunner.ts | 45 +++++----- 10 files changed, 305 insertions(+), 25 deletions(-) create mode 100644 .changeset/protect-check-load-timeout.md create mode 100644 packages/clerk-js/src/core/__tests__/protectChallengeLoadTimeout.test.ts diff --git a/.changeset/protect-check-load-timeout.md b/.changeset/protect-check-load-timeout.md new file mode 100644 index 00000000000..d5888783acd --- /dev/null +++ b/.changeset/protect-check-load-timeout.md @@ -0,0 +1,7 @@ +--- +'@clerk/clerk-js': patch +'@clerk/shared': patch +'@clerk/ui': patch +--- + +Bound loading the verification module rather than running it. The SDK no longer imposes a time limit on a verification once it has started, since how long one legitimately takes depends on the verification itself. The load timeout can be set per loader, falling back to a per-instance value and then to the default. diff --git a/packages/clerk-js/src/core/__tests__/protectChallengeLoadTimeout.test.ts b/packages/clerk-js/src/core/__tests__/protectChallengeLoadTimeout.test.ts new file mode 100644 index 00000000000..348cf62919a --- /dev/null +++ b/packages/clerk-js/src/core/__tests__/protectChallengeLoadTimeout.test.ts @@ -0,0 +1,77 @@ +import type { ProtectLoader } from '@clerk/shared/types'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { Protect } from '../protect'; +import { __internal_resetProtectStorage } from '../protectSession'; +import type { Environment } from '../resources'; + +const environment = (loaders: unknown[]): Environment => ({ protectConfig: { loaders } }) as unknown as Environment; + +/** No `src`, so jsdom does not fetch a real URL and race the assertions here. */ +const loader = (overrides: Partial = {}): ProtectLoader => ({ + target: 'head', + type: 'script', + ...overrides, +}); + +describe('Protect.challengeLoadTimeoutMs', () => { + beforeEach(() => { + document.head.innerHTML = ''; + document.body.innerHTML = ''; + __internal_resetProtectStorage(); + vi.restoreAllMocks(); + }); + + it('is undefined when no loader asks for one, so the caller falls back', () => { + const protect = new Protect(); + protect.load(environment([loader()])); + + expect(protect.challengeLoadTimeoutMs).toBeUndefined(); + }); + + it('reports the value from the loader this browser was assigned', () => { + const protect = new Protect(); + protect.load(environment([loader({ challenge_load_timeout_ms: 25_000 })])); + + expect(protect.challengeLoadTimeoutMs).toBe(25_000); + }); + + // The whole point of putting it on the loader: while a new loader ramps, two are live for the + // same instance, and only the one this browser actually got may speak for it. + it('ignores a loader the rollout dice excluded', () => { + vi.spyOn(Math, 'random').mockReturnValue(0.9); + + const protect = new Protect(); + protect.load( + environment([ + // A 10% ramp, excluded at random() 0.9. + loader({ rollout: 0.1, challenge_load_timeout_ms: 25_000 }), + // The incumbent, which this browser does get. + loader({ challenge_load_timeout_ms: 45_000 }), + ]), + ); + + expect(protect.challengeLoadTimeoutMs).toBe(45_000); + }); + + it('reports the ramping loader once the dice include it', () => { + vi.spyOn(Math, 'random').mockReturnValue(0.01); + + const protect = new Protect(); + protect.load(environment([loader({ rollout: 0.1, challenge_load_timeout_ms: 25_000 }), loader()])); + + expect(protect.challengeLoadTimeoutMs).toBe(25_000); + }); + + it('ignores a non-positive or non-numeric value rather than passing it on', () => { + const protect = new Protect(); + protect.load( + environment([ + loader({ challenge_load_timeout_ms: 0 }), + loader({ challenge_load_timeout_ms: 'soon' as unknown as number }), + ]), + ); + + expect(protect.challengeLoadTimeoutMs).toBeUndefined(); + }); +}); diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 7fee3cd05f4..878d73b5452 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -327,6 +327,16 @@ export class Clerk implements ClerkInterface { return this.#oauthTransport; } + /** + * The verification-module load timeout asked for by the loader THIS browser was assigned, or + * undefined when it asked for nothing. Exposed because the assignment is a random draw per page + * load and cannot be recomputed from the environment config; callers fall back to the + * instance-wide value on that config, and then to the SDK default. + */ + get __internal_protectChallengeLoadTimeoutMs(): number | undefined { + return this.#protect?.challengeLoadTimeoutMs; + } + public __internal_getCachedResources: | (() => Promise<{ client: ClientJSONSnapshot | null; environment: EnvironmentJSONSnapshot | null }>) | undefined; diff --git a/packages/clerk-js/src/core/protect.ts b/packages/clerk-js/src/core/protect.ts index 259de41ee36..e52b382ccbc 100644 --- a/packages/clerk-js/src/core/protect.ts +++ b/packages/clerk-js/src/core/protect.ts @@ -8,6 +8,21 @@ import type { Environment } from './resources'; export class Protect { #initialized: boolean = false; #session?: ProtectSession; + #challengeLoadTimeoutMs?: number; + + /** + * The verification-module LOAD timeout asked for by the loader this browser was assigned, or + * undefined when none asked for one — in which case the caller falls back to the instance-wide + * value and then to the SDK default. + * + * Resolved from the APPLIED loaders rather than from the config, because rollout is decided by + * a random draw per page load: a loader being ramped can carry its own value without changing + * anything for browsers still on the loader it replaces, and which one a browser got cannot be + * recomputed afterwards. + */ + get challengeLoadTimeoutMs(): number | undefined { + return this.#challengeLoadTimeoutMs; + } load(env: Environment): void { const config = env?.protectConfig; @@ -40,6 +55,13 @@ export class Protect { // loaders we are actually going to apply. const loaders = configured.filter(loader => isLoader(loader) && inRollout(loader)); + // Read off the applied set, before any of them run: this only describes config, so a loader + // that later fails to be placed has still spoken for the browser it was assigned to. First + // one that asks for it wins. + this.#challengeLoadTimeoutMs = loaders.find( + loader => typeof loader.challenge_load_timeout_ms === 'number' && loader.challenge_load_timeout_ms > 0, + )?.challenge_load_timeout_ms; + // Only an instance whose loaders reference the correlation id gets a session, so an instance // not using it is unaffected and stores nothing in the browser. const applyLoader: ApplyLoader = (loader, placeholders) => this.applyLoader(loader, placeholders); diff --git a/packages/clerk-js/src/core/resources/ProtectConfig.ts b/packages/clerk-js/src/core/resources/ProtectConfig.ts index 2a5b1a7cfb7..b5a093ede9b 100644 --- a/packages/clerk-js/src/core/resources/ProtectConfig.ts +++ b/packages/clerk-js/src/core/resources/ProtectConfig.ts @@ -11,6 +11,7 @@ export class ProtectConfig extends BaseResource implements ProtectConfigResource id: string = ''; loaders?: ProtectLoader[]; tokens_invalid_before?: number; + challenge_load_timeout_ms?: number; rollout?: number; public constructor(data: ProtectConfigJSON | ProtectConfigJSONSnapshot | null = null) { @@ -27,6 +28,7 @@ export class ProtectConfig extends BaseResource implements ProtectConfigResource this.id = this.withDefault(data.id, this.id); this.loaders = this.withDefault(data.loaders, this.loaders); this.tokens_invalid_before = this.withDefault(data.tokens_invalid_before, this.tokens_invalid_before); + this.challenge_load_timeout_ms = this.withDefault(data.challenge_load_timeout_ms, this.challenge_load_timeout_ms); return this; } @@ -37,6 +39,7 @@ export class ProtectConfig extends BaseResource implements ProtectConfigResource id: this.id, loaders: this.loaders, tokens_invalid_before: this.tokens_invalid_before, + challenge_load_timeout_ms: this.challenge_load_timeout_ms, }; } } diff --git a/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts b/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts index 1d0ac1cfdb6..dd58ea730a4 100644 --- a/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts +++ b/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts @@ -1,8 +1,8 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { ProtectCheckResource } from '@/types'; -import { executeProtectCheck } from '../protectCheck'; +import { DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS, executeProtectCheck } from '../protectCheck'; const fakeContainer = (): HTMLDivElement => ({}) as HTMLDivElement; @@ -199,4 +199,88 @@ describe('executeProtectCheck', () => { }); }); }); + + describe('the load bound covers the handoff and nothing after it', () => { + beforeEach(() => { + // Fake only the timer functions under test. Faking the whole clock also stalls the + // machinery that settles a dynamic import, so the module would never finish loading. + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + // A network that accepts the connection and then never answers is the one load failure that + // cannot report itself — every other one rejects the import on its own. + it('rejects as a load failure when the module never arrives', async () => { + vi.doMock('https://protect.example.com/sdk-hangs.js', () => new Promise(() => {})); + + const running = executeProtectCheck( + protectCheck({ sdkUrl: 'https://protect.example.com/sdk-hangs.js' }), + fakeContainer(), + ); + const assertion = expect(running).rejects.toMatchObject({ code: 'protect_check_script_load_failed' }); + + await vi.advanceTimersByTimeAsync(DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS + 1); + await assertion; + }); + + it('honours a per-instance loadTimeoutMs override instead of the default', async () => { + vi.doMock('https://protect.example.com/sdk-hangs-2.js', () => new Promise(() => {})); + + // Expressed relative to the default, and LONGER than it, so that outliving the default is + // itself the proof the override replaced it rather than racing alongside it. + const override = DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS * 2; + const running = executeProtectCheck( + protectCheck({ sdkUrl: 'https://protect.example.com/sdk-hangs-2.js' }), + fakeContainer(), + { loadTimeoutMs: override }, + ); + let settled = false; + const watch = running.then( + () => (settled = true), + () => (settled = true), + ); + + await vi.advanceTimersByTimeAsync(DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS + 1); + expect(settled).toBe(false); + + await vi.advanceTimersByTimeAsync(override); + await watch; + expect(settled).toBe(true); + }); + + // The point of the whole change: the challenge owns its own duration. A challenge running far + // longer than any load bound must still resolve — proof-of-transfer moves a server-chosen + // number of bytes, and a host-side wall would abort it as a "timeout". + it('never bounds the challenge once the module has taken control', async () => { + let running = false; + let finish!: (proofToken: string) => void; + vi.doMock('https://protect.example.com/sdk-slow.js', () => ({ + default: () => { + running = true; + return new Promise(resolve => (finish = resolve)); + }, + })); + + const loadTimeoutMs = 5_000; + const execution = executeProtectCheck( + protectCheck({ sdkUrl: 'https://protect.example.com/sdk-slow.js' }), + fakeContainer(), + { loadTimeoutMs }, + ); + const assertion = expect(execution).resolves.toBe('proof-after-ages'); + + // Wait on the real event loop until the module has taken control, so what follows is + // unambiguously time spent in the CHALLENGE rather than in the load. + while (!running) { + await new Promise(resolve => setImmediate(resolve)); + } + + // Burn far more time than the load bound. Nothing may abort the challenge for it. + await vi.advanceTimersByTimeAsync(360 * loadTimeoutMs); + finish('proof-after-ages'); + await assertion; + }); + }); }); diff --git a/packages/shared/src/internal/clerk-js/protectCheck.ts b/packages/shared/src/internal/clerk-js/protectCheck.ts index bcdcec43007..019399f7fb3 100644 --- a/packages/shared/src/internal/clerk-js/protectCheck.ts +++ b/packages/shared/src/internal/clerk-js/protectCheck.ts @@ -26,6 +26,50 @@ export interface ExecuteProtectCheckOptions { * Scripts that don't honor the signal will continue to run; this is best-effort by design. */ signal?: AbortSignal; + /** + * Overrides how long to wait for the challenge module to LOAD. Per-instance and per-loader + * config, since the right value depends on the population an instance serves; a non-positive + * or non-numeric value falls back to {@link DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS}. + * + * Bounds the handoff only — never the challenge. See the note on the constant. + */ + loadTimeoutMs?: number; +} + +/** + * Default bound on LOADING the challenge module. + * + * Its only job is a network that accepts a connection and then never answers, because every + * other load failure — a CSP block, DNS, a 404, a body that isn't a valid module — rejects the + * dynamic import on its own and needs no timer to notice. That makes a generous value the safe + * one: nothing legitimate is waiting on this timer, while a value tight enough to fire on a + * genuinely slow connection would fail a load that was going to succeed. + */ +export const DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS = 60_000; + +function resolveLoadTimeoutMs(configured: number | undefined): number { + return typeof configured === 'number' && configured > 0 ? configured : DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS; +} + +/** + * Races the dynamic import against `timeoutMs`, always clearing the timer so a fast load does not + * leave one pending. + * + * The bound stops at the import on purpose. Once `mod.default` is called the challenge owns its + * own deadline and the host imposes none: the host cannot know an honest duration for a challenge + * whose type is chosen server-side, per decision, and whose work it deliberately knows nothing + * about — waiting on a person, or moving a server-chosen number of bytes over an unknown link. A + * host-side wall over execution aborts legitimate challenges and reports them as timeouts, and + * since a re-run restarts the work, retrying cannot win on any connection slow enough to trip it. + */ +function importWithTimeout(url: string, timeoutMs: number): Promise> { + let timeoutId: ReturnType | undefined; + const expiry = new Promise((_, reject) => { + timeoutId = setTimeout(() => reject(new Error('Protect check script load timed out')), timeoutMs); + }); + return Promise.race([import(/* webpackIgnore: true */ url), expiry]).finally(() => { + clearTimeout(timeoutId); + }) as Promise>; } interface ScriptInitOptions { @@ -99,7 +143,7 @@ export async function executeProtectCheck( container: HTMLDivElement, options: ExecuteProtectCheckOptions = {}, ): Promise { - const { signal, setWidgetVisible } = options; + const { signal, setWidgetVisible, loadTimeoutMs } = options; const { sdkUrl, token, uiHints } = protectCheck; const validated = assertValidSdkUrl(sdkUrl); @@ -110,7 +154,7 @@ export async function executeProtectCheck( let mod: Record; try { - mod = await import(/* webpackIgnore: true */ validated.toString()); + mod = await importWithTimeout(validated.toString(), resolveLoadTimeoutMs(loadTimeoutMs)); } catch { // Surface a generic message and deliberately omit the original error: Chromium/Firefox embed // the sdk_url in the dynamic-import failure text, which a tampered response could plant in the UI. diff --git a/packages/shared/src/types/clerk.ts b/packages/shared/src/types/clerk.ts index 73e25a4950b..0f57591517d 100644 --- a/packages/shared/src/types/clerk.ts +++ b/packages/shared/src/types/clerk.ts @@ -332,6 +332,16 @@ export interface Clerk { */ __internal_moduleManager: ModuleManager | undefined; + /** + * The verification-module load timeout asked for by the loader this browser was assigned, or + * undefined when it asked for nothing. The assignment is randomized per page load, so it cannot + * be recomputed from the environment config; callers fall back to the instance-wide value on + * that config, and then to the SDK default. + * + * @internal + */ + __internal_protectChallengeLoadTimeoutMs?: number; + frontendApi: string; /** Your Clerk [Publishable Key](!publishable-key). */ diff --git a/packages/shared/src/types/protectConfig.ts b/packages/shared/src/types/protectConfig.ts index 8ea7fdf30a6..c8ed49331eb 100644 --- a/packages/shared/src/types/protectConfig.ts +++ b/packages/shared/src/types/protectConfig.ts @@ -37,12 +37,30 @@ export interface ProtectLoader { * 5000 and is capped at 10000 by the SDK, so this cannot stall a sign-in. */ token_timeout_ms?: number; + /** + * Overrides {@link ProtectConfigJSON.challenge_load_timeout_ms} for browsers that got this + * loader. Absent inherits the instance-wide value, which may itself be absent and inherit the + * SDK default. + * + * Per loader because loaders roll out gradually: while a new one ramps, two are live for the + * same instance at once, and the new one may need a different value from the one it replaces. + */ + challenge_load_timeout_ms?: number; } export interface ProtectConfigJSON { object: 'protect_config'; id: string; loaders?: ProtectLoader[]; + /** + * How long to wait for a verification module to LOAD before giving up, in milliseconds. Absent + * means "use the SDK default". + * + * It bounds only the load. Once a verification module is running it governs its own duration, + * so this is not a bound on how long verification may take — the two are deliberately separate + * budgets, and only the first is the SDK's to set. + */ + challenge_load_timeout_ms?: number; /** * Unix seconds. A session token acquired while an older value was configured is discarded and * re-acquired, so raising this makes every browser fetch a fresh one as its environment @@ -61,6 +79,8 @@ export interface ProtectConfigResource extends ClerkResource { loaders?: ProtectLoader[]; /** See {@link ProtectConfigJSON.tokens_invalid_before}. */ tokens_invalid_before?: number; + /** See {@link ProtectConfigJSON.challenge_load_timeout_ms}. */ + challenge_load_timeout_ms?: number; __internal_toSnapshot: () => ProtectConfigJSONSnapshot; } diff --git a/packages/ui/src/hooks/useProtectCheckRunner.ts b/packages/ui/src/hooks/useProtectCheckRunner.ts index e80173033d8..04b82a7388a 100644 --- a/packages/ui/src/hooks/useProtectCheckRunner.ts +++ b/packages/ui/src/hooks/useProtectCheckRunner.ts @@ -1,9 +1,11 @@ import { ClerkRuntimeError, isClerkAPIResponseError } from '@clerk/shared/error'; import { ERROR_CODES } from '@clerk/shared/internal/clerk-js/constants'; +import { useClerk } from '@clerk/shared/react'; import type { ProtectCheckResource } from '@clerk/shared/types'; import React from 'react'; import { flushSync } from 'react-dom'; +import { useEnvironment } from '@/ui/contexts/EnvironmentContext'; import { useCardState } from '@/ui/elements/contexts'; import { handleError } from '@/ui/utils/errorHandler'; @@ -17,9 +19,6 @@ import { handleError } from '@/ui/utils/errorHandler'; */ const MAX_EXPIRED_RELOADS = 2; -/** Upper bound on how long we wait for the challenge SDK to settle before failing loud. */ -const PROTECT_CHECK_SCRIPT_TIMEOUT_MS = 60_000; - export interface ProtectCheckRunnerParams { /** * Reads the current protect_check off the resource. Called fresh on each effect run because @@ -68,6 +67,15 @@ export interface ProtectCheckRunner { export function useProtectCheckRunner(params: ProtectCheckRunnerParams): ProtectCheckRunner { const card = useCardState(); + // Override for the module-LOAD bound only (see `executeProtectCheck`), resolved loader first + // and instance second: a loader being rolled out gradually can carry its own value without + // changing anything for browsers still on the loader it replaces. Undefined at both levels + // leaves the SDK default in force. Read here rather than inside the effect so the effect keeps + // depending on primitives. + const loaderTimeoutMs = useClerk().__internal_protectChallengeLoadTimeoutMs; + const instanceTimeoutMs = useEnvironment().protectConfig?.challenge_load_timeout_ms; + const loadTimeoutMs = loaderTimeoutMs ?? instanceTimeoutMs; + // `handleError` re-throws what it does not recognise, and this runner awaits caller code that // raises plain errors (a transient fetch failure, an OAuth continuation that did not complete). const reportError = (err: any) => { @@ -227,7 +235,6 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam setIsRunning(true); const runChallenge = async () => { - let timeoutId: ReturnType | undefined; try { // Load the Protect SDK loader lazily, gated on the same compile-time flag as the // fail-closed guard above. In no-RHC builds `__BUILD_DISABLE_RHC__` is `true`, so this @@ -238,20 +245,19 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam return; } const { executeProtectCheck } = await import('@clerk/shared/internal/clerk-js/protectCheck'); - const proofToken = await Promise.race([ - executeProtectCheck(protectCheck, container, { signal: abortController.signal, setWidgetVisible }), - new Promise((_, reject) => { - timeoutId = setTimeout(() => { - // Stop the (possibly hung) SDK and surface a retryable timeout error. - abortController.abort(); - reject( - new ClerkRuntimeError('Protect verification timed out', { - code: ERROR_CODES.PROTECT_CHECK_TIMED_OUT, - }), - ); - }, PROTECT_CHECK_SCRIPT_TIMEOUT_MS); - }), - ]); + // Deliberately unraced. `executeProtectCheck` bounds LOADING the challenge module and + // nothing after it: once control passes to the challenge, the challenge owns its own + // deadline. We cannot know an honest duration for it — the challenge type is chosen + // server-side, per decision, long after this bundle shipped, and its work may be waiting + // on a person or moving a server-chosen number of bytes over a link we know nothing + // about. The wall that used to be here aborted valid challenges and reported them to the + // user as a timeout, and since a re-run restarts the work from the beginning, retrying + // could never win on any connection slow enough to trip it in the first place. + const proofToken = await executeProtectCheck(protectCheck, container, { + signal: abortController.signal, + setWidgetVisible, + loadTimeoutMs, + }); if (cancelled) { return; } @@ -286,9 +292,6 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam } reportError(err); } finally { - if (timeoutId) { - clearTimeout(timeoutId); - } if (!cancelled) { isRunningRef.current = false; setIsRunning(false); From ee8b3f3db20ea0cc3c5168c15d43489e28eb398d Mon Sep 17 00:00:00 2001 From: Theo Zourzouvillys Date: Fri, 21 Aug 2026 19:10:56 -0800 Subject: [PATCH 2/3] fix(shared,clerk-js): settle on abort, clamp the load bound, pin precedence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects a second-model review found in the load bound. An abort landing while the import was stalled settled nothing: the signal was not part of the race, so an unmounted component kept the promise, its closures and its timer alive for the whole bound and then reported a load failure for what was a cancellation. The signal is raced now and rejects as protect_check_aborted, matching the contract every other path already honours. setTimeout stores its delay in a signed 32-bit int, so a configured value above 2^31-1 overflowed and fired immediately — failing every load instantly, which is the exact opposite of what an operator asking for a long timeout wanted. The value is clamped, and non-finite values fall back to the default rather than being passed to the timer. The precedence doc and the code disagreed. The doc said an absent loader value inherits the instance-wide one, while the code takes the first APPLIED loader that specifies one — so a value set on one of two live loaders would apply to browsers that got the other. The code is the sensible rule, since there is no single "the" loader once more than one is live, so the doc now states it and warns that an instance running two loaders should set this on both or neither. Co-Authored-By: Claude Opus 5 (1M context) --- packages/clerk-js/src/core/protect.ts | 12 +++-- .../clerk-js/__tests__/protectCheck.test.ts | 43 +++++++++++++++++ .../src/internal/clerk-js/protectCheck.ts | 47 +++++++++++++++++-- packages/shared/src/types/protectConfig.ts | 8 +++- 4 files changed, 101 insertions(+), 9 deletions(-) diff --git a/packages/clerk-js/src/core/protect.ts b/packages/clerk-js/src/core/protect.ts index e52b382ccbc..181ae2755f5 100644 --- a/packages/clerk-js/src/core/protect.ts +++ b/packages/clerk-js/src/core/protect.ts @@ -56,10 +56,16 @@ export class Protect { const loaders = configured.filter(loader => isLoader(loader) && inRollout(loader)); // Read off the applied set, before any of them run: this only describes config, so a loader - // that later fails to be placed has still spoken for the browser it was assigned to. First - // one that asks for it wins. + // that later fails to be placed has still spoken for the browser it was assigned to. + // + // First one that asks for it wins, and the instance-wide value applies only when none does — + // the precedence is across the applied set rather than per loader, because there is no single + // "the" loader once more than one is live. Documented on the field. this.#challengeLoadTimeoutMs = loaders.find( - loader => typeof loader.challenge_load_timeout_ms === 'number' && loader.challenge_load_timeout_ms > 0, + loader => + typeof loader.challenge_load_timeout_ms === 'number' && + Number.isFinite(loader.challenge_load_timeout_ms) && + loader.challenge_load_timeout_ms > 0, )?.challenge_load_timeout_ms; // Only an instance whose loaders reference the correlation id gets a session, so an instance diff --git a/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts b/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts index dd58ea730a4..cc74414ad43 100644 --- a/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts +++ b/packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts @@ -250,6 +250,49 @@ describe('executeProtectCheck', () => { expect(settled).toBe(true); }); + // An abort mid-load has to settle the operation, not leave it pending for the whole bound + // holding its closures and timer, and it is a cancellation rather than a load failure. + it('settles as an abort when the caller aborts during a stalled load', async () => { + vi.doMock('https://protect.example.com/sdk-hangs-3.js', () => new Promise(() => {})); + + const controller = new AbortController(); + const running = executeProtectCheck( + protectCheck({ sdkUrl: 'https://protect.example.com/sdk-hangs-3.js' }), + fakeContainer(), + { signal: controller.signal }, + ); + const assertion = expect(running).rejects.toMatchObject({ code: 'protect_check_aborted' }); + + controller.abort(); + // No timer advanced: the abort alone must settle it, well before the load bound. + await assertion; + }); + + // setTimeout stores its delay in a signed 32-bit int, so an oversized value overflows and + // fires immediately — failing every load instantly, the opposite of what was configured. + it('clamps an oversized loadTimeoutMs instead of overflowing the timer', async () => { + vi.doMock('https://protect.example.com/sdk-hangs-4.js', () => new Promise(() => {})); + + const running = executeProtectCheck( + protectCheck({ sdkUrl: 'https://protect.example.com/sdk-hangs-4.js' }), + fakeContainer(), + { loadTimeoutMs: 2_147_483_648 }, + ); + let settled = false; + const watch = running.then( + () => (settled = true), + () => (settled = true), + ); + + // An overflowed timer would already have fired by now. + await vi.advanceTimersByTimeAsync(1_000); + expect(settled).toBe(false); + + await vi.advanceTimersByTimeAsync(600_000); + await watch; + expect(settled).toBe(true); + }); + // The point of the whole change: the challenge owns its own duration. A challenge running far // longer than any load bound must still resolve — proof-of-transfer moves a server-chosen // number of bytes, and a host-side wall would abort it as a "timeout". diff --git a/packages/shared/src/internal/clerk-js/protectCheck.ts b/packages/shared/src/internal/clerk-js/protectCheck.ts index 019399f7fb3..f44e33596cd 100644 --- a/packages/shared/src/internal/clerk-js/protectCheck.ts +++ b/packages/shared/src/internal/clerk-js/protectCheck.ts @@ -47,8 +47,19 @@ export interface ExecuteProtectCheckOptions { */ export const DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS = 60_000; +/** + * Ceiling on the configured load bound. `setTimeout` stores its delay in a signed 32-bit int, so a + * larger value overflows and fires immediately — which would make every load fail instantly, the + * exact opposite of what an operator asking for a long timeout wanted. Clamping rather than + * rejecting keeps a fat-fingered config from breaking sign-in. + */ +const MAX_PROTECT_CHECK_LOAD_TIMEOUT_MS = 600_000; + function resolveLoadTimeoutMs(configured: number | undefined): number { - return typeof configured === 'number' && configured > 0 ? configured : DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS; + if (typeof configured !== 'number' || !Number.isFinite(configured) || configured <= 0) { + return DEFAULT_PROTECT_CHECK_LOAD_TIMEOUT_MS; + } + return Math.min(configured, MAX_PROTECT_CHECK_LOAD_TIMEOUT_MS); } /** @@ -61,14 +72,36 @@ function resolveLoadTimeoutMs(configured: number | undefined): number { * about — waiting on a person, or moving a server-chosen number of bytes over an unknown link. A * host-side wall over execution aborts legitimate challenges and reports them as timeouts, and * since a re-run restarts the work, retrying cannot win on any connection slow enough to trip it. + * + * The abort signal is raced too. A stalled import cannot itself be cancelled, but without this the + * caller's abort would not settle anything: an unmounted component would keep this promise, its + * closures and its timer alive for the whole load bound, and then report a load failure for what + * was really a cancellation. */ -function importWithTimeout(url: string, timeoutMs: number): Promise> { +function importWithTimeout( + url: string, + timeoutMs: number, + signal: AbortSignal | undefined, +): Promise> { let timeoutId: ReturnType | undefined; + let onAbort: (() => void) | undefined; + const expiry = new Promise((_, reject) => { timeoutId = setTimeout(() => reject(new Error('Protect check script load timed out')), timeoutMs); }); - return Promise.race([import(/* webpackIgnore: true */ url), expiry]).finally(() => { + const aborted = new Promise((_, reject) => { + if (!signal) { + return; + } + onAbort = () => reject(new Error('Protect check aborted during load')); + signal.addEventListener('abort', onAbort, { once: true }); + }); + + return Promise.race([import(/* webpackIgnore: true */ url), expiry, aborted]).finally(() => { clearTimeout(timeoutId); + if (signal && onAbort) { + signal.removeEventListener('abort', onAbort); + } }) as Promise>; } @@ -154,8 +187,14 @@ export async function executeProtectCheck( let mod: Record; try { - mod = await importWithTimeout(validated.toString(), resolveLoadTimeoutMs(loadTimeoutMs)); + mod = await importWithTimeout(validated.toString(), resolveLoadTimeoutMs(loadTimeoutMs), signal); } catch { + // An abort that landed mid-load is a cancellation, not a load failure. Checked first so the + // caller gets the same contract it does everywhere else: if you aborted, you never see + // anything but `protect_check_aborted`. + if (signal?.aborted) { + throw new ClerkRuntimeError('Protect check aborted by caller', { code: 'protect_check_aborted' }); + } // Surface a generic message and deliberately omit the original error: Chromium/Firefox embed // the sdk_url in the dynamic-import failure text, which a tampered response could plant in the UI. throw new ClerkRuntimeError( diff --git a/packages/shared/src/types/protectConfig.ts b/packages/shared/src/types/protectConfig.ts index c8ed49331eb..c7275ecc43f 100644 --- a/packages/shared/src/types/protectConfig.ts +++ b/packages/shared/src/types/protectConfig.ts @@ -39,11 +39,15 @@ export interface ProtectLoader { token_timeout_ms?: number; /** * Overrides {@link ProtectConfigJSON.challenge_load_timeout_ms} for browsers that got this - * loader. Absent inherits the instance-wide value, which may itself be absent and inherit the - * SDK default. + * loader. * * Per loader because loaders roll out gradually: while a new one ramps, two are live for the * same instance at once, and the new one may need a different value from the one it replaces. + * + * Precedence is across the APPLIED SET, not per loader: the first applied loader that specifies + * one wins, and the instance-wide value applies only when none of them does. An instance running + * two loaders at once should therefore either set this on both or on neither — setting it on one + * makes it apply to browsers that got the other, which is rarely what is meant. */ challenge_load_timeout_ms?: number; } From ef1c36517510208095b6b8c742fc54c0584d79d9 Mon Sep 17 00:00:00 2001 From: Theo Zourzouvillys Date: Fri, 21 Aug 2026 19:51:53 -0800 Subject: [PATCH 3/3] chore: simplify the changeset entry to one user-facing sentence Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/protect-check-load-timeout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/protect-check-load-timeout.md b/.changeset/protect-check-load-timeout.md index d5888783acd..b02dfbe772e 100644 --- a/.changeset/protect-check-load-timeout.md +++ b/.changeset/protect-check-load-timeout.md @@ -4,4 +4,4 @@ '@clerk/ui': patch --- -Bound loading the verification module rather than running it. The SDK no longer imposes a time limit on a verification once it has started, since how long one legitimately takes depends on the verification itself. The load timeout can be set per loader, falling back to a per-instance value and then to the default. +Fix an issue where a verification that was still progressing normally could be cancelled and reported to the user as having timed out.