express-fileupload - npm Package Compare versions

Comparing version 1.4.1 to 1.4.2

lib/isEligibleRequest.js

@@ -1,7 +0,7 @@
-const ACCEPTABLE_CONTENT_TYPE = /^(multipart\/.+);(.*)$/i;
-const UNACCEPTABLE_METHODS = ['GET', 'HEAD'];
+const ACCEPTABLE_CONTENT_TYPE = /^multipart\/[\w'"()+-_?/:=,.]+(?:; ?[\w'"()+-_?/:=,.]*)+$/i;
+const UNACCEPTABLE_METHODS = new Set(['GET', 'HEAD', 'DELETE', 'OPTIONS', 'CONNECT', 'TRACE']);
 
 /**
  * Ensures the request contains a content body
- * @param  {Object}  req Express req object
+ * @param  {Object} req Express req object
  * @returns {Boolean}
@@ -17,13 +17,19 @@ */
  * such as GET or HEAD
- * @param  {Object}  req Express req object
+ * @param  {Object} req Express req object
  * @returns {Boolean}
  */
-const hasAcceptableMethod = req => !UNACCEPTABLE_METHODS.includes(req.method);
+const hasAcceptableMethod = (req) => !UNACCEPTABLE_METHODS.has(req.method);
 
 /**
  * Ensures that only multipart requests are processed by express-fileupload
+ * ACCEPTABLE_CONTENT_TYPE REgex is based on the RFC 2046
+ * Validates special characters according to RFC 2046, section 5.1.1: '"()+_-=?/:
+ * Also checks for the presence of boundary in the header.
  * @param  {Object}  req Express req object
  * @returns {Boolean}
  */
-const hasAcceptableContentType = req => ACCEPTABLE_CONTENT_TYPE.test(req.headers['content-type']);
+const hasAcceptableContentType = (req) => {
+  const contType = req.headers['content-type'];
+  return contType.includes('boundary=') && ACCEPTABLE_CONTENT_TYPE.test(contType);
+};
 
@@ -35,2 +41,2 @@ /**
  */
-module.exports = req => hasBody(req) && hasAcceptableMethod(req) && hasAcceptableContentType(req);
+module.exports = (req) => hasBody(req) && hasAcceptableMethod(req) && hasAcceptableContentType(req);

lib/processMultipart.js

@@ -34,7 +34,19 @@ const Busboy = require('busboy');
 
-  // Close connection with specified reason and http code, default: 400 Bad Request.
+  /**
+   * Closes connection with specified reason and http code.
+   * @param {number} code HTTP response code, default: 400.
+   * @param {*} reason Reason to close connection, default: 'Bad Request'.
+   */
   const closeConnection = (code, reason) => {
     req.unpipe(busboy);
-    res.writeHead(code || 400, { Connection: 'close' });
-    res.end(reason || 'Bad Request');
+    req.resume();
+    if (res.headersSent) {
+      debugLog(options, 'Headers already sent, can\'t close connection.');
+      return;
+    }
+    const resCode = code || 400;
+    const resReason = reason || 'Bad Request';
+    debugLog(options, `Closing connection with ${resCode}: ${resReason}`);
+    res.writeHead(resCode, { Connection: 'close' });
+    res.end(resReason);
   };
@@ -80,3 +92,6 @@
       // After destroy an error event will be emitted and file clean up will be done.
-      file.destroy(new Error(`Upload timeout ${field}->${filename}, bytes:${getFileSize()}`));
+      // In some cases file.destroy() doesn't exist, so we need to check this, see issue:
+      // https://github.com/richardgirges/express-fileupload/issues/259.
+      const err = new Error(`Upload timeout for ${field}->${filename}, bytes:${getFileSize()}`);
+      return isFunc(file.destroy) ? file.destroy(err) : file.emit('error', err);
     });
@@ -89,7 +104,9 @@
       // Run a user defined limit handler if it has been set.
-      if (isFunc(options.limitHandler)) return options.limitHandler(req, res, next);
+      if (isFunc(options.limitHandler)) {
+        options.limitHandler(req, res, next);
+      }
       // Close connection with 413 code and do cleanup if abortOnLimit set(default: false).
       if (options.abortOnLimit) {
         debugLog(options, `Aborting upload because of size limit ${field}->${filename}.`);
-        !isFunc(options.limitHandler) ? closeConnection(413, options.responseOnLimit) : '';
+        closeConnection(413, options.responseOnLimit);
         cleanup();
@@ -96,0 +113,0 @@ }

lib/tempFileHandler.js

@@ -57,3 +57,3 @@ const fs = require('fs');
       writeStream.end();
