More validation for query parameters

Driver expects query parameters to either be undefined/null or an
object. This was previously not enforced and illegal parameters,
like strings or arrays, were sent to the database. This resulted
in a protocol violation and database closed the connection. Users
were only able to see the actual error/stacktrace in the database
logs. Driver received a `ServiceUnavailable` or `SessionExpired`
error.

This commit adds validation of query parameters. It also prohibits
nodes, relationships and paths from being used as query parameters
in the driver.

Author: lutovich

src/v1/internal/http/http-session.js

@@ -19,7 +19,7 @@
 
 import {WRITE} from '../../driver';
 import Session from '../../session';
-import {assertCypherStatement} from '../util';
+import {validateStatementAndParameters} from '../util';
 import {Neo4jError} from '../../error';
 import HttpRequestRunner from './http-request-runner';
 import {EMPTY_CONNECTION_HOLDER} from '../connection-holder';
@@ -37,15 +37,11 @@ export default class HttpSession extends Session {
   }
 
   run(statement, parameters = {}) {
-    if (typeof statement === 'object' && statement.text) {
-      parameters = statement.parameters || {};
-      statement = statement.text;
-    }
-    assertCypherStatement(statement);
+    const {query, params} = validateStatementAndParameters(statement, parameters);
 
     return this._requestRunner.beginTransaction().then(transactionId => {
       this._ongoingTransactionIds.push(transactionId);
-      const queryPromise = this._requestRunner.runQuery(transactionId, statement, parameters);
+      const queryPromise = this._requestRunner.runQuery(transactionId, query, params);
 
       return queryPromise.then(streamObserver => {
         if (streamObserver.hasFailed()) {
@@ -55,7 +51,7 @@ export default class HttpSession extends Session {
         }
       }).then(streamObserver => {
         this._ongoingTransactionIds = this._ongoingTransactionIds.filter(id => id !== transactionId);
-        return new Result(streamObserver, statement, parameters, this._serverInfoSupplier, EMPTY_CONNECTION_HOLDER);
+        return new Result(streamObserver, query, params, this._serverInfoSupplier, EMPTY_CONNECTION_HOLDER);
       });
     });
   }

src/v1/internal/packstream-v1.js

@@ -129,6 +129,12 @@ class Packer {
       }
     } else if (isIterable(x)) {
       return this.packableIterable(x, onError);
+    } else if (x instanceof Node) {
+      return this._nonPackableValue(`It is not allowed to pass nodes in query parameters, given: ${x}`, onError);
+    } else if (x instanceof Relationship) {
+      return this._nonPackableValue(`It is not allowed to pass relationships in query parameters, given: ${x}`, onError);
+    } else if (x instanceof Path) {
+      return this._nonPackableValue(`It is not allowed to pass paths in query parameters, given: ${x}`, onError);
     } else if (x instanceof Structure) {
       var packableFields = [];
       for (var i = 0; i < x.fields.length; i++) {
@@ -155,10 +161,7 @@ class Packer {
         }
       };
     } else {
-      if (onError) {
-        onError(newError("Cannot pack this value: " + x));
-      }
-      return () => undefined;
+      return this._nonPackableValue(`Unable to pack the given value: ${x}`, onError);
     }
   }
 
