-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,20 @@ 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 | ||
// | ||
// NOTE: RegExp below matches `upgrade` in `Connection: abc, upgrade, def` | ||
// header. | ||
if (upgrade && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @silverwind has valid point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @indutny what do you think about only emitting There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it http-parser which sets the And yeah, you require both header on the request. I still think the upgrade decision could be solely based on the response headers. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -142,6 +156,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 +193,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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
'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(); | ||
|
||
const done = common.mustCall(function done(result) { | ||
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, fire); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice comments @indutny! thanks