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

Streaming multipart parser without tmpfiles #474

Closed
gwicke opened this issue Oct 31, 2013 · 9 comments
Closed

Streaming multipart parser without tmpfiles #474

gwicke opened this issue Oct 31, 2013 · 9 comments

Comments

@gwicke
Copy link
Contributor

gwicke commented Oct 31, 2013

The multipart parser
(https://github.com/mcavage/node-restify/blob/master/lib/plugins/multipart_parser.js) uses formidable currently. Formidable creates tempfiles for each file upload, and never clears those. Even with a tmpreaper in place, this can be exploited for a DOS by sending requests with a lot of file uploads in a short time.

https://github.com/mscdex/busboy looks like a promising alternative to formidable. It supports streaming and dropping file uploads. The interface for streaming would need to be worked out- maybe by passing in something like req.form?

@techguydave
Copy link
Contributor

Any word on this? I'm looking to use multipart streaming to allow uploading a file to AWS S3 through the API.

@epegzz
Copy link

epegzz commented Feb 1, 2015

+1 for file streaming support :)

@micahr micahr added this to the Release: 4.0.x milestone Jun 18, 2015
@cxreg
Copy link

cxreg commented Mar 12, 2016

Is this ever likely to happen?

@gwicke
Copy link
Contributor Author

gwicke commented Mar 12, 2016

FWIW, we have been using https://github.com/mscdex/busboy in other non-restify projects at Wikimedia since, and is has worked well for us (apart from some bugs early on).

@bluesockets
Copy link

👍

@prabhu
Copy link

prabhu commented Aug 18, 2016

I can't bear formidable as well. Busboy seems stable at least for me.

@micahr
Copy link
Member

micahr commented Aug 18, 2016

If anyone is interested, we would welcome a PR to the plugins repository to either change the implementation or swap out the module that it uses.

@prabhu
Copy link

prabhu commented Aug 18, 2016

Some of us do contribute. However, I don't think a simple PR can provide this feature unless there is some refactoring effort along with unit tests. I see the following issues with the current design.

  • bodyParser.js loads all the parsers statically from the same directory. Instead there should be way to register the parser that way people can override based on the mime or content type or url or some other value
  • multipartBodyParser.js should be like a base class with generic api methods. The default implementation class could use formidable and then the community can create their own parser with their favourite library
  • If possible please adopt ES6 and babelify the code to support older node. Cant bear to see function within functions within functions with return (false) and return next() as well as return (object)

Given the popularity of this framework there should be some kind of agreement regards to the changes even before someone could invest their time for a PR.

@DonutEspresso
Copy link
Member

@prabhu All of those suggestions sound great - feel free to kick off a discussion in the plugins repo, as plugin development has stopped in the main repo here.

@retrohacker retrohacker modified the milestone: Release: 4.0.x Apr 29, 2017
retrohacker pushed a commit that referenced this issue Apr 29, 2017
Closes:

#289
#381
#474
#575
#790
#633
#717
#576
#576
#909
#875
#860
#853
#850
#829
#813
#801
#921
#1101
#1019
#989
#632
#708
#737
#859
#1326
#1327
#927
#1099
#1068
#1040
#1035
#957
#948
#1134
#1136
#1183
#1206
#1286
#1323

> Note: this issue closes _but does not resolve_ the issues listed, we are just tracking them in another medium
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

No branches or pull requests

10 participants