fix(clerk-js,react,shared,ui): bound loading a verification, not running one (#9527)

Author: zourzouvillys

.changeset/protect-check-load-timeout.md

@@ -0,0 +1,8 @@
+---
+'@clerk/clerk-js': patch
+'@clerk/react': patch
+'@clerk/shared': patch
+'@clerk/ui': patch
+---
+
+Fix an issue where a verification that was still progressing normally could be cancelled and reported to the user as having timed out.

packages/clerk-js/src/core/__tests__/protectChallengeLoadTimeout.test.ts

@@ -0,0 +1,91 @@
+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> = {}): 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, non-finite, 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: Number.POSITIVE_INFINITY }),
+        loader({ challenge_load_timeout_ms: 'soon' as unknown as number }),
+      ]),
+    );
+
+    expect(protect.challengeLoadTimeoutMs).toBeUndefined();
+  });
+
+  it('reports the first finite, positive value from the applied loaders', () => {
+    const protect = new Protect();
+    protect.load(
+      environment([
+        loader({ challenge_load_timeout_ms: 0 }),
+        loader({ challenge_load_timeout_ms: 25_000 }),
+        loader({ challenge_load_timeout_ms: 45_000 }),
+      ]),
+    );
+
+    expect(protect.challengeLoadTimeoutMs).toBe(25_000);
+  });
+});

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;

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,16 @@ export class Protect {
     // loaders we are actually going to apply.
     const loaders = configured.filter(loader => isLoader(loader) && inRollout(loader));
 
+    // Load timeout applies to a single event and we only need one, so we pick the first valid one.
+    // This happens before applying the loaders since we want to use the value even if the loader
+    // later fails to apply.
+    this.#challengeLoadTimeoutMs = loaders.find(
+      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
     // not using it is unaffected and stores nothing in the browser.
     const applyLoader: ApplyLoader = (loader, placeholders) => this.applyLoader(loader, placeholders);

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,
     };
   }
 }

packages/react/src/__tests__/isomorphicClerk.test.ts

@@ -73,6 +73,20 @@ describe('isomorphicClerk', () => {
     expect(isomorphicClerk.__internal_moduleManager).toBe(mm);
   });
 
+  it('exposes the inner clerk-js Protect challenge load timeout', () => {
+    const isomorphicClerk = new IsomorphicClerk({ publishableKey: 'pk_test_XXX' });
+
+    expect(isomorphicClerk.__internal_protectChallengeLoadTimeoutMs).toBeUndefined();
+
+    const innerClerk: any = {
+      addListener: vi.fn(),
+      __internal_protectChallengeLoadTimeoutMs: 25_000,
+    };
+    (isomorphicClerk as any).replayInterceptedInvocations(innerClerk);
+
+    expect(isomorphicClerk.__internal_protectChallengeLoadTimeoutMs).toBe(25_000);
+  });
+
   it('updates props asynchronously after clerkjs has loaded', async () => {
     const propsHistory: any[] = [];
     const dummyClerkJS = {

packages/react/src/isomorphicClerk.ts

@@ -302,6 +302,10 @@ export class IsomorphicClerk implements IsomorphicLoadedClerk {
     return this.clerkjs?.__internal_moduleManager;
   }
 
+  public get __internal_protectChallengeLoadTimeoutMs(): number | undefined {
+    return this.clerkjs?.__internal_protectChallengeLoadTimeoutMs;
+  }
+
   constructor(options: IsomorphicClerkOptions) {
     this.#publishableKey = options?.publishableKey;
     this.#proxyUrl = options?.proxyUrl;

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,131 @@ 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);
+    });
+
+    // 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".
+    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<string>(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;
+    });
+  });
 });