Better communication / errors around node 12 breaking HTTP parser changes

[mikemaccana]

Previously, on Love Island

If a web server sends both a Content-Length header and a Transfer-Encoding: chunked header, it is a violation of the HTTP spec.

node 10's older HTTP parser didn't error on this (though it should have)

node 12's newer HTTP parser does error on this (which is good)

The issue

As a spicy noderperson, using a REST API client that worked with node 10, I recently moved it to node 12 and did a bunch of research trying to work out what was wrong before finding out the cause of this error.

Important thing since we're communicating of the internet and people tend not to read things before replying:

If it's not clear from the above, I believe node is doing the right thing here, It just needs to be communicated better. Please don't comment saying I support malformed headers, I don't support malformed headers. If you do comment saying I support malformed headers, I'll block you for non-constructive behaviour.

1. Robustness principle / Postel's law

See https://en.wikipedia.org/wiki/Robustness_principle

Be conservative in what you do, be liberal in what you accept from others (often reworded as "Be conservative in what you send, be liberal in what you accept").

ie, node could have:

warning: this response contains both a `Content-Length` header and a `Transfer-Encoding: chunked` header, it is a violation of the HTTP spec. Ignoring (whichever header is being ignored) 

2. Lack of a transition period

Think about the good way node (as a community) is handling uncaught promises. There's a warning, everyone knows what's coming.

Ideally, a transition period could have occurred, ie:

warning: this response contains both a `Content-Length` header and a `Transfer-Encoding: chunked` header, it is a violation of the HTTP spec. In a future version of node this response will be rejected.

3. Poor communication around a breaking change

If node was to break existing behaviour, a CHANGELOG entry that calls this out explicitly would help:

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#12.0.0 states:

switch default parser to llhttp (Anna Henningsen) #24870

The change should be communicated more explicitly.

Note: the new HTTP parser is stricter in some cases, and malformed HTTP responses that may have worked in the previous HTTP parser are no longer handled. See https://someurl for details.

4. error.message around the malformed HTTP response is vague

 Error: Parse Error                                                                     at TLSSocket.socketOnData (_http_client.js:452:22)

• The error is vague.
• Apparently there is some other field (error.reason) that has an explanation, but error.message itself is vague, and I didn't know about error.reason (am I ignorant here? I thought error.message was the reason)
• I appreciate that the response isn't being parsed correctly in that it's invalid, but the error makes it sounds like node's parser has an error, rather than the response being parsed

A better error.message would be:

Error: invalid HTTP response. See https://someUrl

or perhaps:

Error: invalid HTTP response (contains both 'Content-Length' header and a 'Transfer-Encoding: chunked' header)

Just some suggestions. Thanks for reading.

[devsnek]

@nodejs/http @indutny

[awwright]

Was there ever a test for this? (I assume not.) I don't think that points 1-3 would have occurred to anyone if there was no test for this in the first place. There's probably other changes like this that we're completely unaware of.

Now, I don't think it makes sense to add tests ensuring that forbidden behavior is supported; though there should have been a test for the required, correct behavior in the first place.

I think a better written error message is the best solution. It appears Node.js does provide the error code, HPE_UNEXPECTED_CONTENT_LENGTH, but no descriptive error.

I think jumping straight to an error is appropriate; there's other clients that wouldn't be able to correctly parse this response, i.e. the program is already broken.

I'm not sure how much more effort could reasonably be given to this. There's ten thousand of these little issues that pop up every time someone optimizes something; for example, at some point, Node.js broke one of my programs by rewriting querystring.parse to use String#indexOf instead of String#split; I was passing a regular expression to split the query string. I was possibly the only person using this behavior though, how worth it would it have been to have a transition period? Not very much, I assume.

[addaleax]

I think this is a combination of several different issues.

One: the linked commit that introduced throwing an exception when encountering the combination of Content-Length and Transfer-Encoding was landed much earlier than Node 10.x, and as part of a security release. The latter means that, for better or for worse, there is typically no or little public discussion about how Node.js specifically should react in these situations (e.g. warning vs throwing an exception).

In this case, reading up on the private prior discussion, I assume the decision was made this way to make handling of similar situations easier (the linked commit addresses both points 3 and 4 of https://tools.ietf.org/html/rfc7230#section-3.3.3, although only multiple Content-Length headers are requires to be unrecoverable errors as per the spec).

I read the spec text as saying that it would be acceptable to simply drop the Content-Length header in this situation. I’m not sure if this is better than throwing an exception, given that the sender can already send other header combinations that definitely lead to that outcome. I, personally, would not make changes to Node or its HTTP parser(s) at this point.

Two: The lack of having a reason show up in error.message. I fully agree. Most recent errors introduced into Node follow this format, providing the reason as part of the name. I assume this hasn’t happened for HTTP parsing errors for two reasons: 1. The error object is created in C++ land, where everything’s harder to do and less well maintained in general; 2. Only the newer llhttp parser which ended up in Node 12 as the default parser actually provides a human-readable reason property in addition to the code property. Luckily, this is not the hardest problem in the world, and I will open a PR to address it shortly. (I’ll ping you and add a Fixes: tag for this issue to it – if you feel like that PR landing should not close this issue, let me know.)

Three: Release notes don’t always actually call out the really notable changes very well. (I’ve made an attempt to at least call out the “big” changes more directly for v12.x, although that would not have given the amount of information you’d like to see and it ultimately wasn’t included in the release). We run into this issue frequently with regular releases, because releasers aren’t always familiar with the subject matter of all commits, but for breaking changes I agree that we can and should do better. I’d suggest opening a separate issue in https://github.com/nodejs/Release about this, if you like.

[indutny]

I've made an attempt to improve errors: https://gist.github.com/indutny/15352991e85a51786436646a31c790db, but this is a major change.

Going to submit a PR once I'll fix all broken tests 😂

[addaleax]

@indutny Fwiw, I’ve been working on the PR since I announced it in my comment, so I’ll go ahead and open that anyway: #28487

Also, I don’t see why that would necessarily be semver-major, as these errors all have .code properties…

[indutny]

It looks way better than mine, so I'm glad to see you doing it! 😉

[mikemaccana]

All: Thanks. 🙂