From 33ce9a64092b30561bf1941bd2c0243671f75e51 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 12 Apr 2018 19:57:19 +0200 Subject: [PATCH] http: relax requirements on upgrade listener The http spec does not say anything about Upgrade headers making protocol switch mandatory but Node.js implements them as if they are. Relax the requirements to only destroy the socket if no upgrade listener exists on the client when status code is 101. PR-URL: https://github.com/nodejs/node/pull/19981 Fixes: https://github.com/nodejs/node/issues/11552 Refs: https://tools.ietf.org/html/rfc7230#section-6.7 Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Ruben Bridgewater Reviewed-By: Trivikram Kamat --- doc/api/http.md | 15 ++++++++++----- lib/_http_client.js | 8 ++++++-- lib/_http_common.js | 14 +------------- lib/_http_outgoing.js | 8 -------- lib/_http_server.js | 11 +++++++++-- test/parallel/test-http-upgrade-advertise.js | 19 ++++++++++++++----- test/parallel/test-http-upgrade-server.js | 8 +++----- 7 files changed, 43 insertions(+), 40 deletions(-) diff --git a/doc/api/http.md b/doc/api/http.md index e7d4d3b4ac9604..700dce3b94f660 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -471,8 +471,9 @@ added: v0.1.94 * `head` {Buffer} Emitted each time a server responds to a request with an upgrade. If this -event is not being listened for, clients receiving an upgrade header will have -their connections closed. +event is not being listened for and the response status code is 101 Switching +Protocols, clients receiving an upgrade header will have their connections +closed. A client server pair demonstrating how to listen for the `'upgrade'` event. @@ -886,6 +887,11 @@ per connection (in the case of HTTP Keep-Alive connections). ### Event: 'upgrade' * `request` {http.IncomingMessage} Arguments for the HTTP request, as it is in @@ -893,9 +899,8 @@ added: v0.1.94 * `socket` {net.Socket} Network socket between the server and client * `head` {Buffer} The first packet of the upgraded stream (may be empty) -Emitted each time a client requests an HTTP upgrade. If this event is not -listened for, then clients requesting an upgrade will have their connections -closed. +Emitted each time a client requests an HTTP upgrade. Listening to this event +is optional and clients cannot insist on a protocol change. After this event is emitted, the request's socket will not have a `'data'` event listener, meaning it will need to be bound in order to handle data diff --git a/lib/_http_client.js b/lib/_http_client.js index 5be1632fe59433..c3ef9de20498df 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -426,7 +426,7 @@ function socketOnData(d) { req.socket._hadError = true; req.emit('error', ret); } else if (parser.incoming && parser.incoming.upgrade) { - // Upgrade or CONNECT + // Upgrade (if status code 101) or CONNECT var bytesParsed = ret; var res = parser.incoming; req.res = res; @@ -453,7 +453,7 @@ function socketOnData(d) { req.emit(eventName, res, socket, bodyHead); req.emit('close'); } else { - // Got Upgrade header or CONNECT method, but have no handler. + // Requested Upgrade or used CONNECT method, but have no handler. socket.destroy(); } } else if (parser.incoming && parser.incoming.complete && @@ -492,6 +492,10 @@ function parserOnIncomingClient(res, shouldKeepAlive) { } req.res = res; + // Skip body and treat as Upgrade. + if (res.upgrade) + return 2; + // Responses to CONNECT request is handled as Upgrade. const method = req.method; if (method === 'CONNECT') { diff --git a/lib/_http_common.js b/lib/_http_common.js index b101c11911fa1e..bc65443eb3c845 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -83,6 +83,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, parser.incoming.httpVersionMinor = versionMinor; parser.incoming.httpVersion = `${versionMajor}.${versionMinor}`; parser.incoming.url = url; + parser.incoming.upgrade = upgrade; var n = headers.length; @@ -101,19 +102,6 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, parser.incoming.statusMessage = statusMessage; } - if (upgrade && parser.outgoing !== null && !parser.outgoing.upgrading) { - // The client made non-upgrade request, and server is just advertising - // supported protocols. - // - // See RFC7230 Section 6.7 - upgrade = false; - } - - parser.incoming.upgrade = upgrade; - - if (upgrade) - return 2; // Skip body and treat as Upgrade. - return parser.onIncoming(parser.incoming, shouldKeepAlive); } diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index adc4481347f71a..b62dea4a24dd2f 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -98,7 +98,6 @@ function OutgoingMessage() { this.writable = true; this._last = false; - this.upgrading = false; this.chunkedEncoding = false; this.shouldKeepAlive = true; this.useChunkedEncodingByDefault = true; @@ -304,7 +303,6 @@ function _storeHeader(firstLine, headers) { // in the case of response it is: 'HTTP/1.1 200 OK\r\n' var state = { connection: false, - connUpgrade: false, contLen: false, te: false, date: false, @@ -366,10 +364,6 @@ function _storeHeader(firstLine, headers) { } } - // Are we upgrading the connection? - if (state.connUpgrade && state.upgrade) - this.upgrading = true; - // Date header if (this.sendDate && !state.date) { state.header += 'Date: ' + utcDate() + CRLF; @@ -460,8 +454,6 @@ function matchConnValue(self, state, value) { while (m) { if (m[0].length === 5) sawClose = true; - else - state.connUpgrade = true; m = RE_CONN_VALUES.exec(value); } if (sawClose) diff --git a/lib/_http_server.js b/lib/_http_server.js index cba70bd896058c..a8389dbda62f0d 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -530,14 +530,14 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) { parser = null; var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade'; - if (server.listenerCount(eventName) > 0) { + if (eventName === 'upgrade' || server.listenerCount(eventName) > 0) { debug('SERVER have listener for %s', eventName); var bodyHead = d.slice(bytesParsed, d.length); socket.readableFlowing = null; server.emit(eventName, req, socket, bodyHead); } else { - // Got upgrade header or CONNECT method, but have no handler. + // Got CONNECT method, but have no handler. socket.destroy(); } } @@ -592,6 +592,13 @@ function resOnFinish(req, res, socket, state, server) { function parserOnIncoming(server, socket, state, req, keepAlive) { resetSocketTimeout(server, socket, state); + if (req.upgrade) { + req.upgrade = req.method === 'CONNECT' || + server.listenerCount('upgrade') > 0; + if (req.upgrade) + return 2; + } + state.incoming.push(req); // If the writable end isn't consuming, then stop reading diff --git a/test/parallel/test-http-upgrade-advertise.js b/test/parallel/test-http-upgrade-advertise.js index 99a3e8fd353015..6d033ba36a5d60 100644 --- a/test/parallel/test-http-upgrade-advertise.js +++ b/test/parallel/test-http-upgrade-advertise.js @@ -8,7 +8,9 @@ const tests = [ { headers: {}, expected: 'regular' }, { headers: { upgrade: 'h2c' }, expected: 'regular' }, { headers: { connection: 'upgrade' }, expected: 'regular' }, - { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' } + { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' }, + { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'destroy' }, + { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'regular' }, ]; function fire() { @@ -32,10 +34,17 @@ function fire() { done('regular'); }); - req.on('upgrade', function onUpgrade(res, socket) { - socket.destroy(); - done('upgrade'); - }); + if (test.expected === 'destroy') { + req.on('socket', () => req.socket.on('close', () => { + server.removeAllListeners('upgrade'); + done('destroy'); + })); + } else { + req.on('upgrade', function onUpgrade(res, socket) { + socket.destroy(); + done('upgrade'); + }); + } req.end(); } diff --git a/test/parallel/test-http-upgrade-server.js b/test/parallel/test-http-upgrade-server.js index 71a075831603da..d8a193c6edb4ec 100644 --- a/test/parallel/test-http-upgrade-server.js +++ b/test/parallel/test-http-upgrade-server.js @@ -121,8 +121,6 @@ function test_upgrade_with_listener() { /*----------------------------------------------- connection: Upgrade, no listener -----------------------------------------------*/ -let test_upgrade_no_listener_ended = false; - function test_upgrade_no_listener() { const conn = net.createConnection(server.address().port); conn.setEncoding('utf8'); @@ -135,8 +133,9 @@ function test_upgrade_no_listener() { '\r\n'); }); - conn.on('end', function() { - test_upgrade_no_listener_ended = true; + conn.once('data', (data) => { + assert.strictEqual(typeof data, 'string'); + assert.strictEqual(data.substr(0, 12), 'HTTP/1.1 200'); conn.end(); }); @@ -182,5 +181,4 @@ server.listen(0, function() { process.on('exit', function() { assert.strictEqual(3, requests_recv); assert.strictEqual(3, requests_sent); - assert.ok(test_upgrade_no_listener_ended); });