TechDebt: Centralize Virus Scan (#1365)

* Centralize Virus scan
bcgov · Sep 16, 2024 · 556ba8d · 556ba8d
1 parent edb69b0
commit 556ba8d
Show file tree

Hide file tree

Showing 19 changed files with 27 additions and 227 deletions.
diff --git a/api/src/app.ts b/api/src/app.ts
@@ -5,14 +5,15 @@ import { OpenAPIV3 } from 'openapi-types';
 import path from 'path';
 import swaggerUIExperss from 'swagger-ui-express';
 import { defaultPoolConfig, initDBPool } from './database/db';
-import { ensureHTTPError, HTTP500 } from './errors/http-error';
+import { ensureHTTPError, HTTP400, HTTP500 } from './errors/http-error';
 import {
   authorizeAndAuthenticateMiddleware,
   getCritterbaseProxyMiddleware,
   replaceAuthorizationHeaderMiddleware
 } from './middleware/critterbase-proxy';
 import { rootAPIDoc } from './openapi/root-api-doc';
 import { authenticateRequest, authenticateRequestOptional } from './request-handlers/security/authentication';
+import { scanFileForVirus } from './utils/file-utils';
 import { getLogger } from './utils/logger';
 
 const defaultLog = getLogger('app');
@@ -75,7 +76,7 @@ const openAPIFramework = initialize({
     'application/json': express.json({ limit: MAX_REQ_BODY_SIZE }),
     'multipart/form-data': function (req, res, next) {
       const multerRequestHandler = multer({
-        storage: multer.memoryStorage(),
+        storage: multer.memoryStorage(), // TODO change to local/PVC storage and stream file uploads to S3?
         limits: { fileSize: MAX_UPLOAD_FILE_SIZE }
       }).array('media', MAX_UPLOAD_NUM_FILES);
 
@@ -89,11 +90,27 @@ const openAPIFramework = initialize({
        *
        * @see https://www.npmjs.com/package/express-openapi#argsconsumesmiddleware
        */
-      multerRequestHandler(req, res, (error?: any) => {
+      multerRequestHandler(req, res, async (error?: any) => {
         if (error) {
           return next(error);
         }
 
+        // Scan files for malicious content, if enabled
+        const virusScanPromises = (req.files as Express.Multer.File[]).map(async function (file) {
+          const isSafe = await scanFileForVirus(file);
+
+          if (!isSafe) {
+            throw new HTTP400('Malicious file content detected.', [{ file_name: file.originalname }]);
+          }
+        });
+
+        try {
+          await Promise.all(virusScanPromises);
+        } catch (error) {
+          // If a virus is detected, return error and do not continue
+          return next(error);
+        }
+
         // Ensure `req.files` or `req.body.media` is always set to an array
         const multerFiles = req.files ?? [];
 

diff --git a/api/src/paths/project/{projectId}/attachments/report/upload.test.ts b/api/src/paths/project/{projectId}/attachments/report/upload.test.ts
@@ -46,8 +46,6 @@ describe('uploadMedia', () => {
       }
     });
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const expectedError = new Error('cannot process request');
     sinon.stub(AttachmentService.prototype, 'upsertProjectReportAttachment').rejects(expectedError);
 
@@ -69,7 +67,6 @@ describe('uploadMedia', () => {
       }
     });
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
     sinon.stub(file_utils, 'uploadFileToS3').resolves();
 
     const expectedResponse = { attachmentId: 1, revision_count: 1 };

diff --git a/api/src/paths/project/{projectId}/attachments/report/upload.ts b/api/src/paths/project/{projectId}/attachments/report/upload.ts
@@ -2,11 +2,10 @@ import { RequestHandler } from 'express';
 import { Operation } from 'express-openapi';
 import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../constants/roles';
 import { getDBConnection } from '../../../../../database/db';
-import { HTTP400 } from '../../../../../errors/http-error';
 import { fileSchema } from '../../../../../openapi/schemas/file';
 import { authorizeRequestHandler } from '../../../../../request-handlers/security/authorization';
 import { AttachmentService } from '../../../../../services/attachment-service';
-import { scanFileForVirus, uploadFileToS3 } from '../../../../../utils/file-utils';
+import { uploadFileToS3 } from '../../../../../utils/file-utils';
 import { getLogger } from '../../../../../utils/logger';
 import { getFileFromRequest } from '../../../../../utils/request';
 
@@ -158,13 +157,6 @@ export function uploadMedia(): RequestHandler {
     try {
       await connection.open();
 
-      // Scan file for viruses using ClamAV
-      const virusScanResult = await scanFileForVirus(rawMediaFile);
-
-      if (!virusScanResult) {
-        throw new HTTP400('Malicious content detected, upload cancelled');
-      }
-
       const attachmentService = new AttachmentService(connection);
 
       //Upsert a report attachment

diff --git a/api/src/paths/project/{projectId}/attachments/upload.test.ts b/api/src/paths/project/{projectId}/attachments/upload.test.ts
@@ -44,8 +44,6 @@ describe('uploadMedia', () => {
       }
     });
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const expectedError = new Error('cannot process request');
     sinon.stub(AttachmentService.prototype, 'upsertProjectAttachment').rejects(expectedError);
 
@@ -67,7 +65,6 @@ describe('uploadMedia', () => {
       }
     });
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
     sinon.stub(file_utils, 'uploadFileToS3').resolves();
 
     const expectedResponse = { attachmentId: 1, revision_count: 1 };

diff --git a/api/src/paths/project/{projectId}/attachments/upload.ts b/api/src/paths/project/{projectId}/attachments/upload.ts
@@ -3,11 +3,10 @@ import { Operation } from 'express-openapi';
 import { ATTACHMENT_TYPE } from '../../../../constants/attachments';
 import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../constants/roles';
 import { getDBConnection } from '../../../../database/db';
-import { HTTP400 } from '../../../../errors/http-error';
 import { fileSchema } from '../../../../openapi/schemas/file';
 import { authorizeRequestHandler } from '../../../../request-handlers/security/authorization';
 import { AttachmentService } from '../../../../services/attachment-service';
-import { scanFileForVirus, uploadFileToS3 } from '../../../../utils/file-utils';
+import { uploadFileToS3 } from '../../../../utils/file-utils';
 import { getLogger } from '../../../../utils/logger';
 import { getFileFromRequest } from '../../../../utils/request';
 
@@ -133,13 +132,6 @@ export function uploadMedia(): RequestHandler {
     try {
       await connection.open();
 
-      // Scan file for viruses using ClamAV
-      const virusScanResult = await scanFileForVirus(rawMediaFile);
-
-      if (!virusScanResult) {
-        throw new HTTP400('Malicious content detected, upload cancelled');
-      }
-
       const attachmentService = new AttachmentService(connection);
 
       const upsertResult = await attachmentService.upsertProjectAttachment(

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/report/upload.test.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/report/upload.test.ts
@@ -40,8 +40,6 @@ describe('uploadMedia', () => {
       }
     } as any;
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const expectedError = new Error('cannot process request');
     sinon.stub(AttachmentService.prototype, 'upsertSurveyReportAttachment').rejects(expectedError);
 
@@ -59,7 +57,6 @@ describe('uploadMedia', () => {
     const dbConnectionObj = getMockDBConnection();
     sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
     sinon.stub(file_utils, 'uploadFileToS3').resolves();
 
     const mockReq = {

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/report/upload.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/report/upload.ts
@@ -2,11 +2,10 @@ import { RequestHandler } from 'express';
 import { Operation } from 'express-openapi';
 import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../../../constants/roles';
 import { getDBConnection } from '../../../../../../../database/db';
-import { HTTP400 } from '../../../../../../../errors/http-error';
 import { fileSchema } from '../../../../../../../openapi/schemas/file';
 import { authorizeRequestHandler } from '../../../../../../../request-handlers/security/authorization';
 import { AttachmentService } from '../../../../../../../services/attachment-service';
-import { scanFileForVirus, uploadFileToS3 } from '../../../../../../../utils/file-utils';
+import { uploadFileToS3 } from '../../../../../../../utils/file-utils';
 import { getLogger } from '../../../../../../../utils/logger';
 import { getFileFromRequest } from '../../../../../../../utils/request';
 
@@ -175,13 +174,6 @@ export function uploadMedia(): RequestHandler {
     try {
       await connection.open();
 
-      // Scan file for viruses using ClamAV
-      const virusScanResult = await scanFileForVirus(rawMediaFile);
-
-      if (!virusScanResult) {
-        throw new HTTP400('Malicious content detected, upload cancelled');
-      }
-
       const attachmentService = new AttachmentService(connection);
 
       const upsertResult = await attachmentService.upsertSurveyReportAttachment(

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/telemetry/index.test.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/telemetry/index.test.ts
@@ -19,46 +19,10 @@ describe('postSurveyTelemetryCredentialAttachment', () => {
     sinon.restore();
   });
 
-  it('should throw an error when file has malicious content', async () => {
-    const dbConnectionObj = getMockDBConnection();
-    sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);
-
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(false); // fail virus scan
-
-    const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();
-
-    mockReq.keycloak_token = {} as KeycloakUserInformation;
-    mockReq.params = {
-      projectId: '1',
-      surveyId: '2'
-    };
-    mockReq.files = [
-      {
-        fieldname: 'media',
-        originalname: 'test.keyx',
-        encoding: '7bit',
-        mimetype: 'text/plain',
-        size: 340
-      }
-    ] as Express.Multer.File[];
-
-    const requestHandler = postSurveyTelemetryCredentialAttachment();
-
-    try {
-      await requestHandler(mockReq, mockRes, mockNext);
-      expect.fail();
-    } catch (actualError) {
-      expect((actualError as HTTPError).status).to.equal(400);
-      expect((actualError as HTTPError).message).to.equal('Malicious content detected, upload cancelled');
-    }
-  });
-
   it('should throw an error when file type is invalid', async () => {
     const dbConnectionObj = getMockDBConnection();
     sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();
 
     mockReq.keycloak_token = {} as KeycloakUserInformation;
@@ -93,8 +57,6 @@ describe('postSurveyTelemetryCredentialAttachment', () => {
     const dbConnectionObj = getMockDBConnection();
     sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const upsertSurveyTelemetryCredentialAttachmentStub = sinon
       .stub(AttachmentService.prototype, 'upsertSurveyTelemetryCredentialAttachment')
       .resolves({ survey_telemetry_credential_attachment_id: 44, key: 'path/to/file/test.keyx' });
@@ -134,8 +96,6 @@ describe('postSurveyTelemetryCredentialAttachment', () => {
     const dbConnectionObj = getMockDBConnection();
     sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const upsertSurveyTelemetryCredentialAttachmentStub = sinon
       .stub(AttachmentService.prototype, 'upsertSurveyTelemetryCredentialAttachment')
       .resolves({ survey_telemetry_credential_attachment_id: 44, key: 'path/to/file/test.keyx' });
@@ -175,8 +135,6 @@ describe('postSurveyTelemetryCredentialAttachment', () => {
     const dbConnectionObj = getMockDBConnection();
     sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const upsertSurveyTelemetryCredentialAttachmentStub = sinon
       .stub(AttachmentService.prototype, 'upsertSurveyTelemetryCredentialAttachment')
       .resolves({ survey_telemetry_credential_attachment_id: 44, key: 'path/to/file/test.keyx' });

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/telemetry/index.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/telemetry/index.ts
@@ -9,7 +9,7 @@ import { authorizeRequestHandler } from '../../../../../../../request-handlers/s
 import { AttachmentService } from '../../../../../../../services/attachment-service';
 import { BctwKeyxService } from '../../../../../../../services/bctw-service/bctw-keyx-service';
 import { getBctwUser } from '../../../../../../../services/bctw-service/bctw-service';
-import { scanFileForVirus, uploadFileToS3 } from '../../../../../../../utils/file-utils';
+import { uploadFileToS3 } from '../../../../../../../utils/file-utils';
 import { getLogger } from '../../../../../../../utils/logger';
 import { isValidTelementryCredentialFile } from '../../../../../../../utils/media/media-utils';
 import { getFileFromRequest } from '../../../../../../../utils/request';
@@ -139,13 +139,6 @@ export function postSurveyTelemetryCredentialAttachment(): RequestHandler {
         files: { ...rawMediaFile, buffer: 'Too big to print' }
       });
 
-      // Scan file for viruses using ClamAV
-      const virusScanResult = await scanFileForVirus(rawMediaFile);
-
-      if (!virusScanResult) {
-        throw new HTTP400('Malicious content detected, upload cancelled');
-      }
-
       const isTelemetryCredentialFile = isValidTelementryCredentialFile(rawMediaFile);
 
       if (isTelemetryCredentialFile.error) {

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/upload.test.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/upload.test.ts
@@ -44,8 +44,6 @@ describe('uploadMedia', () => {
       }
     });
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
-
     const expectedError = new Error('cannot process request');
     sinon.stub(AttachmentService.prototype, 'upsertSurveyAttachment').rejects(expectedError);
 
@@ -67,7 +65,6 @@ describe('uploadMedia', () => {
       }
     });
 
-    sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
     sinon.stub(file_utils, 'uploadFileToS3').resolves();
 
     const expectedResponse = { attachmentId: 1, revision_count: 1 };

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/upload.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/attachments/upload.ts
@@ -3,11 +3,10 @@ import { Operation } from 'express-openapi';
 import { ATTACHMENT_TYPE } from '../../../../../../constants/attachments';
 import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../../constants/roles';
 import { getDBConnection } from '../../../../../../database/db';
-import { HTTP400 } from '../../../../../../errors/http-error';
 import { fileSchema } from '../../../../../../openapi/schemas/file';
 import { authorizeRequestHandler } from '../../../../../../request-handlers/security/authorization';
 import { AttachmentService } from '../../../../../../services/attachment-service';
-import { scanFileForVirus, uploadFileToS3 } from '../../../../../../utils/file-utils';
+import { uploadFileToS3 } from '../../../../../../utils/file-utils';
 import { getLogger } from '../../../../../../utils/logger';
 import { getFileFromRequest } from '../../../../../../utils/request';
 
@@ -124,13 +123,6 @@ export function uploadMedia(): RequestHandler {
     try {
       await connection.open();
 
-      // Scan file for viruses using ClamAV
-      const virusScanResult = await scanFileForVirus(rawMediaFile);
-
-      if (!virusScanResult) {
-        throw new HTTP400('Malicious content detected, upload cancelled');
-      }
-
       const attachmentService = new AttachmentService(connection);
 
       const upsertResult = await attachmentService.upsertSurveyAttachment(

diff --git a/api/src/paths/project/{projectId}/survey/{surveyId}/critters/captures/import.ts b/api/src/paths/project/{projectId}/survey/{surveyId}/critters/captures/import.ts
@@ -2,12 +2,10 @@ import { RequestHandler } from 'express';
 import { Operation } from 'express-openapi';
 import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../../../constants/roles';
 import { getDBConnection } from '../../../../../../../database/db';
-import { HTTP400 } from '../../../../../../../errors/http-error';
 import { csvFileSchema } from '../../../../../../../openapi/schemas/file';
 import { authorizeRequestHandler } from '../../../../../../../request-handlers/security/authorization';
 import { ImportCapturesStrategy } from '../../../../../../../services/import-services/capture/import-captures-strategy';
 import { importCSV } from '../../../../../../../services/import-services/import-csv';
-import { scanFileForVirus } from '../../../../../../../utils/file-utils';
 import { getLogger } from '../../../../../../../utils/logger';
 import { parseMulterFile } from '../../../../../../../utils/media/media-utils';
 import { getFileFromRequest } from '../../../../../../../utils/request';
@@ -135,13 +133,6 @@ export function importCsv(): RequestHandler {
     try {
       await connection.open();
 
-      // Check for viruses / malware
-      const virusScanResult = await scanFileForVirus(rawFile);
-
-      if (!virusScanResult) {
-        throw new HTTP400('Malicious content detected, import cancelled.');
-      }
-
       const importCsvCaptures = new ImportCapturesStrategy(connection, surveyId);
 
       // Pass CSV file and importer as dependencies