Fix #4979: Replace connect-multiparty with multer to resolve security vulnerability (#5013)

* Fix #4979: Replace connect-multiparty with multer to resolve security vulnerability

- Remove connect-multiparty dependency (deprecated with CVE-2022-29623)
- Add multer v1.4.5-lts.1 as replacement for file upload handling
- Update attachment upload route to use multer.single('file')
- Update rich-text CSV upload to use multer middleware
- Migrate big-upload middleware to multer.any() for chunked uploads
- Update test suite to handle multer file format
- Maintain backward compatibility with file.name fallback
- Move form-data to devDependencies as only needed for testing

* Fix backwards compatibility for big-upload-middleware

- Revert req.files from array back to object format to maintain connect-multiparty API
- Use original property names (name, type) instead of multer names (originalname, mimetype)
- Revert test changes since the public API should not change
- This addresses @boutell's feedback about maintaining the documented interface

* Clean up: Remove temporary test file and fix formatting

- Remove test-big-upload-fix.js (temporary test file)
- Clean up formatting in big-upload.js test

* Upgrade multer to v2.0.2 to address security vulnerability

- Update multer from v1.4.5-lts.1 (vulnerable) to v2.0.2 (secure)
- Maintains full API compatibility with existing implementation
- No code changes required - multer 2.x maintains same interface
- Verified: npm audit shows 0 vulnerabilities

Fixes: #5013 (review)

Author: gingeekrishna

modules/@apostrophecms/attachment/index.js

