Skip to content

Commit

Permalink
fix: FORMS-1292 file upload configuration bugs (bcgov#1398)
Browse files Browse the repository at this point in the history 
* fix: FORMS-1292 file upload bugs

* minor typo
  • Loading branch information
@WalterMoar
WalterMoar authored Jun 24, 2024
1 parent 152cf2d commit 72889f8
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 56 deletions.
123 changes: 74 additions & 49 deletions app/src/forms/file/middleware/upload.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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,
Expand All @@ -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);
}
},
};
14 changes: 7 additions & 7 deletions app/tests/unit/forms/file/middleware/upload.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -271,7 +270,6 @@ describe('fileUpload.upload', () => {
expect(next).toHaveBeenCalledWith(
expect.objectContaining({
detail: detail,
title: 'Upload file error',
})
);
});
Expand All @@ -291,7 +289,6 @@ describe('fileUpload.upload', () => {
expect(next).toHaveBeenCalledWith(
expect.objectContaining({
detail: 'error message',
title: 'Unknown upload file error',
})
);
});
Expand Down Expand Up @@ -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.',
})
);
});
});
});

0 comments on commit 72889f8

Please sign in to comment.