Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added max upload size for files #357

Merged
merged 2 commits into from Mar 1, 2017
Merged

Added max upload size for files #357

merged 2 commits into from Mar 1, 2017

Conversation

ghost
Copy link

@ghost ghost commented Oct 21, 2015

No description provided.

@vslinko
Copy link

vslinko commented Dec 29, 2015

👍

@felixge
Copy link
Collaborator

felixge commented Jan 11, 2017

#412

@jaydenseric
Copy link

Can someone please mash the merge button? This is an important feature and improves security.

@tunnckoCore
Copy link
Member

Seems okey to me.

@tunnckoCore tunnckoCore merged commit b81d1c7 into node-formidable:master Mar 1, 2017
@OrangeDog
Copy link
Contributor

It's much more efficient (for well-behaved clients) if you check the Content-Length first.

@jaydenseric
Copy link

@OrangeDog is that per file though?

@jaydenseric
Copy link

Did a quick Google, also keep in mind Content-Length can be faked.

@felixge
Copy link
Collaborator

felixge commented Mar 1, 2017

I think there are some other problems with this PR:

  • No documentation.
  • No limit on size of all files combined. This sort defeats any security aspect of this I think.

@badeball
Copy link
Contributor

badeball commented Mar 1, 2017

I assume with security aspect, you mean the ability for a client to consume disk space on the server? If so, one also needs to throttle the number of requests. Otherwise, a client can simply perform multiple requests with files matching the max-file-size attribute.

If my assumption is correct, then I consider this "solution" to be a broken attempt at securing.

@jaydenseric
Copy link

We can add another consolidated upload limit as a seperate feature. Primarily most people would be interested in a per-file allowance. This makes for easy communication to a user before uploading, and makes client side validation much more simple. It doesn't make a lot of sense that a user uploading 4 images via a multiple input must reduce the quality/size by 50% compared to if they were uploading 2 sets of 2. A per-file limit is also useful if you know maximum size your thumbnailer can deal with.

The consolidated upload limit, along with a userland throttle would be handy on top of this to ensure no-one tries to bring the site down.

@felixge
Copy link
Collaborator

felixge commented Mar 1, 2017

@badeball you bring up a good point. In order to defend against a DoS, you need to limit the total request size and the allowed number of requests. But limiting the number of requests is IMO out of scope for this library.

@jaydenseric sure, limiting the per-file size is still a useful feature. But you're the one who brought up security, so I thought that was your main use case.

@jaydenseric
Copy link

Multer has good config for limits. It seems formidable is wide open?

@badeball
Copy link
Contributor

badeball commented Mar 1, 2017

I've recently worked on this problem (above-mentioned security aspect) for a very large broadcasting agency. Our conclusion was that this is a difficult problem to handle for a common developer. I believe that any security aspects is way outside the scope of this library.

A library-enforced size-limit is thus only useful for the user experience and hence we only need to consider well-behaved clients. Therefore I think that it is sufficient to only check for Content-Length. This is to not trick any user into believing that they are in any way safe (and the fact should be documented).

@OrangeDog
Copy link
Contributor

It's trivial to implement actual limits on top of formidable's API anyway.
That's what I always do.

@jaydenseric
Copy link

@OrangeDog Do you typically have to abandon some of the more convenient parts of the API? Any suggestions, or juicy snippets?

@felixge
Copy link
Collaborator

felixge commented Mar 1, 2017

@jaydenseric please be more specific with your claims about formdiable and multer.

@OrangeDog yeah, that's what we did at @ transloadit as well when I was still working there.

@badeball I agree that it can be tricky to get right. But I still think that providing "best-effort" options to limit DoS vectors will be useful to those who need them. And we can certainly do better than just "Content-Length" checking. That being said, docs are needed to inform people of the limitations of the options that are provided.

@OrangeDog
Copy link
Contributor

It's pretty simple, you check the headers first, then count the actual bytes as they come through the events.
You have to tailor it to the layer above so you can actually stop the upload and e.g. return 413.

kornelski added a commit that referenced this pull request Dec 22, 2017
Fix using closed _writeStream when error is occurred (#357)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants