meta: express.js internals do not support Node.js 10+

[dougwilson]

It seems that, when trying to get everything working in 10+, I found some Node.js behavior changes that have no work-arounds. I reported to Node.js core in nodejs/node#24585 and nodejs/node#24586 but there is no interest in fixing them. Until there is a fix, I would not recommend using Express.js on Node.js 10+

/cc @addaleax from Node.js core who was interested in seeing http/2 in Express, but now http/1 doesn't even work right 🤣

[addaleax]

@dougwilson I’m happy to take a look. As Matteo said in one of the issues, the most recent security release took basically all the HTTP expertise we could get (and I was travelling as well).

I’m sorry about the impression you’re getting, but believe me, something like express not working on Node.js seems ridiculous to me (and to everyone else, I think), so yes, I’m very much interested in getting this to work again.

[wesleytodd]

To be clear for everyone here, these changes break the ability for Express to detect when an incoming request is in an error state from a miss-behaving client. While this is very much a serious breaking change, it is also not something end users should take much action on.

Mainly this will mean that a client would receive a 400 response and your application would not ever see it (and maybe have some odd behavior if it does). If this is a concern and you cannot downgrade to node 8, then here is a workaround proposed by @veltman:

const server = app.listen(/* ... */);

server.on('clientError', function(err, socket) {
  console.error(err);
  if (socket.writable) {
    socket.write('HTTP/1.1 400 Bad Request\r\n\r\n');
  }
  socket.destroy(err);
});

EDIT: Updated code from conversation below

If anyone has any other better ways to deal with this temporarily please let me know and I can update this example code. As mentioned in the issue links @dougwilson posted above, in node 11 there is a regression where this clientError is not fired, so this will not work for that.

@dougwilson If I am wrong in any of this, which I very well might be, please correct me.

[lpinca]

@wesleytodd the socket must be destroyed as it was before nodejs/node@f2f391e.

in node 11 there is a regression where this clientError is not fired, so this will not work for that.

Also not 100% correct as the event is emitted. The regression is for some particular cases as the one posted in the issue description.

[lpinca]

We have two open PRs to address the situation:

• http_parser: fix error return in Finish() nodejs/node#24738
• http: destroy the socket on parse error nodejs/node#24757

[veltman]

@lpinca will wait to see what lands, what I'm currently doing is:

server.on("clientError", function(err, socket) {
  console.warn(err.toString() + (err.code ? " " + err.code : ""));
  if (socket.writable) {
    socket.end("HTTP/1.1 400 Bad Request\r\n\r\n");
    return;
  }
  socket.destroy(err);
});

In order to match https://github.com/nodejs/node/blob/master/lib/_http_server.js#L513-L519 plus a log statement (since the practical issue for me w/ Express is that parsing errors are silent)

[lpinca]

@veltman by doing that you actually make every Node.js version behave like Node.js >= 9 which is what is causing issues in on-finished and then in turn in Express. Run the following code on Node.js 8 with and without the 'clientError' event listener to see what I mean.

'use strict';

const assert = require('assert');
const { createServer } = require('http');
const { createConnection } = require('net');

const server = createServer();

server.on('connection', (socket) => {
  socket.on('error', (err) => {
    assert(err instanceof Error);
    assert.strictEqual(err.message, 'Parse Error');
    assert.strictEqual(err.code, 'HPE_INVALID_METHOD');
  });

  server.close();
});

server.on('clientError', (err, socket) => {
  if (socket.writable) {
    socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');
    return;
  }
  socket.destroy(err);
});

server.listen(0, () => {
  const socket = createConnection({
    allowHalfOpen: true,
    port: server.address().port
  });

  socket.on('connect', () => {
    socket.write('FOO /\r\n');
  });
});

[veltman]

@lpinca I see what you mean, so it seems like:

if (socket.writable) {
  socket.write('HTTP/1.1 400 Bad Request\r\n\r\n');
}
socket.destroy(err);

is the preferred handling across versions, is that right?

[lpinca]

Yes, correct.

[DRoet]

seems like the fixes landed in 11.4.0 and in 10.14.2?

[dougwilson]

Hi @DRoet this is correct. I just confirmed those two versions are working again with the only known issue.