@@ -159,12 +159,12 @@ module.exports = {
       post: {
         upload: [
           self.canUpload,
-          require('connect-multiparty')(),
+          //In the existing code, we are reading the zeroth element from the files array object, which results in processing only a single file. Therefore, I am currently reading just one file from the Multer package.
+          require('multer')({ dest: require('os').tmpdir() }).single('file'),
           async function (req) {
             try {
-              // The name attribute could be anything because of how fileupload
-              // controls work; we don't really care.
-              const file = Object.values(req.files || {})[0];
+              // The file comes from multer middleware
+              const file = req.file;
 
               if (!file) {
                 throw self.apos.error('invalid');
@@ -175,11 +175,12 @@ module.exports = {
 
               return attachment;
             } finally {
-              for (const file of (Object.values(req.files || {}))) {
+              //Hence I am reading the single file from the upload and I am checking the condtion for the same
+              if (req.file) {
                 try {
-                  fs.unlinkSync(file.path);
+                  fs.unlinkSync(req.file.path);
                 } catch (e) {
-                  self.apos.util.warn(`Uploaded temporary file ${file.path} was already removed, this should have been the responsibility of the upload route`);
+                  self.apos.util.warn(`Uploaded temporary file ${req.file.path} was already removed, this should have been the responsibility of the upload route`);
                 }
               }
             }
@@ -393,7 +394,9 @@ module.exports = {
       // object, suitable for passing to the `url` API and for use as the value
       // of a `type: 'attachment'` schema field.
       async insert(req, file, options = {}) {
-        let extension = path.extname(file.name);
+        // Handle both multer (originalname) and connect-multiparty (name) formats
+        const fileName = file.originalname || file.name;
+        let extension = path.extname(fileName);
         if (extension && extension.length) {
           extension = extension.substr(1);
         }
@@ -420,9 +423,9 @@ module.exports = {
           _id: options.attachmentId ?? self.apos.util.generateId(),
           group: group.name,
           createdAt: new Date(),
-          name: self.apos.util.slugify(path.basename(file.name, path.extname(file.name))),
+          name: self.apos.util.slugify(path.basename(fileName, path.extname(fileName))),
           title: self.apos.util.sortify(
-            path.basename(file.name, path.extname(file.name))
+            path.basename(fileName, path.extname(fileName))
           ),
           extension,
           type: 'attachment',

modules/@apostrophecms/http/lib/big-upload-middleware.js

@@ -1,4 +1,4 @@
-const multiparty = require('connect-multiparty');
+const multer = require('multer');
 const util = require('util');
 const {
   readFile, open, unlink
@@ -17,10 +17,10 @@ module.exports = (self) => ({
 
   bigUploadMiddleware({ authorize } = {}) {
     return (req, res, next) => {
-      // Chain the multiparty middleware to handle normal uploads
+      // Chain the multer middleware to handle normal uploads
       // as chunks (more efficient than base64 etc)
-      const multipartyFn = multiparty();
-      return multipartyFn(req, res, () => {
+      const multerFn = multer({ dest: require('os').tmpdir() }).any();
+      return multerFn(req, res, () => {
         return body(req, res, next);
       });
     };
@@ -56,10 +56,10 @@ module.exports = (self) => ({
           });
         }
       } finally {
-        // Clean up multiparty temporary files
-        for (const { path } of Object.values(origFiles || {})) {
+        // Clean up multer temporary files
+        for (const file of (origFiles || [])) {
           try {
-            await unlink(path);
+            await unlink(file.path);
           } catch (e) {
             // OK if it is already gone
           }
@@ -136,7 +136,7 @@ module.exports = (self) => ({
       if ((chunk < 0) || (chunk >= info.chunks)) {
         throw self.apos.error('invalid', 'chunk out of range');
       }
-      const file = req.files.chunk;
+      const file = req.files.find(f => f.fieldname === 'chunk') || req.files[0];
       const ufs = self.getBigUploadFs();
       const ufsPath = `/big-uploads/${id}-${n}-${chunk}`;
       await ufs.copyIn(file.path, ufsPath);

modules/@apostrophecms/rich-text-widget/lib/apiRoutes.js

@@ -1,5 +1,5 @@
 const fs = require('node:fs');
-const connectMultiparty = require('connect-multiparty');
+const multer = require('multer');
 const { pipeline } = require('stream/promises');
 const { parse: csvParse } = require('csv-parse');
 const { Transform } = require('stream');
@@ -9,14 +9,14 @@ module.exports = self => {
   return {
     post: {
       generateCsvTable: [
-        connectMultiparty(),
+        multer({ dest: require('os').tmpdir() }).single('file'),
         async (req) => {
-          const { file } = req.files || {};
+          const file = req.file;
           if (!file) {
             throw self.apos.error('invalid', 'A file is required');
           }
 
-          const extension = file.name.split('.').pop();
+          const extension = file.originalname.split('.').pop();
           if (extension !== 'csv') {
             throw self.apos.error('invalid', 'Only csv files are supported');
           }

package.json

@@ -65,7 +65,6 @@
     "chokidar": "^3.5.2",
     "common-tags": "^1.8.0",
     "connect-mongo": "^5.1.0",
-    "connect-multiparty": "^2.1.1",
     "cookie-parser": "^1.4.5",
     "cors": "^2.8.5",
     "css-loader": "^5.2.4",
@@ -76,7 +75,6 @@
     "express-bearer-token": "^3.0.0",
     "express-cache-on-demand": "^1.0.3",
     "express-session": "^1.18.2",
-    "form-data": "^4.0.0",
     "fs-extra": "^7.0.1",
     "glob": "^10.4.5",
     "he": "^1.2.0",
@@ -92,6 +90,7 @@
     "mini-css-extract-plugin": "^1.6.0",
     "minimatch": "^3.0.4",
     "mkdirp": "^0.5.5",
+    "multer": "^2.0.2",
     "node-fetch": "^2.6.1",
     "nodemailer": "^6.6.1",
     "nunjucks": "^3.2.1",
@@ -134,6 +133,7 @@
   },
   "devDependencies": {
     "eslint-config-apostrophe": "^5.0.0",
+    "form-data": "^4.0.4",
     "mocha": "^10.7.3",
     "nyc": "^15.1.0",
     "replace-in-file": "^6.1.0",