HTTP/1 request parsing framing error event

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

@dougwilson

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

cc: @nodejs/http

@indutny

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

cc: @indutny

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

@lpinca

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

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.

@dougwilson

Should be fixed in #24738

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

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?

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.

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

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.

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

@lpinca

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

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.

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

http: fix error return in `Finish()`

http: fix error check in `Execute()`

Navigation Menu

Repository navigation

HTTP/1 request parsing framing error event #24585

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

HTTP/1 request parsing framing error event #24585

Description

Activity

lpinca commented on Nov 24, 2018

lpinca commented on Nov 24, 2018

lpinca commented on Nov 24, 2018

dougwilson commented on Nov 29, 2018

indutny commented on Nov 29, 2018

indutny commented on Nov 29, 2018

indutny commented on Nov 29, 2018

dougwilson commented on Nov 29, 2018

dougwilson commented on Nov 29, 2018

indutny commented on Nov 29, 2018

indutny commented on Nov 30, 2018

3 remaining items

dougwilson commented on Nov 30, 2018

lpinca commented on Nov 30, 2018

dougwilson commented on Nov 30, 2018

lpinca commented on Nov 30, 2018

lpinca commented on Nov 30, 2018

dougwilson commented on Nov 30, 2018

lpinca commented on Nov 30, 2018

lpinca commented on Nov 30, 2018

indutny commented on Nov 30, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions