http: relax requirements on upgrade listener

The http spec does not say anything about Upgrade headers making
protocol switch mandatory but Node.js implements them as if they
are. Relax the requirements to only destroy the socket if no
upgrade listener exists on the client when status code is 101.

PR-URL: #19981
Fixes: #11552
Refs: https://tools.ietf.org/html/rfc7230#section-6.7
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

doc/api/http.md

@@ -471,8 +471,9 @@ added: v0.1.94
 * `head` {Buffer}
 
 Emitted each time a server responds to a request with an upgrade. If this
-event is not being listened for, clients receiving an upgrade header will have
-their connections closed.
+event is not being listened for and the response status code is 101 Switching
+Protocols, clients receiving an upgrade header will have their connections
+closed.
 
 A client server pair demonstrating how to listen for the `'upgrade'` event.
 
@@ -886,16 +887,20 @@ per connection (in the case of HTTP Keep-Alive connections).
 ### Event: 'upgrade'
 <!-- YAML
 added: v0.1.94
+changes:
+  - version: REPLACEME
+    pr-url: REPLACEME
+    description: Not listening to this event no longer causes the socket
+                 to be destroyed if a client sends an Upgrade header.
 -->
 
 * `request` {http.IncomingMessage} Arguments for the HTTP request, as it is in
   the [`'request'`][] event
 * `socket` {net.Socket} Network socket between the server and client
 * `head` {Buffer} The first packet of the upgraded stream (may be empty)
 
-Emitted each time a client requests an HTTP upgrade. If this event is not
-listened for, then clients requesting an upgrade will have their connections
-closed.
+Emitted each time a client requests an HTTP upgrade. Listening to this event
+is optional and clients cannot insist on a protocol change.
 
 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

lib/_http_client.js

@@ -426,7 +426,7 @@ function socketOnData(d) {
     req.socket._hadError = true;
     req.emit('error', ret);
   } else if (parser.incoming && parser.incoming.upgrade) {
-    // Upgrade or CONNECT
+    // Upgrade (if status code 101) or CONNECT
     var bytesParsed = ret;
     var res = parser.incoming;
     req.res = res;
@@ -453,7 +453,7 @@ function socketOnData(d) {
       req.emit(eventName, res, socket, bodyHead);
       req.emit('close');
     } else {
-      // Got Upgrade header or CONNECT method, but have no handler.
+      // Requested Upgrade or used CONNECT method, but have no handler.
       socket.destroy();
     }
   } else if (parser.incoming && parser.incoming.complete &&
@@ -492,6 +492,10 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
   }
   req.res = res;
 
+  // Skip body and treat as Upgrade.
+  if (res.upgrade)
+    return 2;
+
   // Responses to CONNECT request is handled as Upgrade.
   const method = req.method;
   if (method === 'CONNECT') {

lib/_http_common.js

@@ -83,6 +83,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
   parser.incoming.httpVersionMinor = versionMinor;
   parser.incoming.httpVersion = `${versionMajor}.${versionMinor}`;
   parser.incoming.url = url;
+  parser.incoming.upgrade = upgrade;
 
   var n = headers.length;
 
@@ -101,19 +102,6 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
     parser.incoming.statusMessage = statusMessage;
   }
 
-  if (upgrade && parser.outgoing !== null && !parser.outgoing.upgrading) {
-    // The client made non-upgrade request, and server is just advertising
-    // supported protocols.
-    //
-    // See RFC7230 Section 6.7
-    upgrade = false;
-  }
-
-  parser.incoming.upgrade = upgrade;
-
-  if (upgrade)
-    return 2;  // Skip body and treat as Upgrade.
-
   return parser.onIncoming(parser.incoming, shouldKeepAlive);
 }
 

lib/_http_outgoing.js

