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

HTTP Parse Error if Response is using transfer-encoding chunked, but includes a content-length header #7136

Closed
kurtnordstrom opened this issue Jun 3, 2016 · 6 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@kurtnordstrom
Copy link

  • v6.2.0:
  • 4.2.0-36-generic rename node.js -> io.js #42~14.04.1-Ubuntu SMP Fri May 13 17:27:22 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux:
  • Subsystem:

In the rare case that nodejs is acting as an http client and the server returns a response that is in Transfer-Encoding: Chunked format, but (for whatever reason), also includes a Content-Length header, node will throw a Parse Error.

Proper behavior, as per RFC2616 would be to discard the Content-Length header if present alongside Transfer-Encoding: Chunked.

@vkurchatkin vkurchatkin added the http Issues or PRs related to the http subsystem. label Jun 3, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 3, 2016

RFC 2616 and 7230 have slightly different wording.

RFC 2616, section 4.4:

If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored.

RFC 7230, section 3.3:

If a message is received with both a Transfer-Encoding and a Content-Length header field, the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt to perform request smuggling (Section 9.5) or response splitting (Section 9.4) and ought to be handled as an error. A sender MUST remove the received Content-Length field prior to forwarding such a message downstream.

"Ought to be handled as an error" is what node.js currently does. RFC 7230 puts the onus on the sender (i.e., the server) to remove the offending Content-Length header. I don't think the built-in HTTP client is in the wrong here.

EDIT: As well, the surrounding paragraphs in both RFCs are full of MUST and MUST NOTs that make it clear it's a protocol violation to send both.

@kurtnordstrom
Copy link
Author

Would it make sense, if node.js is going to handle it as erroneous data from the server, that it explicitly look for this condition and report that specifically, as opposed what appears to be treating the response from the server as if it were using the content-length header and erroring out on the attempt to parse it?

@bnoordhuis
Copy link
Member

It does, doesn't it? The parser fails with err.code === 'HPE_UNEXPECTED_CONTENT_LENGTH'.

@kurtnordstrom
Copy link
Author

Hmm, if this is the case, then that seems sufficient. I wasn't seeing the error code when running the tests, but it is possible I wasn't properly capturing it.

@jasnell
Copy link
Member

jasnell commented Jun 7, 2016

Closing as there does not appear to be anything further to do

@jasnell jasnell closed this as completed Jun 7, 2016
@alextes
Copy link

alextes commented Mar 24, 2017

Just ran into this. Luckily the server is ours and fixing the headers is no biggie.

I think I should add I read the RFC very different from bnoordhuis.

The RFC note on 'Ought to be handled as an error' seems directed at the Content-Length header. i.e. the sender made an error in including the Content-Length header and the client should use Transfer-Encoding instead. Exactly as the first quoted sentence indicates. Finally, they note that any forwarding sender should remove this erroneous header before passing on the message. Again hinting such a request should be handled I think.

Do ignore my comment until the time when, if ever, this discussion becomes relevant again 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants