http: fix connection upgrade checks

This commit fixes connection upgrade checks, specifically when headers
are passed as an array instead of a plain object to http.request()

Fixes: #8235
PR-URL: #8238
Reviewed-By: James M Snell <[email protected]>

lib/_http_common.js

@@ -80,17 +80,11 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
     parser.incoming.statusMessage = statusMessage;
   }
 
-  // The client made non-upgrade request, and server is just advertising
-  // supported protocols.
-  //
-  // See RFC7230 Section 6.7
-  //
-  // NOTE: RegExp below matches `upgrade` in `Connection: abc, upgrade, def`
-  // header.
-  if (upgrade &&
-      parser.outgoing !== null &&
-      (parser.outgoing._headers.upgrade === undefined ||
-       !/(^|\W)upgrade(\W|$)/i.test(parser.outgoing._headers.connection))) {
+  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;
   }
 

lib/_http_outgoing.js

@@ -9,16 +9,18 @@ const Buffer = require('buffer').Buffer;
 const common = require('_http_common');
 
 const CRLF = common.CRLF;
-const chunkExpression = common.chunkExpression;
+const trfrEncChunkExpression = common.chunkExpression;
 const debug = common.debug;
 
-const connectionExpression = /^Connection$/i;
+const upgradeExpression = /^Upgrade$/i;
 const transferEncodingExpression = /^Transfer-Encoding$/i;
-const closeExpression = /close/i;
 const contentLengthExpression = /^Content-Length$/i;
 const dateExpression = /^Date$/i;
 const expectExpression = /^Expect$/i;
 const trailerExpression = /^Trailer$/i;
+const connectionExpression = /^Connection$/i;
+const connCloseExpression = /(^|\W)close(\W|$)/i;
+const connUpgradeExpression = /(^|\W)upgrade(\W|$)/i;
 
 const automaticHeaders = {
   connection: true,
@@ -61,6 +63,7 @@ function OutgoingMessage() {
   this.writable = true;
 
   this._last = false;
+  this.upgrading = false;
   this.chunkedEncoding = false;
   this.shouldKeepAlive = true;
   this.useChunkedEncodingByDefault = true;
@@ -192,11 +195,13 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) {
   // in the case of response it is: 'HTTP/1.1 200 OK\r\n'
   var state = {
     sentConnectionHeader: false,
+    sentConnectionUpgrade: false,
     sentContentLengthHeader: false,
     sentTransferEncodingHeader: false,
     sentDateHeader: false,
     sentExpect: false,
     sentTrailer: false,
+    sentUpgrade: false,
     messageHeader: firstLine
   };
 
@@ -225,6 +230,10 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) {
     }
   }
 
+  // Are we upgrading the connection?
+  if (state.sentConnectionUpgrade && state.sentUpgrade)
+    this.upgrading = true;
+
   // Date header
   if (this.sendDate === true && state.sentDateHeader === false) {
     state.messageHeader += 'Date: ' + utcDate() + CRLF;
@@ -312,15 +321,16 @@ function storeHeader(self, state, field, value) {
 
   if (connectionExpression.test(field)) {
     state.sentConnectionHeader = true;
-    if (closeExpression.test(value)) {
+    if (connCloseExpression.test(value)) {
       self._last = true;
     } else {
       self.shouldKeepAlive = true;
     }
-
+    if (connUpgradeExpression.test(value))
+      state.sentConnectionUpgrade = true;
   } else if (transferEncodingExpression.test(field)) {
     state.sentTransferEncodingHeader = true;
-    if (chunkExpression.test(value)) self.chunkedEncoding = true;
+    if (trfrEncChunkExpression.test(value)) self.chunkedEncoding = true;
 
   } else if (contentLengthExpression.test(field)) {
     state.sentContentLengthHeader = true;
@@ -330,6 +340,8 @@ function storeHeader(self, state, field, value) {
     state.sentExpect = true;
   } else if (trailerExpression.test(field)) {
     state.sentTrailer = true;
+  } else if (upgradeExpression.test(field)) {
+    state.sentUpgrade = true;
   }
 }
 

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

@@ -26,31 +26,51 @@ var srv = net.createServer(function(c) {
 });
 
 srv.listen(0, '127.0.0.1', common.mustCall(function() {
-
-  var req = http.get({
-    port: this.address().port,
-    headers: {
+  var port = this.address().port;
+  var headers = [
+    {
       connection: 'upgrade',
       upgrade: 'websocket'
-    }
-  });
-  req.on('upgrade', common.mustCall(function(res, socket, upgradeHead) {
-    var recvData = upgradeHead;
-    socket.on('data', function(d) {
-      recvData += d;
+    },
+    [
+      ['Host', 'echo.websocket.org'],
+      ['Connection', 'Upgrade'],
+      ['Upgrade', 'websocket'],
+      ['Origin', 'http://www.websocket.org']
+    ]
+  ];
+  var left = headers.length;
+  headers.forEach(function(h) {
+    var req = http.get({
+      port: port,
+      headers: h
     });
+    var sawUpgrade = false;
+    req.on('upgrade', common.mustCall(function(res, socket, upgradeHead) {
+      sawUpgrade = true;
+      var recvData = upgradeHead;
+      socket.on('data', function(d) {
+        recvData += d;
+      });
 
-    socket.on('close', common.mustCall(function() {
-      assert.equal(recvData, 'nurtzo');
-    }));
+      socket.on('close', common.mustCall(function() {
+        assert.equal(recvData, 'nurtzo');
+      }));
 
-    console.log(res.headers);
-    var expectedHeaders = {'hello': 'world',
-                            'connection': 'upgrade',
-                            'upgrade': 'websocket' };
-    assert.deepStrictEqual(expectedHeaders, res.headers);
+      console.log(res.headers);
+      var expectedHeaders = {
+        hello: 'world',
+        connection: 'upgrade',
+        upgrade: 'websocket'
+      };
+      assert.deepStrictEqual(expectedHeaders, res.headers);
 
-    socket.end();
-    srv.close();
-  }));
+      socket.end();
+      if (--left == 0)
+        srv.close();
+    }));
+    req.on('close', common.mustCall(function() {
+      assert.strictEqual(sawUpgrade, true);
+    }));
+  });
 }));