Use spec-compliant URL parser to parse next URL parameter.

.github/CODEOWNERS

@@ -1278,6 +1278,10 @@ x-pack/plugins/cloud_integrations/cloud_full_story/server/config.ts @elastic/kib
 /src/dev/eslint/security_eslint_rule_tests.ts @elastic/kibana-security
 /src/core/server/integration_tests/config/check_dynamic_config.test.ts @elastic/kibana-security
 /src/plugins/telemetry/server/config/telemetry_labels.ts @elastic/kibana-security
+/packages/kbn-std/src/is_internal_url.test.ts @elastic/kibana-core @elastic/kibana-security
+/packages/kbn-std/src/is_internal_url.ts @elastic/kibana-core @elastic/kibana-security
+/packages/kbn-std/src/parse_next_url.test.ts @elastic/kibana-core @elastic/kibana-security
+/packages/kbn-std/src/parse_next_url.ts @elastic/kibana-core @elastic/kibana-security
 /test/interactive_setup_api_integration/ @elastic/kibana-security
 /test/interactive_setup_functional/ @elastic/kibana-security
 /test/plugin_functional/test_suites/core_plugins/rendering.ts @elastic/kibana-security

packages/kbn-std/src/is_internal_url.test.ts

@@ -61,6 +61,84 @@ describe('isInternalURL', () => {
       const href = `${basePath}/app/kibana/../../../management`;
       expect(isInternalURL(href, basePath)).toBe(false);
     });
+
+    it('should return `false` if absolute URL contains tabs or new lines', () => {
+      // These URLs can either be treated as internal or external depending on the presence of the special characters.
+      const getURLsWithCharInRelativeScheme = (char: string) => [
+        `/${char}${basePath}app/kibana`,
+        `/${char}/${basePath}/app/kibana`,
+        `/${char}${basePath}/example.com`,
+        `/${char}/${basePath}/example.com`,
+        `/${char}${basePath}/example.org`,
+        `/${char}/${basePath}/example.org`,
+        `/${char}${basePath}/example.org:5601`,
+        `/${char}/${basePath}/example.org:5601`,
+      ];
+
+      // These URLs can either be treated as internal or external depending on the presence of the special characters
+      // AND spaces since these affect how URL's scheme is parsed.
+      const getURLsWithCharInScheme = (char: string) => [
+        `htt${char}ps://example.com${basePath}`,
+        `htt${char}ps://example.org${basePath}`,
+        `htt${char}ps://example.org:5601${basePath}`,
+        `java${char}script:${basePath}alert(1)`,
+        `htt${char}p:/${basePath}/example.org:5601`,
+        `file${char}:/${basePath}/example.com`,
+      ];
+
+      // These URLs should always be recognized as external irrespective to the presence of the special characters or
+      // spaces since these affect URLs hostname.
+      const getURLsWithCharInHost = (char: string) => [
+        `//${char}${basePath}/app/kibana`,
+        `//${char}/example.com${basePath}`,
+        `//${char}/example.org${basePath}`,
+        `//${char}/example.org:5601${basePath}`,
+        `https:/${char}/example.com${basePath}`,
+        // This URL is only valid if the `char` is a tab or newline character.
+        `https://example${char}.com${basePath}/path`,
+      ];
+
+      // Detection logic should treat URLs as if tab and newline characters weren't present.
+      for (const char of ['', '\t', '\n', '\r', '\t\n\r']) {
+        for (const url of [
+          ...getURLsWithCharInRelativeScheme(char),
+          ...getURLsWithCharInScheme(char),
+          ...getURLsWithCharInHost(char),
+        ]) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+      }
+
+      // Characters that aren't tabs, spaces, or newline characters turn absolute scheme-relative URLs to relative URLs.
+      for (const char of ['1', 'a']) {
+        for (const url of getURLsWithCharInRelativeScheme(char)) {
+          expect(isInternalURL(url)).toBe(true);
+        }
+
+        for (const url of getURLsWithCharInScheme(char)) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+
+        for (const url of getURLsWithCharInHost(char)) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+      }
+
+      // Spaces aren't allowed in scheme definition and turn absolute URLs into relative URLs.
+      for (const char of [' ']) {
+        for (const url of getURLsWithCharInRelativeScheme(char)) {
+          expect(isInternalURL(url)).toBe(true);
+        }
+
+        for (const url of getURLsWithCharInScheme(char)) {
+          expect(isInternalURL(url)).toBe(true);
+        }
+
+        for (const url of getURLsWithCharInHost(char)) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+      }
+    });
   });
 
   describe('without basePath defined', () => {
@@ -86,5 +164,83 @@ describe('isInternalURL', () => {
       const hrefWithThreeSlashes = `///app/kibana`;
       expect(isInternalURL(hrefWithThreeSlashes)).toBe(false);
     });
+
+    it('should properly handle tabs or newline characters in the URL', () => {
+      // These URLs can either be treated as internal or external depending on the presence of the special characters.
+      const getURLsWithCharInRelativeScheme = (char: string) => [
+        `/${char}/app/kibana`,
+        `/${char}//app/kibana`,
+        `/${char}/example.com`,
+        `/${char}//example.com`,
+        `/${char}/example.org`,
+        `/${char}//example.org`,
+        `/${char}/example.org:5601`,
+        `/${char}//example.org:5601`,
+      ];
+
+      // These URLs can either be treated as internal or external depending on the presence of the special characters
+      // AND spaces since these affect how URL's scheme is parsed.
+      const getURLsWithCharInScheme = (char: string) => [
+        `htt${char}ps://example.com`,
+        `htt${char}ps://example.org`,
+        `htt${char}ps://example.org:5601`,
+        `java${char}script:alert(1)`,
+        `htt${char}p://example.org:5601`,
+        `file${char}://example.com`,
+      ];
+
+      // These URLs should always be recognized as external irrespective to the presence of the special characters or
+      // spaces since these affect URLs hostname.
+      const getURLsWithCharInHost = (char: string) => [
+        `//${char}/app/kibana`,
+        `//${char}/example.com`,
+        `//${char}/example.org`,
+        `//${char}/example.org:5601`,
+        `https:/${char}/example.com`,
+        // This URL is only valid if the `char` is a tab or newline character.
+        `https://example${char}.com/path`,
+      ];
+
+      // Detection logic should treat URLs as if tab and newline characters weren't present.
+      for (const char of ['', '\t', '\n', '\r', '\t\n\r']) {
+        for (const url of [
+          ...getURLsWithCharInRelativeScheme(char),
+          ...getURLsWithCharInScheme(char),
+          ...getURLsWithCharInHost(char),
+        ]) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+      }
+
+      // Characters that aren't tabs, spaces, or newline characters turn absolute scheme-relative URLs to relative URLs.
+      for (const char of ['1', 'a']) {
+        for (const url of getURLsWithCharInRelativeScheme(char)) {
+          expect(isInternalURL(url)).toBe(true);
+        }
+
+        for (const url of getURLsWithCharInScheme(char)) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+
+        for (const url of getURLsWithCharInHost(char)) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+      }
+
+      // Spaces aren't allowed in scheme definition and turn absolute URLs into relative URLs.
+      for (const char of [' ']) {
+        for (const url of getURLsWithCharInRelativeScheme(char)) {
+          expect(isInternalURL(url)).toBe(true);
+        }
+
+        for (const url of getURLsWithCharInScheme(char)) {
+          expect(isInternalURL(url)).toBe(true);
+        }
+
+        for (const url of getURLsWithCharInHost(char)) {
+          expect(isInternalURL(url)).toBe(false);
+        }
+      }
+    });
   });
 });

