Multer: fileFilter refused file still uploaded in the diskStorage

[YuriScarbaci]

given a code similar to this:

    const multerDiskStorage = multer.diskStorage({
      destination: function(req, file, cb) {
        cb(null, "tmp/uploads/");
      },
      filename: function(req, file, cb) {
        cb(null, hashFile(file));
      }
    });
    const multerInstance = multer({
      storage: multerDiskStorage,
      fileFilter: (req, file, cb) => {
        let error;
        if (!UploadImagesController.didReceiveFile(file))
          error = new Error("no file");
        if (!UploadImagesController.validImageByteSize(req))
          error = new Error("file bigger then permitted");
        if (!UploadImagesController.validImageType(file))
          error = new Error("file type not permitted");
        if (error) {
          cb(error, false);
        }
        cb(null, true);
      },
      limits: {
        fieldSize: 20000000,
        fileSize: 20000000
      }
    });

The fileFilter works as expected except for the fact that when a fileFilter cb with error and false is invoked the file still gets uploaded in the tmp/uploads folder (with a name that is not the result of invoking the filename callback propriety defined in the storage)

[LinusU]

That is not good, would you be able to submit a failing test case as a PR? That would really help tracking it down

[YuriScarbaci]

would you be able to submit a failing test case as a PR?

no, not on the short-term anyway saddly, i'm quite pressed with time for some deadlines in those months and i'm not really used yet to test-casing git libraries, will try to this weekend tho

[HarshithaKP]

@YuriScarbaci I was able to recreate the problem. Made minor changes and was able to resolve it.

const multerDiskStorage = multer.diskStorage({
      destination: function(req, file, cb) {
        cb(null, "tmp/uploads/");
      },
      filename: function(req, file, cb) {
        cb(null, hashFile(file));
      }
    });
    const multerInstance = multer({
      storage: multerDiskStorage,
      fileFilter: (req, file, cb) => {
        let error;
        if (!UploadImagesController.didReceiveFile(file)) {
          error = new Error("no file");
          return cb(error);
       }
        if (!UploadImagesController.validImageByteSize(req)) {
          error = new Error("file bigger then permitted");
          return cb(error);
       }
        if (!UploadImagesController.validImageType(file)) {
          error = new Error("file type not permitted");
          return cb(error);
        }
        cb(null, true);
      },
      limits: {
        fieldSize: 20000000,
        fileSize: 20000000
      }
    });

[YuriScarbaci]

@HarshithaKP
Ty a lot for it, I will try it our ASAP,

From what i was able to infer the problem is invoking the cb with two arguments instead of invoking it only with the first argument?

[YuriScarbaci]

Based on @HarshithaKP insight

I solved the problem the following way:

1. invoked cb with only one parameter
2. inside fileFilter returned the invokation of the cb instead of simply invoking it
3. instead of passing an Error instance to cb, actually passed a string (passing the Error instance resulted in losing the message!!!)

this seems to have solved the problem.

Based on this I think that the readme needs to be changed to correctly state the required syntax and gotchas to make this work!

https://github.com/expressjs/multer#filefilter

[jonchurch]

For clarity and anyone in the future's sanity, the issue here was that cb was being called twice in the implementation of fileFilter.

@HarshithaKP's code fixes that by returning cb when it is invoked, ensuring the function returns once cb is called.

OP's code called cb(null, true) regardless of whether there was an error. An if/else block there could also have worked to prevent it from being run twice.

I'm not sure that it's really warranted, but multer could prevent this kind of bug by wrapping the cb provided by fileFilter internally in something like lodash's once function. Not really suggesting, since this is more of a Node.js thing, but it does seem reasonable to be defensive about rejecting file uploads.