http: fix parsing of binary upgrade response body

bnoordhuis · evanlucas · commit 3b9ef2142cb6 · 2018-01-30T11:31:53.000-06:00
Fix a bug where a connection upgrade response with a Transfer-Encoding header and a body whose first byte is > 127 causes said byte to be dropped on the floor when passing the remainder of the message to the 'upgrade' event listeners. Fixes: #17789 PR-URL: #17806 Fixes: #17789 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -490,7 +490,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
   var socket = this.socket;
   var req = socket._httpMessage;
 
-
   // propagate "domain" setting...
   if (req.domain && !res.domain) {
     debug('setting "res.domain"');
@@ -503,29 +502,22 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
     // We already have a response object, this means the server
     // sent a double response.
     socket.destroy();
-    return;
+    return 0;  // No special treatment.
   }
   req.res = res;
 
   // Responses to CONNECT request is handled as Upgrade.
-  if (req.method === 'CONNECT') {
+  const method = req.method;
+  if (method === 'CONNECT') {
     res.upgrade = true;
-    return 2; // skip body, and the rest
+    return 2;  // Skip body and treat as Upgrade.
   }
 
-  // Responses to HEAD requests are crazy.
-  // HEAD responses aren't allowed to have an entity-body
-  // but *can* have a content-length which actually corresponds
-  // to the content-length of the entity-body had the request
-  // been a GET.
-  var isHeadResponse = req.method === 'HEAD';
-  debug('AGENT isHeadResponse', isHeadResponse);
-
   if (res.statusCode === 100) {
     // restart the parser, as this is a continue message.
     req.res = null; // Clear res so that we don't hit double-responses.
     req.emit('continue');
-    return true;
+    return 1;  // Skip body but don't treat as Upgrade.
   }
 
   if (req.shouldKeepAlive && !shouldKeepAlive && !req.upgradeOrConnect) {
@@ -535,7 +527,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
     req.shouldKeepAlive = false;
   }
 
-
   DTRACE_HTTP_CLIENT_RESPONSE(socket, req);
   LTTNG_HTTP_CLIENT_RESPONSE(socket, req);
   COUNTER_HTTP_CLIENT_RESPONSE();
@@ -553,7 +544,10 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
   if (!handled)
     res._dump();
 
-  return isHeadResponse;
+  if (method === 'HEAD')
+    return 1;  // Skip body but don't treat as Upgrade.
+
+  return 0;  // No special treatment.
 }
 
 // client
diff --git a/lib/_http_common.js b/lib/_http_common.js
@@ -106,19 +106,10 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
 
   parser.incoming.upgrade = upgrade;
 
-  var skipBody = 0; // response to HEAD or CONNECT
+  if (upgrade)
+    return 2;  // Skip body and treat as Upgrade.
 
-  if (!upgrade) {
-    // For upgraded connections and CONNECT method request, we'll emit this
-    // after parser.execute so that we can capture the first part of the new
-    // protocol.
-    skipBody = parser.onIncoming(parser.incoming, shouldKeepAlive);
-  }
-
-  if (typeof skipBody !== 'number')
-    return skipBody ? 1 : 0;
-  else
-    return skipBody;
+  return parser.onIncoming(parser.incoming, shouldKeepAlive);
 }
 
 // XXX This is a mess.
diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -617,7 +617,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
   } else {
     server.emit('request', req, res);
   }
-  return false; // Not a HEAD response. (Not even a response!)
+  return 0;  // No special treatment.
 }
 
 function resetSocketTimeout(server, socket, state) {
diff --git a/test/parallel/test-http-upgrade-binary.js b/test/parallel/test-http-upgrade-binary.js
@@ -0,0 +1,28 @@
+'use strict';
+const { mustCall } = require('../common');
+const assert = require('assert');
+const http = require('http');
+const net = require('net');
+
+// https://github.com/nodejs/node/issues/17789 - a connection upgrade response
+// that has a Transfer-Encoding header and a body whose first byte is > 127
+// triggers a bug where said byte is skipped over.
+net.createServer(mustCall(function(conn) {
+  conn.write('HTTP/1.1 101 Switching Protocols\r\n' +
+             'Connection: upgrade\r\n' +
+             'Transfer-Encoding: chunked\r\n' +
+             'Upgrade: websocket\r\n' +
+             '\r\n' +
+             '\u0080', 'latin1');
+  this.close();
+})).listen(0, mustCall(function() {
+  http.get({
+    host: this.address().host,
+    port: this.address().port,
+    headers: { 'Connection': 'upgrade', 'Upgrade': 'websocket' },
+  }).on('upgrade', mustCall((res, conn, head) => {
+    assert.strictEqual(head.length, 1);
+    assert.strictEqual(head[0], 128);
+    conn.destroy();
+  }));
+}));

Original file line number	Diff line number	Diff line change
`@@ -617,7 +617,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {`
`617`	`617`	`} else {`
`618`	`618`	`server.emit('request', req, res);`
`619`	`619`	`}`
`620`		`- return false; // Not a HEAD response. (Not even a response!)`
	`620`	`+ return 0; // No special treatment.`
`621`	`621`	`}`
`622`	`622`
`623`	`623`	`function resetSocketTimeout(server, socket, state) {`