replace HTTP::Body with URL::Encode and HTTP::MultiPartParser

[miyagawa]

related to #497, and this has been a wishlist item for a long time, and I've finally got around to it.

The PR replaces the use of HTTP::Body in Plack::Request with much smaller, faster and more maintainable tools: URL::Encode and HTTP::MultiPartParser.

Plack::Request always has saved the request body in its own Stream::Buffer and makes psgi.input seekable, and this patch doesn't change any of that. The temporary files created for Upload objects are now held by Plack::Request::Body, which will be destroyed in the end of request cycle, much like previously.

All tests pass, but here's a few considerations:

• Temporary files are now stored in a new tempdir, rather than root TMP, which i think is fine
• Temporary files have a random name without suffix in them. Previously temp files also have the same suffix as uploaded files, which I think has a security implication. You still have an access to the original filename like before.
• Error checks might not be sufficient for corner cases or buggy upload data.

cc @chansen @avar @kazeburo

[avar]

Untested, looks good to me. I can performance test it if you'd like with the same system I noted in #497.

I'd replace:

File::Temp->newdir;

With something like:

File::Temp->newdir(File::Spec->catdir(File::Spec->tmpdir(), "Plack-Request-Body-XXXXX"));

Though, it's really handy to see what's creating those tempdirs in /tmp when debugging something on the system.

[miyagawa]

Good idea: d15df4d

[miyagawa]

@avar performance testing would be interesting. Be sure to install URL::Encode::XS

[kazeburo]

How about https://metacpan.org/pod/HTTP::Entity::Parser ?
It uses HTTP::MultiPartParser and WWW::Form::UrlEncoded(::XS).

[miyagawa]

I would've chosen it if I knew it beforehand or you had submitted a PR :)

On Fri, Nov 13, 2015, 9:21 PM Masahiro Nagano notifications@github.com
wrote:

How about https://metacpan.org/pod/HTTP::Entity::Parser ?
It uses HTTP::MultiPartParser and WWW::Form::UrlEncoded(::XS).

—
Reply to this email directly or view it on GitHub
#537 (comment).

[miyagawa]

@kazeburo it looks to me the patch to use HTTP::Entity::Parser in Plack::Request would be minimal. If you can try writing one I'd like to see how it performs and how the code base compares.

I'm all for extracting out the complicated bits out of Plack into a standalone module.

[kazeburo]

@miyagawa I try to make PR and patch HTTP::Entity::Parser to create tmpdir.

[chansen]

@miyagawa, in your multi-part decoder you treat all parts as a "upload" part, this is not correct. A "upload" part is indicated by the existence of the filename parameter in the Content-Disposition header. Please see eg/example.pl for an example.

@miyagawa, @kazeburo, parsing the Content-Disposition header is tricky business, please see https://gist.github.com/chansen/7163968 for a robust implementation.

I think kazeburo's PR looks good! kazeburo++

[miyagawa]

@chansen good point, wonder how it's not covered by our tests, can you supply it?

About the Content-Disposition i copied the code out from HTTP::Body.

[miyagawa]

k, added a test with 07377fe.

Would be nice if you or kazeburo can turn that Content-Disposition gist into a unit test for Plack.

[miyagawa]

@chansen Addressed the issue with non-upload parameters in db425e5.

[kazeburo]

@chansen @miyagawa

I added chansen's test and Content-Disposition header parser to HTTP::Entity::Parser.
kazeburo/HTTP-Entity-Parser@551a526, kazeburo/HTTP-Entity-Parser@91490a9

[chansen]

@miyagawa Looks better :) Even though I'm the original author of HTTP::Body, I abandoned it after I came back from a hiatus in the Catalyst community, when I came back it was no longer maintainable due to some insane additions/decisions/subclasses. The research I performed in the Content-Disposition gist was funded for a client of mine and later implemented in production, we also ended up with specific decoders for the raw Content-Disposition filename value depending on the UA (sniffing).

@kazeburo, looks good! For extra credits I would also implement a user configurable max size for non-upload parts and for double credits, also uploads ;o)

@kazeburo++

[miyagawa]

@kazeburo sweet!

@chansen you have any thoughts on WWW::FormUrlEncoded vs URL::Encode? I guess they basically do the same thing.

[miyagawa]

closing in favor of #538