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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
nodejs · Apr 16, 2018 · 33ce9a6 · 33ce9a6
1 parent 6d1c3e5
commit 33ce9a6
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 40 deletions.
diff --git a/doc/api/http.md b/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

diff --git a/lib/_http_client.js b/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') {

diff --git a/lib/_http_common.js b/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);
 }
 

diff --git a/lib/_http_outgoing.js b/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)

diff --git a/lib/_http_server.js b/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

diff --git a/test/parallel/test-http-upgrade-advertise.js b/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();
 }

diff --git a/test/parallel/test-http-upgrade-server.js b/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);
 });