diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts index b692a35216..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'; @@ -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); + }); }); }); }); diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 096260e044..cb080e2fd3 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 { @@ -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`]; 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; }; 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; +};