improve security for file upload

src/controllers/uploadController.js

@@ -1,7 +1,7 @@
 'use strict'
 import multer from 'multer'
 import axios from 'axios'
-import { readFile } from 'fs/promises'
+import fs from 'fs/promises'
 import { lookup } from 'mime-types'
 import PageController from './pageController.js'
 import config from '../../config/index.js'
@@ -34,18 +34,23 @@ 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?
           sessionId: await hash(req.sessionID),
           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)
@@ -70,6 +75,10 @@ class UploadController extends PageController {
         }
       }
     }
+
+    // delete the file from the uploads folder
+    if (req.file && req.file.path) { fs.unlink(req.file.path) }
+
     super.post(req, res, next)
   }
 
@@ -79,21 +88,27 @@ class UploadController extends PageController {
     this.validationErrorMessage = message
   }
 
-  async validateFile ({ filePath, fileName, dataset, dataSubject, organisation, sessionId, ipAddress }) {
-    const validFileType = UploadController.validateFileType({ originalname: fileName })
-
-    if (!validFileType) {
+  async validateFile (datafile) {
+    if (
+      !UploadController.extensionIsValid(datafile) ||
+      !UploadController.sizeIsValid(datafile) ||
+      !UploadController.fileNameIsntTooLong(datafile) ||
+      !UploadController.fileNameIsValid(datafile) ||
+      !UploadController.fileNameDoesntContainDoubleExtension(datafile)
+    ) {
       return false
     }
 
+    const { filePath, fileName, dataset, dataSubject, organisation, sessionId, ipAddress } = datafile
+
     const formData = new FormData()
     formData.append('dataset', dataset)
     formData.append('collection', dataSubject)
     formData.append('organisation', organisation)
     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)
 
@@ -106,25 +121,82 @@ 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
+  }
+
+  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
   }

src/filters/validationMessageLookup.js

@@ -14,7 +14,13 @@ 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',
+    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'

src/routes/form-wizard/fields.js

@@ -13,7 +13,13 @@ 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 },
+      { type: 'mimeType', fn: UploadController.fileMimeTypeIsValid },
+      { type: 'mimeTypeMalformed', fn: UploadController.fileMimeTypeMatchesExtension }
     ],
     invalidates: ['validationResult']
   },

test/unit/uploadController.test.js

@@ -4,6 +4,8 @@ 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)
@@ -15,14 +17,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: vi.fn()
+        }
+      }
+    })
+
     const req = {
       file: {
-        path: 'readme.md',
+        path: 'aHashedFileName.csv',
         originalname: 'conservation_area.csv'
       },
       sessionModel: {
@@ -41,6 +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(fs.unlink).toHaveBeenCalledWith(req.file.path)
   })
 
   it('validateFile correctly calls the API', async () => {
@@ -57,8 +69,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 +82,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)
+      })
+    })
+  })
 })