diff --git a/.changeset/protect-check-runner.md b/.changeset/protect-check-runner.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/protect-check-runner.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/shared/src/internal/clerk-js/__tests__/protectCheckRunner.test.ts b/packages/shared/src/internal/clerk-js/__tests__/protectCheckRunner.test.ts new file mode 100644 index 00000000000..6ca0e433f20 --- /dev/null +++ b/packages/shared/src/internal/clerk-js/__tests__/protectCheckRunner.test.ts @@ -0,0 +1,190 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ClerkAPIResponseError } from '@/error'; +import type { ProtectCheckResource } from '@/types'; + +import type { ProtectCheckRunOptions } from '../protectCheckRunner'; +import { MAX_EXPIRED_PROTECT_CHECK_RELOADS, runProtectCheck } from '../protectCheckRunner'; + +vi.mock('../protectCheck', () => ({ + executeProtectCheck: vi.fn(), +})); + +import { executeProtectCheck } from '../protectCheck'; + +const mockExecute = vi.mocked(executeProtectCheck); + +type Resource = { id: string; protectCheck: ProtectCheckResource | null }; + +const challenge = (overrides: Partial = {}): ProtectCheckResource => ({ + status: 'pending', + token: 'challenge-token', + sdkUrl: 'https://protect.example.com/sdk.js', + ...overrides, +}); + +const alreadyResolved = () => + new ClerkAPIResponseError('already resolved', { + status: 400, + data: [{ code: 'protect_check_already_resolved', message: 'already resolved' }], + }); + +const setup = (initial: ProtectCheckResource | null = challenge()) => { + const live: Resource = { id: 'live', protectCheck: initial }; + const submitted: Resource = { id: 'submitted', protectCheck: null }; + const ops = { + getProtectCheck: vi.fn(() => live.protectCheck), + getResource: vi.fn(() => live), + reload: vi.fn(() => Promise.resolve()), + submitProtectCheck: vi.fn(() => Promise.resolve(submitted)), + }; + const container = document.createElement('div'); + const expiredReloads = { current: 0 }; + const run = (check: ProtectCheckResource, options: Partial = {}) => + runProtectCheck(ops, check, { container, expiredReloads, ...options }); + return { live, submitted, ops, container, expiredReloads, run }; +}; + +beforeEach(() => { + mockExecute.mockReset(); + mockExecute.mockResolvedValue('proof-abc'); +}); + +describe('ProtectCheckRunner', () => { + it('executes the challenge in the container and submits the proof token', async () => { + const { run, ops, container, submitted } = setup(); + const signal = new AbortController().signal; + const setWidgetVisible = vi.fn(() => Promise.resolve()); + + const outcome = await run(challenge(), { container, signal, setWidgetVisible, loadTimeoutMs: 1234 }); + + expect(mockExecute).toHaveBeenCalledWith(expect.objectContaining({ token: 'challenge-token' }), container, { + signal, + setWidgetVisible, + loadTimeoutMs: 1234, + }); + expect(ops.submitProtectCheck).toHaveBeenCalledWith({ proofToken: 'proof-abc' }); + expect(outcome).toEqual({ status: 'resolved', resource: submitted }); + }); + + it('does nothing when the signal is already aborted, even for an expired challenge', async () => { + const { run, ops, container } = setup(); + const controller = new AbortController(); + controller.abort(); + + await expect( + run(challenge({ expiresAt: Date.now() - 1000 }), { container, signal: controller.signal }), + ).rejects.toMatchObject({ code: 'protect_check_aborted' }); + expect(ops.reload).not.toHaveBeenCalled(); + expect(mockExecute).not.toHaveBeenCalled(); + }); + + it('does not submit a proof token that arrives after the signal aborted', async () => { + const { run, ops, container } = setup(); + const controller = new AbortController(); + mockExecute.mockImplementation(() => { + controller.abort(); + return Promise.resolve('late-proof'); + }); + + await expect(run(challenge(), { container, signal: controller.signal })).rejects.toMatchObject({ + code: 'protect_check_aborted', + }); + expect(ops.submitProtectCheck).not.toHaveBeenCalled(); + }); + + it('does not submit when the challenge script fails', async () => { + const { run, ops, container } = setup(); + mockExecute.mockRejectedValue(new Error('script failed')); + + await expect(run(challenge(), { container })).rejects.toThrow('script failed'); + expect(ops.submitProtectCheck).not.toHaveBeenCalled(); + }); + + it('treats protect_check_already_resolved as resolved after a reload', async () => { + const { run, ops, container, live } = setup(); + ops.submitProtectCheck.mockRejectedValue(alreadyResolved()); + ops.reload.mockImplementation(() => { + live.protectCheck = null; + return Promise.resolve(); + }); + + const outcome = await run(challenge(), { container }); + + expect(ops.reload).toHaveBeenCalledTimes(1); + expect(outcome).toEqual({ status: 'resolved', resource: live }); + }); + + it('does not reload or resolve an already-resolved submit once the signal aborted', async () => { + const { run, ops, container } = setup(); + const controller = new AbortController(); + ops.submitProtectCheck.mockImplementation(() => { + controller.abort(); + return Promise.reject(alreadyResolved()); + }); + + await expect(run(challenge(), { container, signal: controller.signal })).rejects.toMatchObject({ + code: 'protect_check_aborted', + }); + expect(ops.reload).not.toHaveBeenCalled(); + }); + + it('rethrows other submit errors without a reload', async () => { + const { run, ops, container } = setup(); + ops.submitProtectCheck.mockRejectedValue(new Error('network')); + + await expect(run(challenge(), { container })).rejects.toThrow('network'); + expect(ops.reload).not.toHaveBeenCalled(); + }); + + describe('an expired challenge', () => { + const expired = () => challenge({ expiresAt: Date.now() - 1000 }); + + it('reloads instead of executing and resolves when the reload clears the gate', async () => { + const { run, ops, container, live } = setup(expired()); + ops.reload.mockImplementation(() => { + live.protectCheck = null; + return Promise.resolve(); + }); + + const outcome = await run(expired(), { container }); + + expect(mockExecute).not.toHaveBeenCalled(); + expect(outcome).toEqual({ status: 'resolved', resource: live }); + }); + + it('reports reissued when the reload produced a fresh challenge', async () => { + const { run, ops, container, live } = setup(expired()); + ops.reload.mockImplementation(() => { + live.protectCheck = challenge({ token: 'challenge-token-2', expiresAt: Date.now() + 60_000 }); + return Promise.resolve(); + }); + + const outcome = await run(expired(), { container }); + + expect(outcome).toEqual({ status: 'reissued' }); + expect(mockExecute).not.toHaveBeenCalled(); + }); + + it('fails with protect_check_timed_out when the reload returns the same expired challenge', async () => { + const { run, ops, container } = setup(expired()); + + await expect(run(expired(), { container })).rejects.toMatchObject({ code: 'protect_check_timed_out' }); + expect(ops.reload).toHaveBeenCalledTimes(1); + }); + + it('stops reloading once the budget is spent and resumes after the caller resets it', async () => { + const { run, ops, container, expiredReloads } = setup(expired()); + + for (let i = 0; i < MAX_EXPIRED_PROTECT_CHECK_RELOADS; i++) { + await expect(run(expired(), { container })).rejects.toMatchObject({ code: 'protect_check_timed_out' }); + } + await expect(run(expired(), { container })).rejects.toMatchObject({ code: 'protect_check_timed_out' }); + expect(ops.reload).toHaveBeenCalledTimes(MAX_EXPIRED_PROTECT_CHECK_RELOADS); + + expiredReloads.current = 0; + await expect(run(expired(), { container })).rejects.toMatchObject({ code: 'protect_check_timed_out' }); + expect(ops.reload).toHaveBeenCalledTimes(MAX_EXPIRED_PROTECT_CHECK_RELOADS + 1); + }); + }); +}); diff --git a/packages/shared/src/internal/clerk-js/protectCheckRunner.ts b/packages/shared/src/internal/clerk-js/protectCheckRunner.ts new file mode 100644 index 00000000000..98f785b688e --- /dev/null +++ b/packages/shared/src/internal/clerk-js/protectCheckRunner.ts @@ -0,0 +1,90 @@ +import { ClerkRuntimeError, isClerkAPIResponseError } from '../../error'; +import type { ProtectCheckResource } from '../../types'; +import { ERROR_CODES } from './constants'; +import { executeProtectCheck } from './protectCheck'; + +// A GET reload does not re-mint an expired challenge today, so an uncapped reload loop would spin forever. +export const MAX_EXPIRED_PROTECT_CHECK_RELOADS = 2; + +export interface ProtectCheckRunnerResource { + getProtectCheck: () => ProtectCheckResource | null | undefined; + getResource: () => TResource; + reload: () => Promise; + submitProtectCheck: (params: { proofToken: string }) => Promise; +} + +export interface ProtectCheckRunOptions { + container: HTMLDivElement; + expiredReloads: { current: number }; + signal?: AbortSignal; + setWidgetVisible?: (visible: boolean) => Promise; + loadTimeoutMs?: number; +} + +/** `reissued` means the expired challenge was replaced by a fresh one on reload, so run again with it. */ +export type ProtectCheckRunOutcome = { status: 'resolved'; resource: TResource } | { status: 'reissued' }; + +const isExpired = (protectCheck: ProtectCheckResource) => + protectCheck.expiresAt !== undefined && protectCheck.expiresAt < Date.now(); + +const expiredError = () => + new ClerkRuntimeError('Protect verification expired', { code: ERROR_CODES.PROTECT_CHECK_TIMED_OUT }); + +const abortedError = () => new ClerkRuntimeError('Protect check aborted by caller', { code: 'protect_check_aborted' }); + +const reloadExpired = async ( + resource: ProtectCheckRunnerResource, + expiredReloads: { current: number }, +): Promise> => { + if (expiredReloads.current >= MAX_EXPIRED_PROTECT_CHECK_RELOADS) { + throw expiredError(); + } + expiredReloads.current += 1; + + await resource.reload(); + + const refreshed = resource.getProtectCheck(); + if (!refreshed) { + return { status: 'resolved', resource: resource.getResource() }; + } + if (isExpired(refreshed)) { + throw expiredError(); + } + return { status: 'reissued' }; +}; + +/** Runs one Protect challenge against a sign-in or sign-up resource and submits the proof token. */ +export async function runProtectCheck( + resource: ProtectCheckRunnerResource, + protectCheck: ProtectCheckResource, + options: ProtectCheckRunOptions, +): Promise> { + const { container, expiredReloads, signal, setWidgetVisible, loadTimeoutMs } = options; + if (signal?.aborted) { + throw abortedError(); + } + if (isExpired(protectCheck)) { + return reloadExpired(resource, expiredReloads); + } + + // Deliberately unraced. Only the module load is bounded. The challenge itself may wait on a + // person, and a timeout here used to abort valid challenges. + const proofToken = await executeProtectCheck(protectCheck, container, { signal, setWidgetVisible, loadTimeoutMs }); + if (signal?.aborted) { + throw abortedError(); + } + + try { + const updated = await resource.submitProtectCheck({ proofToken }); + return { status: 'resolved', resource: updated }; + } catch (err) { + if (signal?.aborted) { + throw abortedError(); + } + if (isClerkAPIResponseError(err) && err.errors?.[0]?.code === ERROR_CODES.PROTECT_CHECK_ALREADY_RESOLVED) { + await resource.reload(); + return { status: 'resolved', resource: resource.getResource() }; + } + throw err; + } +} diff --git a/packages/ui/src/hooks/useProtectCheckRunner.ts b/packages/ui/src/hooks/useProtectCheckRunner.ts index 316a1c8dc43..ac22da7425d 100644 --- a/packages/ui/src/hooks/useProtectCheckRunner.ts +++ b/packages/ui/src/hooks/useProtectCheckRunner.ts @@ -1,7 +1,7 @@ -import { ClerkRuntimeError, isClerkAPIResponseError } from '@clerk/shared/error'; +import { ClerkRuntimeError } from '@clerk/shared/error'; import { ERROR_CODES } from '@clerk/shared/internal/clerk-js/constants'; +import type { ProtectCheckRunnerResource } from '@clerk/shared/internal/clerk-js/protectCheckRunner'; import { useClerk } from '@clerk/shared/react'; -import type { ProtectCheckResource } from '@clerk/shared/types'; import React from 'react'; import { flushSync } from 'react-dom'; @@ -9,29 +9,7 @@ import { useEnvironment } from '@/ui/contexts/EnvironmentContext'; import { useCardState } from '@/ui/elements/contexts'; import { handleError } from '@/ui/utils/errorHandler'; -/** - * A plain GET reload does not re-mint a protect_check challenge server-side, so an expired - * challenge would otherwise reload → still expired → reload again, forever. Cap the attempts - * and surface an error instead of spinning silently. - * - * NOTE: who re-mints an expired challenge on read (FAPI vs. re-running the gated step) is still - * being decided with the clerk_go team; this cap is the defensive floor until that lands. - */ -const MAX_EXPIRED_RELOADS = 2; - -export interface ProtectCheckRunnerParams { - /** - * Reads the current protect_check off the resource. Called fresh on each effect run because - * `fromJSON` mints a new object on every resource update — we key the effect on the token, not - * this reference, so an unrelated refresh doesn't restart the challenge under the user. - */ - getProtectCheck: () => ProtectCheckResource | null | undefined; - /** Returns the live resource, used to route after a reload (which mutates it in place). */ - getResource: () => TResource; - /** Reloads the underlying resource (GET) to pick up fresh server state. */ - reload: () => Promise; - /** Submits the proof token; resolves to the updated resource. */ - submitProtectCheck: (params: { proofToken: string }) => Promise; +export interface ProtectCheckRunnerParams extends ProtectCheckRunnerResource { /** * Continues the flow once the gate clears (or a chained challenge / already-resolved is * detected). Receives the resource to route on (the `submitProtectCheck` result, or the live @@ -57,10 +35,9 @@ export interface ProtectCheckRunner { } /** - * Shared driver for the `` and `` cards. Both run the - * exact same lifecycle — load + execute the Protect SDK, submit the proof token, continue the flow - * — so the abort/cancel/expiry/timeout/no-RHC handling lives here once instead of being duplicated - * (and drifting) across the two components. + * Shared driver for the `` and `` cards. The challenge + * lifecycle itself lives in `runProtectCheck` from `@clerk/shared`. This hook binds it to the + * card's spinner, error, and continuation. * * Must be called from within a `CardStateProvider`. */ @@ -89,7 +66,6 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam const containerRef = React.useRef(null); const isRunningRef = React.useRef(false); - const reloadCountRef = React.useRef(0); // Identifies the most recent run, so a continuation can tell when a newer challenge replaced it. const runIdRef = React.useRef(0); const [isRunning, setIsRunning] = React.useState(false); @@ -112,10 +88,12 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam const paramsRef = React.useRef(params); paramsRef.current = params; + const reloadCountRef = React.useRef(0); + const token = params.getProtectCheck()?.token; React.useEffect(() => { - const { getProtectCheck, getResource, reload, submitProtectCheck, onResolved } = paramsRef.current; + const { getProtectCheck, onResolved } = paramsRef.current; const protectCheck = getProtectCheck(); if (!protectCheck || isRunningRef.current) { return; @@ -126,8 +104,7 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam // Routing after the gate clears must survive the effect re-run that // clearing `protectCheck` triggers — that re-run is our cue to route, not a // reason to bail. So the onResolved paths below key on REAL unmount, not the - // effect's per-run `cancelled` flag (which the re-run's cleanup sets). Same - // rationale the expired-reload path already relies on above. + // effect's per-run `cancelled` flag (which the re-run's cleanup sets). const isUnmounted = () => !mountedRef.current; const cleanup = () => { @@ -138,12 +115,6 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam isRunningRef.current = false; }; - const failWith = (code: string, message: string) => { - isRunningRef.current = false; - setIsRunning(false); - handleError(new ClerkRuntimeError(message, { code }), [], card.setError); - }; - // The script owns the widget-visibility decision: it receives this callback in its init // payload (as `setWidgetVisible`) and calls it right before revealing UI in the container, // and again with `false` once its widget is done. flushSync so the returned promise only @@ -162,64 +133,21 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam // Fail closed in no-RHC builds (chrome extension / clerk.no-rhc.js): the gate requires a // remote `import(sdk_url)` we must not perform there. This guard MUST live in the component - // layer — `executeProtectCheck` is in `@clerk/shared`, compiled once with the flag hard-coded - // `false`, so a guard there would never trip. + // layer. The runner is in `@clerk/shared`, compiled once with the flag hard-coded `false`, + // so a guard there would never trip. if (__BUILD_DISABLE_RHC__) { - failWith( - ERROR_CODES.PROTECT_CHECK_UNSUPPORTED_ENVIRONMENT, - 'Protect verification is not supported in this environment', + isRunningRef.current = false; + setIsRunning(false); + handleError( + new ClerkRuntimeError('Protect verification is not supported in this environment', { + code: ERROR_CODES.PROTECT_CHECK_UNSUPPORTED_ENVIRONMENT, + }), + [], + card.setError, ); return; } - // Expired challenge: reload once to pick up a fresh challenge if the server minted one, but - // cap the attempts (see MAX_EXPIRED_RELOADS) so a server that returns the same expired - // challenge on read can't spin us forever. - if (protectCheck.expiresAt !== undefined && protectCheck.expiresAt < Date.now()) { - if (reloadCountRef.current >= MAX_EXPIRED_RELOADS) { - failWith(ERROR_CODES.PROTECT_CHECK_TIMED_OUT, 'Protect verification expired'); - return; - } - reloadCountRef.current += 1; - isRunningRef.current = true; - setIsRunning(true); - runIdRef.current += 1; - void (async () => { - try { - await reload(); - if (!mountedRef.current) { - return; - } - const refreshed = getProtectCheck(); - const stillExpired = !!refreshed && refreshed.expiresAt !== undefined && refreshed.expiresAt < Date.now(); - if (stillExpired) { - // The server didn't re-mint on read. Don't sit on a spinner — fail loud so the user - // gets a retry instead of an indefinite wait. - failWith(ERROR_CODES.PROTECT_CHECK_TIMED_OUT, 'Protect verification expired'); - } else if (!refreshed) { - // The reload cleared the gate or completed the flow. Route on the refreshed live - // resource so the flow continues (and a completed sign-in still gets `setActive`) - // instead of the route guard bouncing the user back to flow start. Keyed on mount (not - // `cancelled`): clearing protectCheck re-runs/cancels this effect, and that re-run is - // our signal to route — it must not abort the routing. - await onResolved(getResource(), () => !mountedRef.current); - } - // Otherwise the server re-minted a fresh, non-expired challenge whose new token - // re-triggers this effect (keyed on the token), which then runs it. - } catch (err: any) { - if (mountedRef.current) { - reportError(err); - } - } finally { - if (mountedRef.current) { - isRunningRef.current = false; - setIsRunning(false); - } - } - })(); - return cleanup; - } - const container = containerRef.current; if (!container) { return; @@ -257,50 +185,20 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam if (__BUILD_DISABLE_RHC__) { return; } - const { executeProtectCheck } = await import('@clerk/shared/internal/clerk-js/protectCheck'); - // 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, { + const { runProtectCheck } = await import('@clerk/shared/internal/clerk-js/protectCheckRunner'); + const outcome = await runProtectCheck(paramsRef.current, protectCheck, { + container, + expiredReloads: reloadCountRef, signal: abortController.signal, setWidgetVisible, loadTimeoutMs, }); - if (cancelled) { - return; - } - - let updatedResource: TResource; - try { - updatedResource = await submitProtectCheck({ proofToken }); - } catch (err) { - if (cancelled) { - return; - } - // `protect_check_already_resolved` is retry-safe: the server's state has already moved - // past this gate. Reload to clear the stale local protectCheck, then continue routing on - // the refreshed live resource. - if (isClerkAPIResponseError(err) && err.errors?.[0]?.code === ERROR_CODES.PROTECT_CHECK_ALREADY_RESOLVED) { - continuing = true; - await reload(); - if (isUnmounted()) { - return; - } - await onResolved(getResource(), isUnmounted); - return; - } - throw err; - } - if (isUnmounted()) { + // A reissued challenge carries a new token, which re-runs this effect (keyed on the token). + if (outcome.status === 'reissued' || isUnmounted()) { return; } continuing = true; - await onResolved(updatedResource, isUnmounted); + await onResolved(outcome.resource, isUnmounted); } catch (err: any) { if (!ownsOutcome()) { return; @@ -324,8 +222,8 @@ export function useProtectCheckRunner(params: ProtectCheckRunnerParam const retry = React.useCallback(() => { card.setError(''); - reloadCountRef.current = 0; isRunningRef.current = false; + reloadCountRef.current = 0; // The gate already cleared and it was the continuation that failed: there is no challenge left // to re-run, so re-running the effect would do nothing. Retry the continuation instead.