handle mupltiple cookies sent from a browser (elastic#39431)

src/core/server/http/cookie_session_storage.ts

@@ -20,6 +20,7 @@
 import { Request, Server } from 'hapi';
 import hapiAuthCookie from 'hapi-auth-cookie';
 
+import { Logger } from '..';
 import { KibanaRequest, ensureRawRequest } from './router';
 import { SessionStorageFactory, SessionStorage } from './session_storage';
 
@@ -31,11 +32,32 @@ export interface SessionStorageCookieOptions<T> {
 }
 
 class ScopedCookieSessionStorage<T extends Record<string, any>> implements SessionStorage<T> {
-  constructor(private readonly server: Server, private readonly request: Request) {}
+  constructor(
+    private readonly log: Logger,
+    private readonly server: Server,
+    private readonly request: Request
+  ) {}
   public async get(): Promise<T | null> {
     try {
-      return await this.server.auth.test('security-cookie', this.request as Request);
+      const session = await this.server.auth.test('security-cookie', this.request);
+      // A browser can send several cookies, if it's not an array, just return the session value
+      if (!Array.isArray(session)) {
+        return session as T;
+      }
+
+      // If we have an array with one value, we're good also
+      if (session.length === 1) {
+        return session[0] as T;
+      }
+
+      // Otherwise, we have more than one and won't be authing the user because we don't
+      // know which session identifies the actual user. There's potential to change this behavior
+      // to ensure all valid sessions identify the same user, or choose one valid one, but this
+      // is the safest option.
+      this.log.warn(`Found ${session.length} auth sessions when we were only expecting 1.`);
+      return null;
     } catch (error) {
+      this.log.debug(String(error));
       return null;
     }
   }
@@ -55,6 +77,7 @@ class ScopedCookieSessionStorage<T extends Record<string, any>> implements Sessi
  * @param cookieOptions - cookies configuration
  */
 export async function createCookieSessionStorageFactory<T>(
+  log: Logger,
   server: Server,
   cookieOptions: SessionStorageCookieOptions<T>,
   basePath?: string
@@ -74,7 +97,7 @@ export async function createCookieSessionStorageFactory<T>(
 
   return {
     asScoped(request: KibanaRequest) {
-      return new ScopedCookieSessionStorage<T>(server, ensureRawRequest(request));
+      return new ScopedCookieSessionStorage<T>(log, server, ensureRawRequest(request));
     },
   };
 }

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

@@ -22,22 +22,24 @@ import { ByteSizeValue } from '@kbn/config-schema';
 
 import { HttpServer } from './http_server';
 import { HttpConfig } from './http_config';
-import { Router } from './router';
+import { Router, KibanaRequest } from './router';
 import { loggingServiceMock } from '../logging/logging_service.mock';
 
+import { httpServerMock } from './http_server.mocks';
 import { createCookieSessionStorageFactory } from './cookie_session_storage';
 
 let server: HttpServer;
 
-const logger = loggingServiceMock.create();
+let logger: ReturnType<typeof loggingServiceMock.create>;
 const config = {
   host: '127.0.0.1',
   maxPayload: new ByteSizeValue(1024),
   ssl: {},
 } as HttpConfig;
 
 beforeEach(() => {
-  server = new HttpServer(logger.get());
+  logger = loggingServiceMock.create();
+  server = new HttpServer(logger, 'tests');
 });
 
 afterEach(async () => {
@@ -87,7 +89,11 @@ describe('Cookie based SessionStorage', () => {
       const { registerRouter, server: innerServer } = await server.setup(config);
       registerRouter(router);
 
-      const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions);
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        innerServer,
+        cookieOptions
+      );
       await server.start();
 
       const response = await supertest(innerServer.listener)
@@ -107,7 +113,7 @@ describe('Cookie based SessionStorage', () => {
     });
   });
   describe('#get()', () => {
-    it('Should read from session storage', async () => {
+    it('reads from session storage', async () => {
       const router = new Router('');
 
       router.get({ path: '/', validate: false }, async (req, res) => {
@@ -123,7 +129,11 @@ describe('Cookie based SessionStorage', () => {
       const { registerRouter, server: innerServer } = await server.setup(config);
       registerRouter(router);
 
-      const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions);
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        innerServer,
+        cookieOptions
+      );
       await server.start();
 
       const response = await supertest(innerServer.listener)
@@ -141,7 +151,7 @@ describe('Cookie based SessionStorage', () => {
         .set('Cookie', `${sessionCookie.key}=${sessionCookie.value}`)
         .expect(200, { value: userData });
     });
-    it('Should return null for empty session', async () => {
+    it('returns null for empty session', async () => {
       const router = new Router('');
 
       router.get({ path: '/', validate: false }, async (req, res) => {
@@ -153,7 +163,11 @@ describe('Cookie based SessionStorage', () => {
       const { registerRouter, server: innerServer } = await server.setup(config);
       registerRouter(router);
 
-      const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions);
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        innerServer,
+        cookieOptions
+      );
       await server.start();
 
       const response = await supertest(innerServer.listener)
@@ -163,7 +177,8 @@ describe('Cookie based SessionStorage', () => {
       const cookies = response.get('set-cookie');
       expect(cookies).not.toBeDefined();
     });
-    it('Should return null for invalid session & clean cookies', async () => {
+
+    it('returns null for invalid session & clean cookies', async () => {
       const router = new Router('');
 
       let setOnce = false;
@@ -181,7 +196,11 @@ describe('Cookie based SessionStorage', () => {
       const { registerRouter, server: innerServer } = await server.setup(config);
       registerRouter(router);
 
-      const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions);
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        innerServer,
+        cookieOptions
+      );
       await server.start();
 
       const response = await supertest(innerServer.listener)
@@ -204,9 +223,96 @@ describe('Cookie based SessionStorage', () => {
         'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/',
       ]);
     });
+    // use mocks to simplify test setup
+    it('returns null if multiple session cookies are detected.', async () => {
+      const mockServer = {
+        register: jest.fn(),
+        auth: {
+          strategy: jest.fn(),
+          test: jest.fn(() => ['foo', 'bar']),
+        },
+      };
+
+      const mockRequest = httpServerMock.createRawRequest();
+
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        mockServer as any,
+        cookieOptions
+      );
+
+      expect(mockServer.register).toBeCalledTimes(1);
+      expect(mockServer.auth.strategy).toBeCalledTimes(1);
+
+      const session = await factory.asScoped(KibanaRequest.from(mockRequest)).get();
+      expect(session).toBe(null);
+
+      expect(mockServer.auth.test).toBeCalledTimes(1);
+      expect(mockServer.auth.test).toHaveBeenCalledWith('security-cookie', mockRequest);
+
+      expect(loggingServiceMock.collect(logger).warn).toEqual([
+        ['Found 2 auth sessions when we were only expecting 1.'],
+      ]);
+    });
+
+    it('returns session if single session cookie is in an array.', async () => {
+      const mockServer = {
+        register: jest.fn(),
+        auth: {
+          strategy: jest.fn(),
+          test: jest.fn(() => ['foo']),
+        },
+      };
+
+      const mockRequest = httpServerMock.createRawRequest();
+
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        mockServer as any,
+        cookieOptions
+      );
+
+      expect(mockServer.register).toBeCalledTimes(1);
+      expect(mockServer.auth.strategy).toBeCalledTimes(1);
+
+      const session = await factory.asScoped(KibanaRequest.from(mockRequest)).get();
+      expect(session).toBe('foo');
+
+      expect(mockServer.auth.test).toBeCalledTimes(1);
+      expect(mockServer.auth.test).toHaveBeenCalledWith('security-cookie', mockRequest);
+    });
+
+    it('logs the reason of validation function failure.', async () => {
+      const mockServer = {
+        register: jest.fn(),
+        auth: {
+          strategy: jest.fn(),
+          test: () => {
+            throw new Error('Invalid cookie.');
+          },
+        },
+      };
+
+      const mockRequest = httpServerMock.createRawRequest();
+
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        mockServer as any,
+        cookieOptions
+      );
+
+      expect(mockServer.register).toBeCalledTimes(1);
+      expect(mockServer.auth.strategy).toBeCalledTimes(1);
+
+      const session = await factory.asScoped(KibanaRequest.from(mockRequest)).get();
+      expect(session).toBe(null);
+
+      expect(loggingServiceMock.collect(logger).debug).toEqual([['Error: Invalid cookie.']]);
+    });
   });
+
   describe('#clear()', () => {
-    it('Should clear session storage & remove cookies', async () => {
+    it('clears session storage & remove cookies', async () => {
       const router = new Router('');
 
       router.get({ path: '/', validate: false }, async (req, res) => {
@@ -222,7 +328,11 @@ describe('Cookie based SessionStorage', () => {
       const { registerRouter, server: innerServer } = await server.setup(config);
       registerRouter(router);
 
-      const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions);
+      const factory = await createCookieSessionStorageFactory(
+        logger.get(),
+        innerServer,
+        cookieOptions
+      );
       await server.start();
 
       const response = await supertest(innerServer.listener)

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

@@ -19,6 +19,8 @@
 import { Request, ResponseToolkit } from 'hapi';
 import { merge } from 'lodash';
 
+import { KibanaRequest } from './router';
+
 type DeepPartial<T> = T extends any[]
   ? DeepPartialArray<T[number]>
   : T extends object
@@ -52,6 +54,7 @@ function createRawResponseToolkitMock(customization: DeepPartial<ResponseToolkit
 }
 
 export const httpServerMock = {
+  createKibanaRequest: () => KibanaRequest.from(createRawRequestMock()),
   createRawRequest: createRawRequestMock,
   createRawResponseToolkit: createRawResponseToolkitMock,
 };

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

@@ -47,7 +47,7 @@ beforeEach(() => {
     ssl: {},
   } as HttpConfig;
 
-  server = new HttpServer(logger.get());
+  server = new HttpServer(logger, 'tests');
 });
 
 afterEach(async () => {

src/core/server/http/http_server.ts

@@ -19,7 +19,7 @@
 
 import { Request, Server } from 'hapi';
 
-import { Logger } from '../logging';
+import { Logger, LoggerFactory } from '../logging';
 import { HttpConfig } from './http_config';
 import { createServer, getServerOptions } from './http_tools';
 import { adoptToHapiAuthFormat, AuthenticationHandler } from './lifecycle/auth';
@@ -83,12 +83,14 @@ export class HttpServer {
   private registeredRouters = new Set<Router>();
   private authRegistered = false;
 
+  private readonly log: Logger;
   private readonly authState: AuthStateStorage;
   private readonly authHeaders: AuthHeadersStorage;
 
-  constructor(private readonly log: Logger) {
+  constructor(private readonly logger: LoggerFactory, private readonly name: string) {
     this.authState = new AuthStateStorage(() => this.authRegistered);
     this.authHeaders = new AuthHeadersStorage();
+    this.log = logger.get('http', 'server', name);
   }
 
   public isListening() {
@@ -221,6 +223,7 @@ export class HttpServer {
     this.authRegistered = true;
 
     const sessionStorageFactory = await createCookieSessionStorageFactory<T>(
+      this.logger.get('http', 'server', this.name, 'cookie-session-storage'),
       this.server,
       cookieOptions,
       basePath

src/core/server/http/http_service.ts

@@ -58,7 +58,7 @@ export class HttpService implements CoreService<HttpServiceSetup, HttpServiceSta
       .atPath<HttpConfigType>('server')
       .pipe(map(rawConfig => new HttpConfig(rawConfig, coreContext.env)));
 
-    this.httpServer = new HttpServer(coreContext.logger.get('http', 'server'));
+    this.httpServer = new HttpServer(coreContext.logger, 'Kibana');
     this.httpsRedirectServer = new HttpsRedirectServer(
       coreContext.logger.get('http', 'redirect', 'server')
     );
@@ -148,9 +148,8 @@ export class HttpService implements CoreService<HttpServiceSetup, HttpServiceSta
 
     const baseConfig = await this.config$.pipe(first()).toPromise();
     const finalConfig = { ...baseConfig, ...cfg };
-    const log = this.logger.get('http', `server:${port}`);
 
-    const httpServer = new HttpServer(log);
+    const httpServer = new HttpServer(this.logger, `secondary server:${port}`);
     const httpSetup = await httpServer.setup(finalConfig);
     this.secondaryServers.set(port, httpServer);
     return httpSetup;
@@ -175,7 +174,7 @@ export class HttpService implements CoreService<HttpServiceSetup, HttpServiceSta
 
   private async runNotReadyServer(config: HttpConfig) {
     this.log.debug('starting NotReady server');
-    const httpServer = new HttpServer(this.log);
+    const httpServer = new HttpServer(this.logger, 'NotReady');
     const { server } = await httpServer.setup(config);
     this.notReadyServer = server;
     // use hapi server while Kibana ResponseFactory doesn't allow specifying custom headers