Ensure IPv6 hostname is enclosed within square brackets in Host header #5314

mpotra · 2016-02-18T23:40:45Z

As per https://tools.ietf.org/html/rfc7230#section-5.4 and https://tools.ietf.org/html/rfc3986#section-3.2.2 IPv6 addresses in Host header (URI), must be enclosed within square brackets, in order to properly separate the host address from any port reference.

mscdex · 2016-02-19T00:42:21Z

It looks like the file permissions for _http_client.js were changed?

mpotra · 2016-02-19T07:37:43Z

Thank you for noticing, and I apologize for the mistake. Not sure how that happened, but I'll update the PR with the permissions change reverted.

evanlucas · 2016-02-19T16:08:12Z

lib/_http_client.js

+      // For the Host header, ensure that IPv6 addresses are enclosed
+      // in square brackets, as defined by URI formatting
+      // https://tools.ietf.org/html/rfc3986#section-3.2.2
+      if ('[' !== hostHeader[0] && -1 !== hostHeader.indexOf(':')) {


Maybe use net.isIPv6(hostHeader) here instead?

jasnell · 2016-02-19T23:23:51Z

lib/_http_client.js

+      // in square brackets, as defined by URI formatting
+      // https://tools.ietf.org/html/rfc3986#section-3.2.2
+      if (net.isIPv6(hostHeader)) {
+        hostHeader = '[' + hostHeader + ']';


very minor nit... these can use template strings now.. [${hostHeader}]... not critical tho :-)

Isn't string concatenation faster than template strings?

I want to say that some benchmarks have shown that template strings do not introduce any performance issues, maybe even improve performance. Will have to look and see if I can find them though.

jasnell · 2016-02-19T23:24:52Z

LGTM

evanlucas · 2016-02-20T19:01:48Z

test/internet/test-http-host-header-ipv6-fail.js

+  req.end();
+}
+
+server = http.createServer(function(req, res) {


if you wrap this in a common.mustCall(function(req, res) { }), then the hadRequest can be be totally removed. It doesn't look like hadError is used at all, so that can be removed. With those two gone, we can also remove the process.on('exit') listener.

evanlucas · 2016-02-20T19:02:44Z

If you can address the nits that both myself and @jasnell have mentioned, LGTM. Once that is done, we can run the CI and make sure everything is passing. Thanks for the contribution!

vkurchatkin · 2016-02-20T19:07:59Z

lib/_http_client.js

+      // For the Host header, ensure that IPv6 addresses are enclosed
+      // in square brackets, as defined by URI formatting
+      // https://tools.ietf.org/html/rfc3986#section-3.2.2
+      if (net.isIPv6(hostHeader)) {


isn't this too heavyweight?

In my initial PR, IPv6 checking was done by hostHeader.indexOf(':'), just as you also suggested in #5308
However, now I agree that net.isIPv6() is a much more reliable way of testing for IPv6.

I don't necessarily think it's too heavyweight, since it relies on uv_inet_pton. I guess it's slightly slower than indexOf, but since it also checks for valid characters inside the string, I believe it's worth the extra cycles.

There were no check for correctness here before, so I assume that there are two possibilities:

Host is check before and is valid at this point, so if there is : then it can only possibly be IPv6 address.

Host isn't checked before, so it might be invalid. If it has : then it is either IPv6 or invalid. If we wrap invalid host in brackets it would still be invalid, so it doesn't really matter.

isIPv6 involves calling C++ code and also checking if argument is IPv4 before checking if it's IPv6.

Host value doesn't pass through proper validation. It actually relies on either url.parse() to produce a host value, or simply takes options.hostname || options.host. At this stage, especially if options object is passed to the request(), hostHeader could even be www.domain.tld:3000, before reaching my code; (which is not wrong, only unsupported atm). In this case, a simple : check would yield an invalid Host header [www.domain.tld:3000].

Secondly, the RFC says that if (IP is IPv6) then [IP], which in my view makes it mandatory to make absolutely sure that whatever I enclose within square brackets, is an IPv6 address and not anything else (even if invalid).

I've been considering all of the above right from my initial PR, including the fact that indexOf is faster than isIPv6. However, since all points of view express advantages and disadvantages, I also opted for net.isIPv6 in the end, because that allows my fix to avoid type assumptions and ultimately make the code terse, and enforces the RFC statement.

While the above explains my preference for net.isIPv6, if there's consensus on reverting it back to : matching, I'll happily do it.

Secondly, the RFC says that if (IP is IPv6) then [IP], which in my view makes it mandatory to make absolutely sure that whatever I enclose within square bracket

It doesn't say that wrapping invalid host with brackets makes it more invalid, that's all I'm saying.

Doing a quick scan for two :'s may be a simple workaround here. I agree that dropping down to the native code to check is a bit overweight for this.

Yea, I agree. Just ran some benchmarks and net.isIPv6 runs about 3-6 times slower than an indexOf check here.

mpotra · 2016-02-22T09:23:16Z

I've just pushed two commits with the following:

drop net.isIPv6 for indexOf(); checking for the presence of two colons, to avoid false positives when port is present. Reintroduced skipping when the hostname is already an IPv6 within square brackets.
Replaced string concatenation hostHeader = '[' + hostHeader +']' with string literal hostHeader = [${hostHeader}]``

evanlucas · 2016-02-25T12:07:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1747/

jasnell · 2016-03-02T17:24:23Z

Still LGTM. @nodejs/http @nodejs/ctc

jasnell · 2016-03-21T22:58:27Z

@evanlucas ... does this LGTY?

evanlucas · 2016-03-22T04:07:57Z

LGTM

jasnell · 2016-03-22T04:13:55Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/2009/

jasnell · 2016-03-22T04:39:01Z

test/internet/test-http-host-header-ipv6-fail.js

+  req.end();
+}
+
+server = http.createServer(common.mustCall(function(req, res) {


nit: would just make this const server

jasnell · 2016-03-22T04:39:48Z

CI is green, added a small nit.

mpotra · 2016-03-22T14:40:51Z

Modified to use const server

jasnell · 2016-03-22T15:01:43Z

New CI after the rebase: https://ci.nodejs.org/job/node-test-pull-request/2021/

mpotra · 2016-03-24T09:02:04Z

Apparently CI failed on "node-test-binary-arm » 3,pi1-raspbian-wheezy" https://ci.nodejs.org/job/node-test-binary-arm/1448//console
Could it be from my const server commit?

jasnell · 2016-03-24T14:35:51Z

No, we've been seeing an increase in random flakiness with tests on the pi devices. I don't think the failure is related. /cc @nodejs/testing

jasnell · 2016-03-24T14:36:42Z

Can I ask you to please squash the commits down into one commit now? Thank you!

mpotra · 2016-03-24T18:43:52Z

Sure, done.

evanlucas · 2016-03-25T09:59:50Z

@mpotra to clarify, the tests that live in test/internet are not run with our CI. Please put the test in test/parallel and then we will run the CI one last time and get this thing landed!!

mpotra · 2016-03-25T11:30:26Z

Thanks, and sorry for having it under internet in the first place. It's adjusted now.

evanlucas · 2016-03-25T11:31:56Z

No problem at all. Sorry for just noticing :]

I've started another CI run: https://ci.nodejs.org/job/node-test-pull-request/2063/

IPv6 addresses in Host header (URI), must be enclosed within square brackets, in order to properly separate the host address from any port reference. test: add test for IPv6 hostname conformance in Host header http: Ensure IPv6 is enclosed within square brackets in Host header http: use net.isIPv6 in ClientRequest test: update test with const instead of var test: use common.mustCall and cleanup http: ClientRequest, drop isIPv6() in favor of indexOf() checks http: ClientRequest, replace concatenation with string literal test: use const test: clean-up test: move test to parallel test: skip if no IPv6 support

mpotra · 2016-03-25T13:39:54Z

Two failures: 1 is parallel/test-tls-sni-option on smartos, but the arm (p1/p2-raspbian) fails on my test:

not ok 61 test-http-host-header-ipv6-fail.js
# events.js:154
# throw er; // Unhandled 'error' event
# ^
# 
# Error: listen EAFNOSUPPORT ::1:12346
# at Object.exports._errnoException (util.js:893:11)
# at exports._exceptionWithHostPort (util.js:916:20)
# at Server._listen2 (net.js:1225:19)
# at listen (net.js:1274:10)
# at net.js:1384:9
# at _combinedTickCallback (internal/process/next_tick.js:77:11)
# at process._tickCallback (internal/process/next_tick.js:98:9)
# at Function.Module.runMain (module.js:453:11)
# at startup (node.js:155:18)
# at node.js:433:3

Apparently it cannot bind ::1 IPv6. Any suggestions?
I guess I should I check for IPv6 support within interfaces, first..

mpotra · 2016-03-25T14:04:57Z

Updated with IPv6 check, as per nodejs/node-v0.x-archive#7983

evanlucas · 2016-03-29T18:52:55Z

new CI: https://ci.nodejs.org/job/node-test-pull-request/2090/

mpotra · 2016-03-30T05:55:07Z

Everything looks green, except a test-dns.js timeout, which is not related to this PR, on p2-raspbian`.

Let me know if there's anything else I can do.

jasnell · 2016-04-01T15:58:25Z

LGTM

IPv6 addresses in Host header (URI), must be enclosed within square brackets, in order to properly separate the host address from any port reference. PR-URL: #5314 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

jasnell · 2016-04-01T15:59:07Z

Landed in dabe1d5

IPv6 addresses in Host header (URI), must be enclosed within square brackets, in order to properly separate the host address from any port reference. PR-URL: #5314 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

Notable changes: http: * Enclose IPv6 Host header in square brackets. This will enable proper seperation of the host adress from any port reference (Mihai Potra) #5314 path: * Make win32.isAbsolute more consistent (Brian White) #6028

Notable changes: http: * Enclose IPv6 Host header in square brackets. This will enable proper seperation of the host adress from any port reference (Mihai Potra) #5314 path: * Make win32.isAbsolute more consistent (Brian White) #6028 PR-URL: #6060 Reviewed-By: Jeremiah Senkpiel <[email protected]>

IPv6 addresses in Host header (URI), must be enclosed within square brackets, in order to properly separate the host address from any port reference. PR-URL: #5314 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

mscdex added the http Issues or PRs related to the http subsystem. label Feb 19, 2016

mpotra force-pushed the http-host-header-ipv6 branch from c3faa8d to 0f7bf56 Compare February 19, 2016 08:30

evanlucas reviewed Feb 19, 2016
View reviewed changes

himdel mentioned this pull request Feb 19, 2016

[WIP] Rails 5 fix - depend on an express-http-proxy version that can handle Transfer-Encoding: chunked & IPv6 ManageIQ/manageiq-ui-service#8

Closed

jasnell reviewed Feb 19, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Feb 19, 2016

evanlucas reviewed Feb 20, 2016
View reviewed changes

vkurchatkin reviewed Feb 20, 2016
View reviewed changes

jasnell reviewed Mar 22, 2016
View reviewed changes

mpotra force-pushed the http-host-header-ipv6 branch from bc7d9ca to 6634168 Compare March 22, 2016 14:36

Trott mentioned this pull request Mar 24, 2016

How to motivate people to fix flaky tests nodejs/testing#21

Closed

mpotra force-pushed the http-host-header-ipv6 branch from 3ff813d to 7dc4d06 Compare March 24, 2016 18:42

mpotra force-pushed the http-host-header-ipv6 branch from cebe453 to f311efd Compare March 25, 2016 11:28

mpotra force-pushed the http-host-header-ipv6 branch from f311efd to 5132c30 Compare March 25, 2016 14:03

jasnell closed this Apr 1, 2016

This was referenced Apr 5, 2016

V5.10.1 proposal #6059

Closed

V5.10.1 proposal #6060

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels May 3, 2016

MylesBorins mentioned this pull request May 18, 2016

v4.4.5 proposal #6824

Merged

Ensure IPv6 hostname is enclosed within square brackets in Host header #5314

Ensure IPv6 hostname is enclosed within square brackets in Host header #5314

Conversation

mpotra commented Feb 18, 2016

mscdex commented Feb 19, 2016

mpotra commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 19, 2016

Choose a reason for hiding this comment

evanlucas commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpotra commented Feb 22, 2016

evanlucas commented Feb 25, 2016

jasnell commented Mar 2, 2016

jasnell commented Mar 21, 2016

evanlucas commented Mar 22, 2016

jasnell commented Mar 22, 2016

Choose a reason for hiding this comment

jasnell commented Mar 22, 2016

mpotra commented Mar 22, 2016

jasnell commented Mar 22, 2016

mpotra commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

mpotra commented Mar 24, 2016

evanlucas commented Mar 25, 2016

mpotra commented Mar 25, 2016

evanlucas commented Mar 25, 2016

mpotra commented Mar 25, 2016

mpotra commented Mar 25, 2016

evanlucas commented Mar 29, 2016

mpotra commented Mar 30, 2016

jasnell commented Apr 1, 2016

jasnell commented Apr 1, 2016