fix(shared): Correct redirect behavior for OAuth applications on /continue#8391
fix(shared): Correct redirect behavior for OAuth applications on /continue#8391gabrielmeloc22 wants to merge 3 commits intomainfrom
/continue#8391Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/two-factor-authentication |
🦋 Changeset detectedLatest commit: e3bdd9b The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
gabrielmeloc22
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds detection and handling for FAPI-initiated OAuth redirect endpoints. url.ts updates the FAPI allowlist (moves Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/internal/clerk-js/redirectUrls.ts`:
- Around line 102-109: The FAPI priority check uses isFAPIInitiatedFlowPath
which only inspects the pathname and can wrongly match non-FAPI hosts; change
the gating to verify the host as well by using the same logic as
isRedirectForFAPIInitiatedFlow (or call isRedirectForFAPIInitiatedFlow with the
redirect URL and this.frontendApi) instead of isFAPIInitiatedFlowPath in the
block that reads this.fromSearchParams.redirectUrl, and add unit tests verifying
(a) a URL on a non-FAPI host with an allowed FAPI path does NOT bypass
signInForceRedirectUrl, and (b) a valid FAPI continuation URL on the frontend
API host DOES bypass it.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0db0ca53-0f85-47d5-9227-342d40c911ad
📒 Files selected for processing (2)
packages/shared/src/internal/clerk-js/redirectUrls.tspackages/shared/src/internal/clerk-js/url.ts
| // FAPI-initiated flow redirect URLs (e.g. /oauth/authorize/continue) must | ||
| // take highest priority to ensure the IDP authorization flow completes. | ||
| // Without this, a configured signInForceRedirectUrl would override the | ||
| // redirect_url needed to resume the OAuth IDP flow after sign-in. | ||
| const fapiRedirectUrl = this.fromSearchParams.redirectUrl; | ||
| if (fapiRedirectUrl && isFAPIInitiatedFlowPath(fapiRedirectUrl)) { | ||
| return fapiRedirectUrl; | ||
| } |
There was a problem hiding this comment.
FAPI priority check only validates pathname, not host — risk of matching non-FAPI URLs.
isFAPIInitiatedFlowPath (in url.ts, lines 427–435) checks only parsed.pathname against the allowlist, unlike the sibling isRedirectForFAPIInitiatedFlow which also verifies frontendApi === url.host. Any redirect_url whose path happens to be /oauth/authorize/continue, /oauth/authorize, /v1/verify, /oauth/end_session, etc. — even on the customer's own origin or another allowed origin — will now override signInForceRedirectUrl. This silently breaks the documented force-redirect contract for those paths and, depending on how the returned URL is later consumed, could also be abused as an open-redirect-ish escape from force URLs (the value still passes isAllowedRedirect, but force URL is a stronger configured invariant).
Suggest gating on the frontend API host as well, mirroring isRedirectForFAPIInitiatedFlow:
Proposed fix
- // FAPI-initiated flow redirect URLs (e.g. /oauth/authorize/continue) must
- // take highest priority to ensure the IDP authorization flow completes.
- // Without this, a configured signInForceRedirectUrl would override the
- // redirect_url needed to resume the OAuth IDP flow after sign-in.
- const fapiRedirectUrl = this.fromSearchParams.redirectUrl;
- if (fapiRedirectUrl && isFAPIInitiatedFlowPath(fapiRedirectUrl)) {
- return fapiRedirectUrl;
- }
+ // FAPI-initiated flow redirect URLs (e.g. /oauth/authorize/continue) must
+ // take highest priority to ensure the IDP authorization flow completes.
+ const fapiRedirectUrl = this.fromSearchParams.redirectUrl;
+ if (
+ fapiRedirectUrl &&
+ isRedirectForFAPIInitiatedFlow(this.options.frontendApi ?? '', fapiRedirectUrl)
+ ) {
+ return fapiRedirectUrl;
+ }Also worth adding a unit test covering: (a) a non-FAPI-host URL whose path matches the allowlist should NOT bypass signInForceRedirectUrl, and (b) a genuine FAPI continuation URL should bypass it.
📝 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.
| // FAPI-initiated flow redirect URLs (e.g. /oauth/authorize/continue) must | |
| // take highest priority to ensure the IDP authorization flow completes. | |
| // Without this, a configured signInForceRedirectUrl would override the | |
| // redirect_url needed to resume the OAuth IDP flow after sign-in. | |
| const fapiRedirectUrl = this.fromSearchParams.redirectUrl; | |
| if (fapiRedirectUrl && isFAPIInitiatedFlowPath(fapiRedirectUrl)) { | |
| return fapiRedirectUrl; | |
| } | |
| // FAPI-initiated flow redirect URLs (e.g. /oauth/authorize/continue) must | |
| // take highest priority to ensure the IDP authorization flow completes. | |
| const fapiRedirectUrl = this.fromSearchParams.redirectUrl; | |
| if ( | |
| fapiRedirectUrl && | |
| isRedirectForFAPIInitiatedFlow(this.options.frontendApi ?? '', fapiRedirectUrl) | |
| ) { | |
| return fapiRedirectUrl; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/internal/clerk-js/redirectUrls.ts` around lines 102 -
109, The FAPI priority check uses isFAPIInitiatedFlowPath which only inspects
the pathname and can wrongly match non-FAPI hosts; change the gating to verify
the host as well by using the same logic as isRedirectForFAPIInitiatedFlow (or
call isRedirectForFAPIInitiatedFlow with the redirect URL and this.frontendApi)
instead of isFAPIInitiatedFlowPath in the block that reads
this.fromSearchParams.redirectUrl, and add unit tests verifying (a) a URL on a
non-FAPI host with an allowed FAPI path does NOT bypass signInForceRedirectUrl,
and (b) a valid FAPI continuation URL on the frontend API host DOES bypass it.
@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: |
LauraBeatris
changed the title
/continue
LauraBeatris
changed the title
/continue/continue
2 participants
Description
When Clerk acts as an OAuth Identity Provider (e.g. for a desktop app using PKCE), the flow requires the user to sign in first, then return to /oauth/authorize/continue to complete the
authorization and issue a code to the requesting application.
After the user signs in (e.g. with Google), Clerk should redirect back to the FAPI continuation endpoint to finish the IDP flow. However, two issues prevent this:
/oauth/authorize/continueand/oauth/authorize-with-immediate-redirectwere not classified as requiring user input, causing #redirectFAPIInitiatedFlow to prematurely navigate away from the sign-in page — stripping the redirect_url from the URL before the component could read it.signInForceRedirectUrloverrides FAPI redirect URLs in the RedirectUrls priority chain. If a customer has forceRedirectUrl configured (directly or via env var),action_complete_redirect_urlis set to the customer's home page instead of the FAPI continuation URL. The server then redirects there after the social sign-in callback, and the IDP flow never completes.The result: the user is signed in successfully, but the desktop app never receives its authorization code. This only manifests in production — in development,
#redirectFAPIInitiatedFlowmasks the issue by auto-redirecting after sign-in.Changes
url.ts:/oauth/authorize/continueand/oauth/authorize-with-immediate-redirectto frontendApiRedirectPathsWithUserInput so #redirectFAPIInitiatedFlow waits for the user to sign in before redirecting, preserving the redirect_url in the page URL.isFAPIInitiatedFlowPath()helper that checks if a URL pathname matches any known FAPI flow endpoint (without requiring frontendApi host matching).redirectUrls.ts:#getRedirectUrl, whenredirect_urlfrom search params points to a known FAPI endpoint, it now takes highest priority — overridingsignInForceRedirectUrl. This ensures the IDP authorization flow always completes. For non-FAPI redirects,forceRedirectUrlstill works as before.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change