-      deleteFile(tempFilePath, err => (err 
+      deleteFile(tempFilePath, err => (err
         ? debugLog(options, `Cleaning up temporary file ${tempFilePath} failed: ${err}`)
@@ -60,0 +60,0 @@ : debugLog(options, `Cleaning up temporary file ${tempFilePath} done.`)

lib/utilities.js

@@ -188,8 +188,13 @@ 'use strict';
  * @param {string} dst - Path to the destination file.
- * @param {Function} callback - A callback function.
+ * @param {Function} callback - A callback function with renamed flag.
  */
-const moveFile = (src, dst, callback) => fs.rename(src, dst, err => (err
-  ? copyFile(src, dst, err => err ? callback(err) : deleteFile(src, callback))
-  : callback()
-));
+const moveFile = (src, dst, callback) => fs.rename(src, dst, (err) => {
+  if (err) {
+    // Try to copy file if rename didn't work.
+    copyFile(src, dst, (cpErr) => (cpErr ? callback(cpErr) : deleteFile(src, callback)));
+    return;
+  }
+  // File was renamed successfully: Add true to the callback to indicate that.
+  callback(null, true);
+});
 
@@ -229,2 +234,3 @@ /**
  * Decodes uriEncoded file names.
+ * @param {Object} opts - middleware options.
  * @param fileName {String} - file name to decode.
@@ -234,4 +240,3 @@ * @returns {String}
 const uriDecodeFileName = (opts, fileName) => {
-  const options = opts || {};
-  if (!options.uriDecodeFileNames) {
+  if (!opts || !opts.uriDecodeFileNames) {
     return fileName;
@@ -238,0 +243,0 @@ }

package.json

 {
   "name": "express-fileupload",
-  "version": "1.4.1",
+  "version": "1.4.2",
   "author": "Richard Girges <[e-mail]>",
@@ -5,0 +5,0 @@ "description": "Simple express file upload middleware that wraps around Busboy",

test/server.js

@@ -23,6 +23,13 @@ 'use strict';
   try {
-    if (fs.existsSync(dir)) rimraf.sync(dir);
+    try {
+      const stats = fs.statSync(dir);
+      if (stats.isDirectory()) rimraf.sync(dir);
+    } catch (statsErr) {
+      if (statsErr.code !== 'ENOENT') {
+        throw statsErr;
+      }
+    }
     fs.mkdirSync(dir, { recursive: true });
   } catch (err) {
-    // 
+    console.error(err); // eslint-disable-line
   }
@@ -29,0 +36,0 @@ };

test/utilities.spec.js

@@ -20,2 +20,3 @@ 'use strict';
   copyFile,
+  moveFile,
   saveBufferToFile,
@@ -31,7 +32,7 @@ parseFileName,
 
+const mockFileMove = 'car.png';
+const mockBufferMove = fs.readFileSync(path.join(fileDir, mockFileMove));
+const mockHashMove = md5(mockBufferMove);
 
 describe('utilities: Test of the utilities functions', function() {
-  beforeEach(function() {
-    server.clearUploadsDir();
-  });
   //debugLog tests
@@ -251,7 +252,39 @@ describe('Test debugLog function', () => {
   });
+
+  describe('Test moveFile function', function() {
+    beforeEach(() => server.clearUploadsDir());
+
+    it('Should rename a file and check a hash', function(done) {
+      const srcPath = path.join(fileDir, mockFileMove);
+      const dstPath = path.join(uploadDir, mockFileMove);
+
+      moveFile(srcPath, dstPath, function(err, renamed) {
+        if (err) {
+          return done(err);
+        }
+        fs.stat(dstPath, (err) => {
+          if (err) {
+            return done(err);
+          }
+          // Match source and destination files hash.
+          const fileBuffer = fs.readFileSync(dstPath);
+          const fileHash = md5(fileBuffer);
+          if (fileHash !== mockHashMove) {
+            return done(new Error('Hashes do not match'));
+          }
+          // Check that source file was deleted.
+          fs.stat(srcPath, (err) => {
+            if (err) {
+              return done(renamed ? null : new Error('Source file was not renamed'));
+            }
+            done(new Error('Source file was not deleted'));
+          });
+        });
+      });
+    });
+  });
+
   //saveBufferToFile tests
   describe('Test saveBufferToFile function', function(){
-    beforeEach(function() {
-      server.clearUploadsDir();
-    });
+    beforeEach(() => server.clearUploadsDir());
 
@@ -288,5 +321,3 @@ it('Save buffer to a file', function(done) {
   describe('Test deleteFile function', function(){
-    beforeEach(function() {
-      server.clearUploadsDir();
-    });
+    beforeEach(() => server.clearUploadsDir());
 
@@ -306,4 +337,3 @@ it('Failed if nonexistent file passed', function(done){
       let dstPath = path.join(uploadDir, getTempFilename());
-
-      //copy a file
+      // copy a file
       copyFile(srcPath, dstPath, function(err){
@@ -314,3 +344,3 @@ if (err) {
         fs.stat(dstPath, (err)=>{
-          if (err){
+          if (err) {
             return done(err);
@@ -323,10 +353,4 @@ }
             }
-
-            fs.stat(dstPath, (err)=>{
-              if (err){
-                return done();
-              }
-
-              //error if a file still exist
-              done(err);
+            fs.stat(dstPath, (err)=> {
+              return err ? done() : done(new Error('File was not deleted'));
             });
@@ -340,6 +364,4 @@ });
 
-  describe('Test copyFile function', function(){
-    beforeEach(function() {
-      server.clearUploadsDir();
-    });
+  describe('Test copyFile function', function() {
+    beforeEach(() => server.clearUploadsDir());
 
@@ -358,6 +380,6 @@ it('Copy a file and check a hash', function(done) {
           }
-          //Match source and destination files hash.
+          // Match source and destination files hash.
           let fileBuffer = fs.readFileSync(dstPath);
           let fileHash = md5(fileBuffer);
-          return (fileHash === mockHash) ? done() : done(err);
+          return fileHash === mockHash ? done() : done(new Error('Hashes do not match'));
         });
@@ -364,0 +386,0 @@ });

No alert changes

Improved metrics

Total package byte prevSize

111977

increased by6.1%

Number of package files

35

increased by2.94%

Lines of code

2586

increased by7.04%

No dependency changes