http: tell the parser about CONNECT responses

This commit fixes a bug in HTTP CONNECT response parsing. The parser normally continues to look for additional HTTP messages after the first message has completed. However, in the case of CONNECT responses, the parser should stop looking for additional messages and treat any further data as a separate protocol. Because of the way that HTTP messages are parsed in JavaScript, this bug only manifests itself in the case where the socket's `data' handler receives the end of the response message *and also* includes non-HTTP data. In order to implement the fix, an `.upgrade' accessor is exposed to JavaScript on the HTTPParser object that proxies the underlying http_parser's `upgrade' field. Likewise in JavaScript, the `http' client module sets this value to `true' when a response is received to a CONNECT request. The result of this is that callbacks on HTTPParser instances can signal that the message indicates a change in protocol and further HTTP parsing should not occur.
nodejs · slushie · Apr 14, 2016 · Apr 15, 2016 · Apr 15, 2016 · Apr 15, 2016
commit adb1a963226fae3c3dbea3e1fb8067c118fadabc
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -410,6 +410,7 @@ function socketOnData(d) {
 // client
 function parserOnIncomingClient(res, shouldKeepAlive) {
   var socket = this.socket;
+  var parser = socket.parser;
   var req = socket._httpMessage;
 
 
@@ -432,6 +433,7 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
   // Responses to CONNECT request is handled as Upgrade.
   if (req.method === 'CONNECT') {
     res.upgrade = true;
+    parser.upgrade = true;
     return true; // skip body
   }
 

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
@@ -519,6 +519,28 @@ class Parser : public AsyncWrap {
     args.GetReturnValue().Set(ret);
   }
 
+  static void GetUpgrade(Local<String> property,
+                         const PropertyCallbackInfo<Value>& args) {
+    Parser* parser = Unwrap<Parser>(args.Holder());
+
+    Local<Value> ret = Boolean::New(
+        parser->env()->isolate(),
+        parser->parser_.upgrade);
+
+    args.GetReturnValue().Set(ret);
+  }
+
+  static void SetUpgrade(Local<String> property,
+                         Local<Value> value,
+                         const PropertyCallbackInfo<void>& args) {
+    Parser* parser = Unwrap<Parser>(args.Holder());
+
+    bool upgrade = value->BooleanValue();
+    parser->parser_.upgrade = upgrade;
+
+    args.GetReturnValue().Set(value->ToBoolean());
+  }
+
  protected:
   class ScopedRetainParser {
    public:
@@ -736,6 +758,7 @@ void InitHttpParser(Local<Object> target,
                     void* priv) {
   Environment* env = Environment::GetCurrent(context);
   Local<FunctionTemplate> t = env->NewFunctionTemplate(Parser::New);
+
   t->InstanceTemplate()->SetInternalFieldCount(1);
   t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "HTTPParser"));
 
@@ -771,6 +794,10 @@ void InitHttpParser(Local<Object> target,
   env->SetProtoMethod(t, "unconsume", Parser::Unconsume);
   env->SetProtoMethod(t, "getCurrentBuffer", Parser::GetCurrentBuffer);
 
+  Local<v8::ObjectTemplate> o = t->InstanceTemplate();
+  o->SetAccessor(FIXED_ONE_BYTE_STRING(env->isolate(), "upgrade"),
+                 Parser::GetUpgrade, Parser::SetUpgrade);
+
   target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "HTTPParser"),
               t->GetFunction());
 }

diff --git a/test/parallel/test-http-connect-head.js b/test/parallel/test-http-connect-head.js
@@ -0,0 +1,90 @@
+'use strict';
+var common = require('../common');
+var assert = require('assert');
+var http = require('http');
+
+var serverGotConnect = false;
+var clientGotConnect = false;
+
+var server = http.createServer(function(req, res) {
+  assert(false);
+});
+server.on('connect', function(req, socket, firstBodyChunk) {
+  assert.equal(req.method, 'CONNECT');
+  assert.equal(req.url, 'google.com:443');
+  console.error('Server got CONNECT request');
+  serverGotConnect = true;
+
+  // It is legal for the server to send some data intended for the client
+  // along with the CONNECT response
+  socket.write('HTTP/1.1 200 Connection established\r\n\r\nHead');
+
+  var data = firstBodyChunk.toString();
+  socket.on('data', function(buf) {
+    data += buf.toString();
+  });
+  socket.on('end', function() {
+    socket.end(data);
+  });
+});
+server.listen(common.PORT, function() {
+  var req = http.request({
+    port: common.PORT,
+    method: 'CONNECT',
+    path: 'google.com:443'
+  }, function(res) {
+    assert(false);
+  });
+
+  var clientRequestClosed = false;
+  req.on('close', function() {
+    clientRequestClosed = true;
+  });
+
+  req.on('connect', function(res, socket, firstBodyChunk) {
+    console.error('Client got CONNECT request');
+    clientGotConnect = true;
+
+    // Make sure this request got removed from the pool.
+    var name = 'localhost:' + common.PORT;
+    assert(!http.globalAgent.sockets.hasOwnProperty(name));
+    assert(!http.globalAgent.requests.hasOwnProperty(name));
+
+    // Make sure this socket has detached.
+    assert(!socket.ondata);
+    assert(!socket.onend);
+    assert.equal(socket.listeners('connect').length, 0);
+    assert.equal(socket.listeners('data').length, 0);
+
+    // the stream.Duplex onend listener
+    // allow 0 here, so that i can run the same test on streams1 impl
+    assert(socket.listeners('end').length <= 1);
+
+    assert.equal(socket.listeners('free').length, 0);
+    assert.equal(socket.listeners('close').length, 0);
+    assert.equal(socket.listeners('error').length, 0);
+    assert.equal(socket.listeners('agentRemove').length, 0);
+
+    var data = firstBodyChunk.toString();
+
+    // test that the firstBodyChunk was not parsed as HTTP
+    assert.equal(data, 'Head');
+
+    socket.on('data', function(buf) {
+      data += buf.toString();
+    });
+    socket.on('end', function() {
+      assert.equal(data, 'HeadRequestEnd');
+      assert(clientRequestClosed);
+      server.close();
+    });
+    socket.end('End');
+  });
+
+  req.end('Request');
+});
+
+process.on('exit', function() {
+  assert.ok(serverGotConnect);
+  assert.ok(clientGotConnect);
+});