From 8a5a2a36266b674befc75646646ae854b4ef773c Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 30 Apr 2018 12:43:18 +0200 Subject: [PATCH 1/2] 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. --- package-lock.json | 32 +++++++++++++++++--------------- package.json | 2 +- src/v1/internal/url-util.js | 33 ++++++++++++++------------------- test/internal/url-util.test.js | 25 +++++++++++++++++++++++++ test/v1/driver.test.js | 23 +++++++++++++++++++++++ 5 files changed, 80 insertions(+), 35 deletions(-) diff --git a/package-lock.json b/package-lock.json index fd0262cfe..3d5218ae3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8233,11 +8233,6 @@ "integrity": "sha1-nsYfeQSYdXB9aUFFlv2Qek1xHnM=", "dev": true }, - "querystringify": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/querystringify/-/querystringify-1.0.0.tgz", - "integrity": "sha1-YoYkIRLFtxL6ZU5SZlK/ahP/Bcs=" - }, "randomatic": { "version": "1.1.7", "resolved": "https://registry.npmjs.org/randomatic/-/randomatic-1.1.7.tgz", @@ -8554,7 +8549,8 @@ "requires-port": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/requires-port/-/requires-port-1.0.0.tgz", - "integrity": "sha1-kl0mAdOaxIXgkc8NpcbmlNw9yv8=" + "integrity": "sha1-kl0mAdOaxIXgkc8NpcbmlNw9yv8=", + "dev": true }, "resolve": { "version": "1.5.0", @@ -9932,6 +9928,21 @@ } } }, + "uri-js": { + "version": "4.2.1", + "resolved": "https://registry.npmjs.org/uri-js/-/uri-js-4.2.1.tgz", + "integrity": "sha512-jpKCA3HjsBfSDOEgxRDAxQCNyHfCPSbq57PqCkd3gAyBuPb3IWxw54EHncqESznIdqSetHfw3D7ylThu2Kcc9A==", + "requires": { + "punycode": "2.1.0" + }, + "dependencies": { + "punycode": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/punycode/-/punycode-2.1.0.tgz", + "integrity": "sha1-X4Y+3Im5bbCQdLrXlHvwkFbKTn0=" + } + } + }, "urix": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/urix/-/urix-0.1.0.tgz", @@ -9956,15 +9967,6 @@ } } }, - "url-parse": { - "version": "1.2.0", - "resolved": "https://registry.npmjs.org/url-parse/-/url-parse-1.2.0.tgz", - "integrity": "sha512-DT1XbYAfmQP65M/mE6OALxmXzZ/z1+e5zk2TcSKe/KiYbNGZxgtttzC0mR/sjopbpOXcbniq7eIKmocJnUWlEw==", - "requires": { - "querystringify": "1.0.0", - "requires-port": "1.0.0" - } - }, "use": { "version": "2.0.2", "resolved": "https://registry.npmjs.org/use/-/use-2.0.2.tgz", diff --git a/package.json b/package.json index 41829beb5..f98150c40 100644 --- a/package.json +++ b/package.json @@ -73,6 +73,6 @@ }, "dependencies": { "babel-runtime": "^6.18.0", - "url-parse": "^1.2.0" + "uri-js": "^4.2.1" } } diff --git a/src/v1/internal/url-util.js b/src/v1/internal/url-util.js index d19433892..8b2c35f04 100644 --- a/src/v1/internal/url-util.js +++ b/src/v1/internal/url-util.js @@ -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) { diff --git a/test/internal/url-util.test.js b/test/internal/url-util.test.js index f1f62394a..6a8324a4b 100644 --- a/test/internal/url-util.test.js +++ b/test/internal/url-util.test.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); diff --git a/test/v1/driver.test.js b/test/v1/driver.test.js index 2e499bb1b..ae2d93b75 100644 --- a/test/v1/driver.test.js +++ b/test/v1/driver.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')); From 56ff62c9eb51cd26265b26ffcaaede437efdd237 Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 30 Apr 2018 12:57:35 +0200 Subject: [PATCH 2/2] Improve naming of variable and properties To clarify that address being used for connection is not a URL but a host-port string. --- src/v1/driver.js | 18 +++++++++--------- src/v1/internal/connection-providers.js | 10 +++++----- src/v1/internal/connector.js | 20 ++++++++++---------- src/v1/internal/http/http-driver.js | 6 +++--- src/v1/routing-driver.js | 18 +++++++++--------- test/v1/session.test.js | 2 +- 6 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/v1/driver.js b/src/v1/driver.js index 2351f7b0b..fcbc781c6 100644 --- a/src/v1/driver.js +++ b/src/v1/driver.js @@ -57,16 +57,16 @@ class Driver { /** * You should not be calling this directly, instead use {@link driver}. * @constructor - * @param {string} url + * @param {string} hostPort * @param {string} userAgent * @param {object} token * @param {object} config * @protected */ - constructor(url, userAgent, token = {}, config = {}) { + constructor(hostPort, userAgent, token = {}, config = {}) { sanitizeConfig(config); - this._url = url; + this._hostPort = hostPort; this._userAgent = userAgent; this._openSessions = {}; this._sessionIdGenerator = 0; @@ -117,13 +117,13 @@ class Driver { * @return {Connection} new connector-api session instance, a low level session API. * @access private */ - _createConnection(url, release) { + _createConnection(hostPort, release) { let sessionId = this._sessionIdGenerator++; - let conn = connect(url, this._config, this._connectionErrorCode()); + let conn = connect(hostPort, this._config, this._connectionErrorCode()); let streamObserver = new _ConnectionStreamObserver(this, conn); conn.initialize(this._userAgent, this._token, streamObserver); conn._id = sessionId; - conn._release = () => release(url, conn); + conn._release = () => release(hostPort, conn); this._openSessions[sessionId] = conn; return conn; @@ -186,8 +186,8 @@ class Driver { } // Extension point - _createConnectionProvider(address, connectionPool, driverOnErrorCallback) { - return new DirectConnectionProvider(address, connectionPool, driverOnErrorCallback); + _createConnectionProvider(hostPort, connectionPool, driverOnErrorCallback) { + return new DirectConnectionProvider(hostPort, connectionPool, driverOnErrorCallback); } // Extension point @@ -204,7 +204,7 @@ class Driver { _getOrCreateConnectionProvider() { if (!this._connectionProvider) { const driverOnErrorCallback = this._driverOnErrorCallback.bind(this); - this._connectionProvider = this._createConnectionProvider(this._url, this._pool, driverOnErrorCallback); + this._connectionProvider = this._createConnectionProvider(this._hostPort, this._pool, driverOnErrorCallback); } return this._connectionProvider; } diff --git a/src/v1/internal/connection-providers.js b/src/v1/internal/connection-providers.js index 02c46c0a0..106a50bf5 100644 --- a/src/v1/internal/connection-providers.js +++ b/src/v1/internal/connection-providers.js @@ -45,24 +45,24 @@ class ConnectionProvider { export class DirectConnectionProvider extends ConnectionProvider { - constructor(address, connectionPool, driverOnErrorCallback) { + constructor(hostPort, connectionPool, driverOnErrorCallback) { super(); - this._address = address; + this._hostPort = hostPort; this._connectionPool = connectionPool; this._driverOnErrorCallback = driverOnErrorCallback; } acquireConnection(mode) { - const connectionPromise = this._connectionPool.acquire(this._address); + const connectionPromise = this._connectionPool.acquire(this._hostPort); return this._withAdditionalOnErrorCallback(connectionPromise, this._driverOnErrorCallback); } } export class LoadBalancer extends ConnectionProvider { - constructor(address, routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback) { + constructor(hostPort, routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback) { super(); - this._seedRouter = address; + this._seedRouter = hostPort; this._routingTable = new RoutingTable([this._seedRouter]); this._rediscovery = new Rediscovery(new RoutingUtil(routingContext)); this._connectionPool = connectionPool; diff --git a/src/v1/internal/connector.js b/src/v1/internal/connector.js index 522a5edf9..82ab4b177 100644 --- a/src/v1/internal/connector.js +++ b/src/v1/internal/connector.js @@ -97,17 +97,17 @@ class Connection { /** * @constructor * @param {NodeChannel|WebSocketChannel} channel - channel with a 'write' function and a 'onmessage' callback property. - * @param {string} url - the hostname and port to connect to. + * @param {string} hostPort - the hostname and port to connect to. * @param {boolean} disableLosslessIntegers if this connection should convert all received integers to native JS numbers. */ - constructor(channel, url, disableLosslessIntegers = false) { + constructor(channel, hostPort, disableLosslessIntegers = false) { /** * An ordered queue of observers, each exchange response (zero or more * RECORD messages followed by a SUCCESS message) we receive will be routed * to the next pending observer. */ - this.url = url; - this.server = {address: url}; + this.hostPort = hostPort; + this.server = {address: hostPort}; this.creationTimestamp = Date.now(); this._disableLosslessIntegers = disableLosslessIntegers; this._pendingObservers = []; @@ -517,18 +517,18 @@ class ConnectionState { } /** - * Crete new connection to the provided url. + * Crete new connection to the provided address. * @access private - * @param {string} url - 'neo4j'-prefixed URL to Neo4j Bolt endpoint + * @param {string} hostPort - the Bolt endpoint to connect to * @param {object} config - this driver configuration * @param {string=null} connectionErrorCode - error code for errors raised on connection errors * @return {Connection} - New connection */ -function connect(url, config = {}, connectionErrorCode = null) { +function connect(hostPort, config = {}, connectionErrorCode = null) { const Ch = config.channel || Channel; - const parsedUrl = urlUtil.parseDatabaseUrl(url); - const channelConfig = new ChannelConfig(parsedUrl, config, connectionErrorCode); - return new Connection(new Ch(channelConfig), parsedUrl.hostAndPort, config.disableLosslessIntegers); + const parsedAddress = urlUtil.parseDatabaseUrl(hostPort); + const channelConfig = new ChannelConfig(parsedAddress, config, connectionErrorCode); + return new Connection(new Ch(channelConfig), parsedAddress.hostAndPort, config.disableLosslessIntegers); } export { diff --git a/src/v1/internal/http/http-driver.js b/src/v1/internal/http/http-driver.js index 3db3c2421..d4c934649 100644 --- a/src/v1/internal/http/http-driver.js +++ b/src/v1/internal/http/http-driver.js @@ -23,13 +23,13 @@ import HttpSessionTracker from './http-session-tracker'; export default class HttpDriver extends Driver { - constructor(url, userAgent, token, config) { - super(url, userAgent, token, config); + constructor(hostPort, userAgent, token, config) { + super(hostPort, userAgent, token, config); this._sessionTracker = new HttpSessionTracker(); } session() { - return new HttpSession(this._url, this._token, this._config, this._sessionTracker); + return new HttpSession(this._hostPort, this._token, this._config, this._sessionTracker); } close() { diff --git a/src/v1/routing-driver.js b/src/v1/routing-driver.js index 6696a7356..e626dc86a 100644 --- a/src/v1/routing-driver.js +++ b/src/v1/routing-driver.js @@ -25,19 +25,19 @@ import LeastConnectedLoadBalancingStrategy, {LEAST_CONNECTED_STRATEGY_NAME} from import RoundRobinLoadBalancingStrategy, {ROUND_ROBIN_STRATEGY_NAME} from './internal/round-robin-load-balancing-strategy'; /** - * A driver that supports routing in a core-edge cluster. + * A driver that supports routing in a causal cluster. * @private */ class RoutingDriver extends Driver { - constructor(url, routingContext, userAgent, token = {}, config = {}) { - super(url, userAgent, token, validateConfig(config)); + constructor(hostPort, routingContext, userAgent, token = {}, config = {}) { + super(hostPort, userAgent, token, validateConfig(config)); this._routingContext = routingContext; } - _createConnectionProvider(address, connectionPool, driverOnErrorCallback) { + _createConnectionProvider(hostPort, connectionPool, driverOnErrorCallback) { const loadBalancingStrategy = RoutingDriver._createLoadBalancingStrategy(this._config, connectionPool); - return new LoadBalancer(address, this._routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback); + return new LoadBalancer(hostPort, this._routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback); } _createSession(mode, connectionProvider, bookmark, config) { @@ -47,14 +47,14 @@ class RoutingDriver extends Driver { return error; } - const url = conn.url; + const hostPort = conn.hostPort; if (error.code === SESSION_EXPIRED || isDatabaseUnavailable(error)) { - this._connectionProvider.forget(url); + this._connectionProvider.forget(hostPort); return error; } else if (isFailureToWrite(error)) { - this._connectionProvider.forgetWriter(url); - return newError('No longer possible to write to server at ' + url, SESSION_EXPIRED); + this._connectionProvider.forgetWriter(hostPort); + return newError('No longer possible to write to server at ' + hostPort, SESSION_EXPIRED); } else { return error; } diff --git a/test/v1/session.test.js b/test/v1/session.test.js index e0339aeff..f0b3542b5 100644 --- a/test/v1/session.test.js +++ b/test/v1/session.test.js @@ -1124,7 +1124,7 @@ describe('session', () => { function idleConnectionCount(driver) { const connectionProvider = driver._connectionProvider; - const address = connectionProvider._address; + const address = connectionProvider._hostPort; const connectionPool = connectionProvider._connectionPool; const idleConnections = connectionPool._pools[address]; return idleConnections.length;