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]>
nodejs · Nov 22, 2016 · 50a4471 · 50a4471
1 parent 9389572
commit 50a4471
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 47 deletions.
diff --git a/lib/_http_common.js b/lib/_http_common.js
@@ -78,17 +78,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;
   }
 

diff --git a/lib/_http_outgoing.js b/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 lenientHttpHeaders = !!process.REVERT_CVE_2016_2216;
 
 const automaticHeaders = {
@@ -62,6 +64,7 @@ function OutgoingMessage() {
   this.writable = true;
 
   this._last = false;
+  this.upgrading = false;
   this.chunkedEncoding = false;
   this.shouldKeepAlive = true;
   this.useChunkedEncodingByDefault = true;
@@ -191,11 +194,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
   };
 
@@ -224,6 +229,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;
@@ -313,15 +322,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;
@@ -331,6 +341,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;
   }
 }
 

diff --git a/test/parallel/test-http-upgrade-client.js b/test/parallel/test-http-upgrade-client.js
@@ -25,40 +25,52 @@ var srv = net.createServer(function(c) {
   });
 });
 
-var gotUpgrade = false;
-
-srv.listen(0, '127.0.0.1', function() {
-
-  var req = http.get({
-    port: this.address().port,
-    headers: {
+srv.listen(0, '127.0.0.1', common.mustCall(function() {
+  var port = this.address().port;
+  var headers = [
+    {
       connection: 'upgrade',
       upgrade: 'websocket'
-    }
-  });
-  req.on('upgrade', 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');
-    }));
-
-    console.log(res.headers);
-    var expectedHeaders = {'hello': 'world',
-                            'connection': 'upgrade',
-                            'upgrade': 'websocket' };
-    assert.deepEqual(expectedHeaders, res.headers);
+      socket.on('close', common.mustCall(function() {
+        assert.equal(recvData, 'nurtzo');
+      }));
 
-    socket.end();
-    srv.close();
+      console.log(res.headers);
+      var expectedHeaders = {
+        hello: 'world',
+        connection: 'upgrade',
+        upgrade: 'websocket'
+      };
+      assert.deepStrictEqual(expectedHeaders, res.headers);
 
-    gotUpgrade = true;
+      socket.end();
+      if (--left == 0)
+        srv.close();
+    }));
+    req.on('close', common.mustCall(function() {
+      assert.strictEqual(sawUpgrade, true);
+    }));
   });
-});
-
-process.on('exit', function() {
-  assert.ok(gotUpgrade);
-});
+}));