Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/protect-check-load-timeout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': 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.
Original file line number Diff line number Diff line change
@@ -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> = {}): 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();
});
});
10 changes: 10 additions & 0 deletions packages/clerk-js/src/core/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
28 changes: 28 additions & 0 deletions packages/clerk-js/src/core/protect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -40,6 +55,19 @@ 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, 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' &&
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);
Expand Down
3 changes: 3 additions & 0 deletions packages/clerk-js/src/core/resources/ProtectConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}
Expand All @@ -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,
};
}
}
131 changes: 129 additions & 2 deletions packages/shared/src/internal/clerk-js/__tests__/protectCheck.test.ts
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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;
});
});
});
Loading
Loading