-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Parse Error: HPE_INVALID_CONSTANT on Transfer-Encoding response to HEAD request #8361
Comments
Here is a section 3.3:
Going to fix it in a bit. |
When replying to a HEAD request, do not attempt to send the trailers and EOF sequence (`0\r\n\r\n`). The HEAD request MUST not have body. Quote from RFC: The presence of a message body in a response depends on both the request method to which it is responding and the response status code (Section 3.1.2). Responses to the HEAD request method (Section 4.3.2 of [RFC7231]) never include a message body because the associated response header fields (e.g., Transfer-Encoding, Content-Length, etc.), if present, indicate only what their values would have been if the request method had been GET (Section 4.3.1 of [RFC7231]). fix nodejs#8361
Should be fixed by #8365 |
@indutny thanks for the quick patch! But I'm sorry I wasn't clear: my issue is that the client produces the Parse Error when it gets the TE header back in the response to the HEAD request. Maybe I'm in the wrong place – does this sound like an issue with https://github.com/joyent/http-parser instead? |
I think this patch should fix this problem. |
When replying to a HEAD request, do not attempt to send the trailers and EOF sequence (`0\r\n\r\n`). The HEAD request MUST not have body. Quote from RFC: The presence of a message body in a response depends on both the request method to which it is responding and the response status code (Section 3.1.2). Responses to the HEAD request method (Section 4.3.2 of [RFC7231]) never include a message body because the associated response header fields (e.g., Transfer-Encoding, Content-Length, etc.), if present, indicate only what their values would have been if the request method had been GET (Section 4.3.1 of [RFC7231]). fix nodejs#8361
When replying to a HEAD request, do not attempt to send the trailers and EOF sequence (`0\r\n\r\n`). The HEAD request MUST not have body. Quote from RFC: The presence of a message body in a response depends on both the request method to which it is responding and the response status code (Section 3.1.2). Responses to the HEAD request method (Section 4.3.2 of [RFC7231]) never include a message body because the associated response header fields (e.g., Transfer-Encoding, Content-Length, etc.), if present, indicate only what their values would have been if the request method had been GET (Section 4.3.1 of [RFC7231]). fix #8361 Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
fixed in 1fddc1f |
@jeremyruppel Did you ever resolve this issue? I think I'm seeing the same thing — on the client side, seeing that HPE_INVALID_CONSTANT error be thrown specifically when consuming a chunked response from a specific server. (And it's worse, it's a load-balanced API endpoint and only some of their servers are causing the issue, and it's HTTPS-protected so it's a devil of a time actually dumping the wire content to see what's going on, etc.) |
When replying to a HEAD request, do not attempt to send the trailers and EOF sequence (`0\r\n\r\n`). The HEAD request MUST not have body. Quote from RFC: The presence of a message body in a response depends on both the request method to which it is responding and the response status code (Section 3.1.2). Responses to the HEAD request method (Section 4.3.2 of [RFC7231]) never include a message body because the associated response header fields (e.g., Transfer-Encoding, Content-Length, etc.), if present, indicate only what their values would have been if the request method had been GET (Section 4.3.1 of [RFC7231]). fix nodejs#8361 Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
A significant performance regressions has been introduced in 1fddc1f for GET requests which send data through response.end(). The number of requests per second dropped to somewhere around 6% of their previous level. (nodejs#8940) The fix consists of removing a part of the lines added by 1fddc1f, lines which were supposed to affect only HEAD requests, but interfered with GET requests instead. The lines removed would not have affected the behaviour in the case of a HEAD request as this._hasBody would always be false. Therefore, they were not required to fix the issue reported in nodejs#8361.
A significant performance regressions has been introduced in 1fddc1f for GET requests which send data through response.end(). The number of requests per second dropped to somewhere around 6% of their previous level. The fix consists of removing a part of the lines added by 1fddc1f, lines which were supposed to affect only HEAD requests, but interfered with GET requests instead. The lines removed would not have affected the behaviour in the case of a HEAD request as this._hasBody would always be false. Therefore, they were not required to fix the issue reported in nodejs#8361. Fixes nodejs#8940. PR: nodejs#9026 PR-URL: nodejs#9026 Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Was this actually ever fixed, or is this reintroduced? I'm still getting
Of course, one could argue the usefulness of a chunked transfer for a HEAD requests, but a regular browser handles this fine (Testing with Chrome and Postman to simulate the HEAD request). |
@jishi I think http_parser has always considered that an error. With That's without going into whether it's actually desirable to support broken behavior. |
@bnoordhuis Hm, I don't see what After some research, I'm realize that it might be a protocol violation, although there seems to be some discrepancies on current implementations. Allegedly even Google servers did the same thing a while back. I guess the question is whether it should maximize compatibility or enforce protocols. |
Clarification: The violation is in the empty chunk that is sent as part of the message-body. Not the combination of headers. |
The problem with the "be liberal in what you accept" philosophy is that it's a frequent source of bugs and security issues later on, even when the initial compatibility hack looked benign enough. Point in case: next week's security release was prompted by being too lenient in a few places. |
Alright, I'm with you now on the close debate. and fair enough, I get your point. This is no deal breaker for me either. I should probably reach out to Sonos and point out this protocol violation anyway. |
I'm working with a service that sends back a "Transfer-Encoding: chunked" header in response to a HEAD request, which produces the following error:
I can reproduce this behavior with the following client-server setup:
Changing the method to GET does not produce the error, so my expectation is that HEAD wouldn't either. I realize it's pointless for the Transfer-Encoding header to be present in a response without a body, but the RFC says that a response to a HEAD request should contain all of the headers that the GET would:
My node versions:
The text was updated successfully, but these errors were encountered: