fix: apply sanitizeSvg to regular uploads and uploads from manage uploads acp page

src/controllers/admin/uploads.js

@@ -4,7 +4,6 @@ const path = require('path');
 const nconf = require('nconf');
 const fs = require('fs');
 const winston = require('winston');
-const sanitizeHtml = require('sanitize-html');
 
 const meta = require('../../meta');
 const posts = require('../../posts');
@@ -157,50 +156,11 @@ uploadsController.uploadCategoryPicture = async function (req, res, next) {
 		return next(new Error('[[error:invalid-json]]'));
 	}
 
-	if (uploadedFile.path.endsWith('.svg')) {
-		await sanitizeSvg(uploadedFile.path);
-	}
-
 	await validateUpload(uploadedFile, allowedImageTypes);
 	const filename = `category-${params.cid}${path.extname(uploadedFile.name)}`;
 	await uploadImage(filename, 'category', uploadedFile, req, res, next);
 };
 
-async function sanitizeSvg(filePath) {
-	const dirty = await fs.promises.readFile(filePath, 'utf8');
-	const clean = sanitizeHtml(dirty, {
-		allowedTags: [
-			'svg', 'g', 'defs', 'linearGradient', 'radialGradient', 'stop',
-			'circle', 'ellipse', 'polygon', 'polyline', 'path', 'rect',
-			'line', 'text', 'tspan', 'use', 'symbol', 'clipPath', 'mask', 'pattern',
-			'filter', 'feGaussianBlur', 'feOffset', 'feBlend', 'feColorMatrix', 'feMerge', 'feMergeNode',
-		],
-		allowedAttributes: {
-			'*': [
-				// Geometry
-				'x', 'y', 'x1', 'x2', 'y1', 'y2', 'cx', 'cy', 'r', 'rx', 'ry',
-				'width', 'height', 'd', 'points', 'viewBox', 'transform',
-
-				// Presentation
-				'fill', 'stroke', 'stroke-width', 'opacity',
-				'stop-color', 'stop-opacity', 'offset', 'style', 'class',
-
-				// Text
-				'text-anchor', 'font-size', 'font-family',
-
-				// Misc
-				'id', 'clip-path', 'mask', 'filter', 'gradientUnits', 'gradientTransform',
-				'xmlns', 'preserveAspectRatio',
-			],
-		},
-		parser: {
-			lowerCaseTags: false,
-			lowerCaseAttributeNames: false,
-		},
-	});
-	await fs.promises.writeFile(filePath, clean);
-}
-
 uploadsController.uploadFavicon = async function (req, res, next) {
 	const uploadedFile = req.files.files[0];
 	const allowedTypes = ['image/x-icon', 'image/vnd.microsoft.icon'];
@@ -296,10 +256,6 @@ uploadsController.uploadOgImage = async function (req, res, next) {
 async function upload(name, req, res, next) {
 	const uploadedFile = req.files.files[0];
 
-	if (uploadedFile.path.endsWith('.svg')) {
-		await sanitizeSvg(uploadedFile.path);
-	}
-
 	await validateUpload(uploadedFile, allowedImageTypes);
 	const filename = name + path.extname(uploadedFile.name);
 	await uploadImage(filename, 'system', uploadedFile, req, res, next);

src/file.js

@@ -7,6 +7,7 @@ const winston = require('winston');
 const { mkdirp } = require('mkdirp');
 const mime = require('mime');
 const graceful = require('graceful-fs');
+const sanitizeHtml = require('sanitize-html');
 
 const slugify = require('./slugify');
 
@@ -27,6 +28,10 @@ file.saveFileToLocal = async function (filename, folder, tempPath) {
 
 	winston.verbose(`Saving file ${filename} to : ${uploadPath}`);
 	await mkdirp(path.dirname(uploadPath));
+	if (tempPath.endsWith('.svg')) {
+		await sanitizeSvg(tempPath);
+	}
+
 	await fs.promises.copyFile(tempPath, uploadPath);
 	return {
 		url: `/assets/uploads/${folder ? `${folder}/` : ''}${filename}`,
@@ -155,4 +160,39 @@ file.walk = async function (dir) {
 	return files.reduce((a, f) => a.concat(f), []);
 };
 
+async function sanitizeSvg(filePath) {
+	const dirty = await fs.promises.readFile(filePath, 'utf8');
+	const clean = sanitizeHtml(dirty, {
+		allowedTags: [
+			'svg', 'g', 'defs', 'linearGradient', 'radialGradient', 'stop',
+			'circle', 'ellipse', 'polygon', 'polyline', 'path', 'rect',
+			'line', 'text', 'tspan', 'use', 'symbol', 'clipPath', 'mask', 'pattern',
+			'filter', 'feGaussianBlur', 'feOffset', 'feBlend', 'feColorMatrix', 'feMerge', 'feMergeNode',
+		],
+		allowedAttributes: {
+			'*': [
+				// Geometry
+				'x', 'y', 'x1', 'x2', 'y1', 'y2', 'cx', 'cy', 'r', 'rx', 'ry',
+				'width', 'height', 'd', 'points', 'viewBox', 'transform',
+
+				// Presentation
+				'fill', 'stroke', 'stroke-width', 'opacity',
+				'stop-color', 'stop-opacity', 'offset', 'style', 'class',
+
+				// Text
+				'text-anchor', 'font-size', 'font-family',
+
+				// Misc
+				'id', 'clip-path', 'mask', 'filter', 'gradientUnits', 'gradientTransform',
+				'xmlns', 'preserveAspectRatio',
+			],
+		},
+		parser: {
+			lowerCaseTags: false,
+			lowerCaseAttributeNames: false,
+		},
+	});
+	await fs.promises.writeFile(filePath, clean);
+}
+
 require('./promisify')(file);

test/files/dirty.svg

@@ -0,0 +1,4 @@
+<svg width="100" height="100" xmlns="http://www.w3.org/2000/svg">
+	<rect x="10" y="10" width="80" height="80" fill="red" stroke="black" stroke-width="4"/>
+</svg>
+<script>alert('foo');</script>

test/uploads.js

@@ -338,6 +338,15 @@ describe('Upload Controllers', () => {
 			assert.equal(body[0].url, `${nconf.get('relative_path')}/assets/uploads/category/category-1.png`);
 		});
 
+		it('should upload svg as category image after cleaning it up', async () => {
+			const { response, body } = await helpers.uploadFile(`${nconf.get('url')}/api/admin/category/uploadpicture`, path.join(__dirname, '../test/files/dirty.svg'), { params: JSON.stringify({ cid: cid }) }, jar, csrf_token);
+			assert.equal(response.statusCode, 200);
+			assert(Array.isArray(body));
+			assert.equal(body[0].url, `${nconf.get('relative_path')}/assets/uploads/category/category-1.svg`);
+			const svgContents = await fs.readFile(path.join(__dirname, '../test/uploads/category/category-1.svg'), 'utf-8');
+			assert.strictEqual(svgContents.includes('<script>'), false);
+		});
+
 		it('should upload default avatar', async () => {
 			const { response, body } = await helpers.uploadFile(`${nconf.get('url')}/api/admin/uploadDefaultAvatar`, path.join(__dirname, '../test/files/test.png'), { }, jar, csrf_token);
 			assert.equal(response.statusCode, 200);