Conflict between upgrade and chunked encoding

[jcoglan]

As of version 0.4.11 of this library, attempting to parse a WebSocket handshake response that includes the header transfer-encoding: chunked results in the parser consuming any frames that may have been included in the same input chunk. Here is an example:

const { HTTPParser } = require('http-parser-js');

let input = Buffer.concat([
  Buffer.from(
    'HTTP/1.1 101 Switching Protocols\r\n' +
    'Connection: upgrade\r\n' +
    'Upgrade: websocket\r\n' +
    'Sec-WebSocket-Accept: QV3I5XUXU2CdhtjixE7QCkCcMZM=\r\n' +
    'Transfer-Encoding: chunked\r\n' +
    '\r\n'
  ),
  Buffer.from([0x81, 0x05, 0x68, 0x65, 0x6c, 0x6c, 0x6f])
]);

let parser = new HTTPParser(HTTPParser.RESPONSE);
let consumed = parser.execute(input);

console.log(
  input.length,
  consumed,
  input.slice(consumed));

The input here is a handshake response followed by a single message containing the text hello. Using version 0.4.10, this outputs the following, allowing the caller to parse the headers and then retrieve the first frame:

164 157 <Buffer 81 05 68 65 6c 6c 6f>

As of version 0.4.11, the the parser consumes the whole buffer, leaving the caller with no idea where the first message begins.

I've narrowed the introduction of this bug down to 6685c34, but I don't have enough context for that change to know how to fix it.

[Jimbly]

Thanks for the simple repro case and analysis!

Node's changed what behavior and return values are needed over time, so I also don't remember enough context to know what's going on there, but I'll look into it. Which Node version did this reproduce on?

[Jimbly]

It appears the problem is that the response you've got there is invalid - it's saying it's chunked encoding, but the body is not chunked (should be "7\r\n\x81\x05hello" if it was). I suspect chunked encoding is never valid on an upgrade response, at least not a WebSocket upgrade response, so it seems the solution is to ignore "transfer-encoding: chunked" in this case. This seems to fix your case, and not break any of the others (reverting the commit referenced breaks a bunch of things).

I've got a fix in c91bbf1. Can you easily test if this resolves the issue for you? It does solve your simple test included above.

[Jimbly]

Ah, yeah, according to this thread that header should be ignored according to "section 4.4 of RFC 2616", so I think this is a safe and correct fix, so it should be good to publish.

[Jimbly]

This is now published to NPM as v0.5.1

[jcoglan]

Thank you for fixing this so quickly :) I'll bump my dependency and get a new release done asap.