-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: 🐛 request.url getter fails when using IPv6 and HTTP2 [#4560] #4559
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
Changes from 2 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 |
|---|---|---|
|
|
@@ -184,7 +184,9 @@ exports = module.exports = internals.Request = class { | |
| return url; | ||
| } | ||
|
|
||
| this._url = new Url.URL(`${this._core.info.protocol}://${this.info.host || `${this._core.info.host}:${this._core.info.port}`}${source}`); | ||
| const host = this.info.host || this._formatIpv6Host(this._core.info.host, this._core.info.port); | ||
|
|
||
| this._url = new Url.URL(`${this._core.info.protocol}://${host}${source}`); | ||
| } | ||
| else { | ||
|
|
||
|
|
@@ -201,6 +203,18 @@ exports = module.exports = internals.Request = class { | |
| return this._url; | ||
| } | ||
|
|
||
| _isBareIpv6(host) { | ||
|
|
||
| // An IPv6 address contains at least two colons. | ||
|
|
||
| return (host.match(/:/g) || []).length >= 2; | ||
| } | ||
|
|
||
| _formatIpv6Host(host, port) { | ||
|
|
||
| return this._isBareIpv6(host) ? `[${host}]:${port}` : `${host}:${port}`; | ||
| } | ||
|
|
||
| _normalizePath(url, options) { | ||
|
|
||
| let path = this._core.router.normalize(url.pathname); | ||
|
|
@@ -624,7 +638,7 @@ internals.Info = class { | |
| this._request = request; | ||
|
|
||
| const req = request.raw.req; | ||
| const host = req.headers.host ? req.headers.host.trim() : ''; | ||
| const host = (req.headers.host || req.headers[':authority'] || '').trim(); | ||
|
Contributor
Author
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. Perhaps we could test if the req is an IPv6 request somehow, and choose
Contributor
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. Yes, that would be great and expect if we want this included. I've done some light digging and come up with the following to help this along: https://github.com/hapijs/hapi/blob/master/lib/core.js#L334 listens for whatever host / address you pass it, you can set it to loopback Test it using builtins that only request IPv6 or alternatively, reuse some of the patterns in hapi tests: https://github.com/hapijs/hapi/blob/master/test/core.js#L925 The right place to add those tests would be https://github.com/hapijs/hapi/blob/master/test/core.js Checkout some of the commentary around ipv6: https://github.com/hapijs/hapi/blob/master/test/core.js#L102 Seems like there's no explicit ipv6 tests, and they would need to be done only if the host machine has ipv6 enabled to prevent false positives. Hope this helps! |
||
| const received = Date.now(); | ||
|
|
||
| this.received = received; | ||
|
|
||
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.
Since this function is in a hot path, I'd favor a more performant alternative. Having tried a few in this benchmark, the regex for the test version seems consistently better both for the positive and negative scenarios. Let me know if my benchmark is biased though.
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.
Good catch, and thanks for the benchmark! I've validated that
/:[^:]*:/is semantically equivalent to the previous colon-count approach — a string has two or more colons if and only if it contains two colons with zero or more non-colon characters between them, which is exactly what this regex matches.I've applied it in 170a005. One small note: I omitted the
gflag from the benchmark's/:[^:]*:/gsince.test()withgadvanceslastIndexon a shared regex, which can cause subtle bugs. Since we're using it as an inline literal the impact is zero here, but it's cleaner to leave it out.