feat(clerk-js,shared,ui): Add Protect SDK challenge support during sign-up and sign-in#8329
feat(clerk-js,shared,ui): Add Protect SDK challenge support during sign-up and sign-in#8329zourzouvillys wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b745323 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
24c3383 to
8529397
Compare
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
8529397 to
63aa1cd
Compare
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
63aa1cd to
a8d12d1
Compare
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
a8d12d1 to
2e82e39
Compare
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
2e82e39 to
ac07df4
Compare
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
ac07df4 to
b7e6942
Compare
…gn-up and sign-in Adds client-side support for mid-flow SDK challenges issued by the antifraud service during sign-up and sign-in. - New `protectCheck` field and `submitProtectCheck()` method on SignUp and SignIn resources - New `'needs_protect_check'` value on the SignInStatus union - New `protect-check` route on the prebuilt `<SignIn />` and `<SignUp />` components that loads the challenge SDK, submits the proof token, and resumes the flow
zourzouvillys
force-pushed
the
theo/protect-check-sdk-support
branch
from
b7e6942 to
0f726f7
Compare
| return useCallback(async (...args: Parameters<typeof authenticateWithPasskey>) => { | ||
| try { | ||
| const res = await authenticateWithPasskey(...args); | ||
| // Per spec §2.3 / §4: protect_check can fire on attempt_first_factor (which is what |
There was a problem hiding this comment.
[minor] is this a reference to an LLM spec document?
There was a problem hiding this comment.
Good catch — that § reference pointed at the internal Protect FAPI design doc by section number, which isn't useful to reviewers and reads like an artifact. I've reworded this and all the other Per spec §X.X comments across the PR to describe the behavior directly instead. Fixed in ead7059.
| <Route path='create'> | ||
| <Route |
There was a problem hiding this comment.
I believe this route segment also needs a protect-check path. Looks like we have navigations to create/protect-check in the combined flow
| <Route path='create'> | |
| <Route | |
| path='protect-check' | |
| canActivate={clerk => !!clerk.client.signUp.protectCheck} | |
| > | |
| <LazySignUpProtectCheck /> | |
| </Route> | |
| <Route |
There was a problem hiding this comment.
You're right, this was a real gap — handleCombinedFlowTransfer.ts:103 navigates to create/protect-check but no route was registered there, so a Protect gate during combined-flow sign-up would dead-end. Added the nested route in ead7059:
<Route path='create'>
<Route
path='protect-check'
canActivate={clerk => !!clerk.client.signUp.protectCheck}
>
<LazySignUpProtectCheck />
</Route>
...This also needed LazySignUpProtectCheck added to lazy-sign-up.ts and SignUpProtectCheck re-exported from the SignUp barrel (it was only used internally before). Gated on signUp.protectCheck since the combined flow's create/* segment drives the sign-up resource.
| }); | ||
| } | ||
|
|
||
| // Per Protect spec §4.4: OAuth/SAML callbacks can result in a protect_check gate that |
There was a problem hiding this comment.
What spec is this referring to?
There was a problem hiding this comment.
Same as above — internal Protect design doc section. Reworded to explain the behavior directly (OAuth/SAML callbacks can resolve into a protect_check gate that surfaces on the next /v1/client read, so we check before the transfer logic). Fixed in ead7059.
|
@zourzouvillys The core stuff looks good. I think the biggest gap is the routing logic integration. Feels like it this is targeting the standalone |
…k-support # Conflicts: # packages/shared/src/types/signInFuture.ts # packages/shared/src/types/signUpCommon.ts # packages/shared/src/types/signUpFuture.ts # packages/ui/src/elements/contexts/index.tsx
| * also surface `status === 'needs_protect_check'`. Either signal triggers navigation | ||
| * to the protect-check route. | ||
| */ | ||
| export function isSignInProtectGated(signIn: SignInResource): boolean { |
There was a problem hiding this comment.
Since the gate is checked per call site, it's easy for one to slip through, and a missed one strands the user (the email-link and prepare/resend paths and web3 in clerk.authenticateWithWeb3 look like they don't run it). Could we double-check every sign-in response path wires this in? Longer term a single choke point that checks the gate before dispatching would make it so new call sites can't forget.
There was a problem hiding this comment.
Did both. Added navigateOnSignInProtectGate as the single place that turns a gated response into navigation, and routed every existing call site through it (dropping the now-dead needs_protect_check switch branches). For the missed paths you flagged: wired the email-link result handler and the inline web3/Solana path (clerk.authenticateWithWeb3, which doesn't redirect so _handleRedirectCallback never catches it — it now takes a protectCheckUrl). One honest gap remains: a sign-up via web3 wallet that gets gated — I guarded it from falling into /continue but didn't route it to a sign-up challenge, since that's a rarer combo we'll confirm the resource/endpoint for first. For prepare/resend: the re-issue is followed by an attempt, which is already wired — flag me if the server can gate on prepare itself.
| // Reload the resource so the server can mint a fresh challenge before re-routing, | ||
| // otherwise the local stale `signIn.protectCheck` would re-trigger this same effect | ||
| // and loop indefinitely with no user feedback. | ||
| if (protectCheck.expiresAt !== undefined && protectCheck.expiresAt < Date.now()) { |
There was a problem hiding this comment.
I checked the FAPI side and I think this loops forever once a challenge expires. A plain GET on the sign-in doesn't mint a new challenge (only create/attempt/prepare do that), and the serializer returns the protect_check as long as it's pending, expired or not. So the reload here gets the same expired challenge back, the effect runs again, sees it's expired, reloads again, and so on. The user just sees the spinner while we hit FAPI in a loop.
Getting a new challenge means either re-running the gated step or having FAPI re-mint on read, probably worth deciding with the clerk_go folks which side owns that. Either way I'd put a cap on the reloads so it can't spin. (Units are fine btw, FAPI sends expires_at in ms.)
There was a problem hiding this comment.
You're right that a plain GET doesn't re-mint, so I capped it (MAX_EXPIRED_RELOADS) and, after a reload that comes back still-expired, fail loud with a retryable error instead of spinning. Combined with keying the effect on the token (see the :192 thread), it can't loop. The real fix — re-running the gated step vs. FAPI re-minting on read — is a server-side decision we'll settle separately. (And confirmed: expires_at is Unix-epoch milliseconds, so the < Date.now() check is unit-correct; I added a JSDoc noting the unit so it's unambiguous going forward.)
| // chained challenges work correctly across re-renders. | ||
| isRunningRef.current = false; | ||
| }; | ||
| }, [signIn.protectCheck]); |
There was a problem hiding this comment.
fromJSON makes a new protectCheck object on every resource update, so any unrelated refresh re-runs this effect and restarts the challenge under the user. Keying the dep on protectCheck?.token would fix that and still re-run for a real chained challenge. (Same in SignUpProtectCheck.)
There was a problem hiding this comment.
Fixed — the effect is now keyed on protectCheck?.token (a primitive), so an unrelated fromJSON refresh no longer restarts the challenge, while a genuine chained challenge (new token) still re-runs. Applied in both cards via the shared useProtectCheckRunner hook.
| if (cancelled) { | ||
| return; | ||
| } | ||
| handleError(err, [], card.setError); |
There was a problem hiding this comment.
I like that failures surface a proper card error here. A small gap: the message says "please try again" but there's no retry control and nothing re-runs the effect, so recovering takes a full page reload. A retry action on the error state would round this out, and a timeout around the script run would cover the case where the challenge SDK never settles.
There was a problem hiding this comment.
Added both. The error state now renders a "Try again" control (signIn/up.protectCheck.retryButton) that clears the error and re-runs from scratch, and there's a 60s timeout (Promise.race) around the SDK run that aborts and surfaces a retryable protect_check_timed_out.
| if (cancelled) { | ||
| return; | ||
| } | ||
| await navigateNext(signIn, navigate); |
There was a problem hiding this comment.
Can you double-check what happens here when the reload comes back complete? It looks like this branch skips finalizeIfComplete, and navigateNext's complete case just goes to '..' expecting the caller to have run setActive, so the user would land back on the start form without the session being activated. If that's right, mirroring the success path below (finalize, then navigate) should cover it. The sign-up side already does this via completeSignUpFlow's handleComplete.
There was a problem hiding this comment.
Good find — fixed. The continuation now runs through one onResolved(resource) path that finalizes (setActive) the complete case from both the normal success and the protect_check_already_resolved reload, so neither bounces to the start form with an unactivated session. Added a test covering the reload-reveals-complete case.
…k-support # Conflicts: # packages/clerk-js/bundlewatch.config.json # packages/ui/src/components/SignIn/SignInStart.tsx
| // OAuth/SAML callbacks can resolve into a protect_check gate that surfaces on the next | ||
| // /v1/client read, so check for it here before continuing with the transfer logic below. | ||
| // Honor either the explicit `protectCheck` field or the `needs_protect_check` status override. | ||
| if (si.protectCheck || si.status === 'needs_protect_check') { |
There was a problem hiding this comment.
I traced this through clerk_go and I think it can grab the wrong flow. This check runs before all the sign-up branches and reads si straight from this.client.signIn, with no scoping to which flow the callback is for. An abandoned sign-in sticks around on the client for a day (AbandonAt is now + 1 day) and keeps serializing its pending protect_check, and in multi-session mode a later sign-up doesn't clear it. So if someone starts a sign-in, hits the gate, bails, then signs up with Google, the callback sees the stale protectCheck and routes them into the old sign-in's protect-check instead of finishing the Google sign-up.
Could we scope this to the sign-in callback (e.g. gate on the same reloadResource/intent the rest of the function uses) so a sign-up callback won't pick it up?
There was a problem hiding this comment.
Great catch, and thanks for tracing it through clerk_go. Fixed: both gate checks are now scoped to the callback intent — si.protectCheck only routes when params.reloadResource !== 'signUp', and the su.protectCheck check is symmetric (!== 'signIn'). So a Google sign-up callback no longer picks up the stale, abandoned sign-in's protect_check. Undefined intent keeps the legacy behavior, and transfers are unaffected since the signIn.create({ transfer }) path checks its own fresh response. Added two handleRedirectCallback tests (sign-up callback completes instead of routing into the stale gate; sign-in callback routes to /sign-in#/protect-check).
Address PR #8329 review (component effect bugs + no-RHC/UX): - Extract useProtectCheckRunner so SignIn/SignUp cards share one lifecycle (no per-component drift). - Key the effect on protectCheck.token, not the object identity, so an unrelated resource refresh no longer restarts the challenge under the user. - Cap expired-challenge reloads (a plain GET does not re-mint) and fail loud instead of spinning forever; pending a clerk_go decision on read re-minting. - Finalize (setActive) when an already_resolved reload reveals a complete status, instead of bouncing to the start form with an unactivated session. - Fail closed in no-RHC builds (__BUILD_DISABLE_RHC__) before the remote import(sdk_url); the guard lives in the component, not @clerk/shared. - Add a retry control on the error state and a timeout around the SDK run. - New error codes protect_check_timed_out / protect_check_unsupported_environment and a protectCheck.retryButton localization key (en-US).
| legal_accepted_at: number | null; | ||
| locale: string | null; | ||
| verifications: SignUpVerificationsJSON | null; | ||
| protect_check: ProtectCheckJSON | null; |
There was a problem hiding this comment.
[nit] This is required here but optional on SignInJSON (protect_check?). Older FAPI versions don't send the field, so optional matches the wire format better and keeps the two consistent.
There was a problem hiding this comment.
Done — protect_check? is now optional on SignUpJSON, matching SignInJSON and the wire format for older FAPI versions.
| @@ -0,0 +1,21 @@ | |||
| --- | |||
| '@clerk/clerk-js': minor | |||
There was a problem hiding this comment.
@clerk/react is missing here. stateProxy.ts adds protectCheck/submitProtectCheck to the public state proxies, so that's user-facing surface in @clerk/react that needs a '@clerk/react': minor entry, otherwise it ships with no changelog and only an implicit dependency bump.
There was a problem hiding this comment.
Added '@clerk/react': minor. You're right — stateProxy.ts puts protectCheck/submitProtectCheck on the public state proxies, so it's user-facing surface that needs its own changelog entry.
Address PR #8329 review (routing dead-ends): - Add navigateOnSignInProtectGate as the single place that turns a gated sign-in response into navigation; route all existing call sites through it (passkey, password, code, backup-code, reset-password, alt-channel) and drop the now-dead needs_protect_check switch branches. - Pass the protect-check path per caller: the passkey handler took a hard '../protect-check' that 404s from SignInStart (index route); it now accepts the path, and SignInStart passes 'protect-check'. Fixes the autofill passkey dead-end on a gated start page. - Wire the previously-missed paths: the inline web3/Solana sign-in (clerk.authenticateWithWeb3, no redirect) and the email-link verification result now route the gate instead of stranding the user. web3 gains a protectCheckUrl param. - clerk-js _handleRedirectCallback: check the sign-up gate BEFORE the missing_fields short-circuit (a gated sign-up always carries 'protect_check' in missing_fields, so OAuth/SAML callbacks were landing on /continue), and add an early su.protectCheck check mirroring the sign-in one. - Register protect-check routes under continue and create/continue, and parameterize SignUpProtectCheck's continuation paths so a gate at those depths routes within the right subtree (continuePath '..' vs '../continue').
| }); | ||
| }); | ||
|
|
||
| describe('protectCheck', () => { |
There was a problem hiding this comment.
[nit] This block sets BaseResource._fetch = mockFetch but, unlike the sibling describes here, has no afterEach to restore it. It's harmless today since it's the last describe, but it'd leak the mock if another block is added below. Also, the submitProtectCheck assertion checks method/body but not the path, worth asserting it hits .../{id}/protect_check so the test would catch a wrong/dropped action. (Same in SignUp.test.ts.)
There was a problem hiding this comment.
Both fixed, in SignIn.test.ts and SignUp.test.ts: added an afterEach that restores BaseResource._fetch (saved at describe scope) so the mock can't leak, and the submitProtectCheck assertion now checks path: '/client/sign_{ins,ups}/{id}/protect_check' so a wrong/dropped action would fail the test.
| style={{ display: 'block', alignSelf: 'center', minHeight: '60px' }} | ||
| /> | ||
| {isRunning && !card.error ? ( | ||
| <Box style={{ alignSelf: 'center' }}>{t(localizationKeys('signIn.protectCheck.loading'))}</Box> |
There was a problem hiding this comment.
[nit] This loading indicator is bare text with an inline style, no elementDescriptor and no aria-live/aria-busy, so screen readers won't announce that verification is running and themes can't target it. The other waiting cards use LoadingCardContainer with descriptors.spinner, might be nice to reuse that here.
There was a problem hiding this comment.
Reworked in both cards: the bare loading text is now a descriptors.spinner Spinner inside an aria-live='polite' region, and the SDK container is marked aria-busy while running — so screen readers announce it and themes can target it. Went with the inline Spinner (above the SDK container) rather than full LoadingCardContainer, but happy to switch if you'd prefer the consistency.
Address PR #8329 review round 3: - _handleRedirectCallback: an abandoned sign-in keeps serializing its pending protect_check on the client (AbandonAt now+1d; a later sign-up doesn't clear it in multi-session). The unscoped gate check would route a sign-up callback into the stale sign-in's challenge. Scope each gate to params.reloadResource so a sign-up callback can't pick up a stale sign-in (and vice versa); undefined intent keeps the legacy behavior. Transfers are unaffected (the signIn.create({ transfer }) path checks its own fresh response). +2 tests. - Make protect_check optional on SignUpJSON to match SignInJSON / the wire format (older FAPI versions omit it). - Add '@clerk/react': minor to the changeset (stateProxy.ts exposes protectCheck/submitProtectCheck on the public state proxies). - Resource tests: restore BaseResource._fetch in an afterEach and assert the submitProtectCheck request path hits .../{id}/protect_check. - ProtectCheck cards: replace the bare loading text with a descriptors.spinner Spinner in an aria-live region and mark the SDK container aria-busy.
…conds The field is a bare integer whose unit (ms vs s) was ambiguous to readers; note it explicitly on ProtectCheckJSON.expires_at and ProtectCheckResource.expiresAt.
…into theo/protect-check-sdk-support
|
@Ephem on the breaking-change question: |
API Changes Report
Summary
@clerk/clerk-jsCurrent version: 6.16.0 Subpath
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/SignIn/shared.ts (1)
44-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the
useCallbackdependency array.The callback captures multiple values—
authenticateWithPasskey,navigate,protectCheckPath,setActive,navigateOnSetActive,afterSignInUrl,supportEmail,onSecondFactor, andcard.setError—but declares no dependencies. This creates a stale-closure risk: if any of those values change, the callback continues using the old values.🔧 Suggested fix
- }, []); + }, [authenticateWithPasskey, navigate, protectCheckPath, setActive, navigateOnSetActive, afterSignInUrl, supportEmail, onSecondFactor, card]);Note: Include
cardinstead ofcard.setErrorto satisfy exhaustive-deps; thesetErrormethod is stable within the card object's lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/SignIn/shared.ts` around lines 44 - 84, The useCallback returned function closes over many external values but currently has an empty dependency array, causing stale closures; update the dependency array for the callback returned by the hook to include authenticateWithPasskey, navigate, protectCheckPath, setActive, navigateOnSetActive, afterSignInUrl, supportEmail, onSecondFactor, and card (use card instead of card.setError per exhaustive-deps guidance) so the callback updates when any of these change; locate the useCallback invocation in this file (the function that calls authenticateWithPasskey and uses navigateOnSignInProtectGate, setActive, navigateOnSetActive, afterSignInUrl, supportEmail, onSecondFactor, and card.setError) and add those symbols to its dependency array.
🧹 Nitpick comments (5)
packages/shared/src/types/signUp.ts (1)
52-52: ⚡ Quick winDocument new public protect-check members on
SignUpResource.
protectCheckandsubmitProtectCheckare new public API members but currently have no JSDoc here. Please add concise docs (including expected behavior/params/return), and flag for Docs-team visibility since this can affect generated reference docs.As per coding guidelines, public/reference-facing API changes should include accurate JSDoc and may require docs review.
Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/types/signUp.ts` at line 52, The SignUpResource public API has new members protectCheck and submitProtectCheck but lacks JSDoc; add concise JSDoc blocks for both on the SignUpResource type describing purpose, parameters, return types (e.g., ProtectCheckResource | null for protectCheck and args/response shape for submitProtectCheck), expected behavior (when null is returned or when submitProtectCheck should be called), and any errors thrown; include a docs-team visibility tag or comment to flag this change for generated reference docs review so it’s captured by the documentation pipeline.Source: Coding guidelines
packages/shared/src/internal/clerk-js/protectCheck.ts (1)
100-102: 💤 Low valueConsider documenting the webpack magic comment.
The
/* webpackIgnore: true */comment prevents webpack from attempting to bundle this runtime-determined dynamic import. While this is necessary, it's not immediately obvious why. Consider adding a brief inline comment explaining this is required because the URL is determined at runtime from the server response.Suggested documentation
let mod: Record<string, unknown>; try { + // webpackIgnore prevents webpack from trying to bundle this runtime-determined import mod = await import(/* webpackIgnore: true */ validated.toString()); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/internal/clerk-js/protectCheck.ts` around lines 100 - 102, Add a brief inline comment explaining why the webpack magic comment is used on the dynamic import: annotate the line containing "mod = await import(/* webpackIgnore: true */ validated.toString());" (or immediately above it) to state that webpackIgnore:true is required because the import URL is determined at runtime from the server response and must not be bundled or rewritten by the bundler; keep the comment short and focused.packages/shared/src/internal/clerk-js/completeSignUpFlow.ts (1)
44-46: 💤 Low valueClarify comment wording.
The comment states "The protect_check field is the authoritative gating signal" but then immediately treats both the
protectCheckfield and themissingFieldsentry as equivalent via||. Consider rewording to avoid the implication that one is more authoritative than the other.Suggested rewording
- // The protect_check field is the authoritative gating signal. Sign-up also surfaces it - // via a missing_fields entry; treat either as equivalent. + // Protect-check gating is signaled by either the `protectCheck` resource field or a + // `protect_check` entry in `missingFields`; treat both as equivalent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/internal/clerk-js/completeSignUpFlow.ts` around lines 44 - 46, Update the comment above the isProtectGated computation to remove the implication that one signal is "authoritative" since the code treats signUp.protectCheck and signUp.missingFields equivalently; reword to explain that protect_check can be present either as protectCheck or as an entry in missingFields and both should be treated the same (referencing signUp.protectCheck, signUp.missingFields, and the isProtectGated boolean) so the comment accurately reflects the logic.packages/ui/src/components/SignIn/SignInProtectCheck.tsx (1)
33-56: 💤 Low valueConsider adding an explicit return type to
navigateNext.The function's return type (
Promise<unknown>) is easily inferred, but adding it explicitly aligns with the TypeScript coding guideline: "Always define explicit return types for functions."📝 Suggested addition
-function navigateNext(signIn: SignInResource, navigate: (to: string) => Promise<unknown>) { +function navigateNext(signIn: SignInResource, navigate: (to: string) => Promise<unknown>): Promise<unknown> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/SignIn/SignInProtectCheck.tsx` around lines 33 - 56, Add an explicit return type to the navigateNext function signature to satisfy the TypeScript guideline; update the declaration of navigateNext(signIn: SignInResource, navigate: (to: string) => Promise<unknown>) to include an explicit return type (e.g., : Promise<unknown> or a more specific Promise<void> if appropriate) so the function signature clearly documents its async/navigation return contract.packages/ui/src/components/SignIn/handleProtectCheck.ts (1)
28-38: ⚡ Quick winAdd error handling for the navigation call.
The voided
navigate()call on line 34 silently ignores promise rejections. If navigation fails (rare, but possible with route guards or malformed paths), the function returnstrue, the caller stops processing, but the user remains stranded on the current screen with no error message or recovery path.Add a
.catch()handler to log the error or surface feedback to the user:- void navigate(protectCheckPath); + navigate(protectCheckPath).catch(err => { + console.error('Protect check navigation failed:', err); + });Consider whether to return
falseon navigation failure so the caller can continue with status-based routing, or surface the error to the user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/SignIn/handleProtectCheck.ts` around lines 28 - 38, The navigateOnSignInProtectGate function currently voids the navigate(protectCheckPath) promise which discards rejections; update it to attach a .catch handler to the returned promise (from navigate) to log the error (using the app logger or console.error) and surface user feedback if available, and on navigation failure return false so the caller can continue processing; specifically modify navigateOnSignInProtectGate to call navigate(protectCheckPath).catch(err => { /* log and surface error */ }) and ensure the function returns false when the catch handler runs instead of returning true unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/clerk-js/src/core/clerk.ts`:
- Around line 2853-2859: The guard that skips navigateToSignInProtectCheck when
viaSignUp is true causes a protect-gated fallback from
signIn.authenticateWithWeb3() -> signUp.authenticateWithWeb3() to leave the user
stranded; update the logic around the signInOrSignUp check so that if
signInOrSignUp.protectCheck is true or signInOrSignUp.status ===
'needs_protect_check' you always call await navigateToSignInProtectCheck()
(regardless of viaSignUp) before returning, and ensure the subsequent status
switch handles a missing_requirements case if applicable; also add a regression
test that simulates identifier_not_found leading to
signUp.authenticateWithWeb3() which returns protectCheck, asserting that
navigateToSignInProtectCheck() is invoked and the flow does not remain on the
wallet step.
In `@packages/clerk-js/src/core/resources/SignUp.ts`:
- Around line 1176-1183: Add a JSDoc block above the public method
submitProtectCheck on SignUpFutureResource (in SignUp.ts) describing what the
method does, the params shape (params: { proofToken: string }), the return type
(Promise<{ error: ClerkError | null }>), and a short usage example showing
awaiting the call and handling the error; ensure the JSDoc includes `@param` and
`@returns` annotations and a brief one-line description of the method's behavior.
- Around line 200-206: Add a JSDoc comment for the public method
SignUp.submitProtectCheck describing the purpose, parameters, return type, and a
short usage example; specifically document the params object with proofToken:
string, indicate it returns Promise<SignUpResource>, and include a brief example
like const updatedSignUp = await signUp.submitProtectCheck({ proofToken:
'proof_...' }); Place the JSDoc immediately above the submitProtectCheck method
so it appears in generated reference docs.
In `@packages/ui/src/components/SignIn/shared.ts`:
- Around line 26-29: Add an explicit return type to the exported function
useHandleAuthenticateWithPasskey: change its signature to annotate the return as
the callback type returned by useCallback, i.e. (...args: Parameters<typeof
authenticateWithPasskey>) => Promise<void>; ensure the annotation is placed on
the function declaration for useHandleAuthenticateWithPasskey and references
authenticateWithPasskey for the Parameters<> utility so the exported function
complies with the coding guideline.
---
Outside diff comments:
In `@packages/ui/src/components/SignIn/shared.ts`:
- Around line 44-84: The useCallback returned function closes over many external
values but currently has an empty dependency array, causing stale closures;
update the dependency array for the callback returned by the hook to include
authenticateWithPasskey, navigate, protectCheckPath, setActive,
navigateOnSetActive, afterSignInUrl, supportEmail, onSecondFactor, and card (use
card instead of card.setError per exhaustive-deps guidance) so the callback
updates when any of these change; locate the useCallback invocation in this file
(the function that calls authenticateWithPasskey and uses
navigateOnSignInProtectGate, setActive, navigateOnSetActive, afterSignInUrl,
supportEmail, onSecondFactor, and card.setError) and add those symbols to its
dependency array.
---
Nitpick comments:
In `@packages/shared/src/internal/clerk-js/completeSignUpFlow.ts`:
- Around line 44-46: Update the comment above the isProtectGated computation to
remove the implication that one signal is "authoritative" since the code treats
signUp.protectCheck and signUp.missingFields equivalently; reword to explain
that protect_check can be present either as protectCheck or as an entry in
missingFields and both should be treated the same (referencing
signUp.protectCheck, signUp.missingFields, and the isProtectGated boolean) so
the comment accurately reflects the logic.
In `@packages/shared/src/internal/clerk-js/protectCheck.ts`:
- Around line 100-102: Add a brief inline comment explaining why the webpack
magic comment is used on the dynamic import: annotate the line containing "mod =
await import(/* webpackIgnore: true */ validated.toString());" (or immediately
above it) to state that webpackIgnore:true is required because the import URL is
determined at runtime from the server response and must not be bundled or
rewritten by the bundler; keep the comment short and focused.
In `@packages/shared/src/types/signUp.ts`:
- Line 52: The SignUpResource public API has new members protectCheck and
submitProtectCheck but lacks JSDoc; add concise JSDoc blocks for both on the
SignUpResource type describing purpose, parameters, return types (e.g.,
ProtectCheckResource | null for protectCheck and args/response shape for
submitProtectCheck), expected behavior (when null is returned or when
submitProtectCheck should be called), and any errors thrown; include a docs-team
visibility tag or comment to flag this change for generated reference docs
review so it’s captured by the documentation pipeline.
In `@packages/ui/src/components/SignIn/handleProtectCheck.ts`:
- Around line 28-38: The navigateOnSignInProtectGate function currently voids
the navigate(protectCheckPath) promise which discards rejections; update it to
attach a .catch handler to the returned promise (from navigate) to log the error
(using the app logger or console.error) and surface user feedback if available,
and on navigation failure return false so the caller can continue processing;
specifically modify navigateOnSignInProtectGate to call
navigate(protectCheckPath).catch(err => { /* log and surface error */ }) and
ensure the function returns false when the catch handler runs instead of
returning true unconditionally.
In `@packages/ui/src/components/SignIn/SignInProtectCheck.tsx`:
- Around line 33-56: Add an explicit return type to the navigateNext function
signature to satisfy the TypeScript guideline; update the declaration of
navigateNext(signIn: SignInResource, navigate: (to: string) => Promise<unknown>)
to include an explicit return type (e.g., : Promise<unknown> or a more specific
Promise<void> if appropriate) so the function signature clearly documents its
async/navigation return contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f20032a1-6063-45e7-932e-d891e6a37b1e
📒 Files selected for processing (52)
.changeset/protect-check-support.mdpackages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/resources/SignUp.tspackages/clerk-js/src/core/resources/__tests__/SignIn.test.tspackages/clerk-js/src/core/resources/__tests__/SignUp.test.tspackages/localizations/src/en-US.tspackages/react/src/stateProxy.tspackages/shared/src/internal/clerk-js/__tests__/completeSignUpFlow.test.tspackages/shared/src/internal/clerk-js/__tests__/protectCheck.test.tspackages/shared/src/internal/clerk-js/completeSignUpFlow.tspackages/shared/src/internal/clerk-js/constants.tspackages/shared/src/internal/clerk-js/protectCheck.tspackages/shared/src/types/clerk.tspackages/shared/src/types/json.tspackages/shared/src/types/localization.tspackages/shared/src/types/signIn.tspackages/shared/src/types/signInCommon.tspackages/shared/src/types/signInFuture.tspackages/shared/src/types/signUp.tspackages/shared/src/types/signUpCommon.tspackages/shared/src/types/signUpFuture.tspackages/ui/src/common/EmailLinkVerify.tsxpackages/ui/src/components/SignIn/ResetPassword.tsxpackages/ui/src/components/SignIn/SignInFactorOneAlternativeChannelCodeForm.tsxpackages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsxpackages/ui/src/components/SignIn/SignInFactorOneEmailLinkCard.tsxpackages/ui/src/components/SignIn/SignInFactorOnePasswordCard.tsxpackages/ui/src/components/SignIn/SignInFactorOneSolanaWalletsCard.tsxpackages/ui/src/components/SignIn/SignInFactorTwoBackupCodeCard.tsxpackages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsxpackages/ui/src/components/SignIn/SignInProtectCheck.tsxpackages/ui/src/components/SignIn/SignInSocialButtons.tsxpackages/ui/src/components/SignIn/SignInStart.tsxpackages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsxpackages/ui/src/components/SignIn/handleCombinedFlowTransfer.tspackages/ui/src/components/SignIn/handleProtectCheck.tspackages/ui/src/components/SignIn/index.tsxpackages/ui/src/components/SignIn/lazy-sign-up.tspackages/ui/src/components/SignIn/shared.tspackages/ui/src/components/SignUp/SignUpContinue.tsxpackages/ui/src/components/SignUp/SignUpEmailLinkCard.tsxpackages/ui/src/components/SignUp/SignUpProtectCheck.tsxpackages/ui/src/components/SignUp/SignUpStart.tsxpackages/ui/src/components/SignUp/SignUpVerificationCodeForm.tsxpackages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsxpackages/ui/src/components/SignUp/index.tsxpackages/ui/src/elements/contexts/index.tsxpackages/ui/src/hooks/useProtectCheckRunner.tspackages/ui/src/test/fixture-helpers.tsreferences/mosaic-architecture.md
… review Address CodeRabbit review: - authenticateWithWeb3: when the inline web3 attempt falls back to signUp.authenticateWithWeb3 and the sign-up is gated, route to the sign-up protect-check (new signUpProtectCheckUrl param; combined flow passes 'create/protect-check') instead of leaving the user stranded on the wallet step. - Add JSDoc to the public submitProtectCheck methods (SignUp/SignIn, resource + future) so they document correctly in generated reference docs. - Add explicit return types to useHandleAuthenticateWithPasskey and navigateNext.
The SignUpProtectCheck card + shared runner hook push the signup chunk to 11.28KB gzip, just over the 11KB budget. Bump to 12KB to match the legitimate protect-check feature growth.
|
@zourzouvillys Alright, that makes it clearer, thanks! Some follow up questions.
So if a customer is currently using a custom flow and not handling it, flipping that feature gate is the potential breaking change? I'm guessing we do that pretty intentionally though so doesn't seem like a problem, just something to be aware of.
So some small subset of users might perceive this as a breaking change if they see that? This part of the changeset kinda explains it: "surfaced when the server-side SDK-version gate is enabled", but it's not entirely clear that's something we only enable in dialogue with the customer, so might be worth adding some extra clarity there that this is not something we'll turn on randomly. If I see that TS hint after an upgrade I might go to the changeset and will want to know what to do. Could maybe include info in the typedoc too if there is none already (didn't check)? That's just a small NIT though. 😄 |
3 participants
Summary
Adds client-side support for Clerk Protect mid-flow SDK challenges (
protect_check) during both sign-up and sign-in. When the antifraud service gates a step, the SDK exposes the challenge, surfaces a card that loads and runs the challenge script, submits the resulting proof token, and resumes the original flow.protectCheckfield andsubmitProtectCheck()method on bothSignUpandSignInresources (and their future variants), mirrored on the@clerk/reactstate proxies.'needs_protect_check'value on theSignInStatusunion.protect-checkroute on the prebuilt<SignIn />and<SignUp />components (standalone,continue, and combined-flowcreate/create/continuedepths).Background
Previously anti-fraud blocks could only happen at sign-in/sign-up create time. This mechanism lets the service gate at any step. When gated, the response carries:
{ "protect_check": { "status": "pending", "token": "<challenge token>", "sdk_url": "https://.../sdk.js", "expires_at": 1700000000000, "ui_hints": { "reason": "device_new" } } }expires_atis a Unix epoch timestamp in milliseconds (documented on the type). The client loads the SDK atsdk_url, runs the challenge withtoken, and submits the proof token toPATCH /v1/client/sign_{ins,ups}/{id}/protect_check. The response clears the gate, issues a chained challenge, or completes the flow.Implementation
Types (
@clerk/shared)ProtectCheckJSON/ProtectCheckResource{ status: 'pending', token, sdkUrl, expiresAt?, uiHints? };expires_atis optional on bothSignUpJSONandSignInJSON(older FAPI versions omit it).'protect_check'added toSignUpField;'needs_protect_check'added toSignInStatus.submitProtectCheckadded to the sign-up/sign-in resource + future interfaces.Core resources (
@clerk/clerk-js)SignUp/SignInexposeprotectCheckandsubmitProtectCheck({ proofToken });fromJSON/__internal_toSnapshotround-trip the field; future variants mirror the API.SDK loader helper (
@clerk/shared/internal/clerk-js/protectCheck)executeProtectCheck(protectCheck, container, { signal })— validatessdkUrl(must behttps:, no credentials, rejectsdata:/blob:/javascript:), runs the spec-compliant script contract(container, { token, uiHints, signal }), forwards theAbortSignal, and wraps failures in typed error codes without leaking the URL.Shared card runner (
@clerk/ui)Both protect-check cards share one
useProtectCheckRunnerhook so the lifecycle can't drift:protectCheck.token(not object identity) so an unrelated resource refresh doesn't restart the challenge.__BUILD_DISABLE_RHC__) before the remoteimport(sdk_url)— the guard lives in the component layer because@clerk/sharedis compiled once with the flagfalse.setActive) thecompletecase from both the normal success and theprotect_check_already_resolvedreload, so neither strands the user.descriptors.spinnerspinner in anaria-liveregion.Sign-in gate routing — single choke point
navigateOnSignInProtectGate(res, navigate, protectCheckPath)is the one place that turns a gated sign-in response into navigation. Every dispatch site routes through it (start ×2, passkey, password, code, alt-channel, backup-code, factor-two code, reset-password), with the protect-check path passed per caller (index route →'protect-check', factor cards →'../protect-check'). Also wired into the previously-missed email-link result handler and the inline web3/Solana path (clerk.authenticateWithWeb3, which gains aprotectCheckUrlsince it doesn't redirect through_handleRedirectCallback).OAuth / SAML callback (
clerk.ts)_handleRedirectCallbackchecks the gate before its transfer/missing-fields logic, scoped to the callback intent (reloadResource) so an abandoned sign-in's staleprotect_checkcan't hijack a sign-up callback (and vice versa). The sign-up gate check runs before themissing_fieldsshort-circuit so a gatedsignUp.create({ transfer })routes to the challenge instead of/continue.Prebuilt UI routes (
@clerk/ui)protect-checkroutes registered on<SignIn />/<SignUp />at every depth the flow can mount sign-up at;SignUpProtectChecktakes per-mount continuation paths (thecontinue-nested mounts passcontinuePath='..').Localization (
@clerk/localizations,@clerk/shared)Typed
signUp.protectCheck.{title,subtitle,loading,retryButton}/signIn.protectCheck.*keys andunstable__errorsentries for the runtime error codes (protect_check_execution_failed,…_invalid_script,…_invalid_sdk_url,…_script_load_failed,…_timed_out,…_unsupported_environment;…_aborted/…_already_resolvedintentionally undefined).Backwards compatibility
'needs_protect_check'is type-additive — runtime behavior is unchanged (the server emits it only behind a feature gate, andprotectCheckis the authoritative field). Strict-TypeScript consumers with an exhaustiveswitch (signIn.status)will get a new unhandled-branch hint, hence theminorbump.Risks
signIn.statusneed to handle'needs_protect_check'(or theprotectCheckfield). Documented on the resource interface.(container, { token, uiHints, signal }) => Promise<string>. Coordinate with the Protect SDK team before deploying.script-src; the load-failure error calls this out.Test plan
SignUp.test.ts/SignIn.test.ts— serialization, optional fields, snapshot round-trip,submitProtectCheckpath/method/bodyprotectCheck.test.ts— URL validation, script contract, cancellation, error wrappingcompleteSignUpFlow.test.ts— routing priorityclerk.test.ts— gate routing scoped to the callback intent (stale sign-in not picked up by a sign-up callback; sign-in callback routes to the gate)SignUpProtectCheck.test.tsx/SignInProtectCheck.test.tsx— run/expiry/already-resolved/chained/abort/no-submit-on-failure, finalize-on-reload-complete, retry control@clerk/clerk-js,@clerk/shared,@clerk/localizations,@clerk/uiclean; lint cleanFollow-ups (out of scope)
/continue, but not yet routed to a dedicated sign-up challenge.@clerk/backendresource model updates (the backend SDK doesn't drive end-user flows).protect_check(additive when the server starts emitting it).Summary by CodeRabbit
New Features
Localization
Tests