security(ExternalRedirect): stronger validations (#10627)

opencollective · Aug 30, 2024 · 1baa41c · 1baa41c
1 parent 730fda2
commit 1baa41c
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 15 deletions.
diff --git a/lib/__tests__/url-helpers.test.js b/lib/__tests__/url-helpers.test.js
@@ -1,17 +1,86 @@
-import { isTrustedRedirectHost } from '../url-helpers';
+import { isRelativeHref, isTrustedRedirectURL } from '../url-helpers';
+import * as WindowLib from '../window';
+
+// Mock the window object to simulate SSR/CSR
+jest.mock('../window');
 
 describe('isTrustedRedirectHost', () => {
   it('returns true for valid domains', () => {
-    expect(isTrustedRedirectHost('octobox.io')).toBe(true);
-    expect(isTrustedRedirectHost('opencollective.com')).toBe(true);
-    expect(isTrustedRedirectHost('docs.opencollective.com')).toBe(true);
-    expect(isTrustedRedirectHost('app.papertree.earth')).toBe(true);
-    expect(isTrustedRedirectHost('gatherfor.org')).toBe(true);
+    expect(isTrustedRedirectURL(new URL('https://octobox.io'))).toBe(true);
+    expect(isTrustedRedirectURL(new URL('https://opencollective.com'))).toBe(true);
+    expect(isTrustedRedirectURL(new URL('https://docs.opencollective.com'))).toBe(true);
+    expect(isTrustedRedirectURL(new URL('https://app.papertree.earth'))).toBe(true);
+    expect(isTrustedRedirectURL(new URL('https://gatherfor.org'))).toBe(true);
   });
 
   it('returns false for invalid domains', () => {
-    expect(isTrustedRedirectHost('wowoctobox.io')).toBe(false);
-    expect(isTrustedRedirectHost('opencollectivez.com')).toBe(false);
-    expect(isTrustedRedirectHost('malicious-opencollective.com')).toBe(false);
+    expect(isTrustedRedirectURL(new URL('https://wowoctobox.io'))).toBe(false);
+    expect(isTrustedRedirectURL(new URL('https://opencollectivez.com'))).toBe(false);
+    expect(isTrustedRedirectURL(new URL('https://malicious-opencollective.com'))).toBe(false);
+  });
+});
+
+describe('isRelativeHref', () => {
+  describe('on main website, client-side', () => {
+    it('returns true for relative URLs', () => {
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('/foo')).toBe(true);
+
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('/foo/bar')).toBe(true);
+
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('/foo/bar?baz=1')).toBe(true);
+
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('/foo/bar#baz')).toBe(true);
+    });
+
+    it('returns false for absolute URLs', () => {
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('https://octobox.io')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('http://octobox.io')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('//octobox.io')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('foo')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('foo/bar')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('./foo/bar')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: 'https://opencollective.com' });
+      expect(isRelativeHref('../foo/bar')).toBe(false);
+    });
+  });
+
+  describe('without window object set, server-side', () => {
+    it('returns true for relative URLs', () => {
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('/foo')).toBe(true);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('/foo/bar')).toBe(true);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('/foo/bar?baz=1')).toBe(true);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('/foo/bar#baz')).toBe(true);
+    });
+
+    it('returns false for absolute URLs', () => {
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('https://octobox.io')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('http://octobox.io')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('//octobox.io')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('foo')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('foo/bar')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('./foo/bar')).toBe(false);
+      WindowLib.getWindowLocation.mockReturnValueOnce({ origin: undefined });
+      expect(isRelativeHref('../foo/bar')).toBe(false);
+    });
   });
 });
diff --git a/lib/url-helpers.ts b/lib/url-helpers.ts
@@ -5,6 +5,7 @@ import type { TaxFormType } from '../components/dashboard/sections/tax-informati
 import { CollectiveType } from './constants/collectives';
 import { TransactionTypes } from './constants/transactions';
 import { getWebsiteUrl } from './utils';
+import { getWindowLocation } from './window';
 
 export const PDF_SERVICE_URL = process.env.PDF_SERVICE_URL;
 const NEXT_PDF_SERVICE_URL = process.env.NEXT_PDF_SERVICE_URL;
