Repository navigation
HTTP/1 request parsing framing error event #24585
Description
Activity
- changed the title
[-]HTTP/1 request parsing framing error[/-][+]HTTP/1 request parsing framing error event[/+]on Nov 24, 2018 As per #24586 (comment) adding a listener for
'clientError'event on the server works on Node.js 10.13.0 - 11.1.0 but the event is no longer emitted on Node 11.2.0 so there is indeed a regression between v11.1.0 and 11.2.0Reproducible with @dougwilson's code with the following addition.
server.on('clientError', function (err, socket) { socket.destroy(err); });
cc: @nodejs/http
Is the solution to revert that commit, or...?
Looking into this
Here's a preliminary fix:
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 18ac6599d8..519558b10c 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -491,7 +491,10 @@ class Parser : public AsyncWrap, public StreamListener { ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); CHECK(parser->current_buffer_.IsEmpty()); - parser->Execute(nullptr, 0); + Local<Value> ret = parser->Execute(nullptr, 0); + + if (!ret.IsEmpty()) + args.GetReturnValue().Set(ret); } @@ -684,7 +687,12 @@ class Parser : public AsyncWrap, public StreamListener { } #else /* !NODE_EXPERIMENTAL_HTTP */ size_t nread = http_parser_execute(&parser_, &settings, data, len); - if (data != nullptr) { + if (data == nullptr) { + // `http_parser_execute()` returns either `0` or `1` when `len` is 0 + // (part of the finishing sequence). + CHECK_EQ(len, 0); + nread = 0; + } else { Save(); }
Yes, there are two issues. The one I actually reported and still need answered. And the one he found in addition that your patch addresses.
Still need help on the original issue posted above.
There are a few failing tests that I'll have to look into before submitting the final patch.
Should be fixed in #24738
@dougwilson would you mind if I'd use your test?
3 remaining items
Sure, but by using the term "work around" does that mean that too is a regression? Code in an Express.js middleware should not be changing the behavior of the entire server, especially if it would be regressing a Node.js DoS fix, right?
Ok, so not interested in fixing it. I don't know why I bother to report issue to Node.js core. @indutny you do not have permission to use my code in your test case if this is the response I get.
We could replace this line
Line 515 in 171b810
with
this.write(badRequestResponse) this.destroy(e);
but that is not safe as there might be buffered data to be written that could be not sent if we use that.
Also, don't angry, I'm sorry if my words sounded harsh. If you have better ideas please share and find a solution together.
@lpinca I don't understanding the internal workings of the Node.js core myself in order to offer a solution, otherwise I would have tried to open a pull request by now. I tried to explain what the issue I saw was, and also spell out what I'm trying to accomplish for someone to help.
I'm just asking about how to know if it closed from a socket error within the normal request / response transaction processing now in Node.js 10+.
That's what I'm trying to accomplish. If you're saying there is no possible way to accomplish that without reverting that commit, then I guess that is the solution, but it also means Express.js will just never work right on Node.js 10, and that would be a shame, since people have just started trying to move it it now that it became LTS and are running into issues, some of which I tracked down to this change in behavior.
I opened a notice for now to tell Express.js users do not use Node.js 10+ at this time so I should get less reports perhaps and just keep the Express.js user base on 8 and below, which is kind of sucky.
But to re-state, this is the goal I'm looking to accomplish on Node.js 10:
I'm just asking about how to know if it closed from a socket error within the normal request / response transaction processing now in Node.js 10+.
The
'clientError'event onserveris not "within the normal request / response transaction processing", which I'll expand means code that is in the'request'listener on a HTTP/1 server instance.Ok, I've shared my ideas on both this and the other issue you opened. Let's wait fore more contributors to chime in.
On second thought, #24585 (comment) might be a viable solution. I'll open a PR later today if no one does that before me.
Ok, so not interested in fixing it. I don't know why I bother to report issue to Node.js core. @indutny you do not have permission to use my code in your test case if this is the response I get.
As you wish, @dougwilson . I'm sorry that my fix is only a partial solution for your problem. 😢
- added a commit that references this issue
on Dec 5, 2018 - added a commit that references this issue
on Jan 14, 2019 - added a commit that references this issue
on Mar 20, 2019
It seems that in Node.js 10+, it's no longer possible to know with a framing error happened during the request transfer to a server. This is evident in request parsing, for example. A framing error could be when the incoming request is coming in
Tranfer-Encoding: chunkedand a chunk header is no valid hex.Here is an example app that shows the error handing in previous Node.js versions working:
In Node.js 8 and below it prints the following:
In Node.js 10+ is prints the following:
The
req.on('close', ...)fires in both, but I'm just asking about how to know if it closed from a socket error within the normal request / response transaction processing now in Node.js 10+. The best I can see is just if the'close'was before'end', then it could have been due to an error. But of course there is no guarantee that'close'is actually emitted only after'end': thecloseevent is emitted whenever the socket is closed, even if the entire request was received without error and was just pased (which is the state thatreqstarts out in).