fix(clerk-js): Directory traversal relative URL detection (#4483)

Co-authored-by: Nikos Douvlis <[email protected]>

.changeset/chilled-crabs-stare.md

@@ -0,0 +1,5 @@
+---
+'@clerk/clerk-js': patch
+---
+
+Fix an issue where protocol relative URLs were not properly detected as non-relative.

packages/clerk-js/src/utils/__tests__/redirectUrls.test.ts

@@ -227,7 +227,7 @@ describe('redirectUrls', () => {
       expect(redirectUrls.getAfterSignUpUrl()).toBe(`${mockWindowLocation.href}sign-up-force-redirect-url`);
     });
 
-    it('prioritizes redirect_url from searchParamsover fallback urls', () => {
+    it('prioritizes redirect_url from searchParams over fallback urls', () => {
       const redirectUrls = new RedirectUrls(
         {
           signInFallbackRedirectUrl: 'sign-in-fallback-redirect-url',

packages/clerk-js/src/utils/__tests__/url.test.ts

@@ -8,11 +8,11 @@ import {
   getSearchParameterFromHash,
   hasBannedProtocol,
   hasExternalAccountSignUpError,
-  isAllowedRedirectOrigin,
+  isAllowedRedirect,
   isDataUri,
   isDevAccountPortalOrigin,
+  isProblematicUrl,
   isRedirectForFAPIInitiatedFlow,
-  isRelativeUrl,
   isValidUrl,
   mergeFragmentIntoUrl,
   relativeToAbsoluteUrl,
@@ -84,18 +84,38 @@ describe('isValidUrl(url,base)', () => {
   });
 });
 
-describe('isRelativeUrl(url,base)', () => {
+describe('isProblematicUrl(url)', () => {
   const cases: Array<[string, boolean]> = [
-    ['', true],
-    ['/', true],
-    ['/test', true],
-    ['/test?clerk=true', true],
-    ['/?clerk=true', true],
-    ['https://www.clerk.com/', false],
+    // 1. URLs with backslashes instead of forward slashes
+    ['\\evil.com', false],
+    ['/\\evil.com', false],
+    ['\\\\evil.com', false],
+    ['/..\\evil.com', false],
+    ['/\\@evil.com', false],
+
+    // 2. Path traversal attempts
+    ['..//evil.com', true],
+    ['/../evil.com', false],
+    ['../../', false],
+    ['/../../', false],
+
+    // 3. URLs with different schemes
+    ['javascript:alert(1)', true],
+
+    // 4. URLs with control characters and whitespace
+    ['/test ', false],
+    [' /test', false],
+    ['/test\n', false],
+
+    // 5. Fragment identifiers and query parameters
+    ['/#/evil.com', false],
+    ['/path#//evil.com', false],
+    ['/evil.com?redirect=evil.com', false],
+    ['/evil.com?redirect=https://evil.com', false],
   ];
 
-  test.each(cases)('.isRelativeUrl(%s,%s)', (a, expected) => {
-    expect(isRelativeUrl(a)).toBe(expected);
+  test.each(cases)('.isProblematicUrl(%s,%s)', (a, expected) => {
+    expect(isProblematicUrl(new URL(a, 'https://clerk.dummy'))).toBe(expected);
   });
 });
 
@@ -441,7 +461,7 @@ describe('getETLDPlusOneFromFrontendApi(frontendAp: string)', () => {
   });
 });
 
-describe('isAllowedRedirectOrigin', () => {
+describe('isAllowedRedirect', () => {
   const cases: [string, Array<string | RegExp> | undefined, boolean][] = [
     // base cases
     ['https://clerk.com', ['https://www.clerk.com'], false],
@@ -463,7 +483,7 @@ describe('isAllowedRedirectOrigin', () => {
     // empty origins list for relative routes
     ['/', [], true],
     // empty origins list for absolute routes
-    ['https://www.clerk.com/', [], false],
+    ['https://www.example.com/', [], false],
     //undefined origins
     ['https://www.clerk.com/', undefined, true],
     // query params
@@ -475,18 +495,17 @@ describe('isAllowedRedirectOrigin', () => {
     // malformed or protocol-relative URLs
     ['http:evil.com', [/https:\/\/www\.clerk\.com/], false],
     ['https:evil.com', [/https:\/\/www\.clerk\.com/], false],
-    ['http//evil.com', [/https:\/\/www\.clerk\.com/], false],
-    ['https//evil.com', [/https:\/\/www\.clerk\.com/], false],
     ['//evil.com', [/https:\/\/www\.clerk\.com/], false],
+    ['..//evil.com', ['https://www.clerk.com'], false],
   ];
 
   const warnMock = jest.spyOn(logger, 'warnOnce');
 
   beforeEach(() => warnMock.mockClear());
   afterAll(() => warnMock.mockRestore());
 
-  test.each(cases)('isAllowedRedirectOrigin("%s","%s") === %s', (url, allowedOrigins, expected) => {
-    expect(isAllowedRedirectOrigin(allowedOrigins)(url)).toEqual(expected);
+  test.each(cases)('isAllowedRedirect("%s","%s") === %s', (url, allowedOrigins, expected) => {
+    expect(isAllowedRedirect(allowedOrigins, 'https://www.clerk.com')(url)).toEqual(expected);
     expect(warnMock).toHaveBeenCalledTimes(Number(!expected)); // Number(boolean) evaluates to 0 or 1
   });
 });
@@ -532,6 +551,6 @@ describe('relativeToAbsoluteUrl', () => {
   ];
 
   test.each(cases)('relativeToAbsoluteUrl(%s, %s) === %s', (origin, relative, expected) => {
-    expect(relativeToAbsoluteUrl(relative, origin)).toEqual(expected);
+    expect(relativeToAbsoluteUrl(relative, origin)).toEqual(new URL(expected));
   });
 });

packages/clerk-js/src/utils/redirectUrls.ts

@@ -3,7 +3,7 @@ import { camelToSnake } from '@clerk/shared/underscore';
 import type { ClerkOptions, RedirectOptions } from '@clerk/types';
 
 import { assertNoLegacyProp, warnForNewPropShadowingLegacyProp } from './assertNoLegacyProp';
-import { isAllowedRedirectOrigin, relativeToAbsoluteUrl } from './url';
+import { isAllowedRedirect, relativeToAbsoluteUrl } from './url';
 
 export class RedirectUrls {
   private static keys: (keyof RedirectOptions)[] = [
@@ -146,7 +146,9 @@ export class RedirectUrls {
       // @ts-expect-error
       res[key] = obj[key];
     });
-    return this.#toAbsoluteUrls(this.#filterOrigins(res));
+    return applyFunctionToObj(this.#filterRedirects(this.#toAbsoluteUrls(filterProps(res, Boolean))), val =>
+      val.toString(),
+    );
   }
 
   #parseSearchParams(obj: any) {
@@ -156,14 +158,16 @@ export class RedirectUrls {
       res[key] = obj[camelToSnake(key)];
     });
     res['redirectUrl'] = obj.redirect_url;
-    return this.#toAbsoluteUrls(this.#filterOrigins(res));
+    return applyFunctionToObj(this.#filterRedirects(this.#toAbsoluteUrls(filterProps(res, Boolean))), val =>
+      val.toString(),
+    );
   }
 
   #toAbsoluteUrls(obj: RedirectOptions) {
     return applyFunctionToObj(obj, (url: string) => relativeToAbsoluteUrl(url, window.location.origin));
   }
 
-  #filterOrigins = (obj: RedirectOptions) => {
-    return filterProps(obj, isAllowedRedirectOrigin(this.options?.allowedRedirectOrigins));
+  #filterRedirects = (obj: RedirectOptions) => {
+    return filterProps(obj, isAllowedRedirect(this.options?.allowedRedirectOrigins, window.location.origin));
   };
 }

packages/clerk-js/src/utils/url.ts

@@ -234,37 +234,38 @@ export function isValidUrl(val: unknown): val is string {
   }
 }
 
-export function relativeToAbsoluteUrl(url: string, origin: string | URL): string {
-  if (isValidUrl(url)) {
-    return url;
+export function relativeToAbsoluteUrl(url: string, origin: string | URL): URL {
+  try {
+    return new URL(url);
+  } catch (e) {
+    return new URL(url, origin);
   }
-  return new URL(url, origin).href;
 }
 
-export function isRelativeUrl(val: string): boolean {
-  if (val !== val && !val) {
-    return false;
-  }
+// Regular expression to detect disallowed patterns
+const disallowedPatterns = [
+  /\0/, // Null bytes
+  /^\/\//, // Protocol-relative
+  // eslint-disable-next-line no-control-regex
+  /[\x00-\x1F]/, // Control characters
+];
 
-  if (val.startsWith('//') || val.startsWith('http/') || val.startsWith('https/')) {
-    // Protocol-relative URL; consider it absolute.
-    return false;
+/**
+ * Check for potentially problematic URLs that could have been crafted to intentionally bypass the origin check. Note that the URLs passed to this
+ * function are assumed to be from an "allowed origin", so we are not executing origin-specific checks here.
+ */
+export function isProblematicUrl(url: URL): boolean {
+  if (hasBannedProtocol(url)) {
+    return true;
   }
-
-  try {
-    // If this does not throw, it's a valid absolute URL
-    new URL(val);
-    return false;
-  } catch (e) {
-    try {
-      // If this does not throw, it's a valid relative URL
-      new URL(val, DUMMY_URL_BASE);
+  // Check against disallowed patterns
+  for (const pattern of disallowedPatterns) {
+    if (pattern.test(url.pathname)) {
       return true;
-    } catch (e) {
-      // Invalid URL case
-      return false;
     }
   }
+
+  return false;
 }
 
 export function isDataUri(val?: string): val is string {
@@ -352,20 +353,25 @@ export function requiresUserInput(redirectUrl: string): boolean {
   return frontendApiRedirectPathsWithUserInput.includes(url.pathname);
 }
 
-export const isAllowedRedirectOrigin =
-  (allowedRedirectOrigins: Array<string | RegExp> | undefined) => (_url: string) => {
-    if (!allowedRedirectOrigins) {
-      return true;
+export const isAllowedRedirect =
+  (allowedRedirectOrigins: Array<string | RegExp> | undefined, currentOrigin: string) => (_url: URL | string) => {
+    let url = _url;
+    if (typeof url === 'string') {
+      url = relativeToAbsoluteUrl(url, currentOrigin);
     }
 
-    if (isRelativeUrl(_url)) {
+    if (!allowedRedirectOrigins) {
       return true;
     }
 
-    const url = new URL(_url, DUMMY_URL_BASE);
-    const isAllowed = allowedRedirectOrigins
-      .map(origin => (typeof origin === 'string' ? globs.toRegexp(trimTrailingSlash(origin)) : origin))
-      .some(origin => origin.test(trimTrailingSlash(url.origin)));
+    const isSameOrigin = currentOrigin === url.origin;
+
+    const isAllowed =
+      !isProblematicUrl(url) &&
+      (isSameOrigin ||
+        allowedRedirectOrigins
+          .map(origin => (typeof origin === 'string' ? globs.toRegexp(trimTrailingSlash(origin)) : origin))
+          .some(origin => origin.test(trimTrailingSlash(url.origin))));
 
     if (!isAllowed) {
       logger.warnOnce(