Conversation
🦋 Changeset detectedLatest commit: 36bf49c 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| * We delegate bot detection to the following providers, instead of relying on turnstile exclusively | ||
| */ | ||
| protected shouldBypassCaptchaForAttempt(params: SignUpCreateParams) { | ||
| if (!params.strategy) { |
There was a problem hiding this comment.
This is the previous logic, which unconditionally showed captchas on EnterpriseSSO and OAuth transfers, since transfers do not have a strategy. The former behavior is correct, the latter was a bug.
| if (captchaOauthBypass.some(strategy => strategy === params.strategy)) { | ||
| return true; | ||
| // Check for transfer captcha bypass. | ||
| if (params.transfer) { |
There was a problem hiding this comment.
The new logic explicitly checks transfer first and checks both paths that can lead to a bypass: OAuth strategy, and sign up if missing. We fall through on EnterpriseSSO transfers, which then ends up returning false, i.e., we do show captchas on Enterprise SSO.
| if (captchaOauthBypass.some(strategy => strategy === params.strategy)) { | ||
| return true; | ||
| // Check for transfer captcha bypass. | ||
| if (params.transfer) { |
There was a problem hiding this comment.
It's exactly the same change for sign up future.
| expect(CaptchaChallenge).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not skip captcha challenge for enterprise_sso transfer', async () => { |
There was a problem hiding this comment.
New unit test to rule out regressions with enterprise_sso transfer.
| expect(mockFetch.mock.calls[0]?.[0].body).toHaveProperty('captchaError', undefined); | ||
| }); | ||
|
|
||
| it('does not skip captcha challenge for enterprise_sso transfer', async () => { |
There was a problem hiding this comment.
Sign up future regression test for enterprise_sso transfer case.
📝 WalkthroughWalkthroughThis pull request adds support for the Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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/shared/src/types/signInCommon.ts`:
- Around line 165-170: The union branch that currently reads `| { transfer?:
boolean }` should require the discriminator by changing it to `| { transfer:
true }` so calls like `signIn.create({})` or `signIn.create({ transfer: false
})` no longer type-check; also update the intersected type that currently has `&
{ transfer?: boolean; signUpIfMissing?: boolean }` to remove the optional
`transfer?: boolean` (or make it `transfer?: false` where appropriate) so the
`transfer` discriminator is not reintroduced. Locate the relevant type in
packages/shared/src/types/signInCommon.ts (the union used by signIn.create) and
adjust the branches so the transfer-only variant is `{ transfer: true }` and the
other branches keep/allow `signUpIfMissing` as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3b8bf2fc-b256-467a-b56b-b8b2e9f6a9ab
📒 Files selected for processing (8)
.changeset/strict-needles-taste.mdpackages/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/clerk-js/src/utils/captcha/types.tspackages/shared/src/types/signInCommon.tspackages/shared/src/types/signInFuture.ts
| | { | ||
| transfer?: boolean; | ||
| } | ||
| ) & { | ||
| transfer?: boolean; | ||
| signUpIfMissing?: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and find the exact location
find . -name "signInCommon.ts" -type fRepository: clerk/javascript
Length of output: 103
🏁 Script executed:
# Read the signInCommon.ts file to understand the type definition
wc -l packages/shared/src/types/signInCommon.tsRepository: clerk/javascript
Length of output: 105
🏁 Script executed:
# Read the relevant section around lines 165-170
sed -n '150,185p' packages/shared/src/types/signInCommon.tsRepository: clerk/javascript
Length of output: 787
🏁 Script executed:
# Read more context to understand the full type structure
sed -n '140,200p' packages/shared/src/types/signInCommon.tsRepository: clerk/javascript
Length of output: 1294
🏁 Script executed:
# Check the SignIn.ts implementation mentioned in the review
sed -n '169,197p' packages/clerk-js/src/core/resources/SignIn.tsRepository: clerk/javascript
Length of output: 1027
🏁 Script executed:
# Get more context around that area
sed -n '160,210p' packages/clerk-js/src/core/resources/SignIn.tsRepository: clerk/javascript
Length of output: 1622
Require transfer: true for the transfer-only variant.
The new | { transfer?: boolean } branch allows signIn.create({}) and signIn.create({ transfer: false }) to type-check, but the implementation posts these payloads directly to the API without validation. This converts an invalid call into a runtime API failure instead of a compile-time rejection. Change the branch to | { transfer: true } to make transfer required and discriminating.
Suggested fix
- | {
- transfer?: boolean;
- }
+ | {
+ transfer: true;
+ }
) & {
transfer?: boolean;
signUpIfMissing?: boolean;
};📝 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.
| | { | |
| transfer?: boolean; | |
| } | |
| ) & { | |
| transfer?: boolean; | |
| signUpIfMissing?: boolean; | |
| | { | |
| transfer: true; | |
| } | |
| ) & { | |
| transfer?: boolean; | |
| signUpIfMissing?: boolean; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/types/signInCommon.ts` around lines 165 - 170, The union
branch that currently reads `| { transfer?: boolean }` should require the
discriminator by changing it to `| { transfer: true }` so calls like
`signIn.create({})` or `signIn.create({ transfer: false })` no longer
type-check; also update the intersected type that currently has `& { transfer?:
boolean; signUpIfMissing?: boolean }` to remove the optional `transfer?:
boolean` (or make it `transfer?: false` where appropriate) so the `transfer`
discriminator is not reintroduced. Locate the relevant type in
packages/shared/src/types/signInCommon.ts (the union used by signIn.create) and
adjust the branches so the transfer-only variant is `{ transfer: true }` and the
other branches keep/allow `signUpIfMissing` as intended.
|
CI seems broken, I'm going to try converting to draft and then switching back to see if I can get it to re-run. |
@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/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: |
2 participants
Description
This PR re-reverts #7749, which adds support for sign up if missing for custom flows. We do this by adding the optional
signUpIfMissingparam on creating a sign in, and adding support for captchas. This requires both supporting captchas on sign in ifsignUpIfMissingistrue, and making sure we don't show a double captcha on sign up after transfer from a sign up if missing sign in.The original PR had to be reverted because it did not properly render captchas on Enterprise SSO transfers to sign up. The root of this mistake was my confusion over a bug in the existing code: We bailed out early in
shouldBypassCaptchaForAttemptif there was no strategy. This had the effect that we unconditionally showed captchas on all transfers. This was correct behavior for EnterpriseSSO, which does not bypass captcha on transfer. This was a bug for OAuth, since we never checked if we should bypass captchas for OAuth.This PR is identical to the previous one, except the logic in
shouldBypassCaptchaForAttemptis fixed: We do show captchas on EnterpriseSSO transfers, and we properly check the bypass list for OAuth transfers. I also added further unit tests to verify these fixes.This was tested in staging with SCIM to confirm that we did not have the EnterpriseSSO Captcha regression.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
sign_up_if_missingparameter to sign-in requests, enabling automatic account creation for users who don't yet have an account