http: fix parsing of binary upgrade response body

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: https://github.com/nodejs/node/issues/17789

PR-URL: https://github.com/nodejs/node/pull/17806
Fixes: https://github.com/nodejs/node/issues/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>

1 files changed, 9 insertions, 15 deletions

diff --git a/lib/_http_client.js b/lib/_http_client.js
index e2aaf86a5e..b49aa711c4 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -458,7 +458,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"');
@@ -471,29 +470,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) {
@@ -503,7 +495,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
     req.shouldKeepAlive = false;
   }
 
-
   DTRACE_HTTP_CLIENT_RESPONSE(socket, req);
   LTTNG_HTTP_CLIENT_RESPONSE(socket, req);
   COUNTER_HTTP_CLIENT_RESPONSE();
@@ -521,7 +512,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