Fix handling of bolt URL with port 80

URLs are currently parsed in two places in the driver. First is before
driver creation to determine if it should be a direct, routing or http
driver based on the URL scheme. Then scheme is stripped off and rest of
the code uses host and port string. Second time is before network
connection is established. This time code parses host and port string,
which is either the one used to create the driver or arrived in a
routing procedure response. Scheme is missing in this case and dummy
"http://" was attached just to make parser correctly handle the given
string.

Turned out "url-parse" library used for parsing removes port from the
parsed URL if it is the default one for the scheme. So port 80 is
removed when URL scheme is "http". This made driver always treat
"localhost:80" as "localhost:7687" where 7687 is the default bolt port
during the second parsing step. Default port 80 was removed and driver
treated absence of the port as a sign to use the default bolt port.
Same logic handles URLs like "bolt://localhost". Removal of the default
port is problematic because driver can't know if no port was specified
or default port was specified. Experimental HTTP and HTTPS support also
suffers from the same problem. Default port for HTTP should be 7474
and not 80.

This commit fixes the problem by using a different parsing library
"uri-js" that always preserves the port. Dummy "http://" prefix is
also changed to "none://" just in case.

Author: lutovich

package-lock.json

@@ -73,6 +73,6 @@
   },
   "dependencies": {
     "babel-runtime": "^6.18.0",
-    "url-parse": "^1.2.0"
+    "uri-js": "^4.2.1"
   }
 }

package.json

@@ -17,7 +17,7 @@
  * limitations under the License.
  */
 
-import ParsedUrl from 'url-parse';
+import {parse as uriJsParse} from 'uri-js';
 import {assertString} from './util';
 
 const DEFAULT_BOLT_PORT = 7687;
@@ -69,14 +69,14 @@ function parseDatabaseUrl(url) {
   assertString(url, 'URL');
 
   const sanitized = sanitizeUrl(url);
-  const parsedUrl = new ParsedUrl(sanitized.url, {}, query => extractQuery(query, url));
+  const parsedUrl = uriJsParse(sanitized.url);
 
-  const scheme = sanitized.schemeMissing ? null : extractScheme(parsedUrl.protocol);
-  const rawHost = extractHost(parsedUrl.hostname); // has square brackets for IPv6
-  const host = unescapeIPv6Address(rawHost); // no square brackets for IPv6
+  const scheme = sanitized.schemeMissing ? null : extractScheme(parsedUrl.scheme);
+  const host = extractHost(parsedUrl.host); // no square brackets for IPv6
+  const formattedHost = formatHost(host); // has square brackets for IPv6
   const port = extractPort(parsedUrl.port, scheme);
-  const hostAndPort = `${rawHost}:${port}`;
-  const query = parsedUrl.query;
+  const hostAndPort = `${formattedHost}:${port}`;
+  const query = extractQuery(parsedUrl.query, url);
 
   return new Url(scheme, host, port, hostAndPort, query);
 }
@@ -85,8 +85,8 @@ function sanitizeUrl(url) {
   url = url.trim();
 
   if (url.indexOf('://') === -1) {
-    // url does not contain scheme, add dummy 'http://' to make parser work correctly
-    return {schemeMissing: true, url: `http://${url}`};
+    // url does not contain scheme, add dummy 'none://' to make parser work correctly
+    return {schemeMissing: true, url: `none://${url}`};
   }
 
   return {schemeMissing: false, url: url};
@@ -169,17 +169,12 @@ function escapeIPv6Address(address) {
   }
 }
 
-function unescapeIPv6Address(address) {
-  const startsWithSquareBracket = address.charAt(0) === '[';
-  const endsWithSquareBracket = address.charAt(address.length - 1) === ']';
-
-  if (!startsWithSquareBracket && !endsWithSquareBracket) {
-    return address;
-  } else if (startsWithSquareBracket && endsWithSquareBracket) {
-    return address.substring(1, address.length - 1);
-  } else {
-    throw new Error(`Illegal IPv6 address ${address}`);
+function formatHost(host) {
+  if (!host) {
+    throw new Error(`Illegal host ${host}`);
   }
+  const isIPv6Address = host.indexOf(':') >= 0;
+  return isIPv6Address ? escapeIPv6Address(host) : host;
 }
 
 function formatIPv4Address(address, port) {

src/v1/internal/url-util.js

@@ -723,6 +723,31 @@ describe('url-util', () => {
     expect(parse('https://localhost').port).toEqual(urlUtil.defaultPortForScheme('https'));
   });
 
+  it('should parse URLs with port 80', () => {
+    ['http', 'https', 'ws', 'wss', 'bolt', 'bolt+routing'].forEach(scheme => {
+      verifyUrl(`${scheme}://localhost:80`, {
+        scheme: scheme,
+        host: 'localhost',
+        port: 80
+      });
+    });
+
+    ['localhost', '127.0.0.1', '192.168.10.29'].forEach(host => {
+      verifyUrl(`${host}:80`, {
+        host: host,
+        port: 80
+      });
+    });
+
+    ['::1', '1afc:0:a33:85a3::ff2f'].forEach(host => {
+      verifyUrl(`[${host}]:80`, {
+        host: host,
+        port: 80,
+        ipv6: true
+      });
+    });
+  });
+
   function verifyUrl(urlString, expectedUrl) {
     const url = parse(urlString);
 

test/internal/url-util.test.js

@@ -23,6 +23,7 @@ import FakeConnection from '../internal/fake-connection';
 import lolex from 'lolex';
 import {DEFAULT_ACQUISITION_TIMEOUT, DEFAULT_MAX_SIZE} from '../../src/v1/internal/pool-config';
 import {ServerVersion, VERSION_3_1_0} from '../../src/v1/internal/server-version';
+import testUtils from '../internal/test-utils';
 
 describe('driver', () => {
 
@@ -78,6 +79,28 @@ describe('driver', () => {
     startNewTransaction(driver);
   }, 10000);
 
+  it('should fail with correct error message when connecting to port 80', done => {
+    if (testUtils.isClient()) {
+      // good error message is not available in browser
+      done();
+      return;
+    }
+
+    driver = neo4j.driver('bolt://localhost:80', sharedNeo4j.authToken);
+
+    driver.session().run('RETURN 1').then(result => {
+      done.fail('Should not be able to connect. Result: ' + JSON.stringify(result));
+    }).catch(error => {
+      const doesNotContainAddress = error.message.indexOf(':80') < 0;
+      if (doesNotContainAddress) {
+        done.fail(`Expected to contain ':80' but was: ${error.message}`);
+      } else {
+        expect(error.code).toEqual(neo4j.error.SERVICE_UNAVAILABLE);
+        done();
+      }
+    });
+  });
+
   it('should handle wrong scheme', () => {
     expect(() => neo4j.driver("tank://localhost", sharedNeo4j.authToken))
       .toThrow(new Error('Unknown scheme: tank'));