Skip to content

Content-Length header in multipart upload #64

Closed
scriby opened this Issue Jan 16, 2012 · 17 comments

6 participants

@scriby
scriby commented Jan 16, 2012

Hi,

It looks like restler is leaving off the main Content-Length header when doing multi-part uploads (even though it is setting Content-Length headers for individual files).

Nginx is rejecting multipart uploads made from restler because the header is missing.

@disfated

Could you give any test cases to reproduce this?

@scriby
scriby commented Jan 17, 2012

Here's the output from the multipart test in test/restler.js:

POST / HTTP/1.1
accept: */*
user-agent: Restler for node.js
host: localhost:7015
content-type: multipart/form-data; boundary=48940923NODERESLTER3890457293
connection: keep-alive
transfer-encoding: chunked

--48940923NODERESLTER3890457293
Content-Disposition: form-data; name="a"

1
--48940923NODERESLTER3890457293
Content-Disposition: form-data; name="b"

thing
--48940923NODERESLTER3890457293--

Nginx is looking for a Content-Length header at the top level, next to the Content-Type.

@disfated

How am I supposed to reproduce this?
Could you please give us a bunch of code which exposes the issue?

@scriby
scriby commented Jan 24, 2012

You'll need to set up Nginx and attempt to upload a multi-part form to it to reproduce the issue. It's not really a bug in restler so much as an incompatibility with Nginx.

Nginx requires the Content-Length header to be present for a POST. The output I gave above shows that restler does not send the Content-Length header when doing a multipart post. And as far as I can tell, there's no way to set it -- it's something that restler would need to compute based on the size of all files plus boundaries, etc.

Make sense?

Thanks,

Chris

@disfated

Indeed, there's a lot of code has to be rewritten to close this nginx hole...
Maybe content-length trailer could satisfy nginx?

@ilsken
ilsken commented Mar 10, 2012

I've this problem with a few services as well

@ilsken
ilsken commented Mar 10, 2012

This fork seems to of partly solved the issue starkcoffee@a5ea0ff

@rhyolight

I'm having the same problem with Nginx. I also wanted to note that there is an nginx module that helps it handle chunks.

@abh
abh commented Mar 26, 2012

Indeed, there's a lot of code has to be rewritten to close this nginx hole...

That's a little unfair. It's a bug in restler, not in nginx. For POST requests Content-Length or Transfer-Encoding are required to make a valid request as per RFC2616. "The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers."

@rhyolight

I'm not so sure the bug is a restler bug. The spec says the request must contain a "Content-Length" OR "Transfer-Encoding" to identify a message body, and restler is including the transfer-encoding header.

@abh
abh commented Mar 26, 2012

Maybe content-length trailer could satisfy nginx?

A trailer implies, I think, chunked transfer-encoding – just doing that (transfer-encoding chunked) by itself should be enough actually.

@abh
abh commented Mar 26, 2012

The spec says the request must contain a "Content-Length" OR "Transfer-Encoding" to identify a message body, and restler is including the transfer-encoding header.

libaprequest from Apache 1.3 doesn't support chunked, fwiw. Also – where does restler set Transfer-Encoding? I didn't see it anywhere.

@rhyolight

@abh I did not look through the restler source to confirm that it sets a transfer-encoding. I only assumed it did after reading @scriby's comment above, which supposedly came from a restler test and included transfer-encoding.

@abh
abh commented Mar 26, 2012

@rhyolight Looking in the git history it looks like it used to, but doesn't anymore.

@rhyolight

@abh That is indeed curious. Either way, nginx will not handle chunked encoding even if the transfer-encoding is set. Looks like this bug exists in both products.

@abh
abh commented Mar 26, 2012

@rhyolight It's working for us now (with Apache 1.3.x and libaprequest on the server) with the patches I made in #78.

@disfated

Ok, guys i'm currently working to completely solve this problem.
Also planning to refactor multipart module and change it's api as it's quite messy for now.

@danwrong danwrong pushed a commit that closed this issue May 2, 2012
@abh abh Set Content-Length for multipart requests
Closes #64
41cba25
@danwrong danwrong closed this in 41cba25 May 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.