express-fileupload - npm Package Compare versions

Comparing version 1.2.1 to 1.3.0

lib/index.js

@@ -7,2 +7,3 @@ 'use strict';
 const { buildOptions, debugLog } = require('./utilities');
+const busboy = require('busboy'); // eslint-disable-line no-unused-vars
 
@@ -27,3 +28,3 @@ const DEFAULT_OPTIONS = {
  * Expose the file upload middleware
- * @param {Object} options - Middleware options.
+ * @param {DEFAULT_OPTIONS & busboy.BusboyConfig} options - Middleware options.
  * @returns {Function} - express-fileupload middleware.
@@ -30,0 +31,0 @@ */

lib/processMultipart.js

@@ -41,2 +41,6 @@ const Busboy = require('busboy');
 
+  // Express proxies sometimes attach multipart data to a buffer
+  if (req.body instanceof Buffer) {
+    req.body = Object.create(null);
+  }
   // Build multipart req.body fields
@@ -43,0 +47,0 @@ busboy.on('field', (field, val) => req.body = buildFields(req.body, field, val));

lib/processNested.js

@@ -1,9 +0,7 @@
-const OBJECT_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Object.prototype);
-const ARRAY_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Array.prototype);
+const { isSafeFromPollution } = require("./utilities");
 
+module.exports = function(data){
+  if (!data || data.length < 1) return Object.create(null);
 
-module.exports = function(data){
-  if (!data || data.length < 1) return {};
-  
-  let d = {},
+  let d = Object.create(null),
     keys = Object.keys(data);
@@ -18,3 +16,3 @@
         .replace(new RegExp(/\]/g), '')
-        .split('.');  
+        .split('.');
 
@@ -25,4 +23,3 @@ for (let index = 0; index < keyParts.length; index++){
       // Ensure we don't allow prototype pollution
-      const IN_ARRAY_PROTOTYPE = ARRAY_PROTOTYPE_KEYS.includes(k) && Array.isArray(current);
-      if (OBJECT_PROTOTYPE_KEYS.includes(k) || IN_ARRAY_PROTOTYPE) {
+      if (!isSafeFromPollution(current, k)) {
         continue;
@@ -34,3 +31,3 @@ }
       } else {
-        if (!current[k]) current[k] = !isNaN(keyParts[index + 1]) ? [] : {};
+        if (!current[k]) current[k] = !isNaN(keyParts[index + 1]) ? [] : Object.create(null);
         current = current[k];
@@ -40,6 +37,3 @@ }
   }
-  
-  
-  
   return d;
 };

lib/utilities.js

@@ -53,6 +53,15 @@ 'use strict';
  * Return a callback function for promise resole/reject args.
+ * Ensures that callback is called only once.
  * @returns {Function}
  */
 const promiseCallback = (resolve, reject) => {
-  return err => err ? errorFunc(resolve, reject)(err) : resolve();
+  let hasFired = false;
+  return (err) => {
+    if (hasFired) {
+      return;
+    }
+
+    hasFired = true;
+    return err ? errorFunc(resolve, reject)(err) : resolve();
+  };
 };
@@ -73,3 +82,23 @@
 
