diff --git a/x-pack/plugins/security/common/is_internal_url.test.ts b/x-pack/plugins/security/common/is_internal_url.test.ts index d6fe64c0b33ce5..1c63603ee18c64 100644 --- a/x-pack/plugins/security/common/is_internal_url.test.ts +++ b/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); + } + } + }); }); }); diff --git a/x-pack/plugins/security/common/is_internal_url.ts b/x-pack/plugins/security/common/is_internal_url.ts index ea121aeb7cbf83..f713ec12af13e3 100644 --- a/x-pack/plugins/security/common/is_internal_url.ts +++ b/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}/`)) ); } diff --git a/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts b/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts index 4483ed6f5a5cc2..c7b8afe1d642a4 100644 --- a/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts +++ b/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts @@ -68,6 +68,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, diff --git a/x-pack/test/security_functional/tests/oidc/url_capture.ts b/x-pack/test/security_functional/tests/oidc/url_capture.ts index 706189dcb5a2a9..7e6a191df4a4a4 100644 --- a/x-pack/test/security_functional/tests/oidc/url_capture.ts +++ b/x-pack/test/security_functional/tests/oidc/url_capture.ts @@ -70,5 +70,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'); + } + }); }); } diff --git a/x-pack/test/security_functional/tests/saml/url_capture.ts b/x-pack/test/security_functional/tests/saml/url_capture.ts index 2b0b8690ebab66..d8c6d08fa60f5d 100644 --- a/x-pack/test/security_functional/tests/saml/url_capture.ts +++ b/x-pack/test/security_functional/tests/saml/url_capture.ts @@ -70,5 +70,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'); + } + }); }); }