From 7052db3ac75d0214a0bd9be21820928b3be97bbf Mon Sep 17 00:00:00 2001 From: Brock Anderson Date: Wed, 20 Nov 2024 17:29:20 -0800 Subject: [PATCH 1/6] updated the POST /auth/refresh endpoint to confirm that the 'refreshToken' parameter is valid --- .../src/v1/routes/public-auth-routes.spec.ts | 94 +++++-------------- backend/src/v1/routes/public-auth-routes.ts | 15 ++- 2 files changed, 34 insertions(+), 75 deletions(-) diff --git a/backend/src/v1/routes/public-auth-routes.spec.ts b/backend/src/v1/routes/public-auth-routes.spec.ts index 43987c995..8639e48c0 100644 --- a/backend/src/v1/routes/public-auth-routes.spec.ts +++ b/backend/src/v1/routes/public-auth-routes.spec.ts @@ -18,7 +18,7 @@ jest.mock('../services/public-auth-service', () => { const actualPublicAuth = jest.requireActual( '../services/public-auth-service', ); - const mockedPublicAuth = jest.genMockFromModule( + const mockedPublicAuth = jest.createMockFromModule( '../services/public-auth-service', ) as any; @@ -61,16 +61,6 @@ jest.mock('passport', () => ({ jest.fn((req, res, next) => mockAuthenticate(req, res, next)), })); -const mockExists = jest.fn(); -const mockValidationResult = jest.fn(); -jest.mock('express-validator', () => ({ - ...jest.requireActual('express-validator'), - body: (...args) => ({ - exists: () => jest.fn((req, res, next) => mockExists(req, res, next)), - }), - validationResult: jest.fn(() => mockValidationResult()), -})); - const mockLogout = jest.fn(); const mockRequest = { logout: jest.fn(mockLogout), @@ -79,6 +69,7 @@ const mockRequest = { }, user: { idToken: 'ey....', + jwtFrontend: '567ghi', }, }; @@ -264,7 +255,6 @@ describe('public-auth-routes', () => { describe('/refresh', () => { it('return 401 if token company details are missing', () => { - mockExists.mockImplementation((req, res, next) => next()); app.use((req: any, res, next) => { req.session = mockRequest.session; req.user = mockRequest.user; @@ -280,36 +270,31 @@ describe('public-auth-routes', () => { expect(res.body.error).toBe(MISSING_COMPANY_DETAILS_ERROR); }); }); - it('should handle validation errors', () => { - mockExists.mockImplementation((req, res, next) => next()); - mockValidationResult.mockImplementation(() => { - return { - isEmpty: () => false, - array: () => [1, 2], - }; - }); + it("if user isn't initialized, fail with 401", () => { + mockIsTokenExpired.mockReturnValue(false); + mockIsRenewable.mockReturnValue(true); app.use((req: any, res, next) => { req.session = { ...mockRequest.session, companyDetails: { id: 1 } }; req.user = mockRequest.user; req.logout = mockRequest.logout; + req.body = { refreshToken: mockRequest.user.jwtFrontend }; next(); }); app.use('/auth', router); return request(app) .post('/auth/refresh') - .expect(400) + .expect(401) .expect((res) => { - expect(res.body.errors).toEqual([1, 2]); + expect(res.body).toEqual({ + error: 'Unauthorized', + error_description: 'Not logged in', + }); }); }); it('should return unauthorized when jwt or refresh token is not found', () => { - mockExists.mockImplementation((req, res, next) => next()); - mockValidationResult.mockImplementation(() => { - return { - isEmpty: () => true, - }; - }); + mockIsTokenExpired.mockReturnValue(false); + mockIsRenewable.mockReturnValue(false); app.use((req: any, res, next) => { req.session = { ...mockRequest.session, companyDetails: { id: 1 } }; req.user = mockRequest.user; @@ -318,29 +303,14 @@ describe('public-auth-routes', () => { }); app.use('/auth', router); - return request(app) - .post('/auth/refresh') - .expect(401) - .expect((res) => { - expect(res.body).toEqual({ - error: 'Unauthorized', - error_description: 'Not logged in', - }); - }); + return request(app).post('/auth/refresh').expect(401); }); it('should renew the token if renewable', async () => { const mockCorrelationId = 12; const mockFrontendToken = 'jwt_value'; - mockExists.mockImplementation((req, res, next) => next()); - mockValidationResult.mockImplementation(() => { - return { - isEmpty: () => true, - }; - }); mockIsTokenExpired.mockReturnValue(true); mockGenerateFrontendToken.mockReturnValue(mockFrontendToken); mockIsRenewable.mockReturnValue(true); - mockRenew.mockResolvedValue({ jwt: 1, refreshToken: 1 }); mockRenewBackendAndFrontendTokens.mockImplementation((req, res) => res.status(200).json({ jwtFrontend: mockFrontendToken, @@ -356,6 +326,7 @@ describe('public-auth-routes', () => { }; req.user = { ...mockRequest.user, jwt: 'jwt', refreshToken: 'jwt' }; req.logout = mockRequest.logout; + req.body = { refreshToken: mockRequest.user.jwtFrontend }; next(); }); app.use('/auth', router); @@ -371,12 +342,6 @@ describe('public-auth-routes', () => { }); it('should return unauthorized when it fails to renew tokens', () => { const mockCorrelationId = 12; - mockExists.mockImplementation((req, res, next) => next()); - mockValidationResult.mockImplementation(() => { - return { - isEmpty: () => true, - }; - }); mockIsTokenExpired.mockReturnValue(true); mockGenerateFrontendToken.mockReturnValue('jwt_value'); mockIsRenewable.mockReturnValue(true); @@ -399,13 +364,7 @@ describe('public-auth-routes', () => { return request(app).post('/auth/refresh').expect(401); }); - it('should return unauthorized if token is not expired', () => { - mockExists.mockImplementation((req, res, next) => next()); - mockValidationResult.mockImplementation(() => { - return { - isEmpty: () => true, - }; - }); + it('should return unauthorized if token is not renewable', () => { mockIsTokenExpired.mockReturnValue(true); mockIsRenewable.mockReturnValue(false); @@ -417,21 +376,17 @@ describe('public-auth-routes', () => { }; req.user = { ...mockRequest.user, jwt: 'jwt', refreshToken: 'jwt' }; req.logout = mockRequest.logout; + req.body = { refreshToken: mockRequest.user.jwtFrontend }; next(); }); app.use('/auth', router); return request(app).post('/auth/refresh').expect(401); }); - it('should return with current user tokens', () => { - mockExists.mockImplementation((req, res, next) => next()); - mockValidationResult.mockImplementation(() => { - return { - isEmpty: () => true, - }; - }); - mockIsTokenExpired.mockReturnValue(false); + it('should return 200 with current user token', () => { + mockIsTokenExpired.mockReturnValue(false); + mockIsRenewable.mockReturnValue(false); app.use((req: any, res, next) => { req.session = { ...mockRequest.session, @@ -440,21 +395,21 @@ describe('public-auth-routes', () => { }; req.user = { ...mockRequest.user, - jwtFrontend: 'jwt', jwt: 'jwt', refreshToken: 'jwt', }; req.logout = mockRequest.logout; + req.body = { refreshToken: mockRequest.user.jwtFrontend }; next(); }); app.use('/auth', router); - return request(app) .post('/auth/refresh') + .set('Accept', 'application/json') .expect(200) .expect((res) => { expect(res.body).toEqual({ - jwtFrontend: 'jwt', + jwtFrontend: mockRequest.user.jwtFrontend, correlationID: 12, }); }); @@ -463,7 +418,6 @@ describe('public-auth-routes', () => { describe('/token', () => { it('return 401 if token company details are missing', () => { mockRefreshJWT.mockImplementation((req, res, next) => next()); - mockExists.mockImplementation((req, res, next) => next()); app.use((req: any, res, next) => { req.session = mockRequest.session; req.user = mockRequest.user; @@ -481,7 +435,6 @@ describe('public-auth-routes', () => { }); it('return 200 if jwtFrontend and refreshToken are available', () => { mockRefreshJWT.mockImplementation((req, res, next) => next()); - mockExists.mockImplementation((req, res, next) => next()); app.use((req: any, res, next) => { req.session = { ...mockRequest.session, @@ -513,7 +466,6 @@ describe('public-auth-routes', () => { }); it('return 401 unauthorized', () => { mockRefreshJWT.mockImplementation((req, res, next) => next()); - mockExists.mockImplementation((req, res, next) => next()); app.use((req: any, res, next) => { req.session = { ...mockRequest.session, diff --git a/backend/src/v1/routes/public-auth-routes.ts b/backend/src/v1/routes/public-auth-routes.ts index 75802160d..63d0fc6a7 100644 --- a/backend/src/v1/routes/public-auth-routes.ts +++ b/backend/src/v1/routes/public-auth-routes.ts @@ -108,7 +108,16 @@ router.get( //refreshes jwt on refresh if refreshToken is valid router.post( '/refresh', - [body('refreshToken').exists()], + [ + body('refreshToken').exists(), + body('refreshToken').custom(async (refreshToken, { req }) => { + //the value of the 'refreshToken' attribute in the body must equal + //the value of the user's frontend token + if (!req?.user?.jwtFrontend || req?.user?.jwtFrontend != refreshToken) { + throw new Error('Invalid refresh token'); + } + }), + ], utils.asyncHandler(async (req: Request, res: Response) => { const user: any = req.user; const session: any = req.session; @@ -129,9 +138,7 @@ router.post( if (!errors.isEmpty()) { log.error(JSON.stringify(errors)); - return res.status(400).json({ - errors: errors.array(), - }); + return res.status(401).json(UnauthorizedRsp); } if (!user?.refreshToken || !user?.jwt) { log.error(MISSING_TOKENS_ERROR); From 78d2dc2138044e02e7d7dff39ab9edcff32f5ac3 Mon Sep 17 00:00:00 2001 From: Brock Anderson Date: Thu, 21 Nov 2024 09:38:30 -0800 Subject: [PATCH 2/6] added another unit test --- .../src/v1/routes/public-auth-routes.spec.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/backend/src/v1/routes/public-auth-routes.spec.ts b/backend/src/v1/routes/public-auth-routes.spec.ts index 8639e48c0..c795657d0 100644 --- a/backend/src/v1/routes/public-auth-routes.spec.ts +++ b/backend/src/v1/routes/public-auth-routes.spec.ts @@ -305,6 +305,25 @@ describe('public-auth-routes', () => { return request(app).post('/auth/refresh').expect(401); }); + it('should return 401 when the refreshToken body param contains a non-string', async () => { + const mockCorrelationId = 12; + mockIsTokenExpired.mockReturnValue(true); + mockIsRenewable.mockReturnValue(true); + + app.use((req: any, res, next) => { + req.session = { + ...mockRequest.session, + companyDetails: { id: 1 }, + correlationID: mockCorrelationId, + }; + req.user = { ...mockRequest.user, jwt: 'jwt', refreshToken: 'jwt' }; + req.logout = mockRequest.logout; + req.body = { refreshToken: { $ne: '1' } }; + next(); + }); + app.use('/auth', router); + await request(app).post('/auth/refresh').expect(401); + }); it('should renew the token if renewable', async () => { const mockCorrelationId = 12; const mockFrontendToken = 'jwt_value'; From 055936052c7c30f8269d723c988fef9346412955 Mon Sep 17 00:00:00 2001 From: Brock Anderson Date: Thu, 21 Nov 2024 10:16:23 -0800 Subject: [PATCH 3/6] added an extra validation on the /auth/refresh route --- backend/src/v1/routes/public-auth-routes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/v1/routes/public-auth-routes.ts b/backend/src/v1/routes/public-auth-routes.ts index 63d0fc6a7..9c60cd06e 100644 --- a/backend/src/v1/routes/public-auth-routes.ts +++ b/backend/src/v1/routes/public-auth-routes.ts @@ -109,7 +109,7 @@ router.get( router.post( '/refresh', [ - body('refreshToken').exists(), + body('refreshToken').exists().isString(), body('refreshToken').custom(async (refreshToken, { req }) => { //the value of the 'refreshToken' attribute in the body must equal //the value of the user's frontend token From 9fcdd46ecb3164a8716ba1b2edc8987e8f9f5743 Mon Sep 17 00:00:00 2001 From: Brock Anderson Date: Thu, 21 Nov 2024 13:01:31 -0800 Subject: [PATCH 4/6] added additional parameter validation to prevent SLQ injection attacks --- .../src/v1/routes/file-upload-routes.spec.ts | 64 +++++++++++++++---- backend/src/v1/routes/file-upload-routes.ts | 20 +++++- 2 files changed, 69 insertions(+), 15 deletions(-) diff --git a/backend/src/v1/routes/file-upload-routes.spec.ts b/backend/src/v1/routes/file-upload-routes.spec.ts index b40a5cb1a..4767a11b5 100644 --- a/backend/src/v1/routes/file-upload-routes.spec.ts +++ b/backend/src/v1/routes/file-upload-routes.spec.ts @@ -1,9 +1,24 @@ +import bodyParser from 'body-parser'; import express, { Application } from 'express'; import request from 'supertest'; -import { fileUploadRouter } from './file-upload-routes'; import { SubmissionError } from '../services/file-upload-service'; +import { fileUploadRouter } from './file-upload-routes'; let app: Application; +const maliciousObject = { $ne: '1' }; +const validSubmissionBody = { + companyName: 'ABC Company', + companyAddress: '123 Main St', + naicsCode: '11', + employeeCountRangeId: '123434565467678', + startDate: '2024-01-01', + endDate: '2024-12-31', + reportingYear: 2024, + dataConstraints: '

Data constraints here

', + comments: null, + rows: [], +}; + // Mock the module, but only override handleSubmission const mockHandleSubmission = jest.fn().mockResolvedValue({}); jest.mock('../services/file-upload-service', () => { @@ -31,25 +46,46 @@ describe('file-upload', () => { beforeEach(() => { jest.clearAllMocks(); app = express(); + app.use(bodyParser.json()); app.use(fileUploadRouter); }); - describe('/', () => { - it('/ [POST] - should return 200', async () => { - await request(app).post('/').expect(200); - expect(mockStoreError).not.toHaveBeenCalled(); + describe('POST /', () => { + describe('when the request body is valid', () => { + it('should return 200', async () => { + await request(app).post('/').send(validSubmissionBody).expect(200); + expect(mockStoreError).not.toHaveBeenCalled(); + }); }); - it('/ [POST] - should return 400', async () => { - mockHandleSubmission.mockResolvedValue(new SubmissionError('test')); - await request(app).post('/').expect(400); - expect(mockStoreError).toHaveBeenCalled(); + describe('when the request body is empty', () => { + it('should return 400', async () => { + mockHandleSubmission.mockResolvedValue(new SubmissionError('test')); + await request(app).post('/').expect(400); + expect(mockStoreError).toHaveBeenCalled(); + }); }); - it('/ [POST] - should return 500', async () => { - mockHandleSubmission.mockImplementation(() => { - throw Error('test'); + [ + 'companyName', + 'companyAddress', + 'naicsCode', + 'employeeCountRangeId', + 'startDate', + 'endDate', + 'reportingYear', + 'dataConstraints', + 'comments', + 'rows', + ].forEach((fieldName) => { + describe(`when ${fieldName} is invalid`, () => { + it('should return 400', async () => { + const invalidSubmissionBody = { + ...validSubmissionBody, + }; + invalidSubmissionBody[fieldName] = maliciousObject; + await request(app).post('/').send(invalidSubmissionBody).expect(400); + expect(mockStoreError).toHaveBeenCalled(); + }); }); - await request(app).post('/').expect(500); - expect(mockStoreError).toHaveBeenCalled(); }); }); }); diff --git a/backend/src/v1/routes/file-upload-routes.ts b/backend/src/v1/routes/file-upload-routes.ts index 1b8042898..7e257ed57 100644 --- a/backend/src/v1/routes/file-upload-routes.ts +++ b/backend/src/v1/routes/file-upload-routes.ts @@ -1,16 +1,29 @@ import express, { Request, Response } from 'express'; +import { body, validationResult } from 'express-validator'; import { logger as log } from '../../logger'; +import { errorService } from '../services/error-service'; import { ISubmission, SubmissionError, fileUploadService, } from '../services/file-upload-service'; import { utils } from '../services/utils-service'; -import { errorService } from '../services/error-service'; const fileUploadRouter = express.Router(); fileUploadRouter.post( '/', + [ + body('companyName').exists().isString(), + body('companyAddress').exists().isString(), + body('naicsCode').exists().isString(), + body('employeeCountRangeId').exists().isString(), + body('startDate').exists().isString(), + body('endDate').exists().isString(), + body('reportingYear').exists().isNumeric(), + body('dataConstraints').optional({ nullable: true }).isString(), + body('comments').optional({ nullable: true }).isString(), + body('rows').exists().isArray(), + ], utils.asyncHandler( async (req: Request, res: Response) => { const session: any = req?.session; @@ -22,6 +35,11 @@ fileUploadRouter.post( const data: ISubmission = req.body; let error = null; try { + const preliminaryValidationErrors = validationResult(req); + if (!preliminaryValidationErrors.isEmpty()) { + error = new SubmissionError(preliminaryValidationErrors.array()); + return res.status(400).json(error); + } const result: any = await fileUploadService.handleSubmission( userInfo, data, From 856b661d7b7214b595913807a039953c54e4b8ca Mon Sep 17 00:00:00 2001 From: Brock Anderson Date: Thu, 21 Nov 2024 16:59:18 -0800 Subject: [PATCH 5/6] use 'samesite:lax' setting for cookies. it provides some deterance against CSRF attacks, but it's not so strict as to break the login flow. --- backend/src/app.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/backend/src/app.ts b/backend/src/app.ts index 6c3f5e4dd..c4a4cb5c4 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -3,7 +3,7 @@ import express, { NextFunction, Request, Response } from 'express'; import bodyParser from 'body-parser'; import promBundle from 'express-prom-bundle'; import { rateLimit } from 'express-rate-limit'; -import session from 'express-session'; +import session, { CookieOptions } from 'express-session'; import helmet from 'helmet'; import morgan from 'morgan'; import noCache from 'nocache'; @@ -16,14 +16,14 @@ import fileSessionStore from 'session-file-store'; import { config } from './config'; import { logger } from './logger'; import prisma from './v1/prisma/prisma-client'; +import announcementRouter from './v1/routes/announcement-routes'; import codeRouter from './v1/routes/code-routes'; import { router as configRouter } from './v1/routes/config-routes'; import { fileUploadRouter } from './v1/routes/file-upload-routes'; import authRouter from './v1/routes/public-auth-routes'; import { reportRouter } from './v1/routes/report-routes'; -import userRouter from './v1/routes/user-info-routes'; -import announcementRouter from './v1/routes/announcement-routes'; import resourcesRoutes from './v1/routes/resources-routes'; +import userRouter from './v1/routes/user-info-routes'; import { publicAuth } from './v1/services/public-auth-service'; import { utils } from './v1/services/utils-service'; @@ -94,10 +94,18 @@ app.use( }), ); let proxy = true; -const cookie = { +const cookie: CookieOptions = { secure: true, httpOnly: true, - maxAge: 1800000, //30 minutes in ms. this is same as session time. DO NOT MODIFY, IF MODIFIED, MAKE SURE SAME AS SESSION TIME OUT VALUE. + //maxAge: 30 minutes in ms. this is same as session time. + //DO NOT MODIFY, IF MODIFIED, MAKE SURE SAME AS SESSION TIME OUT VALUE. + maxAge: 1800000, + //sameSite: 'strict' would be preferable, but it complicates the + //authentication code flow of the login process (because the callback + //url is issued by the identify provider, which is a third party so the + //browser doesn't pass the cookie to the callback endpoint). + //'lax' is a middle ground that will + sameSite: 'lax', }; if ('local' === config.get('environment')) { cookie.secure = false; From 67bc3562dd57303f823827cb1c79551a60a878ba Mon Sep 17 00:00:00 2001 From: Brock Anderson Date: Tue, 26 Nov 2024 12:54:36 -0800 Subject: [PATCH 6/6] added explicit cookie 'samesite'=lax policy --- backend/src/admin-app.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/backend/src/admin-app.ts b/backend/src/admin-app.ts index ff60879b3..84a1b69c8 100644 --- a/backend/src/admin-app.ts +++ b/backend/src/admin-app.ts @@ -3,7 +3,7 @@ import express, { NextFunction, Request, Response } from 'express'; import bodyParser from 'body-parser'; import promBundle from 'express-prom-bundle'; import { rateLimit } from 'express-rate-limit'; -import session from 'express-session'; +import session, { CookieOptions } from 'express-session'; import helmet from 'helmet'; import morgan from 'morgan'; import noCache from 'nocache'; @@ -24,15 +24,15 @@ import { logger } from './logger'; import prisma from './v1/prisma/prisma-client'; import adminAuthRouter from './v1/routes/admin-auth-routes'; import adminReportRoutes from './v1/routes/admin-report-routes'; -import adminUsersRoutes from './v1/routes/admin-users-routes'; import adminUserRouter from './v1/routes/admin-user-info-routes'; +import adminUsersRoutes from './v1/routes/admin-users-routes'; +import analyticRoutes from './v1/routes/analytic-routes'; +import announcementsRoutes from './v1/routes/announcement-routes'; import codeRouter from './v1/routes/code-routes'; import { router as configRouter } from './v1/routes/config-routes'; -import announcementsRoutes from './v1/routes/announcement-routes'; -import analyticRoutes from './v1/routes/analytic-routes'; -import resourcesRoutes from './v1/routes/resources-routes'; -import employerRoutes from './v1/routes/employer-routes'; import dashboardMetricsRouter from './v1/routes/dashboard'; +import employerRoutes from './v1/routes/employer-routes'; +import resourcesRoutes from './v1/routes/resources-routes'; import { adminAuth } from './v1/services/admin-auth-service'; import { utils } from './v1/services/utils-service'; @@ -77,10 +77,19 @@ adminApp.use( }), ); let proxy = true; -const cookie = { +const cookie: CookieOptions = { secure: true, httpOnly: true, - maxAge: 1800000, //30 minutes in ms. this is same as session time. DO NOT MODIFY, IF MODIFIED, MAKE SURE SAME AS SESSION TIME OUT VALUE. + //30 minutes in ms. this is same as session time. + //DO NOT MODIFY, IF MODIFIED, MAKE SURE SAME AS SESSION TIME OUT VALUE. + maxAge: 1800000, + //sameSite: 'strict' would be preferable, but it complicates the + //authentication code flow of the login process. The callback + //url is accessed by the identify provider, which is a third party. + //With a strict policy the the browser won't pass the cookie to the + //callback endpoint. 'lax' is a middle ground that will provide + //some protection against CSRF attacks. + sameSite: 'lax', }; if ('local' === config.get('environment')) { cookie.secure = false;