From 01fc2e910755da1b489d7eb46e1b8d3ced4e1921 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 17 Dec 2015 17:23:46 -0500 Subject: [PATCH 1/3] http: do not emit `upgrade` on advertisement Do not emit `upgrade` if the server is just advertising its protocols support as per RFC 7230 Section 6.7. A server MAY send an Upgrade header field in any other response to advertise that it implements support for upgrading to the listed protocols, in order of descending preference, when appropriate for a future request. Fix: https://github.com/nodejs/node/issues/4334 --- lib/_http_client.js | 1 + lib/_http_common.js | 16 ++++++ test/parallel/test-http-upgrade-advertise.js | 60 ++++++++++++++++++++ test/parallel/test-http-upgrade-agent.js | 1 + test/parallel/test-http-upgrade-client.js | 8 ++- 5 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http-upgrade-advertise.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 912fbd4f396c4c..62933d7e70cc74 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -466,6 +466,7 @@ function tickOnSocket(req, socket) { parser.reinitialize(HTTPParser.RESPONSE); parser.socket = socket; parser.incoming = null; + parser.outgoing = req; req.parser = parser; socket.parser = parser; diff --git a/lib/_http_common.js b/lib/_http_common.js index 5140d366661cb4..c518d95eb51a71 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -77,6 +77,17 @@ 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 + if (upgrade && + parser.outgoing !== null && + (parser.outgoing._headers.upgrade === undefined || + !/(^|\W)upgrade(\W|$)/i.test(parser.outgoing._headers.connection))) { + upgrade = false; + } + parser.incoming.upgrade = upgrade; var skipBody = false; // response to HEAD or CONNECT @@ -142,6 +153,10 @@ var parsers = new FreeList('parsers', 1000, function() { parser._url = ''; parser._consumed = false; + parser.socket = null; + parser.incoming = null; + parser.outgoing = null; + // Only called in the slow case where slow means // that the request headers were either fragmented // across multiple TCP packets or too large to be @@ -175,6 +190,7 @@ function freeParser(parser, req, socket) { parser.socket.parser = null; parser.socket = null; parser.incoming = null; + parser.outgoing = null; if (parsers.free(parser) === false) parser.close(); parser = null; diff --git a/test/parallel/test-http-upgrade-advertise.js b/test/parallel/test-http-upgrade-advertise.js new file mode 100644 index 00000000000000..ee0d83baad1ea5 --- /dev/null +++ b/test/parallel/test-http-upgrade-advertise.js @@ -0,0 +1,60 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const tests = [ + { headers: {}, expected: 'regular' }, + { headers: { upgrade: 'h2c' }, expected: 'regular' }, + { headers: { connection: 'upgrade' }, expected: 'regular' }, + { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' } +]; + +function fire() { + if (tests.length === 0) + return server.close(); + + const test = tests.shift(); + + var once = false; + + const done = common.mustCall(function done(result) { + assert(!once); + once = true; + assert.equal(result, test.expected); + + fire(); + }); + + const req = http.request({ + port: common.PORT, + path: '/', + headers: test.headers + }, function onResponse(res) { + res.resume(); + done('regular'); + }); + + req.on('upgrade', function onUpgrade(res, socket) { + socket.destroy(); + done('upgrade'); + }); + + req.end(); +} + +const server = http.createServer(function(req, res) { + res.writeHead(200, { + Connection: 'upgrade, keep-alive', + Upgrade: 'h2c' + }); + res.end('hello world'); +}).on('upgrade', function(req, socket) { + socket.end('HTTP/1.1 101 Switching protocols\r\n' + + 'Connection: upgrade\r\n' + + 'Upgrade: h2c\r\n\r\n' + + 'ohai'); +}).listen(common.PORT, function() { + fire(); +}); diff --git a/test/parallel/test-http-upgrade-agent.js b/test/parallel/test-http-upgrade-agent.js index 84cfee90d528de..91fe495da2f49e 100644 --- a/test/parallel/test-http-upgrade-agent.js +++ b/test/parallel/test-http-upgrade-agent.js @@ -36,6 +36,7 @@ srv.listen(common.PORT, '127.0.0.1', function() { port: common.PORT, host: '127.0.0.1', headers: { + 'connection': 'upgrade', 'upgrade': 'websocket' } }; diff --git a/test/parallel/test-http-upgrade-client.js b/test/parallel/test-http-upgrade-client.js index b8ba033f2e01cb..dc4dd47cf202c5 100644 --- a/test/parallel/test-http-upgrade-client.js +++ b/test/parallel/test-http-upgrade-client.js @@ -32,7 +32,13 @@ var gotUpgrade = false; srv.listen(common.PORT, '127.0.0.1', function() { - var req = http.get({ port: common.PORT }); + var req = http.get({ + port: common.PORT, + headers: { + connection: 'upgrade', + upgrade: 'websocket' + } + }); req.on('upgrade', function(res, socket, upgradeHead) { // XXX: This test isn't fantastic, as it assumes that the entire response // from the server will arrive in a single data callback From f64bf3282115163def1b7a2a495a3e43ff23204b Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 17 Dec 2015 19:15:05 -0500 Subject: [PATCH 2/3] add comment --- lib/_http_common.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/_http_common.js b/lib/_http_common.js index c518d95eb51a71..7e23ae98e90175 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -81,6 +81,9 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, // 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 || From a48ed3252490a31498be4c9a316b204cbbc339ea Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 17 Dec 2015 19:23:25 -0500 Subject: [PATCH 3/3] test --- test/parallel/test-http-upgrade-advertise.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/parallel/test-http-upgrade-advertise.js b/test/parallel/test-http-upgrade-advertise.js index ee0d83baad1ea5..fbc183f84b2ada 100644 --- a/test/parallel/test-http-upgrade-advertise.js +++ b/test/parallel/test-http-upgrade-advertise.js @@ -17,11 +17,7 @@ function fire() { const test = tests.shift(); - var once = false; - const done = common.mustCall(function done(result) { - assert(!once); - once = true; assert.equal(result, test.expected); fire(); @@ -55,6 +51,4 @@ const server = http.createServer(function(req, res) { 'Connection: upgrade\r\n' + 'Upgrade: h2c\r\n\r\n' + 'ohai'); -}).listen(common.PORT, function() { - fire(); -}); +}).listen(common.PORT, fire);