-
Notifications
You must be signed in to change notification settings - Fork 458
fix(clerk-js): validate window navigation protocols and honor allowedRedirectProtocols #8961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| '@clerk/clerk-js': patch | ||
| '@clerk/react': patch | ||
| '@clerk/shared': patch | ||
| '@clerk/ui': patch | ||
| --- | ||
|
|
||
| Fix missing redirect URL protocol validation for Clerk UI browser navigations, including the multi-session add-account flow. | ||
|
|
||
| Internal browser navigations now consistently honor configured redirect protocols and fail closed across mixed ClerkJS/UI bundle versions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { ALLOWED_PROTOCOLS, CLERK_BEFORE_UNLOAD_EVENT, windowNavigate } from '../windowNavigate'; | ||
|
|
||
| describe('windowNavigate', () => { | ||
| let originalLocation: Location; | ||
| let hrefSetter: ReturnType<typeof vi.fn>; | ||
| let warnSpy: ReturnType<typeof vi.spyOn>; | ||
| let eventSpy: ReturnType<typeof vi.fn>; | ||
|
|
||
| beforeEach(() => { | ||
| originalLocation = window.location; | ||
| hrefSetter = vi.fn(); | ||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: new Proxy( | ||
| { href: 'https://example.com/' }, | ||
| { | ||
| set: (target, prop, value) => { | ||
| if (prop === 'href') { | ||
| hrefSetter(value); | ||
| (target as any).href = value; | ||
| return true; | ||
| } | ||
| (target as any)[prop] = value; | ||
| return true; | ||
| }, | ||
| }, | ||
| ), | ||
| }); | ||
| warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| eventSpy = vi.fn(); | ||
| window.addEventListener(CLERK_BEFORE_UNLOAD_EVENT, eventSpy); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| window.removeEventListener(CLERK_BEFORE_UNLOAD_EVENT, eventSpy); | ||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: originalLocation, | ||
| }); | ||
| warnSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['absolute https URL', 'https://example.com/dashboard'], | ||
| ['absolute http URL', 'http://example.com/dashboard'], | ||
| ['relative path', '/sign-in'], | ||
| ['wails protocol', 'wails://app/route'], | ||
| ['chrome-extension protocol', 'chrome-extension://abc/route'], | ||
| ])('navigates to %s', (_label, to) => { | ||
| windowNavigate(to); | ||
| expect(hrefSetter).toHaveBeenCalledTimes(1); | ||
| expect(eventSpy).toHaveBeenCalledTimes(1); | ||
| expect(warnSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['javascript', 'javascript:alert(1)'], | ||
| ['data', 'data:text/html,<script>alert(1)</script>'], | ||
| ['file', 'file:///etc/passwd'], | ||
| ['vbscript', 'vbscript:msgbox(1)'], | ||
| ['mixed-case JavaScript', 'JavaScript:alert(1)'], | ||
| ['upper-case JAVASCRIPT', 'JAVASCRIPT:alert(1)'], | ||
| ['leading-whitespace javascript', ' javascript:alert(1)'], | ||
| ['leading-tab javascript', '\tjavascript:alert(1)'], | ||
| ['leading-newline javascript', '\njavascript:alert(1)'], | ||
| ])('blocks %s: protocol and does not navigate', (_label, to) => { | ||
| windowNavigate(to); | ||
| expect(hrefSetter).not.toHaveBeenCalled(); | ||
| expect(eventSpy).not.toHaveBeenCalled(); | ||
| expect(warnSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('blocks javascript: URLs that the URL parser normalizes via the base URL', () => { | ||
| windowNavigate('javascript:alert(location.origin)//'); | ||
| expect(hrefSetter).not.toHaveBeenCalled(); | ||
| expect(eventSpy).not.toHaveBeenCalled(); | ||
| expect(warnSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['scheme-relative //host', '//evil.example/path'], | ||
| ['scheme-relative ///host', '///evil.example/path'], | ||
| ['backslash \\\\host', '\\\\evil.example\\path'], | ||
| ['mixed /\\host', '/\\evil.example/path'], | ||
| ['mixed \\/host', '\\/evil.example/path'], | ||
| ['leading-whitespace scheme-relative', ' //evil.example/path'], | ||
| ['leading-tab scheme-relative', '\t//evil.example/path'], | ||
| ])('blocks %s and does not navigate', (_label, to) => { | ||
| windowNavigate(to); | ||
| expect(hrefSetter).not.toHaveBeenCalled(); | ||
| expect(eventSpy).not.toHaveBeenCalled(); | ||
| expect(warnSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('still rejects scheme-relative URLs when an extended allowlist is supplied', () => { | ||
| windowNavigate('//evil.example/path', { | ||
| allowedProtocols: [...ALLOWED_PROTOCOLS, 'slack:'], | ||
| }); | ||
| expect(hrefSetter).not.toHaveBeenCalled(); | ||
| expect(eventSpy).not.toHaveBeenCalled(); | ||
| expect(warnSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('honors a caller-supplied extended allowlist for custom protocols', () => { | ||
| windowNavigate('slack://channel/123', { | ||
| allowedProtocols: [...ALLOWED_PROTOCOLS, 'slack:'], | ||
| }); | ||
| expect(hrefSetter).toHaveBeenCalledTimes(1); | ||
| expect(hrefSetter).toHaveBeenCalledWith('slack://channel/123'); | ||
| expect(eventSpy).toHaveBeenCalledTimes(1); | ||
| expect(warnSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('still rejects disallowed protocols when an extended allowlist is supplied', () => { | ||
| windowNavigate('javascript:alert(1)', { | ||
| allowedProtocols: [...ALLOWED_PROTOCOLS, 'slack:'], | ||
| }); | ||
| expect(hrefSetter).not.toHaveBeenCalled(); | ||
| expect(eventSpy).not.toHaveBeenCalled(); | ||
| expect(warnSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,48 @@ export const ALLOWED_PROTOCOLS = [ | |
| 'chrome-extension:', | ||
| ]; | ||
|
|
||
| export type WindowNavigateOptions = { | ||
| /** | ||
| * Protocol allowlist applied to the resolved URL. Defaults to `ALLOWED_PROTOCOLS`. Pass an | ||
| * extended list (e.g. `Clerk`'s `#allowedRedirectProtocols`) to honor the customer-supplied | ||
| * `allowedRedirectProtocols` option. | ||
| */ | ||
| allowedProtocols?: ReadonlyArray<string>; | ||
| }; | ||
|
|
||
| const SCHEME_RELATIVE_PREFIX = /^[/\\][/\\]/; | ||
|
|
||
| /** | ||
| * Helper utility to navigate via window.location.href. Also dispatches a clerk:beforeunload custom event. | ||
| * | ||
| * Note that this utility should **never** be called with a user-provided URL. We make no specific checks against the contents of the URL here and assume it is safe. Use `Clerk.navigate()` instead for user-provided URLs. | ||
| * Navigations whose protocol is not in the allowlist (e.g. `javascript:`, `data:`) are aborted. | ||
| * Scheme-relative inputs (`//host`, `\\host`) are also rejected: they adopt the base URL's scheme, | ||
| * which is always in the allowlist, so they would otherwise pass the protocol check while | ||
| * redirecting cross-origin. | ||
| * | ||
| * Callers that have already validated against an extended allowlist should pass it via | ||
| * `options.allowedProtocols` so legitimate custom protocols (Wails, Tauri, etc.) are honored. | ||
| * | ||
| * @deprecated Use `clerk.__internal_windowNavigate` instead. It honors the customer-supplied | ||
| * `allowedRedirectProtocols` option by default, so internal call sites can't accidentally | ||
| * bypass it by forgetting to pass `options.allowedProtocols`. The bare export will be removed | ||
| * in the next major version. | ||
| */ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scheme-relative open-redirect bypass via control characters
Bypasses against base "/\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 Fix — strip parser-ignored chars before the test: if (typeof to === 'string' && SCHEME_RELATIVE_PREFIX.test(to.replace(/[\u0000-\u001F]/g, ''))) {
|
||
| console.warn( | ||
| `Clerk: scheme-relative navigation to "${to}" is not allowed. Provide a same-origin path or an absolute URL.`, | ||
| ); | ||
| return; | ||
| } | ||
| const toURL = new URL(to, window.location.href); | ||
| const allowedProtocols = options?.allowedProtocols ?? ALLOWED_PROTOCOLS; | ||
| if (!allowedProtocols.includes(toURL.protocol)) { | ||
| console.warn( | ||
| `Clerk: "${toURL.protocol}" is not a valid navigation protocol. Aborting navigation. If you think this is a mistake, please open an issue.`, | ||
| ); | ||
| return; | ||
| } | ||
| window.dispatchEvent(new CustomEvent(CLERK_BEFORE_UNLOAD_EVENT)); | ||
| window.location.href = toURL.href; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.