+// The default prototypes for both objects and arrays.
+// Used by isSafeFromPollution
+const OBJECT_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Object.prototype);
+const ARRAY_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Array.prototype);
+
 /**
+ * Determines whether a key insertion into an object could result in a prototype pollution
+ * @param {Object} base - The object whose insertion we are checking
+ * @param {string} key - The key that will be inserted
+ */
+const isSafeFromPollution = (base, key) => {
+  // We perform an instanceof check instead of Array.isArray as the former is more
+  // permissive for cases in which the object as an Array prototype but was not constructed
+  // via an Array constructor or literal.
+  const TOUCHES_ARRAY_PROTOTYPE = (base instanceof Array) && ARRAY_PROTOTYPE_KEYS.includes(key);
+  const TOUCHES_OBJECT_PROTOTYPE = OBJECT_PROTOTYPE_KEYS.includes(key);
+
+  return !TOUCHES_ARRAY_PROTOTYPE && !TOUCHES_OBJECT_PROTOTYPE;
+};
+
+/**
  * Builds request fields (using to build req.body and req.files)
@@ -84,3 +113,7 @@ * @param {Object} instance - request object.
   if (value === null || value === undefined) return instance;
-  instance = instance || {};
+  instance = instance || Object.create(null);
+
+  if (!isSafeFromPollution(instance, field)) {
+    return instance;
+  }
   // Non-array fields
@@ -91,3 +124,3 @@ if (!instance[field]) {
   }
-  // Array fields  
+  // Array fields
   if (instance[field] instanceof Array) {
@@ -115,3 +148,3 @@ instance[field].push(value);
   // Create folder if it doesn't exist.
-  if (!fs.existsSync(parentPath)) fs.mkdirSync(parentPath, { recursive: true }); 
+  if (!fs.existsSync(parentPath)) fs.mkdirSync(parentPath, { recursive: true });
   // Checks folder again and return a results.
@@ -185,4 +218,11 @@ return fs.existsSync(parentPath);
   let fstream = fs.createWriteStream(filePath);
-  fstream.on('error', err => callback(err));
-  fstream.on('close', () => callback());
+  // console.log("Calling saveBuffer");
+  fstream.on('error', err => {
+    // console.log("err cb")
+    callback(err);
+  });
+  fstream.on('close', () => {
+    // console.log("close cb");
+    callback();
+  });
   // Copy file via piping streams.
@@ -208,13 +248,13 @@ readStream.pipe(fstream);
 const parseFileNameExtension = (preserveExtension, fileName) => {
-  const preserveExtensionLengh = parseInt(preserveExtension);
+  const preserveExtensionLength = parseInt(preserveExtension);
   const result = {name: fileName, extension: ''};
-  if (!preserveExtension && preserveExtensionLengh !== 0) return result;
+  if (!preserveExtension && preserveExtensionLength !== 0) return result;
   // Define maximum extension length
-  const maxExtLength = isNaN(preserveExtensionLengh)
+  const maxExtLength = isNaN(preserveExtensionLength)
     ? MAX_EXTENSION_LENGTH
-    : Math.abs(preserveExtensionLengh);
+    : Math.abs(preserveExtensionLength);
 
   const nameParts = fileName.split('.');
   if (nameParts.length < 2) return result;
-  
+
   let extension = nameParts.pop();
@@ -276,3 +316,4 @@ if (
   saveBufferToFile,
-  uriDecodeFileName
+  uriDecodeFileName,
+  isSafeFromPollution
 };

package.json

 {
   "name": "express-fileupload",
-  "version": "1.2.1",
+  "version": "1.3.0",
   "author": "Richard Girges <[e-mail]>",
@@ -8,5 +8,6 @@ "description": "Simple express file upload middleware that wraps around Busboy",
   "scripts": {
-    "test": "istanbul cover node_modules/mocha/bin/_mocha -- -R spec",
+    "test": "nyc  --reporter=html --reporter=text mocha -- -R spec",
     "lint": "eslint ./",
-    "coveralls": "cat ./coverage/lcov.info | coveralls"
+    "lint:fix": "eslint --fix ./",
+    "coveralls": "nyc report --reporter=text-lcov | coveralls"
   },
@@ -33,8 +34,8 @@ "dependencies": {
     "body-parser": "^1.19.0",
-    "coveralls": "^3.0.14",
-    "eslint": "^7.5.0",
+    "coveralls": "^3.1.1",
+    "eslint": "^7.31.0",
     "express": "^4.17.1",
-    "istanbul": "^0.4.5",
-    "md5": "^2.2.1",
-    "mocha": "^8.0.1",
+    "md5": "^2.3.0",
+    "mocha": "^9.2.0",
+    "nyc": "^15.1.0",
     "rimraf": "^3.0.2",
@@ -41,0 +42,0 @@ "supertest": "^4.0.2"

README.md

@@ -5,4 +5,4 @@ # express-fileupload
 [![npm](https://img.shields.io/npm/v/express-fileupload.svg)](https://www.npmjs.org/package/express-fileupload)
-[![Build Status](https://travis-ci.com/richardgirges/express-fileupload.svg?branch=master)](https://travis-ci.com/richardgirges/express-fileupload)
 [![downloads per month](http://img.shields.io/npm/dm/express-fileupload.svg)](https://www.npmjs.org/package/express-fileupload)
+[![CircleCI](https://circleci.com/gh/richardgirges/express-fileupload/tree/master.svg?style=svg)](https://circleci.com/gh/richardgirges/express-fileupload/tree/master)
 [![Coverage Status](https://img.shields.io/coveralls/richardgirges/express-fileupload.svg)](https://coveralls.io/r/richardgirges/express-fileupload)
@@ -123,5 +123,5 @@
 # Help Wanted
-Looking for additional maintainers. Please contact `richardgirges [ at ] gmail.com` if you're interested. Pull Requests are welcomed! 
+Looking for additional maintainers. Please contact `richardgirges [ at ] gmail.com` if you're interested. Pull Requests are welcome! 
 
 # Thanks & Credit
 [Brian White](https://github.com/mscdex) for his stellar work on the [Busboy Package](https://github.com/mscdex/busboy) and the [connect-busboy Package](https://github.com/mscdex/connect-busboy)

test/utilities.spec.js

@@ -22,3 +22,4 @@ 'use strict';
   parseFileName,
-  uriDecodeFileName
+  uriDecodeFileName,
+  isSafeFromPollution
 } = require('../lib/utilities');
@@ -139,3 +140,3 @@
     });
-    
+
     it(
@@ -207,3 +208,3 @@ 'Strips away all non-alphanumeric characters (excluding hyphens/underscores) when enabled.',
       assert.deepStrictEqual(result, expectedAddon);
-    });    
+    });
 
@@ -213,3 +214,3 @@ });
   describe('Test buildOptions function', () => {
-  
+
     it('buildFields does nothing if null value has been passed', () => {
@@ -294,3 +295,3 @@ let fields = null;
       let filePath = path.join(uploadDir, getTempFilename());
-      
+
       deleteFile(filePath, function(err){
@@ -332,7 +333,7 @@ if (err) {
         });
-      });      
+      });
     });
 
   });
-  
+
   describe('Test copyFile function', function(){
@@ -346,3 +347,3 @@ beforeEach(function() {
       let dstPath = path.join(uploadDir, mockFile);
-      
+
       copyFile(srcPath, dstPath, function(err){
@@ -367,3 +368,3 @@ if (err) {
       let dstPath = path.join(uploadDir, mockFile);
-      
+
       copyFile(srcPath, dstPath, function(err){
@@ -379,3 +380,3 @@ if (err) {
       let dstPath = path.join('unknown', 'unknown');
-      
+
       copyFile(srcPath, dstPath, function(err){
@@ -412,2 +413,66 @@ if (err) {
   });
+
+  describe('Test for no prototype pollution in buildFields', function() {
+    const prototypeFields = [
+      { name: '__proto__', data: {} },
+      { name: 'constructor', data: {} },
+      { name: 'toString', data: {} }
+    ];
+
+    const nonPrototypeFields = [
+      { name: 'a', data: {} },
+      { name: 'b', data: {} }
+    ];
+
+    let fieldObject = undefined;
+    [...prototypeFields, ...nonPrototypeFields].forEach((field) => {
+      fieldObject = buildFields(fieldObject, field.name, field.data);
+    });
+
+    it(`Has ${nonPrototypeFields.length} keys`, () => {
+      assert.equal(Object.keys(fieldObject).length, nonPrototypeFields.length);
+    });
+
+    it(`Has null as its prototype`, () => {
+      assert.equal(Object.getPrototypeOf(fieldObject), null);
+    });
+
+    prototypeFields.forEach((field) => {
+      it(`${field.name} property is not an array`, () => {
+        // Note, Array.isArray is an insufficient test due to it returning false
+        // for Objects with an array prototype.
+        assert.equal(fieldObject[field.name] instanceof Array, false);
+      });
+    });
+  });
+
+  describe('Test for correct detection of prototype pollution', function() {
+    const validInsertions = [
+      { base: {}, key: 'a' },
+      { base: { a: 1 }, key: 'a' },
+      { base: { __proto__: { a: 1 } }, key: 'a' },
+      { base: [1], key: 0 },
+      { base: { __proto__: [1] }, key: 0 }
+    ];
+
+    const invalidInsertions = [
+      { base: {}, key: '__proto__' },
+      { base: {}, key: 'constructor' },
+      { base: [1], key: '__proto__' },
+      { base: [1], key: 'length' },
+      { base: { __proto__: [1] }, key: 'length' }
+    ];
+
+    validInsertions.forEach((insertion) => {
+      it(`Key ${insertion.key} should be valid for ${JSON.stringify(insertion.base)}`, () => {
+        assert.equal(isSafeFromPollution(insertion.base, insertion.key), true);
+      });
+    });
+
+    invalidInsertions.forEach((insertion) => {
+      it(`Key ${insertion.key} should not be valid for ${JSON.stringify(insertion.base)}`, () => {
+        assert.equal(isSafeFromPollution(insertion.base, insertion.key), false);
+      });
+    });
+  });
 });

No alert changes

Improved metrics

Total package byte prevSize

1217632

increased by0.34%

Lines of code

2348

increased by4.17%

No dependency changes