Fix infinite redirect loop when multiple cookies are sent (#50452) (#…

…51821)

Cookies are now checked for attributes that match the current
Kibana configuration. Invalid cookies are cleared more reliably.

docs/development/core/server/kibana-plugin-server.md

@@ -115,6 +115,7 @@ The plugin integrates with the core system via lifecycle events: `setup`<!-- -->
 |  [SavedObjectsResolveImportErrorsOptions](./kibana-plugin-server.savedobjectsresolveimporterrorsoptions.md) | Options to control the "resolve import" operation. |
 |  [SavedObjectsUpdateOptions](./kibana-plugin-server.savedobjectsupdateoptions.md) |  |
 |  [SavedObjectsUpdateResponse](./kibana-plugin-server.savedobjectsupdateresponse.md) |  |
+|  [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) | Return type from a function to validate cookie contents. |
 |  [SessionStorage](./kibana-plugin-server.sessionstorage.md) | Provides an interface to store and retrieve data across requests. |
 |  [SessionStorageCookieOptions](./kibana-plugin-server.sessionstoragecookieoptions.md) | Configuration used to create HTTP session storage based on top of cookie mechanism. |
 |  [SessionStorageFactory](./kibana-plugin-server.sessionstoragefactory.md) | SessionStorage factory to bind one to an incoming request |

docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.isvalid.md

@@ -0,0 +1,13 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) &gt; [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md)
+
+## SessionCookieValidationResult.isValid property
+
+Whether the cookie is valid or not.
+
+<b>Signature:</b>
+
+```typescript
+isValid: boolean;
+```

docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md

@@ -0,0 +1,21 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md)
+
+## SessionCookieValidationResult interface
+
+Return type from a function to validate cookie contents.
+
+<b>Signature:</b>
+
+```typescript
+export interface SessionCookieValidationResult 
+```
+
+## Properties
+
+|  Property | Type | Description |
+|  --- | --- | --- |
+|  [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md) | <code>boolean</code> | Whether the cookie is valid or not. |
+|  [path](./kibana-plugin-server.sessioncookievalidationresult.path.md) | <code>string</code> | The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. |
+

docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.path.md

@@ -0,0 +1,13 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) &gt; [path](./kibana-plugin-server.sessioncookievalidationresult.path.md)
+
+## SessionCookieValidationResult.path property
+
+The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it.
+
+<b>Signature:</b>
+
+```typescript
+path?: string;
+```

docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md

@@ -4,7 +4,7 @@
 
 ## SessionStorageCookieOptions.encryptionKey property
 
-A key used to encrypt a cookie value. Should be at least 32 characters long.
+A key used to encrypt a cookie's value. Should be at least 32 characters long.
 
 <b>Signature:</b>
 

docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md

@@ -16,8 +16,8 @@ export interface SessionStorageCookieOptions<T>
 
 |  Property | Type | Description |
 |  --- | --- | --- |
