fix(clerk-js): validate window navigation protocols and honor allowedRedirectProtocols#8961
Open
jacekradko wants to merge 1 commit into
Open
fix(clerk-js): validate window navigation protocols and honor allowedRedirectProtocols#8961jacekradko wants to merge 1 commit into
jacekradko wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 16b7d03 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 |
Contributor
|
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 Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughA new ChangesWindow navigation protocol allowlist enforcement
Sequence Diagram(s)sequenceDiagram
participant UIComponent as UI Component
participant bridge as clerkWindowNavigate()
participant chokepoint as Clerk.__internal_windowNavigate()
participant core as windowNavigate()
rect rgba(70, 130, 180, 0.5)
Note over UIComponent,chokepoint: New ClerkJS bundle path
UIComponent->>bridge: clerkWindowNavigate(clerk, url)
bridge->>chokepoint: call(clerk, url, opts)
chokepoint->>chokepoint: compute allowedProtocols<br/>(static ± customer extension)
chokepoint->>core: windowNavigate(url, {allowedProtocols})
core-->>UIComponent: navigated or blocked+warned
end
rect rgba(180, 100, 70, 0.5)
Note over UIComponent,core: Older ClerkJS fallback path
UIComponent->>bridge: clerkWindowNavigate(clerk, url)
bridge->>core: windowNavigate(url) [static allowlist only]
core-->>UIComponent: navigated or blocked+warned (fail closed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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. ✨ 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: |
Contributor
API Changes Report
Summary
@clerk/clerk-jsCurrent version: 6.20.0 Subpath
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/react/src/isomorphicClerk.ts`:
- Around line 268-270: The `__internal_windowNavigate` method in IsomorphicClerk
always exists as a forwarder, but when the clerkjs version lacks this method,
the optional chain silently fails and navigation is dropped without fallback.
Modify the `__internal_windowNavigate` implementation to check if clerkjs
actually supports this method before delegating to it, and provide an
appropriate fallback behavior (such as returning false or implementing
alternative navigation logic) when the method is not available on the underlying
clerkjs instance, ensuring that older clerkjs versions can still handle
navigation through their default mechanisms.
🪄 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
Run ID: e7aa3b33-c676-4786-b718-c746653758d4
📒 Files selected for processing (15)
.changeset/windownavigate-protocol-allowlist.mdpackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/resources/SignUp.tspackages/react/src/isomorphicClerk.tspackages/shared/src/internal/clerk-js/__tests__/windowNavigate.test.tspackages/shared/src/internal/clerk-js/windowNavigate.tspackages/shared/src/types/clerk.tspackages/ui/src/components/UserButton/useMultisessionActions.tsxpackages/ui/src/components/UserProfile/EnterpriseAccountsSection.tsxpackages/ui/src/contexts/components/SessionTasks.tspackages/ui/src/contexts/components/SignIn.tspackages/ui/src/contexts/components/SignUp.tspackages/ui/src/utils/__tests__/windowNavigate.test.tspackages/ui/src/utils/windowNavigate.ts
jacekradko
force-pushed
the
jacek/validate-window-navigation-protocols
branch
from
0dbc430 to
805453a
Compare
…RedirectProtocols
jacekradko
force-pushed
the
jacek/validate-window-navigation-protocols
branch
from
805453a to
16b7d03
Compare
dominic-clerk
requested changes
Jun 23, 2026
| */ | ||
| export function windowNavigate(to: URL | string): void { | ||
| export function windowNavigate(to: URL | string, options?: WindowNavigateOptions): void { | ||
| if (typeof to === 'string' && SCHEME_RELATIVE_PREFIX.test(to.trim())) { |
Contributor
There was a problem hiding this comment.
Scheme-relative open-redirect bypass via control characters
trim() only strips leading/trailing whitespace, but new URL() (next line) strips tab/LF/CR from anywhere and leading C0 controls — so an interior or leading control char defeats this regex while still resolving to a scheme-relative URL. It then inherits the base https: scheme (allowlisted), so navigation proceeds cross-origin.
Bypasses against base https://victim-app.com/:
"/\t/evil.attacker.com" // navigates cross-origin
"\x00//evil.attacker.com" // navigates cross-origin
"//evil.attacker.com" // correctly blocked (only case covered)The added tests only cover leading/trailing whitespace, which trim() handles, so this slips through.
Fix — strip parser-ignored chars before the test:
if (typeof to === 'string' && SCHEME_RELATIVE_PREFIX.test(to.replace(/[\u0000-\u001F]/g, ''))) {- Generated by Claude with @dominic-clerk's supervision
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
windowNavigatein@clerk/sharedassigned its argument straight towindow.location.hrefon the assumption that callers had already validated it. This change routes every internal browser navigation inclerk-jsand@clerk/uithrough one chokepoint,clerk.__internal_windowNavigate, which checks the resolved URL against the configuredallowedRedirectProtocols(thehttp/https/wails/chrome-extensiondefaults plus any customer additions) and rejects scheme-relative inputs like//hostbefore parsing.The part worth a close look is the version-skew path:
@clerk/uican run against an olderclerk-jsthat does not expose__internal_windowNavigate, soclerkWindowNavigatefalls back to the bare@clerk/sharedhelper and the static allowlist, still rejecting the bad cases. The barewindowNavigateexport stays for that reason and is now@deprecatedin favor of the wrapper.Summary by CodeRabbit
Bug Fixes
Tests