fix(clerk-js): Bypass captcha on OAuth backport for Core 2#8734
Conversation
Backport of the OAuth captcha bypass fix from #8030 to clerk-js 5.x (Core 2). `shouldBypassCaptchaForAttempt` bailed out early when `params.strategy` was unset, which is the case on an OAuth transfer to sign up (the strategy lives on the sign in verification). As a result a captcha was unconditionally requested for brand-new OAuth users, who could hang indefinitely on /sign-in/sso-callback. The transfer bypass is now checked before any strategy-based early return, and the redundant strategy-injection block in `create` is removed. Enterprise SSO transfers still correctly show a captcha. Only the captcha bypass fix is backported; the sign_up_if_missing feature from #8030 is intentionally excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c47dac6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
📝 WalkthroughWalkthroughThis PR fixes OAuth users hanging on SSO callbacks during signup transfers by refactoring the captcha bypass logic. It removes a strategy-override condition from ChangesOAuth Captcha Bypass Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dmoerner
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
| finalParams = { ...finalParams, ...captchaParams }; | ||
| } | ||
|
|
||
| if (finalParams.transfer && this.shouldBypassCaptchaForAttempt(finalParams)) { |
There was a problem hiding this comment.
On the previous code path, this branch would never hit: On a transfer we would have no strategy which would then early return false from shouldBypassCaptchaForAttempt. Clean it up, and it was useless even if it did hit, because the backend deliberately ignores the strategy sent with a transfer in favor of fetching the original strategy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/resources/SignUp.ts (1)
565-569:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nested sign-in strategy access to avoid runtime crashes on transfer flows.
SignUp.clerk.client!.signIn.firstFactorVerification.strategycan throw if any intermediate object is missing. In that case,create()fails instead of falling back to captcha handling.Suggested fix
- if ( - params.transfer && - // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` - captchaOauthBypass.some(strategy => strategy === SignUp.clerk.client!.signIn.firstFactorVerification.strategy) - ) { + const transferStrategy = SignUp.clerk.client?.signIn?.firstFactorVerification?.strategy; + if (params.transfer && transferStrategy && captchaOauthBypass.some(strategy => strategy === transferStrategy)) { return true; }🤖 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/resources/SignUp.ts` around lines 565 - 569, The condition unsafely dereferences SignUp.clerk.client!.signIn.firstFactorVerification.strategy which can throw if clerk/client/signIn/firstFactorVerification is missing; update the check in SignUp.create (the params.transfer branch) to safely guard using optional chaining or explicit null checks (e.g. const currentStrategy = SignUp.clerk.client?.signIn?.firstFactorVerification?.strategy and then use captchaOauthBypass.some(s => s === currentStrategy)) so the code falls back to captcha handling instead of crashing.
🤖 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/resources/SignUp.ts`:
- Around line 565-569: The condition unsafely dereferences
SignUp.clerk.client!.signIn.firstFactorVerification.strategy which can throw if
clerk/client/signIn/firstFactorVerification is missing; update the check in
SignUp.create (the params.transfer branch) to safely guard using optional
chaining or explicit null checks (e.g. const currentStrategy =
SignUp.clerk.client?.signIn?.firstFactorVerification?.strategy and then use
captchaOauthBypass.some(s => s === currentStrategy)) so the code falls back to
captcha handling instead of crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b0ea16c-48a0-4a8c-a980-095c31069109
📒 Files selected for processing (3)
.changeset/oauth-captcha-callback-backport.mdpackages/clerk-js/src/core/resources/SignUp.tspackages/clerk-js/src/core/resources/__tests__/SignUp.test.ts
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
looks good 👍🏼
Im not sure about the release process of Core 2, let's ask @clerk/team-sdk
jacekradko
deleted the
daniel/user-5459-backport-oauth-captcha-callback-fix-to-clerk-js-5x
branch
|
Tested with a dummy Core 2 app on Safari. |
3 participants
Description
Backport of the OAuth captcha bypass fix from #8030 to Core 2.
shouldBypassCaptchaForAttemptbailed out early (therefore showing captcha) whenparams.strategywas unset, which is the case on an OAuth transfer to sign up (the strategy lives on the sign in verification). As a result a captcha was unconditionally requested for brand-new OAuth users. Normally this was a no-op, just wasting cycles to Cloudflare and then our backend would ignore captcha results for bypassed providers anyway. However, we got a report that it was producing a bug on Safari where the frontend would hang indefinitely on /sign-in/sso-callback.The transfer bypass is now checked before any strategy-based early return, and the redundant strategy-injection block in
createis removed. Enterprise SSO transfers still correctly show a captcha.Only the captcha bypass fix is backported; the sign_up_if_missing feature from #8030 is intentionally excluded.
For discussion of the regression introduced in 2024, see https://clerkinc.slack.com/archives/C01KTTYKWBU/p1772566383559539.
Written by Claude under Daniel's supervision.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit