Malformed HTTP request causes connection drop

[triblondon]

Node:

var server = require('http').createServer(function(request, response){
    console.log('foo?');
    response.end('It Works!! Path Hit: ' + request.url);
});

server.listen(3200);

Telnet:

[andrew.betts ~]$ telnet localhost 3200
Connected to localhost.
GET HTTP/1.1 HTTP/1.1
Connection closed by foreign host.

Expected: An error to be thrown or the request to be handled.
Actual: No error is thrown, the server callback is not invoked, and the connection is closed.

This seems to suggest to me that it is impossible for me to correctly handle malformed HTTP requests from my Node application. I would like to be able to return a valid HTTP 400 Bad Request response.

[Trott]

I think you are looking for the clientError event.

var server = require('http').createServer(function(request, response){
    console.log('foo?');
    response.end('It Works!! Path Hit: ' + request.url);
});

server.on('clientError', function(err) {
    console.log('ERROR', err);
});

server.listen(3200);

The server logs: ERROR { [Error: Parse Error] bytesParsed: 8, code: 'HPE_INVALID_URL' }

[triblondon]

oooo. Thanks. I went and looked this up in the docs and I found the definition of that event:

If a client connection emits an 'error' event, it will be forwarded here.

That's not a great definition - if someone could chuck some key words in there like 'malformed', 'bad request', 'invalid http' that would help people find the right event.

Will also open an issue with connect and/or express, neither of which handle this.

[Trott]

/cc @nodejs/http in case there's a better way to handle this and/or someone wants to dive in and expand the doc for the event a bit.

[Trott]

/cc @nodejs/documentation too...

[triblondon]

Followup: so whilst it's possible to see an event when an invalid request is received, I still can't make the server respond with the correct HTTP response?

[triblondon]

The context for this is that HTTP clients representing middleboxes and proxies such as load balancers, CDNs and so on may interpret a prematurely closed connection from a backend as an indication that the backend is sick, rather than that their request was invalid. I still need a way to get Node to response with a 400 Bad Request or 501 Not Implemented.

[Trott]

Not a Node-land answer, but if you put nginx in front of your Node HTTP server, it will return 400 Bad Request for GET HTTP/1.1 HTTP/1.1 (or at least that's what I'm seeing--I suppose it might depend on configuration).

[triblondon]

Yes, I'd do that, but I'm using Heroku (which is obvs a common use case for Node/express), and Heroku router passed malformed requests on to the application to deal with, then throws a platform error if the app closes the connection prematurely, and issues a 503 Service Unavailable with Heroku error HTML to the end user. Not great.

[bnoordhuis]

We could change the semantics of the 'clientError' event to make the listener responsible for closing the socket. That lets the programmer write out a 400 response but then ownership gets hairy when there is more than one listener.

[indutny]

It is very important to take in account that there is no pending request, because the data sent was invalid (be it one request, or several - we don't know for sure).

[dougwilson]

Yea, if the client sends a request like the given malformed one, I don't think Node.js can be expected to even be able to construct a Request or Response object for your code to generate a reply with.

Now, the clientError event does give the underlying socket as the second argument, but eve if you remove the default event listener (https://github.com/nodejs/node/blob/master/lib/_http_server.js#L240-L242) the socket is still destroyed, so you don't have a chance to reply.

If there was at least a way to write user-land code in order to write raw data on the socket after a client error, that would get us into a position where a user can choose to do something different than the default behavior.

[indutny]

Proposed "fix" is there: #4557

[jameshartig]

The default behavior before v6 is that you can only log and the http server itself handled destroying the socket for you. But now if you need to write code to handle <v6 and v6, do you need to check socket.destroyed?

Something like:

server.on('clientError', function(err, socket) {
    console.error('clientError received')
    if (!socket.destroyed) {
        socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');
    }
});

What's the best way of doing this?

[mattolson]

@fastest963 It is impossible to write a reliable handler for clientError that returns a response in Node 4.x because the socket is destroyed by the time the event handler is called.

https://github.com/nodejs/node/blob/v4.x/lib/_http_server.js#L385

The only thing to do if you care about this is to upgrade to 6.x or above.