@@ -161,7 +162,14 @@ const TRUSTED_DOMAINS = [
 ];
 const TRUSTED_ROOT_DOMAINS = ['opencollective.com', 'opencollective.foundation', 'oscollective.org'];
 
-export const isTrustedRedirectHost = host => {
+export const isTrustedRedirectURL = (url: URL) => {
+  if (url.protocol !== 'https:') {
+    return false;
+  } else if (url.port && url.port !== '443') {
+    return false;
+  }
+
+  const host = url.host;
   if (TRUSTED_DOMAINS.includes(host)) {
     return true;
   }
@@ -189,8 +197,30 @@ export const addParentToURLIfMissing = (router, account, url = '', queryParams =
 };
 
 export const isRelativeHref = href => {
+  if (!href) {
+    return true;
+  }
+
+  // We force all relative URLs to start with `/`
+  href = href.trim();
+  if (!href.startsWith('/')) {
+    return false;
+  }
+
+  // If we're in the browser, there's a safe way to check this
+  const windowLocation = getWindowLocation();
+  if (windowLocation) {
+    try {
+      const parsedUrl = new URL(href, windowLocation.origin);
+      return parsedUrl.origin === windowLocation.origin;
+    } catch (e) {
+      return false; // Invalid URL
+    }
+  }
+
+  // Otherwise, we fallback on a regex that will protect against `javascript:`, `//evil.com`, etc.
   href = href.trim();
-  return !href || href.startsWith('#') || href === '/' || new RegExp('^/[^/\\\\]+').test(href);
+  return href.startsWith('#') || href === '/' || new RegExp('^/[^/\\\\]+').test(href);
 };
 
 export async function followOrderRedirectUrl(
@@ -211,7 +241,7 @@ export async function followOrderRedirectUrl(
   }
 
   const fallback = `/${collective.slug}/donate/success?OrderId=${order.id}`;
-  if (isTrustedRedirectHost(url.host)) {
+  if (isTrustedRedirectURL(url)) {
     if (shouldRedirectParent) {
       window.parent.location.href = url.href;
     } else {

diff --git a/lib/window.ts b/lib/window.ts
@@ -0,0 +1,11 @@
+/**
+ * A helper that returns `window.location` if it exists.
+ * Two advantages:
+ * 1. It makes it easier to mock `window.location` in tests.
+ * 2. It doesn't crash the server-side rendering.
+ */
+export const getWindowLocation = (): Location | undefined => {
+  if (typeof window !== 'undefined') {
+    return window.location;
+  }
+};
diff --git a/pages/external-redirect.js b/pages/external-redirect.js
@@ -4,7 +4,7 @@ import { useRouter } from 'next/router';
 import { FormattedMessage } from 'react-intl';
 import { isURL } from 'validator';
 
-import { isRelativeHref, isTrustedRedirectHost } from '../lib/url-helpers';
+import { isRelativeHref, isTrustedRedirectURL } from '../lib/url-helpers';
 import { isValidRelativeUrl, parseToBoolean } from '../lib/utils';
 
 import Container from '../components/Container';
@@ -27,14 +27,20 @@ const getFallback = fallback => {
 };
 
 export const isValidExternalRedirect = url => {
-  const validationParams = process.env.NODE_ENV === 'production' ? {} : { require_tld: false }; // eslint-disable-line camelcase
+  // Default params: { protocols: ['http','https','ftp'], require_tld: true, require_protocol: false, require_host: true, require_port: false, require_valid_protocol: true, allow_underscores: false, host_whitelist: false, host_blacklist: false, allow_trailing_dot: false, allow_protocol_relative_urls: false, allow_fragments: true, allow_query_components: true, disallow_auth: false, validate_length: true }
+  const validationParams = {};
+  validationParams['protocols'] = ['http', 'https'];
+  if (process.env.NODE_ENV !== 'production') {
+    validationParams['require_tld'] = false;
+  }
+
   return url && isURL(url, validationParams);
 };
 
 const shouldRedirectDirectly = urlStr => {
   try {
     const parsedUrl = new URL(urlStr);
-    return isTrustedRedirectHost(parsedUrl.host);
+    return isTrustedRedirectURL(parsedUrl.host);
   } catch {
     return false;
   }