HTTP/1 request parsing framing error event

[dougwilson]

• Version: v10.13.0
• Platform: Windows 10
• Subsystem: http

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: chunked and a chunk header is no valid hex.

Here is an example app that shows the error handing in previous Node.js versions working:

var http = require('http')
var net = require('net')

var server = http.createServer(function (req, res) {
  var bufs = []

  req.on('data', function (chunk) {
    console.log('request recv %d bytes', chunk.length)
    bufs.push(chunk)
  })

  req.on('end', function () {
    var data = Buffer.concat(bufs)
    console.log('request got %d bytes', data.length)
    res.end('OK')
    server.close()
  })

  req.socket.on('error', function (e) {
    console.log('request error %s', e.toString())
    req.destroy()
    server.close()
  })
})

server.listen(0, function () {
  var port = server.address().port
  var sock = net.createConnection(port, '127.0.0.1')

  sock.on('connect', function () {
    sock.write('POST / HTTP/1.1\r\n')
    sock.write('Host: localhost\r\n')
    sock.write('Transfer-Encoding: chunked\r\n')
    sock.write('\r\n')
    sock.write('3\r\n')
    sock.write('foo\r\n')
    sock.write('3\r\n')
    sock.write('bar\r\n')
    sock.write('ff\r\n')
    sock.end()
  })
})

setTimeout(function () {
  console.log('timeout!')
  process.exit(1)
}, 10000).unref()

In Node.js 8 and below it prints the following:

request recv 3 bytes
request recv 3 bytes
request error Error: Parse Error

In Node.js 10+ is prints the following:

request recv 3 bytes
request recv 3 bytes
timeout!

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': the close event is emitted whenever the socket is closed, even if the entire request was received without error and was just pased (which is the state that req starts out in).

[lpinca]

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.0

Reproducible with @dougwilson's code with the following addition.

server.on('clientError', function (err, socket) {
  socket.destroy(err);
});

[lpinca]

cc: @nodejs/http

[lpinca]

It seems the first bad commit on v11.x-staging is 115c57a

cc: @indutny

[dougwilson]

Is the solution to revert that commit, or...?

[indutny]

Looking into this

[indutny]

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();
     }
 

[indutny]

Note that for the test snippet above, the clientError handler has to be added anyway, just as @lpinca mentioned.

[dougwilson]

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.

[dougwilson]

Still need help on the original issue posted above.

[indutny]

There are a few failing tests that I'll have to look into before submitting the final patch.

[indutny]

Should be fixed in #24738

@dougwilson would you mind if I'd use your test?

[dougwilson]

Does that fix the original issue I posted, or the second issue that was injected into this issue report? I'm only interested in the issue I reported here, and the second issue should have a different thread to discuss.

[lpinca]

@dougwilson it only fixes the regression on v11.2.0. For the original issue the only workaround I can think of is #24585 (comment) that will make the 'error' event to be emitted on socket and make the test on the issue description work as expected.

[dougwilson]

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?

[lpinca]

Behaviour changed on purpose in f2f391e, as you can see the socket is no longer destroyed on parse error. There is not much I can do about it. Workaround is indeed the wrong term, as I see it there is no issue, behaviour just changed.

[dougwilson]

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.

[lpinca]

We could replace this line

node/lib/_http_server.js

Line 515 in 171b810

this.end(badRequestResponse);

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]

Revert f2f391e? I'm fine with that. It would be a semver-major change though.

[dougwilson]

@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 on server is 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.

[lpinca]

Ok, I've shared my ideas on both this and the other issue you opened. Let's wait fore more contributors to chime in.

[lpinca]

On second thought, #24585 (comment) might be a viable solution. I'll open a PR later today if no one does that before me.

[indutny]

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. 😢