From 72889f85d67fbee8dbd6051a73ba9ea065b54459 Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Mon, 24 Jun 2024 09:31:58 -0700 Subject: [PATCH] fix: FORMS-1292 file upload configuration bugs (#1398) * fix: FORMS-1292 file upload bugs * minor typo --- app/src/forms/file/middleware/upload.js | 123 +++++++++++------- .../unit/forms/file/middleware/upload.spec.js | 14 +- 2 files changed, 81 insertions(+), 56 deletions(-) diff --git a/app/src/forms/file/middleware/upload.js b/app/src/forms/file/middleware/upload.js index ee1338264..ca3d08d7f 100644 --- a/app/src/forms/file/middleware/upload.js +++ b/app/src/forms/file/middleware/upload.js @@ -14,21 +14,19 @@ const fileSetup = (options) => { const fileUploadsDir = (options && options.dir) || process.env.FILE_UPLOADS_DIR || fs.realpathSync(os.tmpdir()); try { fs.ensureDirSync(fileUploadsDir); - } catch (e) { + } catch (error) { throw new Error(`Could not create file uploads directory '${fileUploadsDir}'.`); } maxFileSize = (options && options.maxFileSize) || process.env.FILE_UPLOADS_MAX_FILE_SIZE || '25MB'; - try { - maxFileSize = bytes.parse(maxFileSize); - } catch (e) { + maxFileSize = bytes.parse(maxFileSize); + if (maxFileSize === null) { throw new Error('Could not determine max file size (bytes) for file uploads.'); } maxFileCount = (options && options.maxFileCount) || process.env.FILE_UPLOADS_MAX_FILE_COUNT || '1'; - try { - maxFileCount = parseInt(maxFileCount); - } catch (e) { + maxFileCount = parseInt(maxFileCount); + if (isNaN(maxFileCount)) { maxFileCount = 1; } @@ -42,19 +40,19 @@ module.exports.fileUpload = { const formFieldName = (options && options.fieldName) || process.env.FILE_UPLOADS_FIELD_NAME || 'files'; storage = multer.diskStorage({ - destination: function (req, file, cb) { - cb(null, fileUploadsDir); + destination: function (_req, _file, callback) { + callback(null, fileUploadsDir); }, }); - // set up the multer + // Set up the multer, either array for multiple upload, or single for one. if (maxFileCount > 1) { uploader = multer({ storage: storage, limits: { fileSize: maxFileSize, files: maxFileCount }, }).array(formFieldName); } else { - // just in case we set a negative number... + // Just in case we set a negative number. maxFileCount = 1; uploader = multer({ storage: storage, @@ -64,45 +62,72 @@ module.exports.fileUpload = { }, async upload(req, res, next) { - if (!uploader) { - return next(new Problem(500, 'File Upload middleware has not been configured.')); - } - uploader(req, res, (err) => { - // detect multer errors, send back nicer through the middleware stack... - if (err instanceof multer.MulterError) { - switch (err.code) { - case 'LIMIT_FILE_SIZE': - next(new Problem(400, 'Upload file error', { detail: `Upload file size is limited to ${maxFileSize} bytes` })); - break; - case 'LIMIT_FILE_COUNT': - next(new Problem(400, 'Upload file error', { detail: `Upload is limited to ${maxFileCount} files` })); - break; - case 'LIMIT_UNEXPECTED_FILE': - next(new Problem(400, 'Upload file error', { detail: 'Upload encountered an unexpected file' })); - break; - // we don't expect that we will encounter these in our api/app, but here for completeness - case 'LIMIT_PART_COUNT': - next(new Problem(400, 'Upload file error', { detail: 'Upload rejected: upload form has too many parts' })); - break; - case 'LIMIT_FIELD_KEY': - next(new Problem(400, 'Upload file error', { detail: 'Upload rejected: upload field name for the files is too long' })); - break; - case 'LIMIT_FIELD_VALUE': - next(new Problem(400, 'Upload file error', { detail: 'Upload rejected: upload field is too long' })); - break; - case 'LIMIT_FIELD_COUNT': - next(new Problem(400, 'Upload file error', { detail: 'Upload rejected: too many fields' })); - break; - default: - next(new Problem(400, 'Upload file error', { detail: `Upload failed with the following error: ${err.message}` })); + try { + if (!uploader) { + throw new Problem(500, { + detail: 'File Upload middleware has not been configured.', + }); + } + + uploader(req, res, (error) => { + // Detect multer errors, send back nicer through the middleware stack. + if (error instanceof multer.MulterError) { + switch (error.code) { + case 'LIMIT_FILE_SIZE': + throw new Problem(400, { + detail: `Upload file size is limited to ${maxFileSize} bytes`, + }); + + case 'LIMIT_FILE_COUNT': + throw new Problem(400, { + detail: `Upload is limited to ${maxFileCount} files`, + }); + + case 'LIMIT_UNEXPECTED_FILE': + throw new Problem(400, { + detail: 'Upload encountered an unexpected file', + }); + + // We don't expect that we will encounter these in our api/app, but + // here for completeness. + + case 'LIMIT_PART_COUNT': + throw new Problem(400, { + detail: 'Upload rejected: upload form has too many parts', + }); + + case 'LIMIT_FIELD_KEY': + throw new Problem(400, { + detail: 'Upload rejected: upload field name for the files is too long', + }); + + case 'LIMIT_FIELD_VALUE': + throw new Problem(400, { + detail: 'Upload rejected: upload field is too long', + }); + + case 'LIMIT_FIELD_COUNT': + throw new Problem(400, { + detail: 'Upload rejected: too many fields', + }); + + default: + throw new Problem(400, { + detail: `Upload failed with the following error: ${error.message}`, + }); + } + } + + if (error) { + throw new Problem(400, { + detail: error.message, + }); } - } else if (err) { - // send this error to express... - next(new Problem(400, 'Unknown upload file error', { detail: err.message })); - } else { - // all good, carry on. + next(); - } - }); + }); + } catch (error) { + next(error); + } }, }; diff --git a/app/tests/unit/forms/file/middleware/upload.spec.js b/app/tests/unit/forms/file/middleware/upload.spec.js index 7eb194abc..fef002faa 100644 --- a/app/tests/unit/forms/file/middleware/upload.spec.js +++ b/app/tests/unit/forms/file/middleware/upload.spec.js @@ -13,7 +13,7 @@ const multerImpl = { }; multer.mockImplementation(() => multerImpl); -// This module has global variables so it need to be re-loaded for each test. +// This module has global variables so it needs to be re-loaded for each test. var fileUpload; beforeEach(() => { @@ -147,8 +147,7 @@ describe('fileUpload.init', () => { ); }); - // TODO: implementation is broken, bytes.parse does not throw exceptions. - test.skip('uses the config but fails on conversion', async () => { + test('uses the config but fails on conversion', async () => { expect(() => fileUpload.init({ maxFileSize: 'qwerty', @@ -271,7 +270,6 @@ describe('fileUpload.upload', () => { expect(next).toHaveBeenCalledWith( expect.objectContaining({ detail: detail, - title: 'Upload file error', }) ); }); @@ -291,7 +289,6 @@ describe('fileUpload.upload', () => { expect(next).toHaveBeenCalledWith( expect.objectContaining({ detail: 'error message', - title: 'Unknown upload file error', }) ); }); @@ -322,8 +319,11 @@ describe('fileUpload.upload', () => { expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith(expect.objectContaining(expectedStatus)); - // TODO: the 500 should be setting the detail, not the title. - expect(next).toHaveBeenCalledWith(expect.objectContaining({ title: 'File Upload middleware has not been configured.' })); + expect(next).toHaveBeenCalledWith( + expect.objectContaining({ + detail: 'File Upload middleware has not been configured.', + }) + ); }); }); });