From 2bdac50fae7394d3df41d2cbd6fe75eed27fcbf3 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 14:25:55 +0000 Subject: [PATCH 1/9] added some more validations on the datafile --- src/controllers/uploadController.js | 84 +++++++++++++++++++------- src/filters/validationMessageLookup.js | 6 +- src/routes/form-wizard/fields.js | 6 +- 3 files changed, 72 insertions(+), 24 deletions(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index 463d0a38..3ef6008f 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -41,11 +41,15 @@ class UploadController extends PageController { ipAddress: await hash(req.ip) }) if (jsonResult) { - try { - this.errorCount = jsonResult['issue-log'].filter(issue => issue.severity === severityLevels.error).length + jsonResult['column-field-log'].filter(log => log.missing).length - req.body.validationResult = jsonResult - } catch (error) { - this.validationError('apiError', 'Error parsing api response error count', error, req) + if (jsonResult.error) { + this.validationError('apiError', jsonResult.message, {}, req) + } else { + try { + this.errorCount = jsonResult['issue-log'].filter(issue => issue.severity === severityLevels.error).length + jsonResult['column-field-log'].filter(log => log.missing).length + req.body.validationResult = jsonResult + } catch (error) { + this.validationError('apiError', 'Error parsing api response error count', error, req) + } } } else { this.validationError('apiError', 'Nothing returned from the api', null, req) @@ -79,10 +83,18 @@ class UploadController extends PageController { this.validationErrorMessage = message } - async validateFile ({ filePath, fileName, dataset, dataSubject, organisation, sessionId, ipAddress }) { - const validFileType = UploadController.validateFileType({ originalname: fileName }) + async validateFile (datafile) { + const { filePath, fileName, dataset, dataSubject, organisation, sessionId, ipAddress } = datafile - if (!validFileType) { + datafile.originalname = datafile.originalname || fileName + + if ( + !UploadController.extensionIsValid(datafile) || + !UploadController.sizeIsValid(datafile) || + !UploadController.fileNameIsntTooLong(datafile) || + !UploadController.fileNameIsValid(datafile) || + !UploadController.fileNameDoesntContainDoubleExtension(datafile) + ) { return false } @@ -106,20 +118,48 @@ class UploadController extends PageController { return validationResult ? !validationResult.error : false } - // this function is a validation function that is called by the form wizard - static validateFileType ({ originalname }) { - const allowedFiletypes = [ - 'csv', - 'xls', - 'xlsx', - 'json', - 'geojson', - 'gml', - 'gpkg' - ] - // check file type - const fileType = originalname.split('.').pop() - if (!allowedFiletypes.includes(fileType)) { + static extensionIsValid (datafile) { + const allowedExtensions = ['csv', 'xls', 'xlsx', 'json', 'geojson', 'gml', 'gpkg'] + + const parts = datafile.originalname.split('.') + + const extension = parts[parts.length - 1] + if (!allowedExtensions.includes(extension)) { + return false + } + + return true + } + + static sizeIsValid (datafile) { + const maxSize = 10 * 1024 * 1024 // 10MB + + if (datafile.size > maxSize) { + return false + } + + return true + } + + static fileNameIsntTooLong (datafile) { + const maxSize = 255 // Maximum filename size + if (datafile.originalname.length > maxSize) { + return false + } + return true + } + + static fileNameIsValid (datafile) { + const invalidCharacters = /[<>:"/\\|?*]/ + if (invalidCharacters.test(datafile.originalname)) { + return false + } + return true + } + + static fileNameDoesntContainDoubleExtension (datafile) { + const parts = datafile.originalname.split('.') + if (parts.length > 2) { return false } return true diff --git a/src/filters/validationMessageLookup.js b/src/filters/validationMessageLookup.js index 57e5f6f7..f53ecd18 100644 --- a/src/filters/validationMessageLookup.js +++ b/src/filters/validationMessageLookup.js @@ -14,7 +14,11 @@ const validationMessages = { }, datafile: { required: 'Select a file', - fileType: 'The selected file must be a CSV, GeoJSON, GML or GeoPackage file' + fileType: 'The selected file must be a CSV, GeoJSON, GML or GeoPackage file', + fileSize: 'The selected file must be smaller than 10MB', + fileNameTooLong: 'The selected file name must be less than 100 characters', + fileNameInvalidCharacters: 'The selected file name must not contain any of the following characters: / \\ : * ? " < > |', + fileNameDoubleExtension: 'The selected file name must not contain two file extensions' }, validationResult: { required: 'Unable to contact the API' diff --git a/src/routes/form-wizard/fields.js b/src/routes/form-wizard/fields.js index 0ea7e3d4..8b58dd64 100644 --- a/src/routes/form-wizard/fields.js +++ b/src/routes/form-wizard/fields.js @@ -13,7 +13,11 @@ export default { datafile: { validate: [ 'required', - { type: 'fileType', fn: UploadController.validateFileType } + { type: 'fileType', fn: UploadController.extensionIsValid }, + { type: 'fileSize', fn: UploadController.sizeIsValid }, + { type: 'fileNameTooLong', fn: UploadController.fileNameIsntTooLong }, + { type: 'fileNameInvalidCharacters', fn: UploadController.fileNameIsValid }, + { type: 'fileNameDoubleExtension', fn: UploadController.fileNameDoesntContainDoubleExtension } ], invalidates: ['validationResult'] }, From 82644ee732972c244fc92d92539dd6dec70565ac Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 14:34:43 +0000 Subject: [PATCH 2/9] add checks for the mime type --- src/controllers/uploadController.js | 29 ++++++++++++++++++++++++++ src/filters/validationMessageLookup.js | 4 +++- src/routes/form-wizard/fields.js | 4 +++- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index 3ef6008f..aa4492df 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -165,6 +165,35 @@ class UploadController extends PageController { return true } + static fileMimeTypeIsValid (datafile) { + const allowedMimeTypes = ['text/csv', 'application/vnd.ms-excel', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', 'application/json', 'application/vnd.geo+json', 'application/gml+xml', 'application/gpkg'] + if (!allowedMimeTypes.includes(datafile.mimetype)) { + return false + } + return true + } + + static fileMimeTypeMatchesExtension (datafile) { + const parts = datafile.originalname.split('.') + const extension = parts[parts.length - 1] + + const mimeTypes = { + csv: 'text/csv', + xls: 'application/vnd.ms-excel', + xlsx: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + json: 'application/json', + geojson: 'application/vnd.geo+json', + gml: 'application/gml+xml', + gpkg: 'application/gpkg' + } + + if (mimeTypes[extension] !== datafile.mimetype) { + return false + } + + return true + } + hasErrors () { return this.errorCount > 0 } diff --git a/src/filters/validationMessageLookup.js b/src/filters/validationMessageLookup.js index f53ecd18..6b6aae59 100644 --- a/src/filters/validationMessageLookup.js +++ b/src/filters/validationMessageLookup.js @@ -18,7 +18,9 @@ const validationMessages = { fileSize: 'The selected file must be smaller than 10MB', fileNameTooLong: 'The selected file name must be less than 100 characters', fileNameInvalidCharacters: 'The selected file name must not contain any of the following characters: / \\ : * ? " < > |', - fileNameDoubleExtension: 'The selected file name must not contain two file extensions' + fileNameDoubleExtension: 'The selected file name must not contain two file extensions', + mimeType: 'The selected file must be a CSV, GeoJSON, GML or GeoPackage file', + mimeTypeMalformed: 'The selected file has a malformed mime type' }, validationResult: { required: 'Unable to contact the API' diff --git a/src/routes/form-wizard/fields.js b/src/routes/form-wizard/fields.js index 8b58dd64..ec5141ec 100644 --- a/src/routes/form-wizard/fields.js +++ b/src/routes/form-wizard/fields.js @@ -17,7 +17,9 @@ export default { { type: 'fileSize', fn: UploadController.sizeIsValid }, { type: 'fileNameTooLong', fn: UploadController.fileNameIsntTooLong }, { type: 'fileNameInvalidCharacters', fn: UploadController.fileNameIsValid }, - { type: 'fileNameDoubleExtension', fn: UploadController.fileNameDoesntContainDoubleExtension } + { type: 'fileNameDoubleExtension', fn: UploadController.fileNameDoesntContainDoubleExtension }, + { type: 'mimeType', fn: UploadController.fileMimeTypeIsValid }, + { type: 'mimeTypeMalformed', fn: UploadController.fileMimeTypeMatchesExtension } ], invalidates: ['validationResult'] }, From 706b0c92d671f98c375cb6acf32a87fb5f10b326 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 15:27:12 +0000 Subject: [PATCH 3/9] delete the uploaded file after processing --- src/controllers/uploadController.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index aa4492df..a8925dfb 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -1,7 +1,7 @@ 'use strict' import multer from 'multer' import axios from 'axios' -import { readFile } from 'fs/promises' +import { readFile, unlink } from 'fs/promises' import { lookup } from 'mime-types' import PageController from './pageController.js' import config from '../../config/index.js' @@ -34,6 +34,7 @@ class UploadController extends PageController { jsonResult = await this.validateFile({ filePath: req.file.path, fileName: req.file.originalname, + originalname: req.file.originalname, dataset: req.sessionModel.get('dataset'), dataSubject: req.sessionModel.get('data-subject'), organisation: 'local-authority-eng:CAT', // ToDo: this needs to be dynamic, not collected in the prototype, should it be? @@ -74,6 +75,10 @@ class UploadController extends PageController { } } } + + // delete the file from the uploads folder + unlink(req.file.path) + super.post(req, res, next) } @@ -84,10 +89,7 @@ class UploadController extends PageController { } async validateFile (datafile) { - const { filePath, fileName, dataset, dataSubject, organisation, sessionId, ipAddress } = datafile - - datafile.originalname = datafile.originalname || fileName - + if ( !UploadController.extensionIsValid(datafile) || !UploadController.sizeIsValid(datafile) || @@ -98,6 +100,8 @@ class UploadController extends PageController { return false } + const { filePath, fileName, dataset, dataSubject, organisation, sessionId, ipAddress } = datafile + const formData = new FormData() formData.append('dataset', dataset) formData.append('collection', dataSubject) From dba0da2b9cf76186a8f396a4e38b758a60669085 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 15:27:18 +0000 Subject: [PATCH 4/9] linting --- src/controllers/uploadController.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index a8925dfb..53430157 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -75,7 +75,7 @@ class UploadController extends PageController { } } } - + // delete the file from the uploads folder unlink(req.file.path) @@ -89,7 +89,6 @@ class UploadController extends PageController { } async validateFile (datafile) { - if ( !UploadController.extensionIsValid(datafile) || !UploadController.sizeIsValid(datafile) || From 122b517675a7b359f28e29a8f67cbe75a0dcf02d Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 16:40:17 +0000 Subject: [PATCH 5/9] import fs instead so it can be mocked --- src/controllers/uploadController.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index 53430157..0bfb7ff1 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -1,7 +1,7 @@ 'use strict' import multer from 'multer' import axios from 'axios' -import { readFile, unlink } from 'fs/promises' +import fs from 'fs/promises' import { lookup } from 'mime-types' import PageController from './pageController.js' import config from '../../config/index.js' @@ -77,7 +77,7 @@ class UploadController extends PageController { } // delete the file from the uploads folder - unlink(req.file.path) + fs.unlink(req.file.path) super.post(req, res, next) } @@ -108,7 +108,7 @@ class UploadController extends PageController { formData.append('sessionId', sessionId) formData.append('ipAddress', ipAddress) - const file = new Blob([await readFile(filePath)], { type: lookup(filePath) }) + const file = new Blob([await fs.readFile(filePath)], { type: lookup(filePath) }) formData.append('upload_file', file, fileName) From b63540d7759ddc2862223f1f532ce610cceeafb5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 16:41:39 +0000 Subject: [PATCH 6/9] added tests for validators and also for file deletion --- test/unit/uploadController.test.js | 75 ++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 4 deletions(-) diff --git a/test/unit/uploadController.test.js b/test/unit/uploadController.test.js index d155b3d6..1f14faff 100644 --- a/test/unit/uploadController.test.js +++ b/test/unit/uploadController.test.js @@ -7,6 +7,7 @@ import UploadController from '../../src/controllers/uploadController.js' describe('UploadController', () => { let uploadController const validateFileMock = vi.fn().mockReturnValue(mockApiValue) + const unlinkMock = vi.fn() beforeEach(() => { const options = { @@ -15,14 +16,23 @@ describe('UploadController', () => { uploadController = new UploadController(options) }) - it('post adds the validation result to the session and the error count to the controller', async () => { + it('post adds the validation result to the session and the error count to the controller while deleting the uploaded file', async () => { expect(uploadController.post).toBeDefined() uploadController.validateFile = validateFileMock + vi.mock('fs/promises', async (importOriginal) => { + return { + default: { + readFile: vi.fn(), + unlink: unlinkMock + } + } + }) + const req = { file: { - path: 'readme.md', + path: 'aHashedFileName.csv', originalname: 'conservation_area.csv' }, sessionModel: { @@ -41,6 +51,7 @@ describe('UploadController', () => { expect(req.body.validationResult).toEqual(mockApiValue) expect(uploadController.errorCount).toEqual(mockApiValue['issue-log'].filter(issue => issue.severity === 'error').length + mockApiValue['column-field-log'].filter(column => column.missing).length) + expect(unlinkMock).toHaveBeenCalledWith(req.file.path) }) it('validateFile correctly calls the API', async () => { @@ -57,8 +68,8 @@ describe('UploadController', () => { expect(uploadController.validateFile).toBeDefined() const params = { - filePath: 'readme.md', - fileName: 'conservation_area.csv', + filePath: 'aHashedFileName.csv', + originalname: 'test.csv', dataset: 'test', dataSubject: 'test', organization: 'local-authority-eng:CAT' @@ -70,4 +81,60 @@ describe('UploadController', () => { // expect().toHaveBeenCalledWith(config.api.url + config.api.validationEndpoint, expect.any(FormData)) }) + + describe('validators', () => { + describe('file extension', () => { + it('should return true if the file extension is one of the accepted extensions', () => { + const allowedExtensions = ['csv', 'xls', 'xlsx', 'json', 'geojson', 'gml', 'gpkg'] + + for (const extension of allowedExtensions) { + expect(UploadController.extensionIsValid({ originalname: `test.${extension}` })).toEqual(true) + } + }) + + it('should return false if the file extension is not one of the accepted extensions', () => { + expect(UploadController.extensionIsValid({ originalname: 'test.exe' })).toEqual(false) + }) + }) + + describe('file size', () => { + it('should return true if the file size is less than the max file size', () => { + expect(UploadController.sizeIsValid({ size: 1000 })).toEqual(true) + }) + + it('should return false if the file size is greater than the max file size', () => { + expect(UploadController.sizeIsValid({ size: 100000000 })).toEqual(false) + }) + }) + + describe('file name length', () => { + it('should return true if the file name is less than the max file name length', () => { + expect(UploadController.fileNameIsntTooLong({ originalname: 'a'.repeat(100) })).toEqual(true) + }) + + it('should return false if the file name is greater than the max file name length', () => { + expect(UploadController.fileNameIsntTooLong({ originalname: 'a'.repeat(1000) })).toEqual(false) + }) + }) + + describe('file name', () => { + it('should return true if the file name is valid', () => { + expect(UploadController.fileNameIsValid({ originalname: 'test.csv' })).toEqual(true) + }) + + it('should return false if the file name contains invalid characters', () => { + expect(UploadController.fileNameIsValid({ originalname: 'test.csv?' })).toEqual(false) + }) + }) + + describe('file name double extension', () => { + it('should return true if the file name does not contain a double extension', () => { + expect(UploadController.fileNameDoesntContainDoubleExtension({ originalname: 'test.csv' })).toEqual(true) + }) + + it('should return false if the file name contains a double extension', () => { + expect(UploadController.fileNameDoesntContainDoubleExtension({ originalname: 'test.csv.csv' })).toEqual(false) + }) + }) + }) }) From b1641dcce7bebf2eac2cf087c9e6b16eda3a726f Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 17 Jan 2024 16:48:16 +0000 Subject: [PATCH 7/9] fix test --- test/unit/uploadController.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/unit/uploadController.test.js b/test/unit/uploadController.test.js index 1f14faff..2bd23538 100644 --- a/test/unit/uploadController.test.js +++ b/test/unit/uploadController.test.js @@ -4,10 +4,11 @@ import mockApiValue from '../testData/API_RUN_PIPELINE_RESPONSE.json' import UploadController from '../../src/controllers/uploadController.js' +import fs from 'fs/promises' + describe('UploadController', () => { let uploadController const validateFileMock = vi.fn().mockReturnValue(mockApiValue) - const unlinkMock = vi.fn() beforeEach(() => { const options = { @@ -25,7 +26,7 @@ describe('UploadController', () => { return { default: { readFile: vi.fn(), - unlink: unlinkMock + unlink: vi.fn() } } }) @@ -51,7 +52,7 @@ describe('UploadController', () => { expect(req.body.validationResult).toEqual(mockApiValue) expect(uploadController.errorCount).toEqual(mockApiValue['issue-log'].filter(issue => issue.severity === 'error').length + mockApiValue['column-field-log'].filter(column => column.missing).length) - expect(unlinkMock).toHaveBeenCalledWith(req.file.path) + expect(fs.unlink).toHaveBeenCalledWith(req.file.path) }) it('validateFile correctly calls the API', async () => { From 1d0a35c34d55b98138672989b339b67da2fb545f Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 18 Jan 2024 10:34:04 +0000 Subject: [PATCH 8/9] fix uncaught error when no file was uploaded --- src/controllers/uploadController.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index 0bfb7ff1..67b8df83 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -77,7 +77,8 @@ class UploadController extends PageController { } // delete the file from the uploads folder - fs.unlink(req.file.path) + if(req.file && req.file.path) + fs.unlink(req.file.path) super.post(req, res, next) } From de353136546507fee43bea156b17cb473bae5fe3 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 18 Jan 2024 10:34:17 +0000 Subject: [PATCH 9/9] linting --- src/controllers/uploadController.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/controllers/uploadController.js b/src/controllers/uploadController.js index 67b8df83..298cf2d9 100644 --- a/src/controllers/uploadController.js +++ b/src/controllers/uploadController.js @@ -77,8 +77,7 @@ class UploadController extends PageController { } // delete the file from the uploads folder - if(req.file && req.file.path) - fs.unlink(req.file.path) + if (req.file && req.file.path) { fs.unlink(req.file.path) } super.post(req, res, next) }