Fix upgrade with request body streaming

Author: pimterry

doc/api/http.md

@@ -1672,6 +1672,11 @@ per connection (in the case of HTTP Keep-Alive connections).
 <!-- YAML
 added: v0.1.94
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/00000
+    description: This event will not fire until the request body has been
+                 received. Previously, it fired immediately and the request
+                 body was incorrectly included in `head` instead.
   - version: v24.9.0
     pr-url: https://github.com/nodejs/node/pull/59824
     description: Whether this event is fired can now be controlled by the
@@ -1703,6 +1708,11 @@ After this event is emitted, the request's socket will not have a `'data'`
 event listener, meaning it will need to be bound in order to handle data
 sent to the server on that socket.
 
+In the (uncommon) case that the incoming request has a body, this event will
+not be emitted until the body has been fully received. This differs from normal
+`'request'` event handling, but ensures that all data received on the socket
+is part of the upgraded protocol, not the initial request.
+
 If an upgrade is accepted by `shouldUpgradeCallback` but no event handler
 is registered then the socket is destroyed, resulting in an immediate
 connection closure for the client.

lib/_http_server.js

@@ -952,6 +952,12 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
     debug('parse error', ret);
     socketOnError.call(socket, ret);
   } else if (parser.incoming?.upgrade) {
+    if (!parser.incoming.complete) {
+      // The request is still incomplete - this means it's an upgrade with a request
+      // body that hasn't yet been received. We have to wait for the body.
+      return;
+    }
+
     // Upgrade or CONNECT
     const req = parser.incoming;
     debug('SERVER upgrade or connect', req.method);

test/parallel/test-http-upgrade-server-with-body.js

@@ -10,37 +10,63 @@ const server = http.createServer();
 server.on('request', common.mustNotCall());
 
 server.on('upgrade', function(req, socket, upgradeHead) {
+  // Read the body:
   let reqBody = '';
   req.on('data', common.mustCall((str) => { reqBody += str; }));
   req.on('end', common.mustCall(() => {
     assert.strictEqual(reqBody, 'request body');
-    socket.end();
   }));
 
   assert.strictEqual(upgradeHead.toString(), 'upgrade head');
+
+  // Confirm the upgrade:
+  socket.write('HTTP/1.1 101 Switching Protocols\r\n' +
+               'Upgrade: custom-protocol\r\n' +
+               'Connection: Upgrade\r\n' +
+               '\r\n');
+
+  // Check we see the upgraded protocol data only (not the
+  // request body):
+  socket.on('data', common.mustCall((d) => {
+    assert.strictEqual(d.toString(), 'post-upgrade message');
+    socket.end();
+  }));
 });
 
-server.listen(0, function() {
+server.listen(0, async function() {
   const conn = net.createConnection(this.address().port);
   conn.setEncoding('utf8');
 
-  conn.on('connect', function() {
-    conn.write(
-      'POST / HTTP/1.1\r\n' +
-      'Upgrade: WebSocket\r\n' +
-      'Connection: Upgrade\r\n' +
-      'Content-Length: 12\r\n' +
-      '\r\n' +
-      'request body' +
-      'upgrade head'
-    );
-  });
-
-  conn.on('end', function() {
-    conn.end();
-  });
-
-  conn.on('close', function() {
-    server.close();
-  });
+  await new Promise((resolve) => conn.on('connect', resolve));
+
+  // Write request headers, leave the body pending:
+  conn.write(
+    'POST / HTTP/1.1\r\n' +
+    'host: localhost\r\n' +
+    'Upgrade: custom-protocol\r\n' +
+    'Connection: Upgrade\r\n' +
+    'transfer-encoding: chunked\r\n' +
+    '\r\n'
+  );
+
+  await new Promise((resolve) => setTimeout(resolve, 10));
+
+  // Write the body and part of the initial upgrade data. Most clients
+  // shouldn't send this until after 101 response, but we want to make
+  // sure it works even if weird cases.
+  conn.write(
+    'C\r\nrequest body\r\n' +
+    '0\r\n\r\n' +
+    'upgrade head'
+  );
+
+  const response = await new Promise((resolve) => conn.once('data', resolve));
+  assert.ok(response.startsWith('HTTP/1.1 101 Switching Protocols\r\n'));
+
+  conn.write('post-upgrade message');
+
+  await new Promise((resolve) => conn.on('end', resolve));
+
+  conn.end();
+  server.close();
 });