File size limit cancels file upload only after all bytes are received

[csaroff]

I've been using multer for file uploads, and I've noticed an issue with the file size limit. I'm using the limits option to prevent the upload of files that are too large.

  const upload = multer({
    storage: multer.memoryStorage(),
    limits: {
      fieldNameSize: 255,
      fileSize: 500000,
      files: 1,
      fields: 1
    }
  });

When uploading an image to my API, multer seems to wait until it has received all of the image bytes before failing with the "LIMIT_FILE_SIZE" code.

Isn't it a security vulnerability if your API allows someone to upload an arbitrarily large file before failing the request? Is there another technique for preventing the behavior?

[Sylcan]

I'm having the same issue. I'd like to check the size before uploading.

Did you find something about this @csaroff ?

Thanks mate!

[csaroff]

@Sylcan I haven't investigated this further, but this seems like a pretty serious issue. Checking the content length header really doesn't solve this issue for the exact reason that you stated. The content-length header is supplied by the api consumer.

@LinusU Do you have any insight here?

[goatandsheep]

@LinusU has this been fixed?

[DiegoRBaquero]

This needs a fix, I'll see if I can PR.

[DiegoRBaquero]

PR sent: https://github.com/expressjs/multer/pull/447

[shiro-42]

Any news ?

[ericzon]

+1

[SachinKalsi]

any updates ?

[mbiakov]

+1

[adrbogacz]

+1

[HarshithaKP]

@csaroff This refers to the same issue https://github.com/expressjs/multer/issues/525

[jonchurch]

Labeling and self assigning to look at this later

[Rc85]

A work around would be to include the filesize in the request headers. Otherwise, I don't think there is a way to check the filesize before uploading. There are no possible ways (that I can think of) to determine the filesize other than data in the file object being sent from the client.

[LinusU]

When uploading an image to my API, multer seems to wait until it has received all of the image bytes before failing with the "LIMIT_FILE_SIZE" code.

@csaroff would you mind elaborating on this part a bit?

Are you seeing that memory usage is growing to the size of the entire file that is being uploaded, even though there is a file size limit in place?

Unless that is the case, I'm not really sure what the problem is? 🤔 Would love some more information!

[jonchurch]

I think this needs a reproduction if someone has the time

[csaroff]

@LinusU 🎯 Yep, that's what I'm saying. Ideally, multer will end the http request(responding with a 413) when it has received more than limits.fileSize bytes. Instead, it waits to receive all bytes before failing the request. It has been quite a while since I've used multer though and I'm not sure if the issue still exists.

[LinusU]

Yep, that's what I'm saying.

Just to be super clear, are you saying that if you set file limit to 1MB, and upload a file that is 700MB, the memory usage of Node.js grows to over 700MB? That is, the entire file is stored in memory before returning the error?

[csaroff]

It's been some time, but IIRC that's the behavior. It's probably worthwhile to note that I was using the in-memory adapter. storage: multer.memoryStorage(). A small reproduction would probably yield more information though 😄

[linus-hologram]

Is there any update in regards to this issue? Does this really persist since 2016?

[gbkwiatt]

Looks like, just had same issue, it uploads file first to memory somewhere, then checks size. not good if someone tries to upload 1GB file when 5MB is allowed

[Antonius-S]

Regarding file size limit, I produced a solution in https://github.com/expressjs/multer/issues/820

[satcasm]

The issue still persists, has anyone found any work around ?

[LinusU]

Can someone please confirm that this is the behaviour seen:

Just to be super clear, are you saying that if you set file limit to 1MB, and upload a file that is 700MB, the memory usage of Node.js grows to over 700MB? That is, the entire file is stored in memory before returning the error?

If that is the case, then that is indeed an error. But if the problem is that the entire file is sent over the socket from the client to the server, I don't think that we can do anything about it. As far as I know, there is no way to tell the client to abort their upload and just accept the servers answer directly instead...

[kazeens]

Is there any updates on that issue? Will there be implemented a flow where it preventively stops file upload once the indicated in limit property size value reached?

This still seems to be a huge security vulnerability

[LinusU]

@kazeens have you seen my questions in this thread asking for clarifications? Unless someone answers me, I cannot really do anything here 🤷‍♂️

[csaroff]

@LinusU What did you mean by "there is no way to tell the client to abort their upload and just accept the servers answer directly instead..."

I'm not sure what api multer has to work with, but as you receive each unit of data from the socket, can you count the number of bytes and keep a running total?

I think @Antonius-S posted a potential solution

