Review#1: handle review feedback.

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

@@ -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);
+    });
+  });
+});

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

@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { parse } from 'url';
+import { parse, URL } from 'url';
 
 export function isInternalURL(url: string, basePath = '') {
   const { protocol, hostname, port, pathname } = parse(
@@ -22,5 +22,17 @@ export function isInternalURL(url: string, basePath = '') {
     return false;
   }
 
-  return String(pathname).startsWith(basePath);
+  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}/`))
+    );
+  }
+
+  return true;
 }

x-pack/plugins/security/server/authentication/providers/saml.test.ts

@@ -110,6 +110,51 @@ describe('SAMLAuthenticationProvider', () => {
       );
     });
 
+    it('gets token and redirects user to the requested URL if SAML Response is valid ignoring Relay State.', async () => {
+      const request = httpServerMock.createKibanaRequest();
+
+      mockOptions.client.callAsInternalUser.mockResolvedValue({
+        username: 'user',
+        access_token: 'some-token',
+        refresh_token: 'some-refresh-token',
+      });
+
+      provider = new SAMLAuthenticationProvider(mockOptions, {
+        realm: 'test-realm',
+        maxRedirectURLSize: new ByteSizeValue(100),
+        useRelayStateDeepLink: true,
+      });
+      await expect(
+        provider.login(
+          request,
+          {
+            type: SAMLLogin.LoginWithSAMLResponse,
+            samlResponse: 'saml-response-xml',
+            relayState: `${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
+          },
+          {
+            requestId: 'some-request-id',
+            redirectURL: '/test-base-path/some-path#some-app',
+            realm: 'test-realm',
+          }
+        )
+      ).resolves.toEqual(
+        AuthenticationResult.redirectTo('/test-base-path/some-path#some-app', {
+          state: {
+            username: 'user',
+            accessToken: 'some-token',
+            refreshToken: 'some-refresh-token',
+            realm: 'test-realm',
+          },
+        })
+      );
+
+      expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith(
+        'shield.samlAuthenticate',
+        { body: { ids: ['some-request-id'], content: 'saml-response-xml', realm: 'test-realm' } }
+      );
+    });
+
     it('fails if SAML Response payload is presented but state does not contain SAML Request token.', async () => {
       const request = httpServerMock.createKibanaRequest();
 
@@ -178,6 +223,45 @@ describe('SAMLAuthenticationProvider', () => {
       );
     });
 
+    it('redirects to the default location if state contains empty redirect URL ignoring Relay State.', async () => {
+      const request = httpServerMock.createKibanaRequest();
+
+      mockOptions.client.callAsInternalUser.mockResolvedValue({
+        access_token: 'user-initiated-login-token',
+        refresh_token: 'user-initiated-login-refresh-token',
+      });
+
+      provider = new SAMLAuthenticationProvider(mockOptions, {
+        realm: 'test-realm',
+        maxRedirectURLSize: new ByteSizeValue(100),
+        useRelayStateDeepLink: true,
+      });
+      await expect(
+        provider.login(
+          request,
+          {
+            type: SAMLLogin.LoginWithSAMLResponse,
+            samlResponse: 'saml-response-xml',
+            relayState: `${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
+          },
+          { requestId: 'some-request-id', redirectURL: '', realm: 'test-realm' }
+        )
+      ).resolves.toEqual(
+        AuthenticationResult.redirectTo('/base-path/', {
+          state: {
+            accessToken: 'user-initiated-login-token',
+            refreshToken: 'user-initiated-login-refresh-token',
+            realm: 'test-realm',
+          },
+        })
+      );
+
+      expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith(
+        'shield.samlAuthenticate',
+        { body: { ids: ['some-request-id'], content: 'saml-response-xml', realm: 'test-realm' } }
+      );
+    });
+
     it('redirects to the default location if state is not presented.', async () => {
       const request = httpServerMock.createKibanaRequest();
 
@@ -233,7 +317,6 @@ describe('SAMLAuthenticationProvider', () => {
 
     describe('IdP initiated login', () => {
       beforeEach(() => {
-        mockOptions = mockAuthenticationProviderOptions({ name: 'saml' });
         mockOptions.basePath.get.mockReturnValue(mockOptions.basePath.serverBasePath);
 
         const mockScopedClusterClient = elasticsearchServiceMock.createScopedClusterClient();
@@ -266,10 +349,10 @@ describe('SAMLAuthenticationProvider', () => {
           provider.login(httpServerMock.createKibanaRequest({ headers: {} }), {
             type: SAMLLogin.LoginWithSAMLResponse,
             samlResponse: 'saml-response-xml',
-            relayState: '/mock-server-basepath/app/some-app#some-deep-link',
+            relayState: `${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
           })
         ).resolves.toEqual(
-          AuthenticationResult.redirectTo('/mock-server-basepath/', {
+          AuthenticationResult.redirectTo(`${mockOptions.basePath.serverBasePath}/`, {
             state: {
               username: 'user',
               accessToken: 'valid-token',
@@ -287,7 +370,7 @@ describe('SAMLAuthenticationProvider', () => {
             samlResponse: 'saml-response-xml',
           })
         ).resolves.toEqual(
-          AuthenticationResult.redirectTo('/mock-server-basepath/', {
+          AuthenticationResult.redirectTo(`${mockOptions.basePath.serverBasePath}/`, {
             state: {
               username: 'user',
               accessToken: 'valid-token',
@@ -303,10 +386,10 @@ describe('SAMLAuthenticationProvider', () => {
           provider.login(httpServerMock.createKibanaRequest({ headers: {} }), {
             type: SAMLLogin.LoginWithSAMLResponse,
             samlResponse: 'saml-response-xml',
-            relayState: 'https://evil.com/mock-server-basepath/app/some-app#some-deep-link',
+            relayState: `https://evil.com${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
           })
         ).resolves.toEqual(
-          AuthenticationResult.redirectTo('/mock-server-basepath/', {
+          AuthenticationResult.redirectTo(`${mockOptions.basePath.serverBasePath}/`, {
             state: {
               username: 'user',
               accessToken: 'valid-token',
@@ -322,10 +405,10 @@ describe('SAMLAuthenticationProvider', () => {
           provider.login(httpServerMock.createKibanaRequest({ headers: {} }), {
             type: SAMLLogin.LoginWithSAMLResponse,
             samlResponse: 'saml-response-xml',
-            relayState: '//mock-server-basepath/app/some-app#some-deep-link',
+            relayState: `//${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
           })
         ).resolves.toEqual(
-          AuthenticationResult.redirectTo('/mock-server-basepath/', {
+          AuthenticationResult.redirectTo(`${mockOptions.basePath.serverBasePath}/`, {
             state: {
               username: 'user',
               accessToken: 'valid-token',
@@ -341,17 +424,20 @@ describe('SAMLAuthenticationProvider', () => {
           provider.login(httpServerMock.createKibanaRequest({ headers: {} }), {
             type: SAMLLogin.LoginWithSAMLResponse,
             samlResponse: 'saml-response-xml',
-            relayState: '/mock-server-basepath/app/some-app#some-deep-link',
+            relayState: `${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
           })
         ).resolves.toEqual(
-          AuthenticationResult.redirectTo('/mock-server-basepath/app/some-app#some-deep-link', {
-            state: {
-              username: 'user',
-              accessToken: 'valid-token',
-              refreshToken: 'valid-refresh-token',
-              realm: 'test-realm',
-            },
-          })
+          AuthenticationResult.redirectTo(
+            `${mockOptions.basePath.serverBasePath}/app/some-app#some-deep-link`,
+            {
+              state: {
+                username: 'user',
+                accessToken: 'valid-token',
+                refreshToken: 'valid-refresh-token',
+                realm: 'test-realm',
+              },
+            }
+          )
         );
       });
     });

x-pack/plugins/security/server/authentication/providers/saml.ts

@@ -349,16 +349,29 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
         },
       });
 
+      // IdP can pass `RelayState` with the deep link in Kibana during IdP initiated login and
+      // depending on the configuration we may need to redirect user to this URL.
+      let redirectURLFromRelayState;
+      if (isIdPInitiatedLogin && relayState) {
+        if (!this.useRelayStateDeepLink) {
+          this.options.logger.debug(
+            `"RelayState" is provided, but deep links support is not enabled for "${this.type}/${this.options.name}" provider.`
+          );
+        } else if (!isInternalURL(relayState, this.options.basePath.serverBasePath)) {
+          this.options.logger.debug(
+            `"RelayState" is provided, but it is not a valid Kibana internal URL.`
+          );
+        } else {
+          this.options.logger.debug(
+            `User will be redirected to the Kibana internal URL specified in "RelayState".`
+          );
+          redirectURLFromRelayState = relayState;
+        }
+      }
+
       this.logger.debug('Login has been performed with SAML response.');
       return AuthenticationResult.redirectTo(
-        // IdP can pass `RelayState` with the deep link in Kibana during IdP initiated login and
-        // depending on the configuration we may need to redirect user to this link.
-        isIdPInitiatedLogin &&
-          relayState &&
-          this.useRelayStateDeepLink &&
-          isInternalURL(relayState, this.options.basePath.serverBasePath)
-          ? relayState
-          : stateRedirectURL || `${this.options.basePath.get(request)}/`,
+        redirectURLFromRelayState || stateRedirectURL || `${this.options.basePath.get(request)}/`,
         { state: { username, accessToken, refreshToken, realm: this.realm } }
       );
     } catch (err) {