Skip to content

Commit

Permalink
BHBC: 1317 - Scan Uploaded Attachments (#506)
Browse files Browse the repository at this point in the history
use clamav to scan file uploaded for malicious content
  • Loading branch information
sdevalapurkar authored Sep 9, 2021
1 parent 57556ab commit 921fcd3
Show file tree
Hide file tree
Showing 34 changed files with 280 additions and 248 deletions.
20 changes: 19 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
export $(shell sed 's/=.*//' .env)

.DEFAULT : help
.PHONY : setup close clean build run run-debug build-backend run-backend run-backend-debug build-web run-web run-web-debug database app api db-setup db-migrate db-rollback install test lint lint-fix format help
.PHONY : setup close clean build run run-debug build-backend run-backend run-backend-debug build-web run-web run-web-debug database app api db-setup db-migrate db-rollback n8n-setup n8n-export clamav install test lint lint-fix format help

## ------------------------------------------------------------------------------
## Alias Commands
Expand Down Expand Up @@ -43,6 +43,8 @@ db-rollback: | build-db-rollback run-db-rollback ## Performs all commands necess
n8n-setup: | build-n8n-setup run-n8n-setup ## Performs all commands necessary to run the n8n setup
n8n-export: | build-n8n-export run-n8n-export ## Performs all commands necessary to export the latest n8n credentials and workflows

clamav: | build-clamav run-clamav ## Performs all commands necessary to run clamav

## ------------------------------------------------------------------------------
## Setup/Cleanup Commands
## ------------------------------------------------------------------------------
Expand Down Expand Up @@ -256,6 +258,22 @@ run-n8n-export: ## Run the n8n export
@echo "==============================================="
@docker-compose -f docker-compose.yml up n8n_export

## ------------------------------------------------------------------------------
## clamav commands
## ------------------------------------------------------------------------------

build-clamav: ## Build the clamav image
@echo "==============================================="
@echo "Make: build-clamav - building clamav image"
@echo "==============================================="
@docker-compose -f docker-compose.yml build clamav

run-clamav: ## Run clamav
@echo "==============================================="
@echo "Make: run-clamav - running clamav"
@echo "==============================================="
@docker-compose -f docker-compose.yml up -d clamav

## ------------------------------------------------------------------------------
## Run `npm` commands for all projects
## ------------------------------------------------------------------------------
Expand Down
4 changes: 4 additions & 0 deletions api/openshift/api.dc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ metadata:
selfLink: ''
name: biohubbc-api-dc
parameters:
- name: ENABLE_FILE_VIRUS_SCAN
value: 'true'
- name: NAME
value: biohubbc-api
- name: SUFFIX
Expand Down Expand Up @@ -114,6 +116,8 @@ objects:
value: ${HOST}
- name: API_PORT
value: ${API_PORT_DEFAULT}
- name: ENABLE_FILE_VIRUS_SCAN
value: ${ENABLE_FILE_VIRUS_SCAN}
- name: DB_HOST
value: ${DB_SERVICE_NAME}
- name: DB_USER_API
Expand Down
5 changes: 5 additions & 0 deletions api/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"db-migrate": "~0.11.11",
"db-migrate-pg": "~1.2.2",
"xlsx": "~0.17.0",
"clamdjs": "~1.0.2",
"express": "~4.17.1",
"express-openapi": "~7.0.1",
"fast-json-patch": "~3.0.0-1",
Expand Down
30 changes: 28 additions & 2 deletions api/src/paths/project/{projectId}/attachments/upload.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ describe('uploadMedia', () => {
}
});

sinon.stub(file_utils, 'scanFileForVirus').resolves(true);

try {
const result = upload.uploadMedia();

Expand All @@ -115,21 +117,45 @@ describe('uploadMedia', () => {
}
});

it('should return a list of file keys on success (with username and email)', async () => {
it('should throw a 400 error when file contains malicious content', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
return 20;
}
});

sinon.stub(file_utils, 'uploadFileToS3').resolves({ Key: '1/1/test.txt' } as any);
sinon.stub(project, 'upsertProjectAttachment').resolves(1);
sinon.stub(file_utils, 'scanFileForVirus').resolves(false);

