fix: dont send 200 status on admin upload errors (#11707)

* fix: dont send 200 status on admin upload errors

* test: update test

* bring back both checks for error

* test: add statusCode tests

public/src/modules/uploader.js

@@ -61,6 +61,7 @@ define('uploader', ['jquery-form'], function () {
 		if (type === 'error') {
 			uploadModal.find('#fileUploadSubmitBtn').removeClass('disabled');
 		}
+		message = message.replace(/&#44/g, '&#44');
 		uploadModal.find('#alert-' + type).translateText(message).removeClass('hide');
 	}
 
@@ -72,7 +73,13 @@ define('uploader', ['jquery-form'], function () {
 			},
 			error: function (xhr) {
 				xhr = maybeParse(xhr);
-				showAlert(uploadModal, 'error', xhr.responseJSON?.status?.message || `[[error:upload-error-fallback, ${xhr.status} ${xhr.statusText}]]`);
+				showAlert(
+					uploadModal,
+					'error',
+					xhr.responseJSON?.status?.message || // apiv3
+					xhr.responseJSON?.error || // { "error": "[[error:some-error]]]" }
+					`[[error:upload-error-fallback, ${xhr.status} ${xhr.statusText}]]`
+				);
 			},
 			uploadProgress: function (event, position, total, percent) {
 				uploadModal.find('#upload-progress-bar').css('width', percent + '%');
@@ -99,7 +106,7 @@ define('uploader', ['jquery-form'], function () {
 	function maybeParse(response) {
 		if (typeof response === 'string') {
 			try {
-				return $.parseJSON(response);
+				return JSON.parse(response);
 			} catch (e) {
 				return { error: '[[error:parse-error]]' };
 			}

src/controllers/admin/uploads.js

@@ -118,17 +118,16 @@ uploadsController.uploadCategoryPicture = async function (req, res, next) {
 		return next(new Error('[[error:invalid-json]]'));
 	}
 
-	if (validateUpload(res, uploadedFile, allowedImageTypes)) {
+	await validateUpload(uploadedFile, allowedImageTypes);
 	const filename = `category-${params.cid}${path.extname(uploadedFile.name)}`;
 	await uploadImage(filename, 'category', uploadedFile, req, res, next);
-	}
 };
 
 uploadsController.uploadFavicon = async function (req, res, next) {
 	const uploadedFile = req.files.files[0];
 	const allowedTypes = ['image/x-icon', 'image/vnd.microsoft.icon'];
 
-	if (validateUpload(res, uploadedFile, allowedTypes)) {
+	await validateUpload(uploadedFile, allowedTypes);
 	try {
 		const imageObj = await file.saveFileToLocal('favicon.ico', 'system', uploadedFile.path);
 		res.json([{ name: uploadedFile.name, url: imageObj.url }]);
@@ -137,7 +136,6 @@ uploadsController.uploadFavicon = async function (req, res, next) {
 	} finally {
 		file.delete(uploadedFile.path);
 	}
-	}
 };
 
 uploadsController.uploadTouchIcon = async function (req, res, next) {
@@ -145,7 +143,7 @@ uploadsController.uploadTouchIcon = async function (req, res, next) {
 	const allowedTypes = ['image/png'];
 	const sizes = [36, 48, 72, 96, 144, 192, 512];
 
-	if (validateUpload(res, uploadedFile, allowedTypes)) {
+	await validateUpload(uploadedFile, allowedTypes);
 	try {
 		const imageObj = await file.saveFileToLocal('touchicon-orig.png', 'system', uploadedFile.path);
 		// Resize the image into squares for use as touch icons at various DPIs
@@ -164,7 +162,6 @@ uploadsController.uploadTouchIcon = async function (req, res, next) {
 	} finally {
 		file.delete(uploadedFile.path);
 	}
-	}
 };
 
 
@@ -172,7 +169,7 @@ uploadsController.uploadMaskableIcon = async function (req, res, next) {
 	const uploadedFile = req.files.files[0];
 	const allowedTypes = ['image/png'];
 
-	if (validateUpload(res, uploadedFile, allowedTypes)) {
+	await validateUpload(uploadedFile, allowedTypes);
 	try {
 		const imageObj = await file.saveFileToLocal('maskableicon-orig.png', 'system', uploadedFile.path);
 		res.json([{ name: uploadedFile.name, url: imageObj.url }]);
@@ -181,7 +178,6 @@ uploadsController.uploadMaskableIcon = async function (req, res, next) {
 	} finally {
 		file.delete(uploadedFile.path);
 	}
-	}
 };
 
 uploadsController.uploadLogo = async function (req, res, next) {
@@ -219,20 +215,16 @@ uploadsController.uploadOgImage = async function (req, res, next) {
 async function upload(name, req, res, next) {
 	const uploadedFile = req.files.files[0];
 
-	if (validateUpload(res, uploadedFile, allowedImageTypes)) {
+	await validateUpload(uploadedFile, allowedImageTypes);
 	const filename = name + path.extname(uploadedFile.name);
 	await uploadImage(filename, 'system', uploadedFile, req, res, next);
 }
-}
 
-function validateUpload(res, uploadedFile, allowedTypes) {
+async function validateUpload(uploadedFile, allowedTypes) {
 	if (!allowedTypes.includes(uploadedFile.type)) {
 		file.delete(uploadedFile.path);
-		res.json({ error: `[[error:invalid-image-type, ${allowedTypes.join(', ')}]]` });
-		return false;
+		throw new Error(`[[error:invalid-image-type, ${allowedTypes.join(', ')}]]`);
 	}
-
-	return true;
 }
 
 async function uploadImage(filename, folder, uploadedFile, req, res, next) {

src/views/modals/upload-file.tpl

@@ -6,10 +6,10 @@
 				<button type="button" class="btn-close" data-bs-dismiss="modal" aria-hidden="true"></button>
 			</div>
 			<div class="modal-body">
-				<form id="uploadForm" action="" method="post" enctype="multipart/form-data">
-					<div class="form-group">
+				<form class="mb-3" id="uploadForm" action="" method="post" enctype="multipart/form-data">
+					<div>
 						{{{ if description }}}
-						<label for="fileInput">{description}</label>
+						<label class="form-label" for="fileInput">{description}</label>
 						{{{ end }}}
 						<input type="file" id="fileInput" name="files[]" {{{ if accept }}}accept="{accept}"{{{ end }}}>
 						{{{ if showHelp }}}
@@ -25,7 +25,7 @@
 					<input type="hidden" id="params" name="params" />
 				</form>
 
-				<div id="upload-progress-box" class="progress progress-striped hide">
+				<div id="upload-progress-box" class="progress progress-striped hide mb-3">
 					<div id="upload-progress-bar" class="progress-bar progress-bar-success" role="progressbar" aria-valuenow="0" aria-valuemin="0">
 						<span class="sr-only"> [[success:success]]</span>
 					</div>

test/uploads.js

@@ -340,7 +340,7 @@ describe('Upload Controllers', () => {
 		it('should upload site logo', (done) => {
 			helpers.uploadFile(`${nconf.get('url')}/api/admin/uploadlogo`, path.join(__dirname, '../test/files/test.png'), {}, jar, csrf_token, (err, res, body) => {
 				assert.ifError(err);
-				assert.equal(res.statusCode, 200);
+				assert.strictEqual(res.statusCode, 200);
 				assert(Array.isArray(body));
 				assert.equal(body[0].url, `${nconf.get('relative_path')}/assets/uploads/system/site-logo.png`);
 				done();
@@ -350,7 +350,8 @@ describe('Upload Controllers', () => {
 		it('should fail to upload invalid file type', (done) => {
 			helpers.uploadFile(`${nconf.get('url')}/api/admin/category/uploadpicture`, path.join(__dirname, '../test/files/503.html'), { params: JSON.stringify({ cid: cid }) }, jar, csrf_token, (err, res, body) => {
 				assert.ifError(err);
-				assert.equal(body.error, '[[error:invalid-image-type, image/png, image/jpeg, image/pjpeg, image/jpg, image/gif, image/svg+xml]]');
+				assert.strictEqual(res.statusCode, 500);
+				assert.equal(body.error, '[[error:invalid-image-type, image/png, image/jpeg, image/pjpeg, image/jpg, image/gif, image/svg+xml]]');
 				done();
 			});
 		});
@@ -358,6 +359,7 @@ describe('Upload Controllers', () => {
 		it('should fail to upload category image with invalid json params', (done) => {
 			helpers.uploadFile(`${nconf.get('url')}/api/admin/category/uploadpicture`, path.join(__dirname, '../test/files/test.png'), { params: 'invalid json' }, jar, csrf_token, (err, res, body) => {
 				assert.ifError(err);
+				assert.strictEqual(res.statusCode, 500);
 				assert.equal(body.error, '[[error:invalid-json]]');
 				done();
 			});