From 9557b557489c60fbd067906afa97a7c418f6569e Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Tue, 5 Nov 2024 23:25:31 -0600 Subject: [PATCH] fix(clerk-js): Directory traversal relative URL detection (#4483) Co-authored-by: Nikos Douvlis --- .changeset/chilled-crabs-stare.md | 5 ++ .../src/utils/__tests__/redirectUrls.test.ts | 2 +- .../clerk-js/src/utils/__tests__/url.test.ts | 55 ++++++++++----- packages/clerk-js/src/utils/redirectUrls.ts | 14 ++-- packages/clerk-js/src/utils/url.ts | 70 ++++++++++--------- 5 files changed, 90 insertions(+), 56 deletions(-) create mode 100644 .changeset/chilled-crabs-stare.md diff --git a/.changeset/chilled-crabs-stare.md b/.changeset/chilled-crabs-stare.md new file mode 100644 index 0000000000..4ef37974d1 --- /dev/null +++ b/.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. diff --git a/packages/clerk-js/src/utils/__tests__/redirectUrls.test.ts b/packages/clerk-js/src/utils/__tests__/redirectUrls.test.ts index a5433d6d6d..bcc95c1a96 100644 --- a/packages/clerk-js/src/utils/__tests__/redirectUrls.test.ts +++ b/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', diff --git a/packages/clerk-js/src/utils/__tests__/url.test.ts b/packages/clerk-js/src/utils/__tests__/url.test.ts index 6d90443ddc..ef37ded892 100644 --- a/packages/clerk-js/src/utils/__tests__/url.test.ts +++ b/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 | 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,9 +495,8 @@ 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'); @@ -485,8 +504,8 @@ describe('isAllowedRedirectOrigin', () => { 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)); }); }); diff --git a/packages/clerk-js/src/utils/redirectUrls.ts b/packages/clerk-js/src/utils/redirectUrls.ts index 9038af2fd8..b31b728cd8 100644 --- a/packages/clerk-js/src/utils/redirectUrls.ts +++ b/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)); }; } diff --git a/packages/clerk-js/src/utils/url.ts b/packages/clerk-js/src/utils/url.ts index 971d670123..e888047dff 100644 --- a/packages/clerk-js/src/utils/url.ts +++ b/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 | undefined) => (_url: string) => { - if (!allowedRedirectOrigins) { - return true; +export const isAllowedRedirect = + (allowedRedirectOrigins: Array | 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(