Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: do not emit upgrade on advertisement #4337

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 16 additions & 0 deletions lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice comments @indutny! thanks

// supported protocols.
//
// See RFC7230 Section 6.7
if (upgrade &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like a kludge that we are doing this here, isn't this more appropriate for http_parser if it's getting it wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Is http-parser aware of the outgoing request? If not, I think the fix here is valid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silverwind has valid point

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny what do you think about only emitting 'upgrade' if both headers are present in the response? That way the decision could be based purely on the response and could be done in http-parser. All three use cases of the upgrade header require both headers to be present in both the request and the response.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, how does http-parser know about response headers? Also, I think I require both headers now, am I not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it http-parser which sets the upgrade argument based on the response header here?

And yeah, you require both header on the request. I still think the upgrade decision could be solely based on the response headers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather, scrap that. I re-read that rfc7230 part, and the advertisement can include both headers, so knowledge of request and response is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course 😉

parser.outgoing !== null &&
(parser.outgoing._headers.upgrade === undefined ||
!/(^|\W)upgrade(\W|$)/i.test(parser.outgoing._headers.connection))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining that the regex is supposed to match Connection: headers containing words besides just the string 'upgrade'?

(At least, that's what I infer it does.)

upgrade = false;
}

parser.incoming.upgrade = upgrade;

var skipBody = false; // response to HEAD or CONNECT
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
60 changes: 60 additions & 0 deletions test/parallel/test-http-upgrade-advertise.js
Original file line number Diff line number Diff line change
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary, common.mustCall() will squeal if the callback is called more than once (at script exit though.)

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you could just pass fire() directly.

});
1 change: 1 addition & 0 deletions test/parallel/test-http-upgrade-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
};
Expand Down
8 changes: 7 additions & 1 deletion test/parallel/test-http-upgrade-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down