fix(js): preserve SignUpFuture reference across SSO-to-sign-up transition#9060
fix(js): preserve SignUpFuture reference across SSO-to-sign-up transition#9060FrancoKaddour wants to merge 4 commits into
Conversation
isAllowedRedirect() compares url.origin against allowedRedirectOrigins patterns. When the redirect URL includes a non-default port (e.g. :5173 from a Vite dev server), url.origin includes the port suffix, while patterns like 'https://*.example.net' have none — causing the match to fail and the redirect to fall back to the home URL silently. Fix: when url.port is non-empty, also test the port-stripped origin (protocol + hostname) against each pattern. Domain validation is preserved — only the port suffix is relaxed. Fixes clerk#8263
…tion
When an SSO sign-in transitions to a sign-up flow (e.g. account does not
exist, legal acceptance required), Client.fromJSON created a brand-new
SignUp instance because the existing one had no id and the incoming data
did. The old SignUpFuture held by useSignUp() hooks pointed at the stale
id-less instance, so subsequent update() calls sent PATCH to
/client/sign_ups instead of /client/sign_ups/{id} and received a 405.
Fix: update the existing SignUp in-place (via __internal_updateFromJSON)
when it has no id yet, which preserves the SignUpFuture reference and
ensures hooks see the correct id after the transition.
Fixes clerk#8338
🦋 Changeset detectedLatest commit: 167447f 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 |
|
@FrancoKaddour is attempting to deploy a commit to the Clerk Production Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR fixes two unrelated bugs: redirect URLs with non-default ports are now matched against wildcard ChangesRedirect Origin Port Matching
Stale SignUp Reference Fix
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related issues
Suggested reviewers: Poem
🚥 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
🧹 Nitpick comments (2)
packages/clerk-js/src/core/resources/Client.ts (2)
146-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix logic correctly addresses the stale
SignUpreference bug.The broadened condition
if (data.sign_up && this.signUp instanceof SignUp && (this.signUp.id === data.sign_up.id || !this.signUp.id))correctly preserves the existingSignUpinstance (and thus theSignUpFuturereference) when the current instance has no id yet, matching the fix described in the PR objectives. The downstreampath()/isNew()mechanics inBase.tsmean this instance will now correctly route PATCH requests to/client/sign_ups/{id}once the id arrives.One minor readability nit:
BaseResourcealready exposes a publicisNew()helper (return !this.id) that is used elsewhere for the same semantic check (e.g.path()). Usingthis.signUp.isNew()instead of!this.signUp.idhere would be more idiomatic and self-documenting.♻️ Optional refactor for consistency
- if (data.sign_up && this.signUp instanceof SignUp && (this.signUp.id === data.sign_up.id || !this.signUp.id)) { + if (data.sign_up && this.signUp instanceof SignUp && (this.signUp.id === data.sign_up.id || this.signUp.isNew())) {🤖 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/Client.ts` around lines 146 - 154, The SignUp update branch in Client should use the existing resource helper for the “no id yet” check instead of reading the id directly. In the conditional around this.signUp.__internal_updateFromJSON, replace the raw !this.signUp.id part with this.signUp.isNew() so the logic matches BaseResource semantics and stays consistent with path() and other resource checks.
141-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the no-id → id sign-up transition. Existing coverage checks matching ids, but not the case where
this.signUp.idis unset andfromJSON()receives asign_uppayload. A unit test asserting the sameSignUpinstance is reused and itsidis populated would lock in the SSO→sign-up flow behavior.🤖 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/Client.ts` around lines 141 - 171, Add a regression test for Client.fromJSON covering the no-id to id sign-up transition: exercise the branch where this.signUp is an existing SignUp with no id and data.sign_up has an id, and verify the same SignUp instance is reused rather than replaced. Assert that __internal_updateFromJSON is effectively applied by checking the original SignUp reference is preserved and its id is populated after the call, alongside the existing matching-id coverage.
🤖 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.
Inline comments:
In `@packages/shared/src/internal/clerk-js/url.ts`:
- Around line 449-455: The allowlist check in the URL validation logic is too
permissive because `portlessOrigin` is being matched for every pattern, causing
exact entries in the `isAllowed` flow to accept any port on the same host.
Update the logic in `url.ts` so only explicit wildcard or dev-only allowlist
entries can use the portless fallback, while exact allowlist strings remain
port-sensitive. Keep the fix localized to the `isAllowed` computation and the
`patterns.some(...)` matching path so `https://www.clerk.com` does not
implicitly allow `https://www.clerk.com:3000`.
---
Nitpick comments:
In `@packages/clerk-js/src/core/resources/Client.ts`:
- Around line 146-154: The SignUp update branch in Client should use the
existing resource helper for the “no id yet” check instead of reading the id
directly. In the conditional around this.signUp.__internal_updateFromJSON,
replace the raw !this.signUp.id part with this.signUp.isNew() so the logic
matches BaseResource semantics and stays consistent with path() and other
resource checks.
- Around line 141-171: Add a regression test for Client.fromJSON covering the
no-id to id sign-up transition: exercise the branch where this.signUp is an
existing SignUp with no id and data.sign_up has an id, and verify the same
SignUp instance is reused rather than replaced. Assert that
__internal_updateFromJSON is effectively applied by checking the original SignUp
reference is preserved and its id is populated after the call, alongside the
existing matching-id coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e0af06f2-d013-4b37-b8e8-8e4a7c8c9fc7
📒 Files selected for processing (5)
.changeset/fix-allowed-redirect-origins-port.md.changeset/fix-signup-future-sso-transfer-stale-ref.mdpackages/clerk-js/src/core/resources/Client.tspackages/shared/src/internal/clerk-js/__tests__/url.test.tspackages/shared/src/internal/clerk-js/url.ts
Exact string entries in allowedRedirectOrigins (e.g. https://www.clerk.com) were also matching ports they never declared because the portless-origin test ran against all patterns. Port is part of the browser origin, so an exact entry must remain port-sensitive. The portless fallback now only applies when the original entry is a glob (contains '*'), which covers the satellite-app use case where the pattern is https://*.example.net and the dev server runs on :5173.
1 participant
Fixes #8338
When an SSO sign-in transitions to a sign-up flow (e.g. the account does not exist and legal acceptance is required), Client.fromJSON created a brand-new SignUp instance because the existing one had no id while the incoming server data did. The SignUpFuture held by useSignUp() hooks pointed at the stale, id-less instance. Calling signUp.update({ legalAccepted: true }) then sent PATCH /client/sign_ups (no id segment) instead of PATCH /client/sign_ups/{id}, resulting in a 405 error.
Root cause: Client.fromJSON only reuses the existing SignUp instance when this.signUp.id === data.sign_up.id. During SSO transfer the old instance has no id, so the condition was false and a new instance was created — discarding the SignUpFuture reference held by hooks.
Fix: Also update in-place when !this.signUp.id (the instance is still uninitialized). __internal_updateFromJSON populates all fields including id, so the existing SignUpFuture transparently sees the correct id after the transition.
Summary by CodeRabbit
signUp.update()targets the correct sign-up resource and continues updating reliably.