@@ -98,7 +98,6 @@ function OutgoingMessage() {
   this.writable = true;
 
   this._last = false;
-  this.upgrading = false;
   this.chunkedEncoding = false;
   this.shouldKeepAlive = true;
   this.useChunkedEncodingByDefault = true;
@@ -304,7 +303,6 @@ function _storeHeader(firstLine, headers) {
   // in the case of response it is: 'HTTP/1.1 200 OK\r\n'
   var state = {
     connection: false,
-    connUpgrade: false,
     contLen: false,
     te: false,
     date: false,
@@ -366,10 +364,6 @@ function _storeHeader(firstLine, headers) {
     }
   }
 
-  // Are we upgrading the connection?
-  if (state.connUpgrade && state.upgrade)
-    this.upgrading = true;
-
   // Date header
   if (this.sendDate && !state.date) {
     state.header += 'Date: ' + utcDate() + CRLF;
@@ -460,8 +454,6 @@ function matchConnValue(self, state, value) {
   while (m) {
     if (m[0].length === 5)
       sawClose = true;
-    else
-      state.connUpgrade = true;
     m = RE_CONN_VALUES.exec(value);
   }
   if (sawClose)

lib/_http_server.js

@@ -530,14 +530,14 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
     parser = null;
 
     var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';
-    if (server.listenerCount(eventName) > 0) {
+    if (eventName === 'upgrade' || server.listenerCount(eventName) > 0) {
       debug('SERVER have listener for %s', eventName);
       var bodyHead = d.slice(bytesParsed, d.length);
 
       socket.readableFlowing = null;
       server.emit(eventName, req, socket, bodyHead);
     } else {
-      // Got upgrade header or CONNECT method, but have no handler.
+      // Got CONNECT method, but have no handler.
       socket.destroy();
     }
   }
@@ -592,6 +592,13 @@ function resOnFinish(req, res, socket, state, server) {
 function parserOnIncoming(server, socket, state, req, keepAlive) {
   resetSocketTimeout(server, socket, state);
 
+  if (req.upgrade) {
+    req.upgrade = req.method === 'CONNECT' ||
+                  server.listenerCount('upgrade') > 0;
+    if (req.upgrade)
+      return 2;
+  }
+
   state.incoming.push(req);
 
   // If the writable end isn't consuming, then stop reading

test/parallel/test-http-upgrade-advertise.js

@@ -8,7 +8,9 @@ const tests = [
   { headers: {}, expected: 'regular' },
   { headers: { upgrade: 'h2c' }, expected: 'regular' },
   { headers: { connection: 'upgrade' }, expected: 'regular' },
-  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' }
+  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' },
+  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'destroy' },
+  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'regular' },
 ];
 
 function fire() {
@@ -32,10 +34,17 @@ function fire() {
     done('regular');
   });
 
-  req.on('upgrade', function onUpgrade(res, socket) {
-    socket.destroy();
-    done('upgrade');
-  });
+  if (test.expected === 'destroy') {
+    req.on('socket', () => req.socket.on('close', () => {
+      server.removeAllListeners('upgrade');
+      done('destroy');
+    }));
+  } else {
+    req.on('upgrade', function onUpgrade(res, socket) {
+      socket.destroy();
+      done('upgrade');
+    });
+  }
 
   req.end();
 }

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

@@ -121,8 +121,6 @@ function test_upgrade_with_listener() {
 /*-----------------------------------------------
   connection: Upgrade, no listener
 -----------------------------------------------*/
-let test_upgrade_no_listener_ended = false;
-
 function test_upgrade_no_listener() {
   const conn = net.createConnection(server.address().port);
   conn.setEncoding('utf8');
@@ -135,8 +133,9 @@ function test_upgrade_no_listener() {
              '\r\n');
   });
 
-  conn.on('end', function() {
-    test_upgrade_no_listener_ended = true;
+  conn.once('data', (data) => {
+    assert.strictEqual(typeof data, 'string');
+    assert.strictEqual(data.substr(0, 12), 'HTTP/1.1 200');
     conn.end();
   });
 
@@ -182,5 +181,4 @@ server.listen(0, function() {
 process.on('exit', function() {
   assert.strictEqual(3, requests_recv);
   assert.strictEqual(3, requests_sent);
-  assert.ok(test_upgrade_no_listener_ended);
 });