Repository navigation
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
|
Hello 👋 We currently close PRs after 60 days of inactivity. It's been 50 days since the last update here. If we missed this PR, please reply here. Otherwise, we'll close this PR in 10 days. Thanks for being a part of the Clerk community! 🙏 |
|
|
||
| import type { Clerk } from '../core/clerk'; | ||
|
|
||
| const popupCallbackPath = '/popup_auth_callback'; |
There was a problem hiding this comment.
[MEDIUM] FAPI popup callback leaks the active session ID to any origin; state is not validated server-side
Pointing the popup flow at /popup_auth_callback makes the FAPI handler the default callback for all popup sign-in/sign-up. That handler (api/fapi/v1/auth_popup_callback/http.go in clerk_go) posts its payload with targetOrigin "*" and only echoes back the state it was given, never checking that it minted it. The route needs no auth (router.go:622) and is in the ignoredExactPaths set that validateRequestOrigin skips (auth_middleware.go:111), and __client is SameSite=Lax, so it rides along on the top-level GET that opening a popup performs. Any site can therefore call window.open('https://clerk.<app>.com/v1/popup_auth_callback?state=x') and read the reply, learning whether the visitor is signed in and what their active session ID is.
The state added here correctly stops a spoofed message from being accepted by clerk-js, but it cannot stop an attacker who initiates the callback themselves. The fix belongs on the FAPI side: bind state to a server-side pending popup attempt so an unknown state returns nothing, and post to a concrete allowlisted origin instead of "*".
— Comment generated 🤖 with @dominic-clerk's supervision (ai-security-code-review)
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