packages/kbn-std/src/is_internal_url.ts

@@ -6,37 +6,35 @@
  * Side Public License, v 1.
  */
 
-import { parse as parseUrl } from 'url';
-
 /**
  * Determine if url is outside of this Kibana install.
  */
 export function isInternalURL(url: string, basePath = '') {
-  const { protocol, hostname, port, pathname } = parseUrl(
-    url,
-    false /* parseQueryString */,
-    true /* slashesDenoteHost */
-  );
-
-  // We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not
-  // detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but
-  // browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser
-  // hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`)
-  // and the first slash that belongs to path.
-  if (protocol !== null || hostname !== null || port !== null) {
+  // We use the WHATWG parser TWICE with completely different dummy base URLs to ensure that the parsed URL always
+  // inherits the origin of the base URL. This means that the specified URL isn't an absolute URL, or a scheme-relative
+  // URL (//), or a scheme-relative URL with an empty host (///). Browsers may process such URLs unexpectedly due to
+  // backward compatibility reasons (e.g., a browser may treat `///abc.com` as just `abc.com`). For more details, refer
+  // to https://url.spec.whatwg.org/#concept-basic-url-parser and https://url.spec.whatwg.org/#url-representation.
+  let normalizedURL: URL;
+  try {
+    for (const baseURL of ['http://example.org:5601', 'https://example.com']) {
+      normalizedURL = new URL(url, baseURL);
+      if (normalizedURL.origin !== baseURL) {
+        return false;
+      }
+    }
+  } catch {
     return false;
   }
 
+  // Now we need to normalize URL to make sure any relative path segments (`..`) cannot escape expected base path.
   if (basePath) {
-    // Now we need to normalize URL to make sure any relative path segments (`..`) cannot escape expected
-    // base path. We can rely on `URL` with a localhost to automatically "normalize" the URL.
-    const normalizedPathname = new URL(String(pathname), 'https://localhost').pathname;
-
     return (
       // Normalized pathname can add a leading slash, but we should also make sure it's included in
-      // the original URL too
-      pathname?.startsWith('/') &&
-      (normalizedPathname === basePath || normalizedPathname.startsWith(`${basePath}/`))
+      // the original URL too. We can safely use non-null assertion operator here since we know `normalizedURL` is
+      // always defined, otherwise we would have returned `false` already.
+      url.startsWith('/') &&
+      (normalizedURL!.pathname === basePath || normalizedURL!.pathname.startsWith(`${basePath}/`))
     );
   }
 

x-pack/test/security_functional/tests/login_selector/basic_functionality.ts

@@ -65,6 +65,44 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       expect(currentURL.pathname).to.eql('/app/management/security/users');
     });
 
