Merge pull request #1025 from spotify/nonce-headers

Auth backend: Nonce and header checks

plugins/auth-backend/src/providers/google/provider.test.ts

@@ -14,7 +14,11 @@
  * limitations under the License.
  */
 
-import { GoogleAuthProvider, THOUSAND_DAYS_MS } from './provider';
+import {
+  GoogleAuthProvider,
+  THOUSAND_DAYS_MS,
+  TEN_MINUTES_MS,
+} from './provider';
 import passport from 'passport';
 import express from 'express';
 import * as utils from './../utils';
@@ -52,11 +56,16 @@ describe('GoogleAuthProvider', () => {
   });
 
   describe('start authentication handler', () => {
-    const mockResponse = ({} as unknown) as express.Response;
+    const mockResponse = ({
+      send: jest.fn().mockReturnThis(),
+      status: jest.fn().mockReturnThis(),
+      cookie: jest.fn().mockReturnThis(),
+    } as unknown) as express.Response;
     const mockNext: express.NextFunction = jest.fn();
 
     it('should initiate authenticate request with provided scopes', () => {
       const mockRequest = ({
+        header: () => 'XMLHttpRequest',
         query: {
           scope: 'a,b',
         },
@@ -75,11 +84,36 @@ describe('GoogleAuthProvider', () => {
         scope: 'a,b',
         accessType: 'offline',
         prompt: 'consent',
+        state: expect.any(String),
       });
     });
 
+    it('should set a nonce cookie', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        query: {
+          scope: 'a,b',
+        },
+      } as unknown) as express.Request;
+
+      const googleAuthProvider = new GoogleAuthProvider(
+        googleAuthProviderConfig,
+      );
+      googleAuthProvider.start(mockRequest, mockResponse, mockNext);
+      expect(mockResponse.cookie).toBeCalledTimes(1);
+      expect(mockResponse.cookie).toBeCalledWith(
+        'google-nonce',
+        expect.any(String),
+        expect.objectContaining({
+          maxAge: TEN_MINUTES_MS,
+          path: `/auth/${googleAuthProviderConfig.provider}/handler`,
+        }),
+      );
+    });
+
     it('should throw error if no scopes provided', () => {
       const mockRequest = ({
+        header: () => 'XMLHttpRequest',
         query: {},
       } as unknown) as express.Request;
 
@@ -93,7 +127,9 @@ describe('GoogleAuthProvider', () => {
   });
 
   describe('logout handler', () => {
-    const mockRequest = ({} as unknown) as express.Request;
+    const mockRequest = ({
+      header: () => 'XMLHttpRequest',
+    } as unknown) as express.Request;
 
     it('should perform logout and respond with 200', () => {
       const mockResponse: any = ({
@@ -122,7 +158,6 @@ describe('GoogleAuthProvider', () => {
   });
 
   describe('redirect frame handler', () => {
-    const mockRequest = ({} as unknown) as express.Request;
     const mockResponse: any = ({
       status: jest.fn().mockReturnThis(),
       send: jest.fn().mockReturnThis(),
@@ -131,6 +166,14 @@ describe('GoogleAuthProvider', () => {
     const mockNext: express.NextFunction = jest.fn();
 
     it('should call authenticate and post a response', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        cookies: { 'google-nonce': 'NONCE' },
+        query: {
+          state: 'NONCE',
+        },
+      } as unknown) as express.Request;
+
       const spyPostMessage = jest
         .spyOn(utils, 'postMessageResponse')
         .mockImplementation(() => jest.fn());
@@ -164,6 +207,14 @@ describe('GoogleAuthProvider', () => {
     });
 
     it('should respond with a error message if no refresh token returned', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        cookies: { 'google-nonce': 'NONCE' },
+        query: {
+          state: 'NONCE',
+        },
+      } as unknown) as express.Request;
+
       const spyPassport = jest
         .spyOn(passport, 'authenticate')
         .mockImplementation((_x, callbackFunc) => {
@@ -190,6 +241,14 @@ describe('GoogleAuthProvider', () => {
     });
 
     it('should respond with a error message if auth failed', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        cookies: { 'google-nonce': 'NONCE' },
+        query: {
+          state: 'NONCE',
+        },
+      } as unknown) as express.Request;
+
       const spyPassport = jest
         .spyOn(passport, 'authenticate')
         .mockImplementation((_x, callbackFunc) => {
@@ -214,6 +273,60 @@ describe('GoogleAuthProvider', () => {
         error: new Error('Google auth failed, Error: TokenError'),
       });
     });
+
+    it('should respond with a error message if cookie nonce is missing', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        cookies: {},
+        query: { state: 'NONCE' },
+      } as unknown) as express.Request;
+
+      const googleAuthProvider = new GoogleAuthProvider(
+        googleAuthProviderConfig,
+      );
+
+      googleAuthProvider.frameHandler(mockRequest, mockResponse, mockNext);
+      expect(mockResponse.send).toBeCalledTimes(1);
+      expect(mockResponse.send).toBeCalledWith('Missing nonce');
+      expect(mockResponse.status).toBeCalledTimes(1);
+      expect(mockResponse.status).toBeCalledWith(401);
+    });
+
+    it('should respond with a error message if state nonce is missing', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        cookies: { 'google-nonce': 'NONCE' },
+        query: {},
+      } as unknown) as express.Request;
+
+      const googleAuthProvider = new GoogleAuthProvider(
+        googleAuthProviderConfig,
+      );
+
+      googleAuthProvider.frameHandler(mockRequest, mockResponse, mockNext);
+      expect(mockResponse.send).toBeCalledTimes(1);
+      expect(mockResponse.send).toBeCalledWith('Missing nonce');
+      expect(mockResponse.status).toBeCalledTimes(1);
+      expect(mockResponse.status).toBeCalledWith(401);
+    });
+
+    it('should respond with a error message if nonce mismatch', () => {
+      const mockRequest = ({
+        header: () => 'XMLHttpRequest',
+        cookies: { 'google-nonce': 'NONCA' },
+        query: { state: 'NONCEB' },
+      } as unknown) as express.Request;
+
+      const googleAuthProvider = new GoogleAuthProvider(
+        googleAuthProviderConfig,
+      );
+
+      googleAuthProvider.frameHandler(mockRequest, mockResponse, mockNext);
+      expect(mockResponse.send).toBeCalledTimes(1);
+      expect(mockResponse.send).toBeCalledWith('Invalid nonce');
+      expect(mockResponse.status).toBeCalledTimes(1);
+      expect(mockResponse.status).toBeCalledWith(401);
+    });
   });
 
   describe('strategy handler', () => {
@@ -245,6 +358,7 @@ describe('GoogleAuthProvider', () => {
       it('should respond with a 401', () => {
         const mockRequest = ({
           cookies: jest.fn(),
+          header: () => 'XMLHttpRequest',
         } as unknown) as express.Request;
 
         const googleAuthProvider = new GoogleAuthProvider(
@@ -262,6 +376,7 @@ describe('GoogleAuthProvider', () => {
 
     describe('refresh token cookie, no scope', () => {
       const mockRequest = ({
+        header: () => 'XMLHttpRequest',
         cookies: { 'google-refresh-token': 'REFRESH_TOKEN' },
         query: {},
       } as unknown) as express.Request;
@@ -349,6 +464,7 @@ describe('GoogleAuthProvider', () => {
 
     describe('refresh token cookie and scope', () => {
       const mockRequest = ({
+        header: () => 'XMLHttpRequest',
         cookies: { 'google-refresh-token': 'REFRESH_TOKEN' },
         query: {
           scope: 'a,b',
@@ -387,6 +503,20 @@ describe('GoogleAuthProvider', () => {
           scope: 'a,b',
         });
       });
+
+      it('ensures x-requested-with header', () => {
+        const mockHeaderRequest = ({
+          header: () => 'TEST',
+        } as unknown) as express.Request;
+
+        googleAuthProvider.refresh(mockHeaderRequest, mockResponse);
+        expect(mockResponse.send).toBeCalledTimes(1);
+        expect(mockResponse.send).toBeCalledWith(
+          'Invalid X-Requested-With header',
+        );
+        expect(mockResponse.status).toBeCalledTimes(1);
+        expect(mockResponse.status).toBeCalledWith(401);
+      });
     });
   });
 });

plugins/auth-backend/src/providers/google/provider.ts

@@ -16,6 +16,7 @@
 
 import passport from 'passport';
 import express, { CookieOptions } from 'express';
+import crypto from 'crypto';
 import { Strategy as GoogleStrategy } from 'passport-google-oauth20';
 import refresh from 'passport-oauth2-refresh';
 import {
@@ -23,10 +24,11 @@ import {
   AuthProviderRouteHandlers,
   AuthProviderConfig,
 } from './../types';
-import { postMessageResponse } from './../utils';
+import { postMessageResponse, ensuresXRequestedWith } from './../utils';
 import { InputError } from '@backstage/backend-common';
 
 export const THOUSAND_DAYS_MS = 1000 * 24 * 60 * 60 * 1000;
+export const TEN_MINUTES_MS = 600 * 1000;
 export class GoogleAuthProvider
   implements AuthProvider, AuthProviderRouteHandlers {
   private readonly providerConfig: AuthProviderConfig;
@@ -39,6 +41,19 @@ export class GoogleAuthProvider
     res: express.Response,
     next: express.NextFunction,
   ) {
+    const nonce = crypto.randomBytes(16).toString('base64');
+
+    const options: CookieOptions = {
+      maxAge: TEN_MINUTES_MS,
+      secure: false,
+      sameSite: 'none',
+      domain: 'localhost',
+      path: `/auth/${this.providerConfig.provider}/handler`,
+      httpOnly: true,
+    };
+
+    res.cookie(`${this.providerConfig.provider}-nonce`, nonce, options);
+
     const scope = req.query.scope?.toString() ?? '';
     if (!scope) {
       throw new InputError('missing scope parameter');
@@ -47,6 +62,7 @@ export class GoogleAuthProvider
       scope,
       accessType: 'offline',
       prompt: 'consent',
+      state: nonce,
     })(req, res, next);
   }
 
@@ -55,6 +71,17 @@ export class GoogleAuthProvider
     res: express.Response,
     next: express.NextFunction,
   ) {
+    const cookieNonce = req.cookies[`${this.providerConfig.provider}-nonce`];
+    const stateNonce = req.query.state;
+
+    if (!cookieNonce || !stateNonce) {
+      return res.status(401).send('Missing nonce');
+    }
+
+    if (cookieNonce !== stateNonce) {
+      return res.status(401).send('Invalid nonce');
+    }
+
     return passport.authenticate('google', (err, user) => {
       if (err) {
         return postMessageResponse(res, {
@@ -95,7 +122,11 @@ export class GoogleAuthProvider
     })(req, res, next);
   }
 
-  async logout(_req: express.Request, res: express.Response) {
+  async logout(req: express.Request, res: express.Response) {
+    if (!ensuresXRequestedWith(req)) {
+      return res.status(401).send('Invalid X-Requested-With header');
+    }
+
     const options: CookieOptions = {
       maxAge: 0,
       secure: false,
@@ -110,6 +141,10 @@ export class GoogleAuthProvider
   }
 
   async refresh(req: express.Request, res: express.Response) {
+    if (!ensuresXRequestedWith(req)) {
+      return res.status(401).send('Invalid X-Requested-With header');
+    }
+
     const refreshToken =
       req.cookies[`${this.providerConfig.provider}-refresh-token`];
 

plugins/auth-backend/src/providers/index.ts

@@ -20,11 +20,11 @@ import { ProviderFactories } from './factories';
 
 export const defaultRouter = (provider: AuthProviderRouteHandlers) => {
   const router = Router();
-  router.get('/start', provider.start);
-  router.get('/handler/frame', provider.frameHandler);
-  router.get('/logout', provider.logout);
+  router.get('/start', provider.start.bind(provider));
+  router.get('/handler/frame', provider.frameHandler.bind(provider));
+  router.get('/logout', provider.logout.bind(provider));
   if (provider.refresh) {
-    router.get('/refresh', provider.refresh);
+    router.get('/refresh', provider.refresh.bind(provider));
   }
   return router;
 };

plugins/auth-backend/src/providers/utils.ts

@@ -39,3 +39,12 @@ export const postMessageResponse = (
 </html>
   `);
 };
+
+export const ensuresXRequestedWith = (req: express.Request) => {
+  const requiredHeader = req.header('X-Requested-With');
+
+  if (!requiredHeader || requiredHeader !== 'XMLHttpRequest') {
+    return false;
+  }
+  return true;
+};