From c0a0ef01f5aacd2f1ada433493547a17ad4d448c Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 16 Apr 2018 16:55:33 +0200 Subject: [PATCH 1/2] 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. --- src/v1/internal/http/http-session.js | 12 +++---- src/v1/internal/packstream-v1.js | 18 ++++++++--- src/v1/internal/util.js | 38 +++++++++++++++++++--- src/v1/session.js | 12 +++---- src/v1/transaction.js | 10 ++---- test/internal/http/http-session.test.js | 42 +++++++++++++++++++++++++ test/internal/util.test.js | 35 ++++++++++++++++++--- test/v1/session.test.js | 30 ++++++++++++++++++ test/v1/transaction.test.js | 10 ++++++ 9 files changed, 171 insertions(+), 36 deletions(-) create mode 100644 test/internal/http/http-session.test.js diff --git a/src/v1/internal/http/http-session.js b/src/v1/internal/http/http-session.js index 0e455ae6a..dcc5cc288 100644 --- a/src/v1/internal/http/http-session.js +++ b/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); }); }); } diff --git a/src/v1/internal/packstream-v1.js b/src/v1/internal/packstream-v1.js index e773fc26f..d5755ac4e 100644 --- a/src/v1/internal/packstream-v1.js +++ b/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; + } } /** diff --git a/src/v1/internal/util.js b/src/v1/internal/util.js index df8ef88fe..54c41fa01 100644 --- a/src/v1/internal/util.js +++ b/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 } diff --git a/src/v1/session.js b/src/v1/session.js index 894affc15..2a12c1066 100644 --- a/src/v1/session.js +++ b/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) ); } diff --git a/src/v1/transaction.js b/src/v1/transaction.js index ee21f76de..d7551ddbc 100644 --- a/src/v1/transaction.js +++ b/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); } /** diff --git a/test/internal/http/http-session.test.js b/test/internal/http/http-session.test.js new file mode 100644 index 000000000..4ce7276a5 --- /dev/null +++ b/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()); + }); + +}); diff --git a/test/internal/util.test.js b/test/internal/util.test.js index 9f9a1eb9c..e219f0a5e 100644 --- a/test/internal/util.test.js +++ b/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); } }); diff --git a/test/v1/session.test.js b/test/v1/session.test.js index 07e499766..e0339aeff 100644 --- a/test/v1/session.test.js +++ b/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(); + }); + } + }); diff --git a/test/v1/transaction.test.js b/test/v1/transaction.test.js index 67d2a9365..b078f56a8 100644 --- a/test/v1/transaction.test.js +++ b/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'); } From 40f5258130133fc1e36e36e7a1af0dd74fb441fe Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 19 Apr 2018 11:47:34 +0200 Subject: [PATCH 2/2] Simplified query parameters type check --- src/v1/internal/util.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/v1/internal/util.js b/src/v1/internal/util.js index 54c41fa01..960cb8475 100644 --- a/src/v1/internal/util.js +++ b/src/v1/internal/util.js @@ -39,7 +39,7 @@ function isEmptyObjectOrNull(obj) { } function isObject(obj) { - return typeof obj === 'object' && !Array.isArray(obj) && Boolean(obj); + return typeof obj === 'object' && !Array.isArray(obj) && obj !== null; } /** @@ -79,7 +79,7 @@ function assertCypherStatement(obj) { } function assertQueryParameters(obj) { - if (!isObject(obj) && Boolean(obj)) { + if (!isObject(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}`);