-
Notifications
You must be signed in to change notification settings - Fork 353
feat(clerk-js): Trigger a new request to submit the captcha token #6076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(clerk-js): Trigger a new request to submit the captcha token #6076
Conversation
🦋 Changeset detectedLatest commit: fe50d7d The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 Git ↗︎
|
!snapshot |
Hey @anagstef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
📝 WalkthroughWalkthroughThis change introduces a feature-flagged two-step sign-up creation flow in the sign-up process for the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…reate-into-2-requests
!snapshot |
Hey @anagstef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.changeset/dark-moons-sort.md (1)
1-6
: Expand the changeset description for clarityThe summary is accurate but very terse. Mentioning the new
skipChallenge
option and thetwo_step_sign_up_create_enabled
flag would help downstream readers understand why a patch is published and what behaviour changes to expect.packages/types/src/signUp.ts (1)
74-75
: Document the newoptions
parameter
SignUpResource.create
now accepts a secondoptions
argument but the interface lacks a short comment explaining the intent ofskipChallenge
. Adding a one-liner here (and in generated docs) prevents confusion for SDK consumers.Also applies to: 202-205
packages/clerk-js/src/utils/captcha/CaptchaChallenge.ts (1)
64-66
: Comment & code out of syncThe inline comment says “if captcha action is signup, we return undefined”, yet the guard actually checks for
'verify'
.
This mismatch can confuse future readers and reviewers.-// if captcha action is signup, we return undefined, because it means that the bot protection is disabled +// When bot protection is disabled we return undefined for non-'verify' actions, +// allowing sign-up flows to proceed without a captcha.packages/clerk-js/src/core/resources/SignUp.ts (2)
87-95
:skipChallenge
silently ignored when two-step flow is offIf the feature flag
twoStepSignUpCreateEnabled
is false, callers can still passoptions.skipChallenge
, but the value is dropped when falling back tolegacyCreate
.
Either document thatskipChallenge
is only honoured in the two-step flow, or plumb the option through for consistency.
116-138
: Lost return value from the initial POST
await this._basePost(...)
is awaited but its returnedSignUpResource
is discarded.
Even though_basePost
mutatesthis
, capturing the promise result would:
- Make side-effects explicit.
- Protect against future refactors where
_basePost
stops mutating in place.- await this._basePost({ ... }); + const created = await this._basePost({ ... });Then continue operating on
created
(or assign back tothis
) to avoid accidental behavioural drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/dark-moons-sort.md
(1 hunks)packages/clerk-js/src/core/resources/DisplayConfig.ts
(3 hunks)packages/clerk-js/src/core/resources/SignUp.ts
(5 hunks)packages/clerk-js/src/ui/components/SignUp/SignUpStart.tsx
(1 hunks)packages/clerk-js/src/utils/captcha/CaptchaChallenge.ts
(1 hunks)packages/types/src/displayConfig.ts
(2 hunks)packages/types/src/signUp.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/clerk-js/src/core/resources/DisplayConfig.ts (1)
29-30
: 👍 New feature flag is wired end-to-endProperty declaration, JSON deserialization and snapshot serialization are all covered. No issues spotted.
Also applies to: 84-87, 138-139
packages/types/src/displayConfig.ts (1)
27-28
: LGTM – types stay in sync with runtime flagThe additional property is optional in both JSON & resource interfaces, matching the runtime default of
false
.Also applies to: 73-74
// This will be handled by the backend (FAPI) in the future. | ||
void (await signUp.create({}, { skipChallenge: true })); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove redundant void (await …)
wrapper
Inside an async
function you can simply await the promise. The current form is harder to read and offers no benefit.
- void (await signUp.create({}, { skipChallenge: true }));
+ await signUp.create({}, { skipChallenge: true });
If the result should be ignored but failures tolerated, consider wrapping in try/catch
instead of swallowing silently.
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/components/SignUp/SignUpStart.tsx around lines 218
to 220, remove the redundant 'void (await ...)' wrapper and simply use 'await'
to handle the promise. If the intention is to ignore the result but still handle
potential errors, wrap the await call in a try/catch block instead of silently
swallowing any failures.
body: normalizeUnsafeMetadata(params), | ||
}); | ||
|
||
if (!this.shouldBypassCaptchaForAttempt(params) && !options?.skipChallenge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should whether challenge
is a missing field factor into this check?
Description
This PR introduces support for a two-request sign up creation. This only happens when the bot-protection is enabled, so the client fires a POST on
/sign_ups
without a captcha token, and immediately a PATCH on/sign_ups/sua_XXX
with the captcha token.After the first API call, the Sign Up Attempt will include the
challenge
string inside themissing_fields
array.This is part of a bigger effort to decouple the initial sign up attempt creation from the submission of a challenge (like CAPTCHA token, or a device attestation proof).
It also fixes a rare bug on sign ups, where a captcha challenge was triggered, while we only wanted to clear the error message by triggering a new sign up attempt. That's why an option named
skipChallenge
is also introduced.The new functionality is enabled only when the
display_config.two_step_sign_up_create_enabled
boolean feature flag istrue
. This way we will rollout the new functionality gradually, and eventually totally remove the legacy code.Our backend will still support both ways of submitting a CAPTCHA to provide backwards compatibility.
Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Refactor