richardgirges / express-fileupload

Simple express file upload middleware that wraps around busboy
MIT License
1.52k stars 261 forks source link

express-fileupload seems to not respect cors with abortOnLimit #303

Open andrecp opened 2 years ago

andrecp commented 2 years ago

Hey folks,

I was playing with express-fileupload today and hit what I think is a bug.

const express = require('express');
const cors = require('cors');
const fileUpload = require('express-fileupload');

const app = express();
app.use(cors());
app.use(express.json());
app.use(fileUpload({
  useTempFiles: true,
  tempFileDir: '/tmp/',
  limits: { fileSize: 50 * 1024 * 1024 },
  abortOnLimit: true,
  preserveExtension: true,
}));

With the above, if my upload view is over the limit (50MB), it should return a 413, however, I believe because it isn't setting the CORS headers correctly it doesn't return 413, instead the client side gets a CORS error.

This is my view

app.post('/upload', (req, res) => {
    const uploadedFile = req.files[Object.keys(req.files)[0]];
    uploadedFile.mv(`/tmp/user1/${uploadedFile.name}`, (err) => {
      if (err) { return res.status(500).send(err); }
      return res.send('File uploaded!');
    });
});

If I register my own limitHandler it then works correctly and the cors headers get set

app.use(fileUpload({
  useTempFiles: true,
  tempFileDir: '/tmp/',
  limits: { fileSize: 50 * 1024 * 1024 },
  abortOnLimit: true,
  // eslint-disable-next-line no-unused-vars
  limitHandler: (req, res, next) => {
    res.status(400).json({ error: 'File is too large.' });
  },
  preserveExtension: true,
}));

Another interesting thing is that with my limitHandler I need to check if the headers were already sent, otherwise my app crashes

  if (!res.headersSent) {
    const uploadedFile = req.files[Object.keys(req.files)[0]];
    uploadedFile.mv(`/tmp/user1/${uploadedFile.name}`, (err) => {
      if (err) { return res.status(500).send(err); }
      return res.send('File uploaded!');
    });
  }

Thank you

richardgirges commented 2 years ago

It looks like the headers being sent is a bug. I'm not sure what's going on with the CORS issue though, curious if the headers being sent by express-fileupload is creating a CORS issue as a red herring. Would you be able to provide a simple app with a repro?

RomanBurunkov commented 9 months ago

Hi,

Issue with headers were already sent fixed in version 1.4.2 by PR #238. In the case described above, middleware won't call next and route handler will not be invoked.