fix: GEO-1237, GEO-1238 - fixed critical security issues identified i…

…n WAVA scan (#858)

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;

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;

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: '<p>Data constraints here</p>',
+  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();
     });
   });
 });

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<null, null, null, ISubmission>, 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,