Skip to content

HTTP Upgrade IncomingMessage does not parse body #58394

Description

@mscdex

Version

22.15.0

Platform

Linux

Subsystem

http

What steps will reproduce the bug?

'use strict';

const { createServer, request } = require('http');

createServer((req, res) => {
  res.writeHead(200).end();
}).on('upgrade', (req, reqSocket, head) => {
  console.log('Server Upgrade', req.headers);
  let buf = '';
  req.on('data', (str) => buf += str);
  req.on('close', () => {
    console.log(
      'Request close',
      'data', JSON.stringify(buf.trim()),
      'head', Buffer.from(head)
    );
  });
  req.setEncoding('utf8');
}).listen(0, '127.0.0.1', function() {
  const req = request({
    host: '127.0.0.1',
    port: this.address().port,
    method: 'POST',
    path: '/foo',
    headers: {
      Connection: 'Upgrade',
      Upgrade: 'db',
    },
  });
  req.on('upgrade', (res, socket, head) => {
    console.log(
      'Client Upgrade',
      'status', res.statusCode,
      'head', Buffer.from(head)
    );
  });
  setTimeout(() => {
    req.write('2');
    setTimeout(() => req.end(), 1000);
  }, 1000);
});

Output:

Server Upgrade {                                                                                                                                                                                                   
  connection: 'Upgrade',                                                                                                                                                                                           
  upgrade: 'db',                                                                                                                                                                                                   
  host: '127.0.0.1:39511',                                                                                                                                                                                         
  'transfer-encoding': 'chunked'                                                                                                                                                                                   
}                                                                                                                                                                                                                  
Request close data "" head <Buffer 31 0d 0a 32 0d 0a>

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

Reproduces every time.

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

The expected behavior is to have node parse the body like it normally would.

What do you see instead?

Node ignores the content or potentially puts some of it (unparsed -- especially evident in the case of a chunked encoded body) in the head argument passed to the 'upgrade' event.

Additional information

No response

Activity

  1. lpinca commented on May 19, 2025

    @lpinca
    Member

    Isn't this expected? It is upgrading to a different protocol. The HTTP parser is freed after the 'upgrade' event.

  2. mscdex commented on May 19, 2025

    @mscdex
    ContributorAuthor

    Isn't this expected?

    Not to me. Node can tell if there is a body just like any normal HTTP request and so it should be able to parse that. This is very useful if you want to pass information to the server before switching protocols and without resorting to spamming the querystring or headers.

  3. added
    httpIssues and PRs related to the http subsystem.
    on May 20, 2025
  4. lpinca commented on May 20, 2025

    @lpinca
    Member

    FWIW, it worked like this since forever, and I think it makes sense. You can't tell beforehand if the body need to be used as data for the upgraded protocol. For example, even though the WebSocket protocol spec specifies that no data should be exchanged until the opening handshake is complete, a lot of implementations use the body of the handshake request as WebSocket data. I think this is also why the callback of the 'upgrade' events takes the head parameter.

  5. mscdex commented on May 20, 2025

    @mscdex
    ContributorAuthor

    You can't tell beforehand if the body need to be used as data for the upgraded protocol.

    Why would a client bother sending a body (in the HTTP sense indicated via Content-Length or Transfer-Encoding: chunked) if they could just send the same data as part of the upgraded protocol (whether it's before or after the server sends the 101 response)?

    For example, even though the WebSocket protocol spec specifies that no data should be exchanged until the opening handshake is complete, a lot of implementations use the body of the handshake request as WebSocket data.

    The difference is that WebSocket requests must be GET requests, which means they can never have a body, so there is no problem there.

    It's only a problem for other/custom upgraded protocols that allow non-GET upgrade requests.

  6. lpinca commented on May 20, 2025

    @lpinca
    Member

    Why would a client bother sending a body (in the HTTP sense indicated via Content-Length or Transfer-Encoding: chunked) if they could just send the same data as part of the upgraded protocol (whether it's before or after the server sends the 101 response)?

    How and when do you decide to which protocol the incoming data belongs to?

    The difference is that WebSocket requests must be GET requests, which means they can never have a body, so there is no problem there.

    There are misbehaving clients that actually send data in the body of the initial GET request.

  7. mscdex commented on May 20, 2025

    @mscdex
    ContributorAuthor

    How and when do you decide to which protocol the incoming data belongs to?

    I see two scenarios:

    1. A client has indicated their HTTP request includes a body. Any upgraded protocol data would come immediately after the end of the body.

    2. A client has not indicated their HTTP request includes a body. Any upgraded protocol data would come immediately after the end of the client HTTP headers.

    There are misbehaving clients that actually send data in the body of the initial GET request.

    Perhaps there are, but how common is that? I would wager that browsers make up the bulk majority of WebSocket requests, which do not misbehave (especially in 2025).

    Additionally, there should be a distinction between misbehaving WebSocket clients sending handshake data immediately after the HTTP headers and those who are indicating an HTTP body in their request (by setting the appropriate HTTP headers). The former does not matter because they never indicated there was a body to be parsed and the latter is a bit of a moot case because the client is just extra broken at that point (and if node really wanted to care about that kind of case, you could just check for that specific scenario -- GET + "Upgrade: Websocket" + HTTP body).

  8. lpinca commented on May 20, 2025

    @lpinca
    Member

    I never investigated, but is the current behavior Node.js specific? Is there anything about this in HTTP spec? How are other runtimes / languages handling this?

  9. mscdex commented on May 20, 2025

    @mscdex
    ContributorAuthor

    Is there anything about this in HTTP spec?

    The current RFC says this about upgrade requests: (emphasis mine)

    A client cannot begin using an upgraded protocol on the connection until it has completely sent the request message (i.e., the client can't change the protocol it is sending in the middle of a message).

    Earlier in the same RFC it describes the definition of a "message", which includes everything from the first line containing (in the case of a request) the method and path to the optional HTTP trailers.

    This seems to align with my expectation that the server should be processing any HTTP request body as normal and anything after that is processed as the upgraded protocol (if the server sends a 101).

  10. pimterry commented on Sep 10, 2025

    @pimterry
    Member

    I'm been having a little look into this. Pretty sure @mscdex is right and that properly included bodies (e.g. content-length: N + body) should be considered part of the request, not the upgrade stream. Unfortunately llhttp doesn't support that at all, and always assumes there's no body if you upgrade (see the return values for on_headers_complete in https://github.com/nodejs/llhttp).

    I'm looking into it, but I haven't touched llhttp before now. Seems like an interesting way to explore it but might take me a while, anybody else is welcome to dig in too if they have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    httpIssues and PRs related to the http subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions