feat(clerk-js): Use FAPI for OAuth popup callbacks#9174
Conversation
🦋 Changeset detectedLatest commit: b369daf The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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.
|
📝 WalkthroughWalkthroughOAuth popup callback handling now targets the Frontend API ChangesPopup authentication routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SSO
participant PopupHelpers
participant FrontendAPI
participant Popup
SSO->>PopupHelpers: Wrap redirect routes
PopupHelpers->>FrontendAPI: Build state-bearing callback URLs
FrontendAPI->>Popup: Send verification message
PopupHelpers->>Popup: Validate origin, source, and state
PopupHelpers->>SSO: Provide verification redirect
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@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: |
brkalow
changed the title
ApprovabilityVerdict: Needs human review This PR modifies OAuth popup authentication infrastructure, changing callback endpoints from Account Portal to FAPI and adding state-based validation. Authentication-related changes warrant human review regardless of author trust level. You can customize Macroscope's approvability policy. Learn more. |
![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 (1)
packages/clerk-js/src/core/resources/__tests__/SignUp.test.ts (1)
1043-1045: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant test comment.
Line 1044 merely restates line 1045.
As per coding guidelines, “Keep code comments minimal” and “never restate the code.”
🤖 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/__tests__/SignUp.test.ts` around lines 1043 - 1045, Remove the redundant inline comment inside the _futureAuthenticateWithPopup mockImplementation callback, leaving the popup.location.href assignment unchanged.Source: Coding guidelines
🤖 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/clerk-js/src/utils/__tests__/authenticateWithPopup.test.ts`:
- Around line 73-89: Extend the test around _authenticateWithPopup to dispatch
valid-FAPI-origin message events with an incorrect state and an incorrect source
before the matching event. Assert navigate is not called after each rejected
event, then retain the existing matching event and assert navigation occurs with
returnUrl.
---
Nitpick comments:
In `@packages/clerk-js/src/core/resources/__tests__/SignUp.test.ts`:
- Around line 1043-1045: Remove the redundant inline comment inside the
_futureAuthenticateWithPopup mockImplementation callback, leaving the
popup.location.href assignment unchanged.
🪄 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: 0342ae97-0285-418e-a8b1-85d1c971497e
📒 Files selected for processing (7)
.changeset/tidy-moose-travel.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/__tests__/authenticateWithPopup.test.tspackages/clerk-js/src/utils/authenticateWithPopup.ts
| window.dispatchEvent( | ||
| new MessageEvent('message', { | ||
| data: { return_url: returnUrl, state }, | ||
| origin: 'https://accounts.example.com', | ||
| source: popup, | ||
| }), | ||
| ); | ||
| expect(navigate).not.toHaveBeenCalled(); | ||
|
|
||
| window.dispatchEvent( | ||
| new MessageEvent('message', { | ||
| data: { return_url: returnUrl, state }, | ||
| origin: 'https://clerk.example.com', | ||
| source: popup, | ||
| }), | ||
| ); | ||
| expect(navigate).toHaveBeenCalledWith(returnUrl); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Cover direct popup state and source rejection.
This test only rejects an incorrect origin. Add valid-FAPI-origin events with an incorrect state and incorrect source before the matching event, so regressions in the new _authenticateWithPopup checks cannot permit unintended navigation.
Suggested coverage
+ const otherPopup = { location: { href: '' } } as unknown as Window;
+
+ window.dispatchEvent(
+ new MessageEvent('message', {
+ data: { return_url: returnUrl, state: 'wrong_state' },
+ origin: 'https://clerk.example.com',
+ source: popup,
+ }),
+ );
+ window.dispatchEvent(
+ new MessageEvent('message', {
+ data: { return_url: returnUrl, state },
+ origin: 'https://clerk.example.com',
+ source: otherPopup,
+ }),
+ );
+ expect(navigate).not.toHaveBeenCalled();As per coding guidelines, “Unit tests are required for all new functionality” and “Verify proper error handling and edge cases.”
📝 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.
| window.dispatchEvent( | |
| new MessageEvent('message', { | |
| data: { return_url: returnUrl, state }, | |
| origin: 'https://accounts.example.com', | |
| source: popup, | |
| }), | |
| ); | |
| expect(navigate).not.toHaveBeenCalled(); | |
| window.dispatchEvent( | |
| new MessageEvent('message', { | |
| data: { return_url: returnUrl, state }, | |
| origin: 'https://clerk.example.com', | |
| source: popup, | |
| }), | |
| ); | |
| expect(navigate).toHaveBeenCalledWith(returnUrl); | |
| window.dispatchEvent( | |
| new MessageEvent('message', { | |
| data: { return_url: returnUrl, state }, | |
| origin: 'https://accounts.example.com', | |
| source: popup, | |
| }), | |
| ); | |
| expect(navigate).not.toHaveBeenCalled(); | |
| const otherPopup = { location: { href: '' } } as unknown as Window; | |
| window.dispatchEvent( | |
| new MessageEvent('message', { | |
| data: { return_url: returnUrl, state: 'wrong_state' }, | |
| origin: 'https://clerk.example.com', | |
| source: popup, | |
| }), | |
| ); | |
| window.dispatchEvent( | |
| new MessageEvent('message', { | |
| data: { return_url: returnUrl, state }, | |
| origin: 'https://clerk.example.com', | |
| source: otherPopup, | |
| }), | |
| ); | |
| expect(navigate).not.toHaveBeenCalled(); | |
| window.dispatchEvent( | |
| new MessageEvent('message', { | |
| data: { return_url: returnUrl, state }, | |
| origin: 'https://clerk.example.com', | |
| source: popup, | |
| }), | |
| ); | |
| expect(navigate).toHaveBeenCalledWith(returnUrl); |
🤖 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/utils/__tests__/authenticateWithPopup.test.ts` around
lines 73 - 89, Extend the test around _authenticateWithPopup to dispatch
valid-FAPI-origin message events with an incorrect state and an incorrect source
before the matching event. Assert navigate is not called after each rejected
event, then retain the existing matching event and assert navigation occurs with
returnUrl.
Source: Coding guidelines
1 participant
OAuth popup flows currently depend on the Account Portal hostname even when an application sends all Frontend API traffic through its own proxy. That leaves a hostname dependency outside the proxy and prevents the popup flow from staying on the application's domain.
This changes Clerk.js to use the new FAPI popup callback route by default for both existing and future SignIn/SignUp flows. Clerk.js generates a random state value before starting the flow and accepts the callback message only when its origin matches the effective FAPI origin, its source is the opened popup, and its state matches. Direct FAPI and proxied FAPI URLs use the same path-building behavior.
Depends on https://github.com/clerk/clerk_go/pull/20467 being deployed before this Clerk.js version is released.
Summary by CodeRabbit