http: reject pipelined protocol switches

doc/api/http.md

@@ -1631,7 +1631,8 @@ added: v0.7.0
 
 Emitted each time a client requests an HTTP `CONNECT` method. If this event is
 not listened for, then clients requesting a `CONNECT` method will have their
-connections closed.
+connections closed. If an earlier response is still pending on the same
+connection, the connection is closed and this event is not emitted.
 
 This event is guaranteed to be passed an instance of the {net.Socket} class,
 a subclass of {stream.Duplex}, unless the user specifies a socket
@@ -1738,7 +1739,9 @@ change.
 
 If an upgrade is accepted by `shouldUpgradeCallback` but no event handler
 is registered then the socket will be destroyed, resulting in an immediate
-connection closure for the client.
+connection closure for the client. If an earlier response is still pending on
+the same connection, the connection is also closed and this event is not
+emitted.
 
 In the uncommon case that the incoming request has a body, this body will be
 parsed as normal, separate to the upgrade stream, and the raw stream data will

lib/_http_server.js

@@ -1144,6 +1144,13 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
     debug('SERVER upgrade or connect', req.method);
     const eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';
 
+    // Switching protocols while an earlier response is still pending would
+    // allow the upgrade response to be written out of order.
+    if (socket._httpMessage || state.outgoing.length > 0) {
+      socket.destroy();
+      return;
+    }
+
     let upgradeStream;
     if (req.complete) {
       d ||= parser.getCurrentBuffer();

test/parallel/test-http-pipeline-upgrade-response-order.mjs

@@ -0,0 +1,97 @@
+import * as common from '../common/index.mjs';
+import assert from 'node:assert';
+import { once } from 'node:events';
+import http from 'node:http';
+import net from 'node:net';
+
+const CRLF = '\r\n';
+const firstRequest = `GET /first HTTP/1.1${CRLF}Host: localhost${CRLF}${CRLF}`;
+
+function switchRequest(eventName) {
+  if (eventName === 'connect') {
+    return `CONNECT localhost:443 HTTP/1.1${CRLF}` +
+           `Host: localhost:443${CRLF}${CRLF}`;
+  }
+
+  return `GET /second HTTP/1.1${CRLF}` +
+         `Host: localhost${CRLF}` +
+         `Connection: Upgrade${CRLF}` +
+         `Upgrade: websocket${CRLF}${CRLF}`;
+}
+
+function switchResponse(eventName) {
+  if (eventName === 'connect') {
+    return `HTTP/1.1 200 Connection Established${CRLF}${CRLF}SWITCHED`;
+  }
+
+  return `HTTP/1.1 101 Switching Protocols${CRLF}` +
+         `Connection: Upgrade${CRLF}` +
+         `Upgrade: websocket${CRLF}${CRLF}SWITCHED`;
+}
+
+async function listen(server) {
+  server.listen(0);
+  await once(server, 'listening');
+}
+
+async function close(server) {
+  await new Promise((resolve) => server.close(resolve));
+}
+
+async function testPipelinedSwitchIsRejected(eventName) {
+  const server = http.createServer(common.mustCall((req) => {
+    assert.strictEqual(req.url, '/first');
+  }));
+
+  server.on(eventName, common.mustNotCall());
+  await listen(server);
+
+  const client = net.connect(server.address().port);
+  let received = '';
+  client.setEncoding('latin1');
+  client.on('data', (data) => received += data);
+  client.on('error', () => {});
+  const closed = new Promise((resolve) => client.once('close', resolve));
+  client.end(firstRequest + switchRequest(eventName));
+
+  await closed;
+  assert.strictEqual(received, '');
+  await close(server);
+}
+
+async function testSwitchAfterResponseIsAccepted(eventName) {
+  const responseFinished = Promise.withResolvers();
+  const server = http.createServer(common.mustCall((req, res) => {
+    assert.strictEqual(req.url, '/first');
+    res.on('finish', responseFinished.resolve);
+    res.end('FIRST');
+  }));
+
+  server.on(eventName, common.mustCall((req, socket) => {
+    socket.end(switchResponse(eventName));
+  }));
+  await listen(server);
+
+  const client = net.connect(server.address().port);
+  let received = '';
+  client.setEncoding('latin1');
+  client.on('data', (data) => received += data);
+  client.on('error', () => {});
+  client.write(firstRequest);
+
+  await responseFinished.promise;
+  client.end(switchRequest(eventName));
+  await once(client, 'close');
+
+  const firstIndex = received.indexOf('FIRST');
+  const switchedIndex = received.indexOf('SWITCHED');
+  assert.notStrictEqual(firstIndex, -1);
+  assert.notStrictEqual(switchedIndex, -1);
+  assert.ok(firstIndex < switchedIndex);
+  await close(server);
+}
+
+for (const eventName of ['upgrade', 'connect']) {
+  await testPipelinedSwitchIsRejected(eventName);
+  await testSwitchAfterResponseIsAccepted(eventName);
+}