try {
const result = upload.uploadMedia();

await result(sampleReq, sampleRes as any, (null as unknown) as any);
expect.fail();
} catch (actualError) {
expect(actualError.status).to.equal(400);
expect(actualError.message).to.equal('Malicious content detected, upload cancelled');
}
});

it('should return file key on success (with username and email)', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
return 20;
}
});

sinon.stub(file_utils, 'scanFileForVirus').resolves(true);
sinon.stub(file_utils, 'uploadFileToS3').resolves({ Key: '1/1/test.txt' } as any);
sinon.stub(project, 'upsertProjectAttachment').resolves(1);

const result = upload.uploadMedia();

await result(sampleReq, sampleRes as any, (null as unknown) as any);

expect(actualResult).to.eql(['1/1/test.txt']);
expect(actualResult).to.eql('1/1/test.txt');
});
});
86 changes: 33 additions & 53 deletions api/src/paths/project/{projectId}/attachments/upload.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
'use strict';

import { ManagedUpload } from 'aws-sdk/clients/s3';
import { RequestHandler } from 'express';
import { Operation } from 'express-openapi';
import { SYSTEM_ROLE } from '../../../../constants/roles';
import { getDBConnection } from '../../../../database/db';
import { HTTP400 } from '../../../../errors/CustomError';
import { generateS3FileKey, uploadFileToS3 } from '../../../../utils/file-utils';
import { generateS3FileKey, scanFileForVirus, uploadFileToS3 } from '../../../../utils/file-utils';
import { getLogger } from '../../../../utils/logger';
import { upsertProjectAttachment } from '../../../project';

const defaultLog = getLogger('/api/project/{projectId}/attachments/upload');

