feat(clerk-js): Add sign_up_if_missing for SignIn.create#7749
feat(clerk-js): Add sign_up_if_missing for SignIn.create#7749
Conversation
We are building a new sign-in-or-sign-up flow compatible with strict enumeration protection. It is meant to complement our existing sign-in-or-sign-up flow. Our current sign-in-or-sign-up flow is managed by the SDKs: We start with a sign in attempt, and on a 422 (when an account does not exist), we redirect to a sign up. The flow is thus: Sign In (422) -> Sign Up (200) -> Attempt Verifications for VerifiedAtSignUp identifiers (200). This is vulnerable to user enumeration attacks because the attacker sees the sign in to sign up redirect before they prove their identity by completing a verification. When `sign_up_if_missing` is passed as a param when POSTing a sign in, instead we do the following: Sign In (200) -> Attempt Verification for Identifier (200) -> Create User and Session. (In future work this third step will be modified to support adding additional information to the user, either via AccountTransfer or Session Tasks). This is enumeration safe, because you only see if an account already existed or was created after you verify your identity. This PR is the first step in SDK support for this new flow. We add support for the optional `sign_up_if_missing` param on `SignIn`. We also add captcha support for `SignIn`. This is all optional and currently in testing with custom components. Support in Clerk components will be in future PRs.
🦋 Changeset detectedLatest commit: 968af9e The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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.
|
| finalParams = { ...finalParams, ...captchaParams }; | ||
| } | ||
|
|
||
| if (params.transfer && this.shouldBypassCaptchaForAttempt(params)) { |
There was a problem hiding this comment.
This check guards against an attacker who tries to make a fake transfer and then changes strategy at the same time, which would be an attempt to get around WAF rules and Captcha rules that exclude transfers. We already guard against this on the backend on both sign in and sign up, but in the SDK we were only checking it on sign up. So I took this opportunity to also add the check on sign in.
📝 WalkthroughWalkthroughAdds a changeset and implements 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/clerk-js/src/core/resources/SignIn.ts`:
- Around line 185-192: The code accesses nested properties on
SignIn.clerk.client without guarding signUp/verifications/externalAccount;
change the retrieval of strategy in the SignIn transfer branch to use safe
checks (e.g. optional chaining or explicit null checks) so you do const strategy
= SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy and only
assign (finalParams as TransferParams).strategy = strategy as
TransferParams['strategy'] if strategy is not undefined/null; update the logic
inside the transfer block (related to shouldBypassCaptchaForAttempt and
finalParams assignment) to avoid any potential TypeError from missing
intermediate objects.
- Around line 621-629: The transfer branch accesses
SignIn.clerk.client!.signUp.verifications.externalAccount.strategy directly
which can throw if signUp/verifications/externalAccount is undefined; update the
condition inside the captchaOauthBypass check to use optional chaining (e.g.,
SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy) and
compare safely (guarding null/undefined) so the predicate returns false when the
nested property is missing, ensuring the transfer path doesn't cause a runtime
error.
| if (params.transfer && this.shouldBypassCaptchaForAttempt(params)) { | ||
| const strategy = SignIn.clerk.client?.signUp.verifications.externalAccount.strategy; | ||
| if (strategy) { | ||
| // When transfer is true, we're in the OAuth/Enterprise SSO transfer case | ||
| type TransferParams = Extract<SignInCreateParams, { transfer: true }>; | ||
| (finalParams as TransferParams).strategy = strategy as TransferParams['strategy']; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential null reference when accessing signUp properties during transfer.
Line 186 uses optional chaining on client? but not on the subsequent property chain. If client exists but signUp, verifications, or externalAccount is null/undefined, this will throw a TypeError.
Proposed fix
if (params.transfer && this.shouldBypassCaptchaForAttempt(params)) {
- const strategy = SignIn.clerk.client?.signUp.verifications.externalAccount.strategy;
+ const strategy = SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy;
if (strategy) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (params.transfer && this.shouldBypassCaptchaForAttempt(params)) { | |
| const strategy = SignIn.clerk.client?.signUp.verifications.externalAccount.strategy; | |
| if (strategy) { | |
| // When transfer is true, we're in the OAuth/Enterprise SSO transfer case | |
| type TransferParams = Extract<SignInCreateParams, { transfer: true }>; | |
| (finalParams as TransferParams).strategy = strategy as TransferParams['strategy']; | |
| } | |
| } | |
| if (params.transfer && this.shouldBypassCaptchaForAttempt(params)) { | |
| const strategy = SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy; | |
| if (strategy) { | |
| // When transfer is true, we're in the OAuth/Enterprise SSO transfer case | |
| type TransferParams = Extract<SignInCreateParams, { transfer: true }>; | |
| (finalParams as TransferParams).strategy = strategy as TransferParams['strategy']; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/clerk-js/src/core/resources/SignIn.ts` around lines 185 - 192, The
code accesses nested properties on SignIn.clerk.client without guarding
signUp/verifications/externalAccount; change the retrieval of strategy in the
SignIn transfer branch to use safe checks (e.g. optional chaining or explicit
null checks) so you do const strategy =
SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy and only
assign (finalParams as TransferParams).strategy = strategy as
TransferParams['strategy'] if strategy is not undefined/null; update the logic
inside the transfer block (related to shouldBypassCaptchaForAttempt and
finalParams assignment) to avoid any potential TypeError from missing
intermediate objects.
There was a problem hiding this comment.
If it's a sign in and a transfer and there's no signup or verification or externalaccount, that would be an error.
There was a problem hiding this comment.
@dmoerner, thank you for the clarification. You're right—in this transfer flow context, if those nested properties are missing, that would indicate a legitimate error state that should surface rather than be silently handled. The current implementation with optional chaining only on client is appropriate here.
| if ( | ||
| params.transfer && | ||
| captchaOauthBypass.some( | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| strategy => strategy === SignIn.clerk.client!.signUp.verifications.externalAccount.strategy, | ||
| ) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Same null reference risk in the transfer branch.
Line 625 accesses signUp.verifications.externalAccount.strategy without optional chaining. This could throw if the signUp state is incomplete during a transfer scenario.
Proposed fix
if (
params.transfer &&
captchaOauthBypass.some(
// eslint-disable-next-line `@typescript-eslint/no-non-null-assertion`
- strategy => strategy === SignIn.clerk.client!.signUp.verifications.externalAccount.strategy,
+ strategy => strategy === SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy,
)
) {🤖 Prompt for AI Agents
In `@packages/clerk-js/src/core/resources/SignIn.ts` around lines 621 - 629, The
transfer branch accesses
SignIn.clerk.client!.signUp.verifications.externalAccount.strategy directly
which can throw if signUp/verifications/externalAccount is undefined; update the
condition inside the captchaOauthBypass check to use optional chaining (e.g.,
SignIn.clerk.client?.signUp?.verifications?.externalAccount?.strategy) and
compare safely (guarding null/undefined) so the predicate returns false when the
nested property is missing, ensuring the transfer path doesn't cause a runtime
error.
There was a problem hiding this comment.
As in the other case, a transfer with a sign up that's incomplete in the sense of missing an external account is an error.
There was a problem hiding this comment.
@dmoerner, understood. If the external account is expected to exist during transfer scenarios, the assertion is appropriate to catch that error early. Thanks for the clarification.
| captcha_token?: string; | ||
| captcha_error?: unknown; | ||
| captcha_widget_type?: string | null; | ||
| }; |
There was a problem hiding this comment.
My intention with these types was for SignInCreateParams to match what we send to FAPI, which is why I added to the discriminated union. Another approach would be to reserve it for what is passed into the SDK method, and use implicit typing or just Record<string,unknown> for what we sent to FAPI. The latter approach actually fits better what we already do on sign up but it felt haphazard to me.
1 participant
Description
We are building a new sign-in-or-sign-up flow compatible with strict enumeration protection. It is meant to complement our existing sign-in-or-sign-up flow.
Our current sign-in-or-sign-up flow is managed by the SDKs: We start with a sign in attempt, and on a 422 (when an account does not exist), we redirect to a sign up. The flow is thus: Sign In (422) -> Sign Up (200) -> Attempt Verifications for VerifiedAtSignUp identifiers (200). This is vulnerable to user enumeration attacks because the attacker sees the sign in to sign up redirect before they prove their identity by completing a verification.
When
sign_up_if_missingis passed as a param when POSTing a sign in, instead we do the following: Sign In (200) -> Attempt Verification for Identifier (200) -> Create User and Session. (In future work this third step will be modified to support adding additional information to the user, either via AccountTransfer or Session Tasks). This is enumeration safe, because you only see if an account already existed or was created after you verify your identity.This PR is the first step in SDK support for this new flow. We add support for the optional
sign_up_if_missingparam onSignIn. We also add captcha support forSignIn. This is all optional and currently in testing with custom components. Support in Clerk components will be in future PRs.Fixes USER-4596
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Improvements
Tests
Chores