feat(ui): Support signUpIfMissing with Clerk <SignIn> component - #7928
feat(ui): Support signUpIfMissing with Clerk <SignIn> component#7928dmoerner wants to merge 23 commits into
Conversation
The `<SignIn>` component can already be used in a sign-in-or-sign-up flow (`CombinedFlow`) under certain conditions. When strict enumeration protection is enabled, make that combined flow pass the `signUpIfMissing` parameter to the backend to allow an enumeration-safe combined flow. Previously, attempting to use a combined flow with strict enumeration protection enabled was silently broken. Under the hood, the backend treats sign up if missing as an account transfer. We therefore add support for this account transfer logic when handling first factor verification in the combined sign in flow when strict enumeration protection is enabled.
🦋 Changeset detectedLatest commit: 1cbe26e The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds transferable email-link verification support for the combined sign-in-or-sign-up flow. The changes update verification polling and redirect handling, add transferable UI rendering and localization keys, and extend tests and release metadata. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/clerk-js/src/core/resources/SignIn.ts`:
- Around line 311-312: SignInFuture.waitForEmailLinkVerification currently
treats only 'verified' and 'expired' as terminal statuses, which causes
transferable flows started by createEmailLinkFlow to poll indefinitely; update
the status check inside SignInFuture.waitForEmailLinkVerification to include
'transferable' alongside 'verified' and 'expired' so the polling loop calls
stop()/resolves when status === 'transferable', ensuring transferable email-link
flows terminate properly.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.changeset/fancy-candies-slide.mdpackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/resources/UserSettings.tspackages/shared/src/errors/emailLinkError.tspackages/shared/src/types/userSettings.tspackages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsxpackages/ui/src/components/SignIn/SignInFactorOneEmailLinkCard.tsxpackages/ui/src/components/SignIn/SignInStart.tsxpackages/ui/src/components/SignIn/__tests__/SignInFactorOneTransfer.test.tsxpackages/ui/src/components/SignIn/__tests__/SignInStart.test.tsxpackages/ui/src/components/SignIn/__tests__/handleSignUpIfMissingTransfer.test.tspackages/ui/src/components/SignIn/handleSignUpIfMissingTransfer.tspackages/ui/src/test/fixture-helpers.tspackages/ui/src/test/fixtures.ts
This is connected to custom flows and was missed in the previous PRs supporting custom flows. Let's add it now while we are here.
| const res = await this.#resource.__internal_baseGet(); | ||
| const status = res.firstFactorVerification.status; | ||
| if (status === 'verified' || status === 'expired') { | ||
| if (status === 'verified' || status === 'expired' || status === 'transferable') { |
There was a problem hiding this comment.
This change is not necessary for Clerk builtin components but was missed in #7749. As suggested by CodeRabbit, let's just fix it now.
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
|
|
Working scenarios I tested locally:
Scenarios I didn't test locally:
Under investigation:
|
The link-click tab of a signUpIfMissing email-link flow previously showed 'Successfully signed in', but the user is not signed in yet - the email was verified and the original tab continues the flow as a sign-up. Render a dedicated 'Email verified' card (new signIn.emailLink.verifiedTransferable localization keys) driven by the __clerk_status=transferable query param. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/core/clerk.ts (1)
3275-3293: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAbort the token refresh before falling back.
timeLimitonly rejects the wrapper here; the underlyingsession.getToken()keeps running and can still emitevents.TokenUpdateafter the cookie fallback has been applied and polling resumes. Make the refresh cancelable or ignore late completions so a stale request can’t overwrite the fallback auth state.🤖 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/clerk-js/src/core/clerk.ts` around lines 3275 - 3293, Update the token refresh flow around session.getToken() and timeLimit() so a timeout cancels the underlying refresh, or otherwise prevents its late completion from applying state after the cookie fallback. Ensure the fallback createClientFromJwt(jwt) result remains authoritative before startPollingForToken() resumes, and prevent stale events.TokenUpdate emissions from overwriting it.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/clerk.ts (1)
2290-2297: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for
__clerk_status=transferable.Test that
onVerifiedOnOtherDeviceis invoked,setActiveis not called, and noEmailLinkErroris thrown. The existing test atpackages/clerk-js/src/core/__tests__/clerk.test.ts:2875-2899covers onlyverified.🤖 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/clerk-js/src/core/clerk.ts` around lines 2290 - 2297, The existing clerk verification tests lack regression coverage for the transferable status branch. Extend the relevant test near the existing verified case to exercise __clerk_status=transferable, asserting onVerifiedOnOtherDevice is invoked, setActive is not called, and no EmailLinkError is thrown.
🤖 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.
Outside diff comments:
In `@packages/clerk-js/src/core/clerk.ts`:
- Around line 3275-3293: Update the token refresh flow around session.getToken()
and timeLimit() so a timeout cancels the underlying refresh, or otherwise
prevents its late completion from applying state after the cookie fallback.
Ensure the fallback createClientFromJwt(jwt) result remains authoritative before
startPollingForToken() resumes, and prevent stale events.TokenUpdate emissions
from overwriting it.
---
Nitpick comments:
In `@packages/clerk-js/src/core/clerk.ts`:
- Around line 2290-2297: The existing clerk verification tests lack regression
coverage for the transferable status branch. Extend the relevant test near the
existing verified case to exercise __clerk_status=transferable, asserting
onVerifiedOnOtherDevice is invoked, setActive is not called, and no
EmailLinkError is thrown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1acd0b4d-6e3a-4071-b846-0000ad8dd77d
📒 Files selected for processing (3)
packages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/resources/__tests__/SignIn.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/src/core/resources/tests/SignIn.test.ts
- packages/clerk-js/src/core/resources/SignIn.ts
There was a problem hiding this comment.
Left some minor comments.
Overall the approach makes sense to me, and to be clear on rollout, the current behavior is already broken for this population. A non-existent account hitting the combined flow with enumeration protection enabled dead-ends today.
We can ship as minor, and maybe with a changelog/docs entry.
There was a problem hiding this comment.
Appreciate you moving this to a helper 🙏🏼
| // stay at the call sites. | ||
| const signUpIfMissingEnabled = | ||
| isCombinedFlow && | ||
| userSettings.attackProtection.enumeration_protection.enabled && |
There was a problem hiding this comment.
attackProtection is added to the resource in this PR, so older clerk-js runtimes don't populate it and this throws under ui/clerk-js version skew. Other userSettings reads are safe to deref because their fields exist in every runtime in the wild, this one isn't yet.
It's a small safe change
| userSettings.attackProtection.enumeration_protection.enabled && | |
| Boolean(userSettings.attackProtection?.enumeration_protection?.enabled) && |
There was a problem hiding this comment.
Thank you, implemented!
| throw new EmailLinkError(EmailLinkErrorCodeStatus.Expired); | ||
| } else if (verificationStatus === 'client_mismatch') { | ||
| throw new EmailLinkError(EmailLinkErrorCodeStatus.ClientMismatch); | ||
| } else if (verificationStatus === 'transferable') { |
There was a problem hiding this comment.
Since no session exists here yet, if the verification link replaces the polling tab or the original tab is gone, nothing runs the transfer and the user is told to return to a tab that no longer exists, correct? Should this branch complete the transfer on the current client and mirror the verified handling further down?
There was a problem hiding this comment.
That's a good point. I went back and forth here on how to handle the polling tab or the new tab. I'm going to switch to using the other tab. Currently re-running my E2E tests with this change.
| return clerk.__internal_navigateWithError('..', err.errors[0]); | ||
| } | ||
|
|
||
| if (signUpIfMissingEnabled && signIn.firstFactorVerification.status === 'transferable') { |
There was a problem hiding this comment.
The tests covering this branch mock the rejection as form_identifier_not_found, but FAPI returns sign_up_if_missing_transfer for this case and marks firstFactorVerification.status as transferable, correct?
There was a problem hiding this comment.
Great point, fixed.
`attackProtection` is introduced on the UserSettings resource in this same change, so an app pinned to an older clerk-js runtime builds the resource without the field and the combined SignIn crashes on the deref. Optional-chain it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FAPI answers attempt_first_factor with 404 sign_up_if_missing_transfer and marks firstFactorVerification as transferable; the mocks used form_identifier_not_found. Behaviour is unchanged because the component branches on the resource status rather than the code, but the mocks should reflect what the backend actually returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A transferable email-link verification was always transferred by the polling tab, so the tab the link opened in showed "return to the original tab" and dead-ended when that tab had been closed or replaced. It also inverted the convention the non-transferable path already follows, where the newly opened tab takes over and the polling tab points at it. Check verifiedFromTheSameClient() before the transferable branch so the same-client case hands off to the new tab, and give the SignIn verify route an onTransferable hook that runs the transfer when this client holds the banked account transfer. Cross-device is unchanged and still transfers from the polling tab, which is forced anyway: the transfer is banked on the sign-in's client, so no other client has one to consume. Exactly one tab transfers in both cases, so the tabs cannot race for it. The polling tab's hand-off card reuses verifiedSwitchTab.subtitleNewTab rather than adding a near-duplicate string to every locale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@wobsoriano Thanks for the review. I made the changes you suggested. I also re-ran my E2E tests against staging covering the following flows:
|
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
Thank you so much for resolving the comments 🙏🏼
…n-or-up On an instance with both password and strict enumeration protection enabled, a visitor without an account is routed to the password screen and cannot get any further: enumeration protection makes the sign-in advertise every first factor the instance supports, password is preferred by default, and no sign-up transfer is possible from there. This predates the sign-up-if-missing work — the combined flow was already a dead end for new users on that configuration. The component cannot recover on its own, because a wrong password and a non-existent user are deliberately indistinguishable to the client. Neither can the build: the flow is selected from SDK options that are only observable in the browser bundle, and the instance settings only arrive with the environment fetch, so there is no point where both are known. A development-mode warning naming both settings is the most that can be detected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the review. I also added a dev-only logger warning if the combined flow is enabled with password enabled and strict enumeration protection enabled, since that's the path that's still not covered by this fix. Going to work on docs and changelog now so we can get this all out together. |
2 participants
Description
The
<SignIn>component can already be used in a sign-in-or-sign-up flow (CombinedFlow) under certain conditions. When strict enumeration protection is enabled, make that combined flow pass thesignUpIfMissingparameter to the backend to allow an enumeration-safe combined flow. Previously, attempting to use a combined flow with strict enumeration protection enabled was silently broken.Under the hood, the backend treats sign up if missing as an account transfer. We therefore add support for this account transfer logic when handling first factor verification in the combined sign in flow when strict enumeration protection is enabled.
Fixes USER-4769.
We should merge this in coordination with documentation and a Changelog release, since it will change existing behavior (although with a fix for something that is currently broken)
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change