@@ -334,6 +337,13 @@ class Packer {
   disableByteArrays() {
     this._byteArraysSupported = false;
   }
+
+  _nonPackableValue(message, onError) {
+    if (onError) {
+      onError(newError(message, PROTOCOL_ERROR));
+    }
+    return () => undefined;
+  }
 }
 
 /**

src/v1/internal/util.js

@@ -39,8 +39,29 @@ function isEmptyObjectOrNull(obj) {
 }
 
 function isObject(obj) {
-  const type = typeof obj;
-  return type === 'function' || type === 'object' && Boolean(obj);
+  return typeof obj === 'object' && !Array.isArray(obj) && Boolean(obj);
+}
+
+/**
+ * Check and normalize given statement and parameters.
+ * @param {string|{text: string, parameters: object}} statement the statement to check.
+ * @param {object} parameters
+ * @return {{query: string, params: object}} the normalized query with parameters.
+ * @throws TypeError when either given query or parameters are invalid.
+ */
+function validateStatementAndParameters(statement, parameters) {
+  let query = statement;
+  let params = parameters || {};
+
+  if (typeof statement === 'object' && statement.text) {
+    query = statement.text;
+    params = statement.parameters || {};
+  }
+
+  assertCypherStatement(query);
+  assertQueryParameters(params);
+
+  return {query, params};
 }
 
 function assertString(obj, objName) {
@@ -52,10 +73,17 @@ function assertString(obj, objName) {
 
 function assertCypherStatement(obj) {
   assertString(obj, 'Cypher statement');
-  if (obj.trim().length == 0) {
+  if (obj.trim().length === 0) {
     throw new TypeError('Cypher statement is expected to be a non-empty string.');
   }
-  return obj;
+}
+
+function assertQueryParameters(obj) {
+  if (!isObject(obj) && Boolean(obj)) {
+    // objects created with `Object.create(null)` do not have a constructor property
+    const constructor = obj.constructor ? ' ' + obj.constructor.name : '';
+    throw new TypeError(`Query parameters are expected to either be undefined/null or an object, given:${constructor} ${obj}`);
+  }
 }
 
 function isString(str) {
@@ -66,7 +94,7 @@ export {
   isEmptyObjectOrNull,
   isString,
   assertString,
-  assertCypherStatement,
+  validateStatementAndParameters,
   ENCRYPTION_ON,
   ENCRYPTION_OFF
 }

src/v1/session.js

@@ -20,7 +20,7 @@ import StreamObserver from './internal/stream-observer';
 import Result from './result';
 import Transaction from './transaction';
 import {newError} from './error';
-import {assertCypherStatement} from './internal/util';
+import {validateStatementAndParameters} from './internal/util';
 import ConnectionHolder from './internal/connection-holder';
 import Driver, {READ, WRITE} from './driver';
 import TransactionExecutor from './internal/transaction-executor';
@@ -60,14 +60,10 @@ class Session {
    * @return {Result} - New Result
    */
   run(statement, parameters = {}) {
-    if (typeof statement === 'object' && statement.text) {
-      parameters = statement.parameters || {};
-      statement = statement.text;
-    }
-    assertCypherStatement(statement);
+    const {query, params} = validateStatementAndParameters(statement, parameters);
 
-    return this._run(statement, parameters, (connection, streamObserver) =>
-      connection.run(statement, parameters, streamObserver)
+    return this._run(query, params, (connection, streamObserver) =>
+      connection.run(query, params, streamObserver)
     );
   }
 

src/v1/transaction.js

@@ -18,7 +18,7 @@
  */
 import StreamObserver from './internal/stream-observer';
 import Result from './result';
-import {assertCypherStatement} from './internal/util';
+import {validateStatementAndParameters} from './internal/util';
 import {EMPTY_CONNECTION_HOLDER} from './internal/connection-holder';
 import Bookmark from './internal/bookmark';
 
@@ -60,13 +60,9 @@ class Transaction {
    * @return {Result} New Result
    */
   run(statement, parameters) {
-    if(typeof statement === 'object' && statement.text) {
-      parameters = statement.parameters || {};
-      statement = statement.text;
-    }
-    assertCypherStatement(statement);
+    const {query, params} = validateStatementAndParameters(statement, parameters);
 
-    return this._state.run(this._connectionHolder,  new _TransactionStreamObserver(this), statement, parameters);
+    return this._state.run(this._connectionHolder, new _TransactionStreamObserver(this), query, params);
   }
 
   /**

test/internal/http/http-session.test.js

@@ -0,0 +1,42 @@
+/**
+ * Copyright (c) 2002-2018 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import sharedNeo4j from '../../internal/shared-neo4j';
+import urlUtil from '../../../src/v1/internal/url-util';
+import testUtil from '../test-utils';
+import HttpSession from '../../../src/v1/internal/http/http-session';
+import HttpSessionTracker from '../../../src/v1/internal/http/http-session-tracker';
+
+describe('http session', () => {
+
+  it('should fail for invalid query parameters', done => {
+    if (testUtil.isServer()) {
+      done();
+      return;
+    }
+
+    const session = new HttpSession(urlUtil.parseDatabaseUrl('http://localhost:7474'), sharedNeo4j.authToken, {}, new HttpSessionTracker());
+
+    expect(() => session.run('RETURN $value', [1, 2, 3])).toThrowError(TypeError);
+    expect(() => session.run('RETURN $value', '123')).toThrowError(TypeError);
+    expect(() => session.run('RETURN $value', () => [123])).toThrowError(TypeError);
+
+    session.close(() => done());
+  });
+
+});

test/internal/util.test.js

@@ -24,13 +24,12 @@ describe('util', () => {
   it('should check empty objects', () => {
     expect(util.isEmptyObjectOrNull(null)).toBeTruthy();
     expect(util.isEmptyObjectOrNull({})).toBeTruthy();
-    expect(util.isEmptyObjectOrNull([])).toBeTruthy();
+
+    expect(util.isEmptyObjectOrNull([])).toBeFalsy();
 
     const func = () => {
       return 42;
     };
-    expect(util.isEmptyObjectOrNull(func)).toBeTruthy();
-    func.foo = 'bar';
     expect(util.isEmptyObjectOrNull(func)).toBeFalsy();
 
     expect(util.isEmptyObjectOrNull()).toBeFalsy();
@@ -74,6 +73,26 @@ describe('util', () => {
     verifyInvalidCypherStatement(console.log);
   });
 
+  it('should check valid query parameters', () => {
+    verifyValidQueryParameters(null);
+    verifyValidQueryParameters(undefined);
+    verifyValidQueryParameters({});
+    verifyValidQueryParameters({a: 1, b: 2, c: 3});
+    verifyValidQueryParameters({foo: 'bar', baz: 'qux'});
+  });
+
+  it('should check invalid query parameters', () => {
+    verifyInvalidQueryParameters('parameter');
+    verifyInvalidQueryParameters(123);
+    verifyInvalidQueryParameters([]);
+    verifyInvalidQueryParameters([1, 2, 3]);
+    verifyInvalidQueryParameters([null]);
+    verifyInvalidQueryParameters(['1', '2', '3']);
+    verifyInvalidQueryParameters(() => [1, 2, 3]);
+    verifyInvalidQueryParameters(() => '');
+    verifyInvalidQueryParameters(() => null);
+  });
+
   function verifyValidString(str) {
     expect(util.assertString(str, 'Test string')).toBe(str);
   }
@@ -83,7 +102,15 @@ describe('util', () => {
   }
 
   function verifyInvalidCypherStatement(str) {
-    expect(() => util.assertCypherStatement(str)).toThrowError(TypeError);
+    expect(() => util.validateStatementAndParameters(str, {})).toThrowError(TypeError);
+  }
+
+  function verifyValidQueryParameters(obj) {
+    expect(() => util.validateStatementAndParameters('RETURN 1', obj)).not.toThrow();
+  }
+
+  function verifyInvalidQueryParameters(obj) {
+    expect(() => util.validateStatementAndParameters('RETURN 1', obj)).toThrowError(TypeError);
   }
 
 });

test/v1/session.test.js

@@ -1072,6 +1072,26 @@ describe('session', () => {
     });
   });
 
+  it('should fail for invalid query parameters', () => {
+    expect(() => session.run('RETURN $value', '42')).toThrowError(TypeError);
+    expect(() => session.run('RETURN $value', 42)).toThrowError(TypeError);
+    expect(() => session.run('RETURN $value', () => 42)).toThrowError(TypeError);
+  });
+
+  it('should fail to pass node as a query parameter', done => {
+    testUnsupportedQueryParameter(new neo4j.types.Node(neo4j.int(1), ['Person'], {name: 'Bob'}), done);
+  });
+
+  it('should fail to pass relationship as a query parameter', done => {
+    testUnsupportedQueryParameter(new neo4j.types.Relationship(neo4j.int(1), neo4j.int(2), neo4j.int(3), 'KNOWS', {since: 42}), done);
+  });
+
+  it('should fail to pass path as a query parameter', done => {
+    const node1 = new neo4j.types.Node(neo4j.int(1), ['Person'], {name: 'Alice'});
+    const node2 = new neo4j.types.Node(neo4j.int(2), ['Person'], {name: 'Bob'});
+    testUnsupportedQueryParameter(new neo4j.types.Path(node1, node2, []), done);
+  });
+
   function serverIs31OrLater(done) {
     if (serverVersion.compareTo(VERSION_3_1_0) < 0) {
       done();
@@ -1176,4 +1196,14 @@ describe('session', () => {
     });
   }
 
+  function testUnsupportedQueryParameter(value, done) {
+    session.run('RETURN $value', {value: value}).then(() => {
+      done.fail(`Should not be possible to send ${value.constructor.name} ${value} as a query parameter`);
+    }).catch(error => {
+      expect(error.name).toEqual('Neo4jError');
+      expect(error.code).toEqual(neo4j.error.PROTOCOL_ERROR);
+      done();
+    });
+  }
+
 });

test/v1/transaction.test.js

@@ -509,6 +509,16 @@ describe('transaction', () => {
     testConnectionTimeout(true, done);
   });
 
+  it('should fail for invalid query parameters', done => {
+    const tx = session.beginTransaction();
+
+    expect(() => tx.run('RETURN $value', 'Hello')).toThrowError(TypeError);
+    expect(() => tx.run('RETURN $value', 12345)).toThrowError(TypeError);
+    expect(() => tx.run('RETURN $value', () => 'Hello')).toThrowError(TypeError);
+
+    tx.rollback().then(() => done());
+  });
+
   function expectSyntaxError(error) {
     expect(error.code).toBe('Neo.ClientError.Statement.SyntaxError');
   }