https://github.com/expressjs/multer/issues/820#issuecomment-717233421

[LinusU]

@csaroff is this the behaviour you are seeing? 👇

Just to be super clear, are you saying that if you set file limit to 1MB, and upload a file that is 700MB, the memory usage of Node.js grows to over 700MB? That is, the entire file is stored in memory before returning the error?

Because as far as I know, our implementation will simply throw away the bytes for each unit of data from the socket once the limit has been reached. If this is not the case, we should make it so.

What I don't believe that we can fix because of how browsers work today, would be that the client will still send all 700MB to the server. There simply isn't a way in HTTP/1.1 to tell the client to stop sending any more bytes, as far as I know...

[csaroff]

Because as far as I know, our implementation will simply throw away the bytes for each unit of data from the socket once the limit has been reached. If this is not the case, we should make it so.

Gotcha. It's been more than 6 years, but IIRC that's the behavior. It's probably worthwhile to note that I was using the in-memory adapter. storage: multer.memoryStorage(). Probably needs to be reproduced though.

What I don't believe that we can fix because of how browsers work today, would be that the client will still send all 700MB to the server. There simply isn't a way in HTTP/1.1 to tell the client to stop sending any more bytes, as far as I know...

So it sounds like you can send back a 413 response early and close the connection, but most browsers will show a "connection reset" error.

https://stackoverflow.com/a/18370751/6051733

The heroic thing to do here would be to supply an option in multer to support:

1. Waiting until all bytes have been sent by the browser before returning 413
2. Returning 413 as soon as bytes exceed the limit.

Once all major browsers support (2), you could change the default. Idk if its worthwhile, just an idea

[guisaulo]

Is there any update to fix this vulnerability in next versions?

https://ossindex.sonatype.org/vulnerability/sonatype-2016-0121?component-type=npm&component-name=multer&utm_source=dependency-track&utm_medium=integration&utm_content=v4.4.2](url)

[Schoonology]

I'm not sure what information is missing here. What multer should do is:

1. Keep a running tally of the data received for a given file.
2. If the bytes received over the readable stream exceed the configured limit, send a 413 response and close the stream. This is how HTTP handles this situation: you close the underlying TCP socket, and it no longer matters what the other side does. Because TCP is a duplex protocol, you do not need to wait until the other side stops to send your response.
3. Discard the cached file automatically.

Is this not what multer does already? That's what the documentation indicates, but all of these 413-related issues certainly make it seem like it's not working today.

[noisyblue]

I've experienced same problem on nest.js project and my only possible workaround was to check content-length header of the request. It's cumbersome to add some boilerplate code like this, but at least it just throws on start of the uploading.

const upload = multer({
    storage: multer.memoryStorage(),
    limits: {
      fieldNameSize: 255,
      fileSize: ACCEPTED_MAX_FILE_SIZE,
      files: 1,
      fields: 1
    },
    fileFilter: (req, file, cb) => {
      const shouldAccept = req.headers['content-length'] <= ACCEPTED_MAX_FILE_SIZE;
      shouldAccept ? cb(null, true) : cb(new PayloadTooLargeException(), false);
    }
});

I think the limit of limits.fileSize option should be at least documented on README to prevent the waste of unnecessary traffics.

[ian117]

Hi, can anyone tell me if this cancels the stream correctly? I saw a post where someone use the readable stream inside multer and make this happen

It works i think, but i have my doupts even with my test done

//"multer": "^1.4.5-lts.1"
const multer = require('multer')

 const multerPublicationsPhotos = multer({
  dest: 'uploads/',
  limits: {
    fileSize: 1048576, // 1 Mb
  },
  fileFilter: (request, file, cb) => {
    request.on('aborted', () => {
      file.stream.on('end', () => {
        cb(new Error('Cancel Photo Upload'), false)
        // file.stream.destroy()
      })
      file.stream.emit('end')
    })
    if (file.mimetype == 'image/png' || file.mimetype == 'image/jpg' || file.mimetype == 'image/jpeg') {
      cb(null, true)
    } else {
      cb(null, false)
      return cb(new Error('Only .png, .jpg and .jpeg format allowed!'))
    }
  }
})

[neeeeecka]

Busboy (multer's dependency) suggests otherwise:

Note: If a configured limits.fileSize limit was reached for a file, stream will both have a boolean property truncated set to true (best checked at the end of the stream) and emit a 'limit' event to notify you when this happens.

Is the issue mentioned by OP actually happening? Busboy seems to be actively checking the stream size against fileSizeLimit and fires limit event when the limit is reached. 🤔🤔🤔