HTTP Response read past end

[bmeck]

• Version: 15.8.*
• Platform: All
• Subsystem: http

What steps will reproduce the bug?

import net from 'net';
import http from 'http';

// create a simple server with too small of content-length
const body = 'HTTP/1.1 200 OK\r\n' +
  'Content-Length: 5\r\n' +
  'Connection: close\r\n' +
  '\r\n' +
  '2ad731e3-4dcd-4f70-b871-0ad284b29ffc'
const server = net.createServer((conn) => conn.end(body));
const port = 9191;
server.listen(port, () => {
  // try to GET from the server
  http.get('http://localhost:' + port);
});

// causes Error: Parse Error: Expected HTTP/

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

Always

What is the expected behavior?

No error, doesn't read past content-length and/or doesn't try to parse a second response from a connection: close response. Unclear on expected HTTP semantics and common leniency here. Likely shouldn't do either I suspect.

What do you see instead?

$ node bug.js
node:events:355
      throw er; // Unhandled 'error' event
      ^

Error: Parse Error: Expected HTTP/
    at Socket.socketOnData (node:_http_client:502:22)
    at Socket.emit (node:events:378:20)
    at addChunk (node:internal/streams/readable:313:12)
    at readableAddChunk (node:internal/streams/readable:288:9)
    at Socket.Readable.push (node:internal/streams/readable:227:10)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23)
Emitted 'error' event on ClientRequest instance at:
    at Socket.socketOnData (node:_http_client:509:9)
    at Socket.emit (node:events:378:20)
    [... lines matching original stack trace ...]
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23) {
  bytesParsed: 62,
  code: 'HPE_INVALID_CONSTANT',
  reason: 'Expected HTTP/',
  rawPacket: Buffer(93) [Uint8Array] [
     72,  84,  84,  80,  47,  49,  46,  49,  32,  50,  48,  48,
     32,  79,  75,  13,  10,  67, 111, 110, 116, 101, 110, 116,
     45,  76, 101, 110, 103, 116, 104,  58,  32,  53,  13,  10,
     67, 111, 110, 110, 101,  99, 116, 105, 111, 110,  58,  32,
     99, 108, 111, 115, 101,  13,  10,  13,  10,  50,  97, 100,
     55,  51,  49, 101,  51,  45,  52, 100,  99, 100,  45,  52,
    102,  55,  48,  45,  98,  56,  55,  49,  45,  48,  97, 100,
     50,  56,  52,  98,  50,  57, 102, 102,  99
  ]
}

Additional information

[jasnell]

/cc @indutny @nodejs/http

[indutny]

I believe this is working as expected. The error message would have been clearer if Node.js used strict mode of llhttp:

off=71 error code=5 reason="Data after `Connection: close`"

The spec describes how to determine the message body length and there is only one note about Connection: close case that loose (not strict) mode violates:

If the final response to the last request on a connection has been
completely received and there remains additional data to read, a user
agent MAY discard the remaining data or attempt to determine if that
data belongs as part of the prior response body, which might be the
case if the prior message's Content-Length value is incorrect.  A
client MUST NOT process, cache, or forward such extra data as a
separate response, since such behavior would be vulnerable to cache
poisoning.

Which makes me worry that loose mode is implemented this way, but doesn't change the fact that error is expected anyway.

[bmeck]

@indutny something seems odd here though, if I put a valid response in that location like below, there isn't the same error.

import net from 'net';
import http from 'http';
const body = 'HTTP/1.1 200 OK\r\n' +
  'Content-Length: 5\r\n' +
  'Connection: close\r\n' +
  '\r\n' +
  '2ad73HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n'
const server = net.createServer((conn) => conn.end(body));
const port = 9191;
server.listen(port, () => {
  http.get('http://localhost:' + port, (res) => {
    // drain it
    res.on('data', console.log)
    res.on('close', () => server.close());
  }).end();
});

This seems problematic given the:

MUST NOT process, cache, or forward such extra data as a
separate response, since such behavior would be vulnerable to cache
poisoning.

point it does seem to be processing it.

[indutny]

@bmeck yeah, precisely! I'm thinking about patching this up in llhttp, but it was ported over from http_parser AFAIK so I'm not sure how much servers/clients we'll break...

The reason why this happens is that during the build of llhttp (and http_parser) you have to decide whether to build it in strict or loose mode. strict is rather hardcore, but follows the spec very very closely. loose allows certain characters out of the normal range, and overall is more lenient. This leniency includes parsing the second request with Connection: close. IMO, this is a bug and I don't mind fixing it if everyone is on the same page about it.

[bmeck]

Is there any reason to error instead of discarding? I would expect not to error in the result and it actually was found due to https://github.com/http-tests/cache-tests being unable to run against node due to the unrecoverable error.

[indutny]

@bmeck yeah, discarding is fine too according to spec. strict mode can error, and loose can discard. Is this what you are suggesting?

[bmeck]

@indutny yes, I would prefer discarding since node's http client can actually pass those tests if we do. Also in theory it could be less compute (since we already track the length).

[mcollina]

I agree that discarding would be a good approach.

[indutny]

I technically have a patch doing this, but just thought I'd run by you all the tests that no longer pass with discarding of the extra data:

• https://github.com/nodejs/llhttp/blob/242cfd4ebc2b57dd84904cb67b617bf4a6b6fa93/test/request/connection.md#resetting-flags-when-keep-alive-is-off-10-and-parser-is-in-loose-mode
• https://github.com/nodejs/llhttp/blob/242cfd4ebc2b57dd84904cb67b617bf4a6b6fa93/test/request/connection.md#crlf-between-requests-explicit-close-loose-mode
• https://github.com/nodejs/llhttp/blob/242cfd4ebc2b57dd84904cb67b617bf4a6b6fa93/test/response/connection.md#http11-with-keep-alive-disabled-and-204-status-in-loose-mode

It is definitely going to be a major version bump for llhttp, but more importantly I think I have to make it continue work with the lenient flags so that affected users will still have a way of allowing it.

[indutny]

Here's the Pull Request. Would appreciate a review for it, so that I can merge and release it.

[indutny]

(...and sorry for a very long delay in responding to y'all. Things have been quite busy at my new job)

[bmeck]

@indutny LGTM

[mcollina]

Fixed in nodejs/llhttp#94 as well as #38146.