Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support deep links inside of RelayState for SAML IdP initiated login. #69401

Merged
merged 6 commits into from
Jun 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions x-pack/dev-tools/jest/setup/polyfills.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,7 @@ const MutationObserver = require('mutation-observer');
Object.defineProperty(window, 'MutationObserver', { value: MutationObserver });

require('whatwg-fetch');
const URL = { createObjectURL: () => '' };
Object.defineProperty(window, 'URL', { value: URL });

if (!global.URL.hasOwnProperty('createObjectURL')) {
Object.defineProperty(global.URL, 'createObjectURL', { value: () => '' });
}
88 changes: 88 additions & 0 deletions x-pack/plugins/security/common/is_internal_url.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { isInternalURL } from './is_internal_url';

describe('isInternalURL', () => {
describe('with basePath defined', () => {
const basePath = '/iqf';

it('should return `true `if URL includes hash fragment', () => {
const href = `${basePath}/app/kibana#/discover/New-Saved-Search`;
expect(isInternalURL(href, basePath)).toBe(true);
});

it('should return `false` if URL includes a protocol/hostname', () => {
const href = `https://example.com${basePath}/app/kibana`;
expect(isInternalURL(href, basePath)).toBe(false);
});

it('should return `false` if URL includes a port', () => {
const href = `http://localhost:5601${basePath}/app/kibana`;
expect(isInternalURL(href, basePath)).toBe(false);
});

it('should return `false` if URL does not specify protocol', () => {
const hrefWithTwoSlashes = `/${basePath}/app/kibana`;
expect(isInternalURL(hrefWithTwoSlashes)).toBe(false);

const hrefWithThreeSlashes = `//${basePath}/app/kibana`;
expect(isInternalURL(hrefWithThreeSlashes)).toBe(false);
});

it('should return `true` if URL starts with a basepath', () => {
for (const href of [basePath, `${basePath}/`, `${basePath}/login`, `${basePath}/login/`]) {
expect(isInternalURL(href, basePath)).toBe(true);
}
});

it('should return `false` if URL does not start with basePath', () => {
for (const href of [
'/notbasepath/app/kibana',
`${basePath}_/login`,
basePath.slice(1),
`${basePath.slice(1)}/app/kibana`,
]) {
expect(isInternalURL(href, basePath)).toBe(false);
}
});

it('should return `true` if relative path does not escape base path', () => {
const href = `${basePath}/app/kibana/../../management`;
expect(isInternalURL(href, basePath)).toBe(true);
});

it('should return `false` if relative path escapes base path', () => {
const href = `${basePath}/app/kibana/../../../management`;
expect(isInternalURL(href, basePath)).toBe(false);
});
});

describe('without basePath defined', () => {
it('should return `true `if URL includes hash fragment', () => {
const href = '/app/kibana#/discover/New-Saved-Search';
expect(isInternalURL(href)).toBe(true);
});

it('should return `false` if URL includes a protocol/hostname', () => {
const href = 'https://example.com/app/kibana';
expect(isInternalURL(href)).toBe(false);
});

it('should return `false` if URL includes a port', () => {
const href = 'http://localhost:5601/app/kibana';
expect(isInternalURL(href)).toBe(false);
});

it('should return `false` if URL does not specify protocol', () => {
const hrefWithTwoSlashes = `//app/kibana`;
expect(isInternalURL(hrefWithTwoSlashes)).toBe(false);

const hrefWithThreeSlashes = `///app/kibana`;
expect(isInternalURL(hrefWithThreeSlashes)).toBe(false);
});
});
});
38 changes: 38 additions & 0 deletions x-pack/plugins/security/common/is_internal_url.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { parse } from 'url';

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) {
return false;
}

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;
Copy link
Member Author

@azasypkin azasypkin Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: at some point we may want to migrate from legacy url.parse API to this WHATWG URL API completely, but I couldn't find a reasonable way to figure out whether URL is relative or absolute just using this new API. The only option I can think of is something like this:

const normalizedURL = new URL(url, 'https://localhost');
const isAbsolute =
  // `true` if `url` has its own origin different from our "marker" origin
  normalizedURL.origin !== 'https://localhost' ||
  // `true` if `url` has its own origin and it's equal to our "marker" origin
  url.trim().toLowerCase().startsWith('https://localhost');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to get this to work locally. When trying to login with a base path, I'm presented with an error:

url__WEBPACK_IMPORTED_MODULE_0__.URL is not a constructor

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, nice find! That's because URL got into import somehow. It's should be a global instead. But now I'm worried it may not be supported by IE11 - checking..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've pushed the fix (moved URL out of import as it should be). I see we use polyfill from CoreJS for the URL so it should work in IE 11. I'll manually pick that fix to 7.8 to check, cause master doesn't work in IE anymore 😢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked on 7.8 - the URL polyfill works fine in IE 11 :phew:

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}/`))
);
}

return true;
}
20 changes: 3 additions & 17 deletions x-pack/plugins/security/common/parse_next.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { parse } from 'url';
import { isInternalURL } from './is_internal_url';

export function parseNext(href: string, basePath = '') {
const { query, hash } = parse(href, true);
Expand All @@ -20,23 +21,8 @@ export function parseNext(href: string, basePath = '') {
}

// validate that `next` is not attempting a redirect to somewhere
// outside of this Kibana install
const { protocol, hostname, port, pathname } = parse(
next,
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) {
return `${basePath}/`;
}

if (!String(pathname).startsWith(basePath)) {
// outside of this Kibana install.
if (!isInternalURL(next, basePath)) {
return `${basePath}/`;
}

Expand Down
Loading