[8.14] Use spec-compliant URL parser to parse next URL parameter. (#…

…183521) (#183726)

# Backport

This will backport the following commits from `main` to `8.14`:
- [Use spec-compliant URL parser to parse `next` URL parameter.
(#183521)](#183521)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Aleh
Zasypkin","email":"aleh.zasypkin@elastic.co"},"sourceCommit":{"committedDate":"2024-05-17T11:39:52Z","message":"Use
spec-compliant URL parser to parse `next` URL parameter. (#183521)\n\n##
Summary\r\n\r\nThe Node.js URL parser [has
been\r\ndeprecated](https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost)\r\nand
we should start slowly switching to the [spec-compliant
URL\r\nparser](https://url.spec.whatwg.org/#url-parsing) to parse URLs
in\r\nKibana starting from the `next` query string
parameter.\r\n\r\n__Flaky Test Runner:__ [x65 for every added
functional\r\ntest](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6010)","sha":"b306f007fbcc723bffffd67b3867f6a563ffae2d","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["chore","Team:Security","release_note:skip","backport:all-open","v8.15.0"],"number":183521,"url":"https://github.com/elastic/kibana/pull/183521","mergeCommit":{"message":"Use
spec-compliant URL parser to parse `next` URL parameter. (#183521)\n\n##
Summary\r\n\r\nThe Node.js URL parser [has
been\r\ndeprecated](https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost)\r\nand
we should start slowly switching to the [spec-compliant
URL\r\nparser](https://url.spec.whatwg.org/#url-parsing) to parse URLs
in\r\nKibana starting from the `next` query string
parameter.\r\n\r\n__Flaky Test Runner:__ [x65 for every added
functional\r\ntest](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6010)","sha":"b306f007fbcc723bffffd67b3867f6a563ffae2d"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.15.0","labelRegex":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/183521","number":183521,"mergeCommit":{"message":"Use
spec-compliant URL parser to parse `next` URL parameter. (#183521)\n\n##
Summary\r\n\r\nThe Node.js URL parser [has
been\r\ndeprecated](https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost)\r\nand
we should start slowly switching to the [spec-compliant
URL\r\nparser](https://url.spec.whatwg.org/#url-parsing) to parse URLs
in\r\nKibana starting from the `next` query string
parameter.\r\n\r\n__Flaky Test Runner:__ [x65 for every added
functional\r\ntest](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6010)","sha":"b306f007fbcc723bffffd67b3867f6a563ffae2d"}}]}]
BACKPORT-->

x-pack/plugins/security/common/is_internal_url.test.ts

@@ -60,6 +60,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', () => {
@@ -85,5 +163,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);
+        }
+      }
+    });
   });
 });

x-pack/plugins/security/common/is_internal_url.ts

@@ -5,33 +5,35 @@
  * 2.0.
  */
 
-import { parse } from 'url';
-
+/**
+ * Determine if url is outside of this Kibana install.
+ */
 export function isInternalURL(url: string, basePath = '') {
-  const { protocol, hostname, port, pathname } = parse(
-    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');
+      }
+    });
   });
 }