Skip to content

Commit

Permalink
http: relax requirements on upgrade listener
Browse files Browse the repository at this point in the history
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: #19981
Fixes: #11552
Refs: https://tools.ietf.org/html/rfc7230#section-6.7
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
  • Loading branch information
apapirovski authored and jasnell committed Apr 16, 2018
1 parent 6d1c3e5 commit 33ce9a6
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 40 deletions.
15 changes: 10 additions & 5 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -886,16 +887,20 @@ per connection (in the case of HTTP Keep-Alive connections).
### Event: 'upgrade'
<!-- YAML
added: v0.1.94
changes:
- version: REPLACEME
pr-url: REPLACEME
description: Not listening to this event no longer causes the socket
to be destroyed if a client sends an Upgrade header.
-->

* `request` {http.IncomingMessage} Arguments for the HTTP request, as it is in
the [`'request'`][] event
* `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
Expand Down
8 changes: 6 additions & 2 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 &&
Expand Down Expand Up @@ -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') {
Expand Down
14 changes: 1 addition & 13 deletions lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
}

Expand Down
8 changes: 0 additions & 8 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ function OutgoingMessage() {
this.writable = true;

this._last = false;
this.upgrading = false;
this.chunkedEncoding = false;
this.shouldKeepAlive = true;
this.useChunkedEncodingByDefault = true;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 9 additions & 2 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Expand Down Expand Up @@ -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
Expand Down
19 changes: 14 additions & 5 deletions test/parallel/test-http-upgrade-advertise.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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();
}
Expand Down
8 changes: 3 additions & 5 deletions test/parallel/test-http-upgrade-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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();
});

Expand Down Expand Up @@ -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);
});

0 comments on commit 33ce9a6

Please sign in to comment.