From adb1a963226fae3c3dbea3e1fb8067c118fadabc Mon Sep 17 00:00:00 2001 From: Josh Leder Date: Thu, 14 Apr 2016 09:30:22 -0600 Subject: [PATCH 1/3] 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. --- lib/_http_client.js | 2 + src/node_http_parser.cc | 27 ++++++++ test/parallel/test-http-connect-head.js | 90 +++++++++++++++++++++++++ 3 files changed, 119 insertions(+) create mode 100644 test/parallel/test-http-connect-head.js diff --git a/lib/_http_client.js b/lib/_http_client.js index bc294263c7f881..1427d78714433a 100644 --- 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 index 4087ed263fb1a9..570820a07c8bfe 100644 --- 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 property, + const PropertyCallbackInfo& args) { + Parser* parser = Unwrap(args.Holder()); + + Local ret = Boolean::New( + parser->env()->isolate(), + parser->parser_.upgrade); + + args.GetReturnValue().Set(ret); + } + + static void SetUpgrade(Local property, + Local value, + const PropertyCallbackInfo& args) { + Parser* parser = Unwrap(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 target, void* priv) { Environment* env = Environment::GetCurrent(context); Local 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 target, env->SetProtoMethod(t, "unconsume", Parser::Unconsume); env->SetProtoMethod(t, "getCurrentBuffer", Parser::GetCurrentBuffer); + Local 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 new file mode 100644 index 00000000000000..dc66e1c9eeafd5 --- /dev/null +++ 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); +}); From 3c295e503a596f487763a1271eefaf7f3f2fd345 Mon Sep 17 00:00:00 2001 From: Josh Leder Date: Fri, 15 Apr 2016 12:50:29 -0600 Subject: [PATCH 2/3] clean up tests to comply with some best practices --- test/parallel/test-http-connect-head.js | 54 +++++++++---------------- 1 file changed, 18 insertions(+), 36 deletions(-) diff --git a/test/parallel/test-http-connect-head.js b/test/parallel/test-http-connect-head.js index dc66e1c9eeafd5..60d0cc1c857d13 100644 --- a/test/parallel/test-http-connect-head.js +++ b/test/parallel/test-http-connect-head.js @@ -1,23 +1,24 @@ 'use strict'; -var common = require('../common'); -var assert = require('assert'); -var http = require('http'); - -var serverGotConnect = false; -var clientGotConnect = false; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); var server = http.createServer(function(req, res) { assert(false); }); -server.on('connect', function(req, socket, firstBodyChunk) { +server.on('connect', common.mustCall(function(req, socket, firstBodyChunk) { assert.equal(req.method, 'CONNECT'); - assert.equal(req.url, 'google.com:443'); + assert.equal(req.url, 'example.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'); + socket.write( + 'HTTP/1.1 200 Connection established\r\n' + + 'Date: Tue, 15 Nov 1994 08:12:31 GMT\r\n' + + '\r\n' + + 'Head' + ); var data = firstBodyChunk.toString(); socket.on('data', function(buf) { @@ -26,24 +27,20 @@ server.on('connect', function(req, socket, firstBodyChunk) { socket.on('end', function() { socket.end(data); }); -}); -server.listen(common.PORT, function() { +})); +server.listen(common.PORT, common.mustCall(function() { var req = http.request({ port: common.PORT, method: 'CONNECT', - path: 'google.com:443' + path: 'example.com:443' }, function(res) { assert(false); }); - var clientRequestClosed = false; - req.on('close', function() { - clientRequestClosed = true; - }); + req.on('close', common.mustCall(function() { })); - req.on('connect', function(res, socket, firstBodyChunk) { + req.on('connect', common.mustCall(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; @@ -56,15 +53,6 @@ server.listen(common.PORT, function() { 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 @@ -75,16 +63,10 @@ server.listen(common.PORT, function() { }); 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); -}); +})); From 08284b383f03079611498e5cee2475e57e12345c Mon Sep 17 00:00:00 2001 From: Josh Leder Date: Fri, 15 Apr 2016 12:50:41 -0600 Subject: [PATCH 3/3] remove return value from .upgrade setter --- src/node_http_parser.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 570820a07c8bfe..06d8a5395dfc5f 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -537,8 +537,6 @@ class Parser : public AsyncWrap { bool upgrade = value->BooleanValue(); parser->parser_.upgrade = upgrade; - - args.GetReturnValue().Set(value->ToBoolean()); } protected: