Node incorrectly responds 400 to invalid data sent after requests with `Connection: close`

[kenballus]

Version

v23.0.0-pre

Platform

Linux cef57c85a6b6 6.8.9-arch1-2 #1 SMP PREEMPT_DYNAMIC Tue, 07 May 2024 21:35:54 +0000 x86_64 GNU/Linux

Subsystem

http

What steps will reproduce the bug?

0. Copy the following into server.js:

   require('http').createServer((request, response) => {
   let body = [];
   request.on('data', (chunk) => {
       body.push(chunk);
   }).on('end', () => {
       response.end(JSON.stringify({
           "headers": Object.keys(request.headers).map((key) => [btoa(key), btoa(request.headers[key])]),
           "body": Buffer.concat(body).toString('base64'),
           "uri": btoa(request.url),
           "method": btoa(request.method),
           "version": btoa(request.httpVersion),
       }));
   });
   }).listen(80);

1. Run it:

   node server.js

2. Send it a request with Connection: close, then send some junk data:

   printf 'POST / HTTP/1.1\r\nHost: whatever\r\nConnection: close\r\nContent-Length: 0\r\n\r\nInvalid!' | nc localhost 80

How often does it reproduce? Is there a required condition?

Reproduces every time with no special conditions required.

What is the expected behavior? Why is that the expected behavior?

The expected behavior is for the server to respond to the initial request, and then close the connection without processing the invalid data.

This is the expected behavior because of the following line in RFC 9112:

A server that receives a "close" connection option MUST initiate closure of the connection (see below) after it sends the final response to the request that contained the "close" connection option. The server SHOULD send a "close" connection option in its final response on that connection. The server MUST NOT process any further requests received on that connection.

Other servers (AIOHTTP, Apache httpd, CherryPy, Daphne, Deno, FastHTTP, Go net/http, Gunicorn, H2O, Hyper, Jetty, Libevent, Libsoup, Lighttpd, Mongoose, Nginx, Passenger, Proxygen, Apache Tomcat, Tornado, OpenWrt uhttpd, Unicorn, Waitress, WEBrick, Werkzeug) handle this in the expected way.

What do you see instead?

The server responds 400, indicating that the invalid data was processed:

HTTP/1.1 400 Bad Request\r\n
Connection: close\r\n
\r\n

Additional information

No response

[tomksck]

According to RFC 7231 sending a body within a GET request is undefined behavior:

A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.

RFC 7231: 4.3.1: Method Definitions - GET

Bad Request seems to be a valid response in my opinion.

[kenballus]

My example does not use a GET request with a message body. It uses a GET request followed by some invalid bytes.

This is distinct from a message body because there is no Content-Length or Transfer-Encoding header. In the absence of both, a request's message body is defined to be empty.

That said, I'll still update the comment to use an empty POST just to make this point clear.

[tomksck]

Reproduced the behavior with node version v22.2.0.

[tomksck]

But this seems to still be intentional behavior. Using POST with Content-Length: 0, but providing a body, fails even without Connection: close.

[kenballus]

I'm not "providing a body" after the request, I'm just sending some more bytes on the connection. These extra bytes are not part of the message body because of the Content-Length: 0 header. There is no interpretation for these bytes; the standard says that bytes sent after a request containing Connection: close must not be processed. Responding 400 to the message due to invalid bytes sent after the message indicates the processing of those bytes, and is therefore in violation of the RFCs.

The correct behavior is to ignore bytes sent after the connection is closed. This is what other HTTP implementations do.

[tomksck]

With the Content-Length header set to 0, the server does not expect any Content. If additional data is provided it should result in a 400 (Bad Request). This is expected behavior as described in RFC 7230 3.3.3.4. The Connection: close header indicates that following requests must not be processed AFTER the current request is processed. Edit: However this is handled differently on many other servers, as you described.

[kenballus]

Again, the bytes sent after the request are not part of the message body of the first request, because of the Content-Length: 0 header. The processing of the first request does not entail processing the bytes sent after the first request. Processing these bytes (and consequently realizing that they are invalid) indicates that the connection is not being closed.

This does not fall under the scope of 7230 3.3.3.4 because the Content-Length value is valid. What is meant by "invalid value" in that context is a value that doesn't match the grammar (i.e., a value that is not one or more ASCII digits).

To further drive this point home, suppose the payload were the following:

GET / HTTP/1.1\r\n
Host: whatever\r\n
Content-Length: 0\r\n
Connection: close\r\n
\r\n
GET / HTTP/1.1\r\n
Host: whatever\r\n
\r\n

Node will respond 400 to this, which is the wrong response. The 400 indicates an invalid request, but there is no invalid request here. There is a valid request, which closes the connection. No further requests should be processed. The fact that a 400 response is sent indicates that the rest of the bytes on the connetion (which constitute the second request) were processed.