diff --git a/.changeset/protect-token-timeout-10s.md b/.changeset/protect-token-timeout-10s.md new file mode 100644 index 00000000000..6d514b66cc2 --- /dev/null +++ b/.changeset/protect-token-timeout-10s.md @@ -0,0 +1,10 @@ +--- +'@clerk/clerk-js': patch +'@clerk/shared': patch +--- + +Wait up to 10 seconds, instead of 5, for the Device Intelligence check to finish before sending a sign-in or sign-up without its result. This applies when an instance hasn't configured its own limit. Users on slow connections are less likely to have sign-ups blocked by protections that require a Device Intelligence result. + +Keep the wait bounded when the device clock changes so later attempts can recover after another tab's lock is released. + +The shared loader type reference now documents the same 10-second default. diff --git a/integration/tests/session-tasks-sign-up.test.ts b/integration/tests/session-tasks-sign-up.test.ts index 1753a9efeed..3432235aa92 100644 --- a/integration/tests/session-tasks-sign-up.test.ts +++ b/integration/tests/session-tasks-sign-up.test.ts @@ -1,6 +1,7 @@ import { createClerkClient } from '@clerk/backend'; import { expect, test } from '@playwright/test'; +import { environmentConfig } from '../models/environment'; import { appConfigs } from '../presets'; import { resolveInstanceKeys } from '../presets/envs'; import type { FakeUser } from '../testUtils'; @@ -79,6 +80,17 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withSessionTasks] })( test('with sso, navigate to task on after sign-up', async ({ page, context }) => { const u = await createTestUtils({ app, page, context }); + const providerKeys = await resolveInstanceKeys('oauth-provider'); + await createTestUtils({ + app: { + ...app, + env: environmentConfig() + .setEnvVariable('public', 'CLERK_PUBLISHABLE_KEY', providerKeys.pk) + .setEnvVariable('private', 'CLERK_SECRET_KEY', providerKeys.sk), + }, + page, + context, + }); await u.po.signUp.goTo(); await u.page.getByRole('button', { name: 'E2E OAuth Provider' }).click(); diff --git a/integration/tests/transitive-state.test.ts b/integration/tests/transitive-state.test.ts index 17d6566cd43..1026d593476 100644 --- a/integration/tests/transitive-state.test.ts +++ b/integration/tests/transitive-state.test.ts @@ -227,6 +227,7 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('transitiv await u.po.signIn.continue(); await u.po.signIn.setPassword(userB.password); await u.po.signIn.continue(); + await u.page.waitForFunction(userId => window.Clerk?.user?.id === userId, userBId); const pathA = '/transitive-state/sign-out'; const pathB = '/transitive-state/sign-out/sign-in'; diff --git a/packages/clerk-js/src/core/__tests__/protectSession.test.ts b/packages/clerk-js/src/core/__tests__/protectSession.test.ts index bb8056d59e9..4154641cd79 100644 --- a/packages/clerk-js/src/core/__tests__/protectSession.test.ts +++ b/packages/clerk-js/src/core/__tests__/protectSession.test.ts @@ -686,7 +686,7 @@ describe('ProtectSession deadlines', () => { const startedAt = Date.now(); await expect(created?.getRequestParams()).resolves.toMatchObject({ __clerk_protect_status: 'timeout' }); - // Misread, this would fall back to the 5s default and be two orders of magnitude out. + // Misread, this would fall back to the 10s default and be two orders of magnitude out. expect(Date.now() - startedAt).toBeLessThan(1_000); }); @@ -733,11 +733,157 @@ describe('ProtectSession deadlines', () => { // `tokenTimeoutMs` is server config with no upper bound of its own, so the ceiling is ours. expect(clampTimeout(120_000)).toBe(10_000); expect(clampTimeout(1_500)).toBe(1_500); - expect(clampTimeout(undefined)).toBe(5_000); - expect(clampTimeout(0)).toBe(5_000); - expect(clampTimeout(-1)).toBe(5_000); - expect(clampTimeout('soon')).toBe(5_000); - expect(clampTimeout(Number.POSITIVE_INFINITY)).toBe(5_000); + expect(clampTimeout(undefined)).toBe(10_000); + expect(clampTimeout(0)).toBe(10_000); + expect(clampTimeout(-1)).toBe(10_000); + expect(clampTimeout('soon')).toBe(10_000); + expect(clampTimeout(Number.POSITIVE_INFINITY)).toBe(10_000); + }); +}); + +describe('ProtectSession default deadline', () => { + const defaultLoader = () => loader({ token_timeout_ms: undefined }); + + const track = (params: Promise | undefined) => { + const state = { settled: false }; + void params?.then(() => { + state.settled = true; + }); + return state; + }; + + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + delete (navigator as unknown as Record).locks; + }); + + it('accepts a token that arrives after 7s', async () => { + const { session: created, elements } = session([defaultLoader()]); + created?.start(); + const params = created?.getRequestParams(); + + await vi.advanceTimersByTimeAsync(7_000); + serveInline(elements[0], { cid: created?.placeholders().cid }); + + await expect(params).resolves.toMatchObject({ __clerk_protect_status: 'ok' }); + }); + + it('gives up at 10s when the loader never loads', async () => { + const { session: created } = session([defaultLoader()]); + created?.start(); + const params = created?.getRequestParams(); + const state = track(params); + + await vi.advanceTimersByTimeAsync(9_999); + expect(state.settled).toBe(false); + await vi.advanceTimersByTimeAsync(1); + await expect(params).resolves.toMatchObject({ __clerk_protect_status: 'timeout' }); + }); + + it('holds a sign-in made mid-acquisition only for what is left of it', async () => { + const { session: created } = session([defaultLoader()]); + created?.start(); + await vi.advanceTimersByTimeAsync(6_000); + + const params = created?.getRequestParams(); + const state = track(params); + + await vi.advanceTimersByTimeAsync(3_999); + expect(state.settled).toBe(false); + await vi.advanceTimersByTimeAsync(1); + await expect(params).resolves.toMatchObject({ __clerk_protect_status: 'timeout' }); + }); + + it('never holds one sign-in past the deadline when the lock is granted late', async () => { + Object.defineProperty(navigator, 'locks', { + value: { + request: vi.fn((_key: string, _options: unknown, callback: () => Promise) => + new Promise(resolve => setTimeout(resolve, 4_900)).then(() => callback()), + ), + }, + configurable: true, + }); + + const { session: created } = session([defaultLoader()]); + created?.start(); + const params = created?.getRequestParams(); + const state = track(params); + + await vi.advanceTimersByTimeAsync(9_999); + expect(state.settled).toBe(false); + await vi.advanceTimersByTimeAsync(1); + await expect(params).resolves.toMatchObject({ __clerk_protect_status: 'timeout' }); + }); + + it('keeps waiting for a leader tab after SafeLock gives up, until its own deadline', async () => { + Object.defineProperty(navigator, 'locks', { + value: { + request: vi.fn( + (_key: string, options: { signal: AbortSignal }) => + new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => reject(new DOMException('aborted', 'AbortError'))); + }), + ), + }, + configurable: true, + }); + + const { session: created, elements } = session([defaultLoader()]); + created?.start(); + const params = created?.getRequestParams(); + + await vi.advanceTimersByTimeAsync(7_000); + localStorage.setItem('__clerk_protect_st', storedEntry({ token: 'v1.leader.mac', rid: 'c'.repeat(26) })); + await vi.advanceTimersByTimeAsync(100); + + await expect(params).resolves.toMatchObject({ + __clerk_protect_token: 'v1.leader.mac', + __clerk_protect_status: 'ok', + }); + expect(elements).toHaveLength(0); + }); + + it.each([ + [2_000, -3_600_000], + [6_000, -3_600_000], + [2_000, 3_600_000], + [6_000, 3_600_000], + ])('settles follower polling after a clock change at %s ms by %s ms', async (changeAt, correction) => { + let available = false; + const request = vi.fn((_key: string, options: { signal: AbortSignal }, callback: () => Promise) => { + if (available) { + return callback(); + } + return new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => reject(new DOMException('aborted', 'AbortError'))); + }); + }); + Object.defineProperty(navigator, 'locks', { configurable: true, value: { request } }); + const { session: created, elements } = session([defaultLoader()]); + created?.start(); + const params = created?.getRequestParams(); + const state = track(params); + + await vi.advanceTimersByTimeAsync(changeAt); + vi.setSystemTime(Date.now() + correction); + await vi.advanceTimersByTimeAsync(9_999 - changeAt); + expect(state.settled).toBe(false); + await vi.advanceTimersByTimeAsync(1); + await expect(params).resolves.toMatchObject({ __clerk_protect_status: 'timeout' }); + expect(vi.getTimerCount()).toBe(0); + + available = true; + await vi.advanceTimersByTimeAsync(30_000); + const retry = created?.getRequestParams(); + await vi.advanceTimersByTimeAsync(0); + expect(request).toHaveBeenCalledTimes(2); + expect(elements).toHaveLength(1); + serveInline(elements[0], { cid: created?.placeholders().cid }); + await expect(retry).resolves.toMatchObject({ __clerk_protect_status: 'ok' }); }); }); diff --git a/packages/clerk-js/src/core/protectSession.ts b/packages/clerk-js/src/core/protectSession.ts index ee36a82b0e9..ca9d75f59c8 100644 --- a/packages/clerk-js/src/core/protectSession.ts +++ b/packages/clerk-js/src/core/protectSession.ts @@ -31,9 +31,10 @@ const INLINE_TOKEN_GLOBAL = '__clerk_specter'; /** A token this close to expiry is treated as absent, so a fresh run starts before it lapses. */ const TOKEN_REFRESH_MARGIN_MS = 60 * 1_000; /** Acquisition deadline when the instance does not configure one. */ -const DEFAULT_TOKEN_TIMEOUT_MS = 5 * 1_000; +const DEFAULT_TOKEN_TIMEOUT_MS = 10 * 1_000; /** Ceiling on the instance-configured deadline, so no server value can stall a sign-in. */ const MAX_TOKEN_TIMEOUT_MS = 10 * 1_000; +const STORE_POLL_INTERVAL_MS = 100; /** A stored token claiming more remaining life than this was not minted by us. */ const MAX_TOKEN_LIFETIME_MS = 24 * 60 * 60 * 1_000; /** Longest token we will hand back, so a planted store entry cannot bloat a sign-in body. */ @@ -404,16 +405,17 @@ function isRetryableStatus(status: number): boolean { } function sleep(ms: number, signal: AbortSignal): Promise { + if (signal.aborted) { + return Promise.resolve(); + } return new Promise(resolve => { - const timer = setTimeout(resolve, ms); - signal.addEventListener( - 'abort', - () => { - clearTimeout(timer); - resolve(); - }, - { once: true }, - ); + const settle = () => { + clearTimeout(timer); + signal.removeEventListener('abort', settle); + resolve(); + }; + const timer = setTimeout(settle, ms); + signal.addEventListener('abort', settle, { once: true }); }); } @@ -622,7 +624,8 @@ export class ProtectSession { if (readStoredToken(this.#tokenStorageKey, TOKEN_SEND_MARGIN_MS, this.#floor)) { return; } - if (Date.now() - this.#lastAttemptAt < this.#backoffMs()) { + const elapsed = Date.now() - this.#lastAttemptAt; + if (elapsed >= 0 && elapsed < this.#backoffMs()) { return; } this.#startAcquisition(); @@ -666,22 +669,38 @@ export class ProtectSession { return 'ok'; } - const result = await this.#lock.acquireLockAndRun(async () => { - // Double-checked inside the lock. Without this re-read every tab runs the loader in turn, - // which is the exact failure this design exists to prevent. - if (this.#freshStoredToken()) { - return 'ok' satisfies ProtectStatus; + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), this.#timeoutMs); + try { + const result = await this.#lock.acquireLockAndRun(async () => { + // Double-checked inside the lock. Without this re-read every tab runs the loader in turn, + // which is the exact failure this design exists to prevent. + if (this.#freshStoredToken()) { + return 'ok' satisfies ProtectStatus; + } + return await this.#mint(); + }); + + if (typeof result === 'string') { + return result as ProtectStatus; } - return await this.#mint(); - }); - if (typeof result === 'string') { - return result as ProtectStatus; + // The lock was never held — `SafeLock` gives up waiting after ~5s so a wedged leader delays + // nobody. The timer also bounds follower polling when the system clock changes. + return (await this.#awaitStoredToken(controller.signal)) ? 'ok' : 'timeout'; + } finally { + clearTimeout(timer); } + } - // The lock was never held — `SafeLock` gives up waiting after ~5s so a wedged leader delays - // nobody. Whatever a leader did manage to write is still ours to use. - return this.#freshStoredToken() ? 'ok' : 'timeout'; + async #awaitStoredToken(signal: AbortSignal): Promise { + while (!signal.aborted) { + if (this.#freshStoredToken()) { + return true; + } + await sleep(STORE_POLL_INTERVAL_MS, signal); + } + return false; } /** Runs the loader and takes the token it was served with. Never rejects. */ diff --git a/packages/shared/src/types/protectConfig.ts b/packages/shared/src/types/protectConfig.ts index 45ea9c20f01..c6b1b637656 100644 --- a/packages/shared/src/types/protectConfig.ts +++ b/packages/shared/src/types/protectConfig.ts @@ -34,7 +34,7 @@ export interface ProtectLoader { token_url?: string; /** * How long to wait for the token before giving up and reporting a status instead. Defaults to - * 5000 and is capped at 10000 by the SDK, so this cannot stall a sign-in. + * 10000 and is capped at 10000 by the SDK, so this cannot stall a sign-in. */ token_timeout_ms?: number; /**