url.parse does not handle IPv6 link-local addresses correctly

[cthayer]

In version 0.12.0, url.parse() does not parse a URL containing an IPv6 link-local address correctly.

Specifically, adding a zone to the address in the host portion of the URL causes url.parse() to fail to detect the host portion of the address and considers everything after the protocol to be part of the path.

Example of failure with IPv6 zone:

var url = require('url');

url.parse('ws://[fe80::2e0:4cff:fe68:23ff%en7]:51877/engine.io/?EIO=3&transport=websocket');
{ protocol: 'ws:',
  slashes: true,
  auth: null,
  host: '',
  port: null,
  hostname: '',
  hash: null,
  search: '?EIO=3&transport=websocket',
  query: 'EIO=3&transport=websocket',
  pathname: '/[fe80::2e0:4cff:fe68:23ff%en7]:51877/engine.io/',
  path: '/[fe80::2e0:4cff:fe68:23ff%en7]:51877/engine.io/?EIO=3&transport=websocket',
  href: 'ws:///[fe80::2e0:4cff:fe68:23ff%en7]:51877/engine.io/?EIO=3&transport=websocket' }

Example of success without the IPv6 zone:

var url = require('url');

url.parse('ws://[fe80::2e0:4cff:fe68:23ff]:51877/engine.io/?EIO=3&transport=websocket');
{ protocol: 'ws:',
  slashes: true,
  auth: null,
  host: '[fe80::2e0:4cff:fe68:23ff]:51877',
  port: '51877',
  hostname: 'fe80::2e0:4cff:fe68:23ff',
  hash: null,
  search: '?EIO=3&transport=websocket',
  query: 'EIO=3&transport=websocket',
  pathname: '/engine.io/',
  path: '/engine.io/?EIO=3&transport=websocket',
  href: 'ws://[fe80::2e0:4cff:fe68:23ff]:51877/engine.io/?EIO=3&transport=websocket' }

Expected behavior

I would expect url.parse() of ws://[fe80::2e0:4cff:fe68:23ff%en7]:51877/engine.io/?EIO=3&transport=websocket to produce the following result:

{ protocol: 'ws:',
  slashes: true,
  auth: null,
  host: '[fe80::2e0:4cff:fe68:23ff%en7]:51877',
  port: 51877,
  hostname: 'fe80::2e0:4cff:fe68:23ff%en7',
  hash: null,
  search: '?EIO=3&transport=websocket',
  query: 'EIO=3&transport=websocket',
  pathname: '/engine.io/',
  path: '/engine.io/?EIO=3&transport=websocket',
  href: 'ws://[fe80::2e0:4cff:fe68:23ff%en7]:51877/engine.io/?EIO=3&transport=websocket' }

[cthayer]

Looking through the code, it seems like the IPv6 hostname detection may need to be done before the invalid host character detection so that different invalid host characters can be used for IPv6 hostnames.

[cthayer]

According to that reasoning, I would imagine changing lib/url.js like so:

diff --git a/lib/url.js b/lib/url.js
index c5a3793..e696547 100644
--- a/lib/url.js
+++ b/lib/url.js
@@ -70,6 +70,7 @@ var protocolPattern = /^([a-z0-9.+-]+:)/i,
     // are the ones that are *expected* to be seen, so we fast-path
     // them.
     nonHostChars = ['%', '/', '?', ';', '#'].concat(autoEscape),
+    nonHostCharsIPv6 = ['/', '?', ';', '#'].concat(autoEscape),
     hostEndingChars = ['/', '?', '#'],
     hostnameMaxLen = 255,
     hostnamePartPattern = /^[+a-z0-9A-Z_-]{0,63}$/,
@@ -216,11 +217,19 @@ Url.prototype.parse = function(url, parseQueryString, slashesDenoteHost) {
       rest = rest.slice(atSign + 1);
       this.auth = decodeURIComponent(auth);
     }
+    
+    // if hostname begins with [ and is followed by ]
+    // assume that it's an IPv6 address.
+    // :: is the smallest IPv6 address possible, so ] must be > 2 positions to the right of [
+    var ipv6Hostname = rest[0] === '[' &&
+        rest.indexOf(']') > 2;

     // the host is the remaining to the left of the first non-host char
+    // use the appropriate non-host char set
+    var invalidHostChars = ipv6Hostname ? nonHostCharsIPv6 : nonHostChars;
     hostEnd = -1;
-    for (var i = 0; i < nonHostChars.length; i++) {
-      var hec = rest.indexOf(nonHostChars[i]);
+    for (var i = 0; i < invalidHostChars.length; i++) {
+      var hec = rest.indexOf(invalidHostChars[i]);
       if (hec !== -1 && (hostEnd === -1 || hec < hostEnd))
         hostEnd = hec;
     }
@@ -238,11 +247,6 @@ Url.prototype.parse = function(url, parseQueryString, slashesDenoteHost) {
     // so even if it's empty, it has to be present.
     this.hostname = this.hostname || '';

-    // if hostname begins with [ and ends with ]
-    // assume that it's an IPv6 address.
-    var ipv6Hostname = this.hostname[0] === '[' &&
-        this.hostname[this.hostname.length - 1] === ']';
-
     // validate a little.
     if (!ipv6Hostname) {
       var hostparts = this.hostname.split(/\./);

Full code is here

I can create a pull request if that will be helpful.

[misterdjules]

@cthayer Thank you for reporting and investigating this issue!

Adding to milestone 0.12.2, because:

• It's a bug that is present in v0.12.0 and v0.10.x, so probably not urgent to fix.
• We already have a lot of issues to fix in v0.12.1 that are more critical in my opinion.

I'm not familiar with IPv6 addresses' format, but please submit a PR with your change, it'll make it much easier to discuss it.

Thank you again! 👍

[cthayer]

@misterdjules No problem. Happy to be able to help out.

Submitted pull request #9411 with my proposed changes and relevant tests.

Your reasoning on the importance of the bug fix makes sense, though of course I would love to see this fixed in 0.12.1 since it's making it hard to use npm modules in a project I'm working on, but I will defer to your work flow. Currently, I'm using the sockets directly to avoid the use of url.parse that many modules use.

In IPv6, link-local addresses can contain a zone that specifies the interface that the address belongs to. The zone is in the hostname portion of a URL and is separated from the address by a % character (which is not a valid host character in IPv4).

[misterdjules]

@cthayer Thanks for the clarification! My goal this week is to prioritize issues in the 0.12.1 milestone to get it out in a reasonable amount of time, so hopefully we'll get to v0.12.2 sooner rather than later!

[cthayer]

@misterdjules Sounds good. Looking forward to the 0.12.2 release!

[misterdjules]

@cthayer Because of the recent vulnerabilities in OpenSSL, we had to do a v0.12.1 release with just security fixes, so all v0.12.x releases have been shifted. As a result, what I called v0.12.2 in my previous comment is now v0.12.3.

[cthayer]

@misterdjules Understood. Thanks for keeping me updated!