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

Activity

1. dougwilson

   added

   meta

   on Nov 30, 2018
2. addaleax commented on Nov 30, 2018

   

   addaleax

   on Nov 30, 2018

   More actions

   @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.
3. wesleytodd commented on Nov 30, 2018

   

   wesleytodd

   on Nov 30, 2018

   Last edited by wesleytodd

   Member

   More actions

   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.
4. lpinca commented on Dec 1, 2018

   

   lpinca

   on Dec 1, 2018

   More actions

   @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.
5. lpinca commented on Dec 1, 2018

   

   lpinca

   on Dec 1, 2018

   More actions

   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
6. veltman commented on Dec 1, 2018

   

   veltman

   on Dec 1, 2018

   More actions

   @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)
7. lpinca commented on Dec 1, 2018

   

   lpinca

   on Dec 1, 2018

   More actions

   @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');
     });
   });
8. veltman commented on Dec 1, 2018

   

   veltman

   on Dec 1, 2018

   More actions

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

   👍React with 👍2Reacted by Luigi Pinca and Wes Todd
9. lpinca commented on Dec 1, 2018

   

   lpinca

   on Dec 1, 2018

   More actions

   Yes, correct.

   👍React with 👍1Reacted by Noah Veltman
10. DRoet commented on Dec 11, 2018

    

    DRoet

    on Dec 11, 2018

    Last edited by DRoet

    More actions

    seems like the fixes landed in 11.4.0 and in 10.14.2?
11. dougwilson commented on Dec 12, 2018

    

    dougwilson

    on Dec 12, 2018

    ContributorAuthor

    More actions

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

    closed this as completedon Dec 12, 2018