From 77d1e7382d660d902b49139bb52f4e9315f2d608 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Tue, 26 May 2020 14:04:46 +0200 Subject: [PATCH 1/4] add check for x-requested-with header --- .../src/providers/google/provider.test.ts | 32 +++++++++++++++++-- .../src/providers/google/provider.ts | 12 +++++-- plugins/auth-backend/src/providers/utils.ts | 9 ++++++ 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts index b692a35216..874b89d92c 100644 --- a/plugins/auth-backend/src/providers/google/provider.test.ts +++ b/plugins/auth-backend/src/providers/google/provider.test.ts @@ -52,11 +52,15 @@ describe('GoogleAuthProvider', () => { }); describe('start authentication handler', () => { - const mockResponse = ({} as unknown) as express.Response; + const mockResponse = ({ + send: jest.fn().mockReturnThis(), + status: 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', }, @@ -80,6 +84,7 @@ describe('GoogleAuthProvider', () => { it('should throw error if no scopes provided', () => { const mockRequest = ({ + header: () => 'XMLHttpRequest', query: {}, } as unknown) as express.Request; @@ -93,7 +98,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 +129,9 @@ describe('GoogleAuthProvider', () => { }); describe('redirect frame handler', () => { - const mockRequest = ({} as unknown) as express.Request; + const mockRequest = ({ + header: () => 'XMLHttpRequest', + } as unknown) as express.Request; const mockResponse: any = ({ status: jest.fn().mockReturnThis(), send: jest.fn().mockReturnThis(), @@ -245,6 +254,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 +272,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 +360,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 +399,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); + }); }); }); }); diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 096260e044..35c7b3f6e2 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -23,7 +23,7 @@ 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; @@ -95,7 +95,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 +114,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`]; diff --git a/plugins/auth-backend/src/providers/utils.ts b/plugins/auth-backend/src/providers/utils.ts index 7fb906ad7c..83229e55d4 100644 --- a/plugins/auth-backend/src/providers/utils.ts +++ b/plugins/auth-backend/src/providers/utils.ts @@ -39,3 +39,12 @@ export const postMessageResponse = ( `); }; + +export const ensuresXRequestedWith = (req: express.Request) => { + const requiredHeader = req.header('X-Requested-With'); + + if (!requiredHeader || requiredHeader !== 'XMLHttpRequest') { + return false; + } + return true; +}; From fccdb7e3ef4da63e158841cc735c07ad9c54b621 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Tue, 26 May 2020 14:55:24 +0200 Subject: [PATCH 2/4] add nonce check for the google auth dance in popup --- .../src/providers/google/provider.ts | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 35c7b3f6e2..6850c527f1 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/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 { @@ -27,6 +28,7 @@ import { postMessageResponse, ensuresXRequestedWith } from './../utils'; import { InputError } from '@backstage/backend-common'; export const THOUSAND_DAYS_MS = 1000 * 24 * 60 * 60 * 1000; +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/google/handler`, + httpOnly: true, + }; + + res.cookie(`google-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[`google-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, { @@ -79,15 +106,11 @@ export class GoogleAuthProvider secure: false, sameSite: 'none', domain: 'localhost', - path: `/auth/${this.providerConfig.provider}`, + path: `/auth/google`, httpOnly: true, }; - res.cookie( - `${this.providerConfig.provider}-refresh-token`, - refreshToken, - options, - ); + res.cookie(`google-refresh-token`, refreshToken, options); return postMessageResponse(res, { type: 'auth-result', payload: user, @@ -105,11 +128,11 @@ export class GoogleAuthProvider secure: false, sameSite: 'none', domain: 'localhost', - path: `/auth/${this.providerConfig.provider}`, + path: `/auth/google`, httpOnly: true, }; - res.cookie(`${this.providerConfig.provider}-refresh-token`, '', options); + res.cookie(`google-refresh-token`, '', options); return res.send('logout!'); } @@ -118,8 +141,7 @@ export class GoogleAuthProvider return res.status(401).send('Invalid X-Requested-With header'); } - const refreshToken = - req.cookies[`${this.providerConfig.provider}-refresh-token`]; + const refreshToken = req.cookies[`google-refresh-token`]; if (!refreshToken) { return res.status(401).send('Missing session cookie'); From 12d41d0f0a51b1ffa97ee95a70aff818c09ef7b7 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Tue, 26 May 2020 16:43:08 +0200 Subject: [PATCH 3/4] use providerFactory.provider instead of hardcoding google --- .../src/providers/google/provider.ts | 21 ++++++++++++------- plugins/auth-backend/src/providers/index.ts | 8 +++---- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 6850c527f1..a631f86fa6 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -48,11 +48,11 @@ export class GoogleAuthProvider secure: false, sameSite: 'none', domain: 'localhost', - path: `/auth/google/handler`, + path: `/auth/${this.providerConfig.provider}/handler`, httpOnly: true, }; - res.cookie(`google-nonce`, nonce, options); + res.cookie(`${this.providerConfig.provider}-nonce`, nonce, options); const scope = req.query.scope?.toString() ?? ''; if (!scope) { @@ -71,7 +71,7 @@ export class GoogleAuthProvider res: express.Response, next: express.NextFunction, ) { - const cookieNonce = req.cookies[`google-nonce`]; + const cookieNonce = req.cookies[`${this.providerConfig.provider}-nonce`]; const stateNonce = req.query.state; if (!cookieNonce || !stateNonce) { @@ -106,11 +106,15 @@ export class GoogleAuthProvider secure: false, sameSite: 'none', domain: 'localhost', - path: `/auth/google`, + path: `/auth/${this.providerConfig.provider}`, httpOnly: true, }; - res.cookie(`google-refresh-token`, refreshToken, options); + res.cookie( + `${this.providerConfig.provider}-refresh-token`, + refreshToken, + options, + ); return postMessageResponse(res, { type: 'auth-result', payload: user, @@ -128,11 +132,11 @@ export class GoogleAuthProvider secure: false, sameSite: 'none', domain: 'localhost', - path: `/auth/google`, + path: `/auth/${this.providerConfig.provider}`, httpOnly: true, }; - res.cookie(`google-refresh-token`, '', options); + res.cookie(`${this.providerConfig.provider}-refresh-token`, '', options); return res.send('logout!'); } @@ -141,7 +145,8 @@ export class GoogleAuthProvider return res.status(401).send('Invalid X-Requested-With header'); } - const refreshToken = req.cookies[`google-refresh-token`]; + const refreshToken = + req.cookies[`${this.providerConfig.provider}-refresh-token`]; if (!refreshToken) { return res.status(401).send('Missing session cookie'); diff --git a/plugins/auth-backend/src/providers/index.ts b/plugins/auth-backend/src/providers/index.ts index 68fe61c7a6..1b33391c27 100644 --- a/plugins/auth-backend/src/providers/index.ts +++ b/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; }; From 86518a0210e502fd152d2e886eb9893ba9ba673f Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Tue, 26 May 2020 17:48:38 +0200 Subject: [PATCH 4/4] add test cases for nonce checks --- .../src/providers/google/provider.test.ts | 112 +++++++++++++++++- .../src/providers/google/provider.ts | 2 +- 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts index 874b89d92c..327bf0260b 100644 --- a/plugins/auth-backend/src/providers/google/provider.test.ts +++ b/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'; @@ -55,6 +59,7 @@ describe('GoogleAuthProvider', () => { 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(); @@ -79,9 +84,33 @@ 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', @@ -129,9 +158,6 @@ describe('GoogleAuthProvider', () => { }); describe('redirect frame handler', () => { - const mockRequest = ({ - header: () => 'XMLHttpRequest', - } as unknown) as express.Request; const mockResponse: any = ({ status: jest.fn().mockReturnThis(), send: jest.fn().mockReturnThis(), @@ -140,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()); @@ -173,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) => { @@ -199,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) => { @@ -223,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', () => { diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index a631f86fa6..cb080e2fd3 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -28,7 +28,7 @@ import { postMessageResponse, ensuresXRequestedWith } from './../utils'; import { InputError } from '@backstage/backend-common'; export const THOUSAND_DAYS_MS = 1000 * 24 * 60 * 60 * 1000; -const TEN_MINUTES_MS = 600 * 1000; +export const TEN_MINUTES_MS = 600 * 1000; export class GoogleAuthProvider implements AuthProvider, AuthProviderRouteHandlers { private readonly providerConfig: AuthProviderConfig;