+    it('can login with Login Form resetting target URL', async () => {
+      this.timeout(120000);
+
+      for (const targetURL of [
+        '///example.com',
+        '//example.com',
+        'https://example.com',
+
+        '/\t/example.com',
+        '/\n/example.com',
+        '/\r/example.com',
+
+        '/\t//example.com',
+        '/\n//example.com',
+        '/\r//example.com',
+
+        '//\t/example.com',
+        '//\n/example.com',
+        '//\r/example.com',
+
+        'ht\ttps://example.com',
+        'ht\ntps://example.com',
+        'ht\rtps://example.com',
+      ]) {
+        await browser.get(
+          `${deployment.getHostPort()}/login?next=${encodeURIComponent(targetURL)}`
+        );
+
+        await PageObjects.common.waitUntilUrlIncludes('next=');
+        await PageObjects.security.loginSelector.login('basic', 'basic1');
+        // We need to make sure that both path and hash are respected.
+        const currentURL = parse(await browser.getCurrentUrl());
+
+        expect(currentURL.pathname).to.eql('/app/home');
+        await PageObjects.security.forceLogout();
+      }
+    });
+
     it('can login with SSO preserving original URL', async () => {
       await PageObjects.common.navigateToUrl('management', 'security/users', {
         ensureCurrentUrl: false,

x-pack/test/security_functional/tests/oidc/url_capture.ts

@@ -63,5 +63,43 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       const currentURL = parse(await browser.getCurrentUrl());
       expect(currentURL.path).to.eql('/authentication/app?one=two');
     });
+
+    it('resets invalid target URL', async () => {
+      this.timeout(120000);
+
+      for (const targetURL of [
+        '///example.com',
+        '//example.com',
+        'https://example.com',
+
+        '/\t/example.com',
+        '/\n/example.com',
+        '/\r/example.com',
+
+        '/\t//example.com',
+        '/\n//example.com',
+        '/\r//example.com',
+
+        '//\t/example.com',
+        '//\n/example.com',
+        '//\r/example.com',
+
+        'ht\ttps://example.com',
+        'ht\ntps://example.com',
+        'ht\rtps://example.com',
+      ]) {
+        await browser.get(
+          `${deployment.getHostPort()}/internal/security/capture-url?next=${encodeURIComponent(
+            targetURL
+          )}`
+        );
+
+        await find.byCssSelector('[data-test-subj="userMenuButton"]', 20000);
+        expect(parse(await browser.getCurrentUrl()).pathname).to.eql('/app/home');
+
+        await browser.get(deployment.getHostPort() + '/logout');
+        await PageObjects.common.waitUntilUrlIncludes('logged_out');
+      }
+    });
   });
 }

x-pack/test/security_functional/tests/saml/url_capture.ts

@@ -63,5 +63,43 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       const currentURL = parse(await browser.getCurrentUrl());
       expect(currentURL.path).to.eql('/authentication/app?one=two');
     });
+
+    it('resets invalid target URL', async () => {
+      this.timeout(120000);
+
+      for (const targetURL of [
+        '///example.com',
+        '//example.com',
+        'https://example.com',
+
+        '/\t/example.com',
+        '/\n/example.com',
+        '/\r/example.com',
+
+        '/\t//example.com',
+        '/\n//example.com',
+        '/\r//example.com',
+
+        '//\t/example.com',
+        '//\n/example.com',
+        '//\r/example.com',
+
+        'ht\ttps://example.com',
+        'ht\ntps://example.com',
+        'ht\rtps://example.com',
+      ]) {
+        await browser.get(
+          `${deployment.getHostPort()}/internal/security/capture-url?next=${encodeURIComponent(
+            targetURL
+          )}`
+        );
+
+        await find.byCssSelector('[data-test-subj="userMenuButton"]', 20000);
+        expect(parse(await browser.getCurrentUrl()).pathname).to.eql('/app/home');
+
+        await browser.get(deployment.getHostPort() + '/logout');
+        await PageObjects.common.waitUntilUrlIncludes('logged_out');
+      }
+    });
   });
 }