-|  [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | <code>string</code> | A key used to encrypt a cookie value. Should be at least 32 characters long. |
+|  [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | <code>string</code> | A key used to encrypt a cookie's value. Should be at least 32 characters long. |
 |  [isSecure](./kibana-plugin-server.sessionstoragecookieoptions.issecure.md) | <code>boolean</code> | Flag indicating whether the cookie should be sent only via a secure connection. |
 |  [name](./kibana-plugin-server.sessionstoragecookieoptions.name.md) | <code>string</code> | Name of the session cookie. |
-|  [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | <code>(sessionValue: T) =&gt; boolean &#124; Promise&lt;boolean&gt;</code> | Function called to validate a cookie content. |
+|  [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | <code>(sessionValue: T &#124; T[]) =&gt; SessionCookieValidationResult</code> | Function called to validate a cookie's decrypted value. |
 

docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md

@@ -4,10 +4,10 @@
 
 ## SessionStorageCookieOptions.validate property
 
-Function called to validate a cookie content.
+Function called to validate a cookie's decrypted value.
 
 <b>Signature:</b>
 
 ```typescript
-validate: (sessionValue: T) => boolean | Promise<boolean>;
+validate: (sessionValue: T | T[]) => SessionCookieValidationResult;
 ```

src/core/server/http/cookie_session_storage.ts

@@ -34,19 +34,34 @@ export interface SessionStorageCookieOptions<T> {
    */
   name: string;
   /**
-   * A key used to encrypt a cookie value. Should be at least 32 characters long.
+   * A key used to encrypt a cookie's value. Should be at least 32 characters long.
    */
   encryptionKey: string;
   /**
-   * Function called to validate a cookie content.
+   * Function called to validate a cookie's decrypted value.
    */
-  validate: (sessionValue: T) => boolean | Promise<boolean>;
+  validate: (sessionValue: T | T[]) => SessionCookieValidationResult;
   /**
    * Flag indicating whether the cookie should be sent only via a secure connection.
    */
   isSecure: boolean;
 }
 
+/**
+ * Return type from a function to validate cookie contents.
+ * @public
+ */
+export interface SessionCookieValidationResult {
+  /**
+   * Whether the cookie is valid or not.
+   */
+  isValid: boolean;
+  /**
+   * The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it.
+   */
+  path?: string;
+}
+
 class ScopedCookieSessionStorage<T extends Record<string, any>> implements SessionStorage<T> {
   constructor(
     private readonly log: Logger,
@@ -98,15 +113,31 @@ export async function createCookieSessionStorageFactory<T>(
   cookieOptions: SessionStorageCookieOptions<T>,
   basePath?: string
 ): Promise<SessionStorageFactory<T>> {
+  function clearInvalidCookie(req: Request | undefined, path: string = basePath || '/') {
+    // if the cookie did not include the 'path' attribute in the session value, it is a legacy cookie
+    // we will assume that the cookie was created with the current configuration
+    log.debug(`Clearing invalid session cookie`);
+    // need to use Hapi toolkit to clear cookie with defined options
+    if (req) {
+      (req.cookieAuth as any).h.unstate(cookieOptions.name, { path });
+    }
+  }
+
   await server.register({ plugin: hapiAuthCookie });
 
   server.auth.strategy('security-cookie', 'cookie', {
     cookie: cookieOptions.name,
     password: cookieOptions.encryptionKey,
-    validateFunc: async (req, session: T) => ({ valid: await cookieOptions.validate(session) }),
+    validateFunc: async (req, session: T | T[]) => {
+      const result = cookieOptions.validate(session);
+      if (!result.isValid) {
+        clearInvalidCookie(req, result.path);
+      }
+      return { valid: result.isValid };
+    },
     isSecure: cookieOptions.isSecure,
     path: basePath,
-    clearInvalid: true,
+    clearInvalid: false,
     isHttpOnly: true,
     isSameSite: false,
   });

src/core/server/http/cookie_sesson_storage.test.ts

@@ -80,6 +80,7 @@ interface User {
 interface Storage {
   value: User;
   expires: number;
+  path: string;
 }
 
 function retrieveSessionCookie(cookies: string) {
@@ -92,13 +93,21 @@ function retrieveSessionCookie(cookies: string) {
 
 const userData = { id: '42' };
 const sessionDurationMs = 1000;
+const path = '/';
+const sessVal = () => ({ value: userData, expires: Date.now() + sessionDurationMs, path });
 const delay = (ms: number) => new Promise(res => setTimeout(res, ms));
 const cookieOptions = {
   name: 'sid',
   encryptionKey: 'something_at_least_32_characters',
-  validate: (session: Storage) => session.expires > Date.now(),
+  validate: (session: Storage | Storage[]) => {
+    if (Array.isArray(session)) {
+      session = session[0];
+    }
+    const isValid = session.path === path && session.expires > Date.now();
+    return { isValid, path: session.path };
+  },
   isSecure: false,
-  path: '/',
+  path,
 };
 
 describe('Cookie based SessionStorage', () => {
@@ -107,9 +116,9 @@ describe('Cookie based SessionStorage', () => {
       const { server: innerServer, createRouter } = await server.setup(setupDeps);
       const router = createRouter('');
 
-      router.get({ path: '/', validate: false }, (context, req, res) => {
+      router.get({ path, validate: false }, (context, req, res) => {
         const sessionStorage = factory.asScoped(req);
-        sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs });
+        sessionStorage.set(sessVal());
         return res.ok({});
       });
 
@@ -136,6 +145,7 @@ describe('Cookie based SessionStorage', () => {
       expect(sessionCookie.httpOnly).toBe(true);
     });
   });
+
   describe('#get()', () => {
     it('reads from session storage', async () => {
       const { server: innerServer, createRouter } = await server.setup(setupDeps);
@@ -145,7 +155,7 @@ describe('Cookie based SessionStorage', () => {
         const sessionStorage = factory.asScoped(req);
         const sessionValue = await sessionStorage.get();
         if (!sessionValue) {
-          sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs });
+          sessionStorage.set(sessVal());
           return res.ok();
         }
         return res.ok({ body: { value: sessionValue.value } });
@@ -173,6 +183,7 @@ describe('Cookie based SessionStorage', () => {
         .set('Cookie', `${sessionCookie.key}=${sessionCookie.value}`)
         .expect(200, { value: userData });
     });
+
     it('returns null for empty session', async () => {
       const { server: innerServer, createRouter } = await server.setup(setupDeps);
 
@@ -198,7 +209,7 @@ describe('Cookie based SessionStorage', () => {
       expect(cookies).not.toBeDefined();
     });
 
-    it('returns null for invalid session & clean cookies', async () => {
+    it('returns null for invalid session (expired) & clean cookies', async () => {
       const { server: innerServer, createRouter } = await server.setup(setupDeps);
 
       const router = createRouter('');
@@ -208,7 +219,7 @@ describe('Cookie based SessionStorage', () => {
         const sessionStorage = factory.asScoped(req);
         if (!setOnce) {
           setOnce = true;
-          sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs });
+          sessionStorage.set(sessVal());
           return res.ok({ body: { value: userData } });
         }
         const sessionValue = await sessionStorage.get();
@@ -242,6 +253,50 @@ describe('Cookie based SessionStorage', () => {
         'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/',
       ]);
     });
+
+    it('returns null for invalid session (incorrect path) & clean cookies accurately', async () => {
+      const { server: innerServer, createRouter } = await server.setup(setupDeps);
+
+      const router = createRouter('');
+
+      let setOnce = false;
+      router.get({ path: '/', validate: false }, async (context, req, res) => {
+        const sessionStorage = factory.asScoped(req);
+        if (!setOnce) {
+          setOnce = true;
+          sessionStorage.set({ ...sessVal(), path: '/foo' });
+          return res.ok({ body: { value: userData } });
+        }
+        const sessionValue = await sessionStorage.get();
+        return res.ok({ body: { value: sessionValue } });
+      });
+
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        innerServer,
+        cookieOptions
+      );
+      await server.start();
+
+      const response = await supertest(innerServer.listener)
+        .get('/')
+        .expect(200, { value: userData });
+
+      const cookies = response.get('set-cookie');
+      expect(cookies).toBeDefined();
+
+      const sessionCookie = retrieveSessionCookie(cookies[0]);
+      const response2 = await supertest(innerServer.listener)
+        .get('/')
+        .set('Cookie', `${sessionCookie.key}=${sessionCookie.value}`)
+        .expect(200, { value: null });
+
+      const cookies2 = response2.get('set-cookie');
+      expect(cookies2).toEqual([
+        'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/foo',
+      ]);
+    });
+
     // use mocks to simplify test setup
     it('returns null if multiple session cookies are detected.', async () => {
       const mockServer = {
@@ -342,7 +397,7 @@ describe('Cookie based SessionStorage', () => {
           sessionStorage.clear();
           return res.ok({});
         }
-        sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs });
+        sessionStorage.set(sessVal());
         return res.ok({});
       });
 

src/core/server/http/http_server.test.ts

@@ -34,7 +34,7 @@ import { HttpServer } from './http_server';
 const cookieOptions = {
   name: 'sid',
   encryptionKey: 'something_at_least_32_characters',
-  validate: () => true,
+  validate: () => ({ isValid: true }),
   isSecure: false,
 };
 

src/core/server/http/index.ts

@@ -60,6 +60,9 @@ export {
 } from './lifecycle/auth';
 export { OnPostAuthHandler, OnPostAuthToolkit } from './lifecycle/on_post_auth';
 export { SessionStorageFactory, SessionStorage } from './session_storage';
-export { SessionStorageCookieOptions } from './cookie_session_storage';
+export {
+  SessionStorageCookieOptions,
+  SessionCookieValidationResult,
+} from './cookie_session_storage';
 export * from './types';
 export { BasePath, IBasePath } from './base_path_service';

src/core/server/http/integration_tests/core_services.test.ts

@@ -39,7 +39,7 @@ describe('http service', () => {
       const cookieOptions = {
         name: 'sid',
         encryptionKey: 'something_at_least_32_characters',
-        validate: (session: StorageData) => true,
+        validate: () => ({ isValid: true }),
         isSecure: false,
         path: '/',
       };

src/core/server/http/integration_tests/lifecycle.test.ts

@@ -408,7 +408,7 @@ describe('Auth', () => {
   const cookieOptions = {
     name: 'sid',
     encryptionKey: 'something_at_least_32_characters',
-    validate: () => true,
+    validate: () => ({ isValid: true }),
     isSecure: false,
   };
 

src/core/server/index.ts

@@ -117,6 +117,7 @@ export {
   RouteRegistrar,
   SessionStorage,
   SessionStorageCookieOptions,
+  SessionCookieValidationResult,
   SessionStorageFactory,
 } from './http';
 export { Logger, LoggerFactory, LogMeta, LogRecord, LogLevel } from './logging';

src/core/server/server.api.md

@@ -1577,6 +1577,12 @@ export class ScopedClusterClient implements IScopedClusterClient {
     callAsInternalUser(endpoint: string, clientParams?: Record<string, any>, options?: CallAPIOptions): Promise<any>;
     }
 
+// @public
+export interface SessionCookieValidationResult {
+    isValid: boolean;
+    path?: string;
+}
+
 // @public
 export interface SessionStorage<T> {
     clear(): void;
@@ -1589,7 +1595,7 @@ export interface SessionStorageCookieOptions<T> {
     encryptionKey: string;
     isSecure: boolean;
     name: string;
-    validate: (sessionValue: T) => boolean | Promise<boolean>;
+    validate: (sessionValue: T | T[]) => SessionCookieValidationResult;
 }
 
 // @public