export const POST: Operation = [uploadMedia()];
POST.apiDoc = {
description: 'Upload project-specific attachments.',
tags: ['attachments'],
description: 'Upload a project-specific attachment.',
tags: ['attachment'],
security: [
{
Bearer: [SYSTEM_ROLE.SYSTEM_ADMIN, SYSTEM_ROLE.PROJECT_ADMIN]
Expand All @@ -29,19 +28,15 @@ POST.apiDoc = {
}
],
requestBody: {
description: 'Attachments upload post request object.',
description: 'Attachment upload post request object.',
content: {
'multipart/form-data': {
schema: {
type: 'object',
properties: {
media: {
type: 'array',
description: 'An array of attachments to upload',
items: {
type: 'string',
format: 'binary'
}
type: 'string',
format: 'binary'
}
}
}
Expand All @@ -50,24 +45,12 @@ POST.apiDoc = {
},
responses: {
200: {
description: 'Attachments upload response.',
description: 'Attachment upload response.',
content: {
'application/json': {
schema: {
type: 'array',
items: {
type: 'object',
properties: {
mediaKey: {
type: 'string',
description: 'The S3 unique key for this file.'
},
lastModified: {
type: 'string',
description: 'The date the object was last modified'
}
}
}
type: 'string',
description: 'The S3 unique key for this file.'
}
}
}
Expand Down Expand Up @@ -99,12 +82,12 @@ export function uploadMedia(): RequestHandler {
throw new HTTP400('Missing upload data');
}

const rawMediaFile: Express.Multer.File = rawMediaArray[0];

defaultLog.debug({
label: 'uploadMedia',
message: 'files',
files: rawMediaArray.map((item) => {
return { ...item, buffer: 'Too big to print' };
})
message: 'file',
file: { ...rawMediaFile, buffer: 'Too big to print' }
});

if (!req.params.projectId) {
Expand All @@ -113,41 +96,38 @@ export function uploadMedia(): RequestHandler {

const connection = getDBConnection(req['keycloak_token']);

// Insert file metadata into project_attachment table
try {
await connection.open();

const insertProjectAttachmentsPromises =
rawMediaArray.map((file: Express.Multer.File) =>
upsertProjectAttachment(file, Number(req.params.projectId), connection)
) || [];
// Scan file for viruses using ClamAV
const virusScanResult = await scanFileForVirus(rawMediaFile);

await Promise.all([...insertProjectAttachmentsPromises]);
if (!virusScanResult) {
throw new HTTP400('Malicious content detected, upload cancelled');
}

// Upload files to S3
const s3UploadPromises: Promise<ManagedUpload.SendData>[] = [];
// Insert file metadata into project_attachment table
await upsertProjectAttachment(rawMediaFile, Number(req.params.projectId), connection);

rawMediaArray.forEach((file: Express.Multer.File) => {
const key = generateS3FileKey({
projectId: Number(req.params.projectId),
fileName: file.originalname
});
// Upload file to S3
const key = generateS3FileKey({
projectId: Number(req.params.projectId),
fileName: rawMediaFile.originalname
});

const metadata = {
filename: file.originalname,
username: (req['auth_payload'] && req['auth_payload'].preferred_username) || '',
email: (req['auth_payload'] && req['auth_payload'].email) || ''
};
const metadata = {
filename: rawMediaFile.originalname,
username: (req['auth_payload'] && req['auth_payload'].preferred_username) || '',
email: (req['auth_payload'] && req['auth_payload'].email) || ''
};

s3UploadPromises.push(uploadFileToS3(file, key, metadata));
});
const result = await uploadFileToS3(rawMediaFile, key, metadata);

const results = await Promise.all(s3UploadPromises);
defaultLog.debug({ label: 'uploadMedia', message: 'results', results });
defaultLog.debug({ label: 'uploadMedia', message: 'result', result });

await connection.commit();

return res.status(200).json(results.map((result) => result.Key));
return res.status(200).json(result.Key);
} catch (error) {
defaultLog.debug({ label: 'uploadMedia', message: 'error', error });
await connection.rollback();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ describe('uploadMedia', () => {
}
});

sinon.stub(file_utils, 'scanFileForVirus').resolves(true);

try {
const result = upload.uploadMedia();

Expand All @@ -121,7 +123,30 @@ describe('uploadMedia', () => {
}
});

it('should return a list of file keys on success (with username and email)', async () => {
it('should throw a 400 error when file contains malicious content', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
return 20;
}
});

sinon.stub(file_utils, 'uploadFileToS3').resolves({ Key: '1/1/test.txt' } as any);
sinon.stub(upload, 'upsertSurveyAttachment').resolves(1);
sinon.stub(file_utils, 'scanFileForVirus').resolves(false);

try {
const result = upload.uploadMedia();

await result(sampleReq, sampleRes as any, (null as unknown) as any);
expect.fail();
} catch (actualError) {
expect(actualError.status).to.equal(400);
expect(actualError.message).to.equal('Malicious content detected, upload cancelled');
}
});

it('should return file key on success (with username and email)', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
Expand All @@ -131,15 +156,16 @@ describe('uploadMedia', () => {

sinon.stub(file_utils, 'uploadFileToS3').resolves({ Key: '1/1/test.txt' } as any);
sinon.stub(upload, 'upsertSurveyAttachment').resolves(1);
sinon.stub(file_utils, 'scanFileForVirus').resolves(true);

const result = upload.uploadMedia();

await result(sampleReq, sampleRes as any, (null as unknown) as any);

expect(actualResult).to.eql(['1/1/test.txt']);
expect(actualResult).to.eql('1/1/test.txt');
});

it('should return a list of file keys on success (without username and email)', async () => {
it('should return file key on success (without username and email)', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
Expand All @@ -149,6 +175,7 @@ describe('uploadMedia', () => {

sinon.stub(file_utils, 'uploadFileToS3').resolves({ Key: '1/1/test.txt' } as any);
sinon.stub(upload, 'upsertSurveyAttachment').resolves(1);
sinon.stub(file_utils, 'scanFileForVirus').resolves(true);

const result = upload.uploadMedia();

Expand All @@ -158,7 +185,7 @@ describe('uploadMedia', () => {
(null as unknown) as any
);

expect(actualResult).to.eql(['1/1/test.txt']);
expect(actualResult).to.eql('1/1/test.txt');
});
});

Expand Down
Loading

0 comments on commit 921fcd3

Please sign in to comment.