From 960575280b974493e7ab010938616658fab5358a Mon Sep 17 00:00:00 2001
From: matt <matt@fiddaman.net>
Date: Thu, 20 Jan 2022 22:57:17 +0000
Subject: [PATCH] Added validation and sanitisation for sql queries passed into
 runQuery

---
 lib/DatabaseConnectionPool.js           | 50 +++++++++++++++++++------
 lib/DatabaseConnectionPool.test.js      | 45 +++++++++++++++++++---
 lib/__mocks__/DatabaseConnectionPool.js | 46 ++++++++++++++++++-----
 3 files changed, 116 insertions(+), 25 deletions(-)

diff --git a/lib/DatabaseConnectionPool.js b/lib/DatabaseConnectionPool.js
index f83d5a5..e48c048 100644
--- a/lib/DatabaseConnectionPool.js
+++ b/lib/DatabaseConnectionPool.js
@@ -24,6 +24,7 @@ class DatabaseConnectionPool {
 
 	/**
 	 * Create a database connection pool
+	 *
 	 * @param {object} [dbOptions] Optional database params to connect with
 	 */
 	constructor(dbOptions) {
@@ -40,26 +41,53 @@ class DatabaseConnectionPool {
 	}
 
 	/**
-	 * Run a query against the database connection
+	 * Sanitise and validate an sql query
+	 *
 	 * @param {string} sql The query to be executed
 	 * @param {(Array<string|number>)} Values to replace prepared statement
-	 * @return {(Array<object>|object)} Data returned from the database
+	 *
+	 * @return {string} Sanitised and validated sql query
 	 */
-	async runQuery(sql, params) {
+	static validateQuery(sql, params) {
 		sql = sql.trim();
 
 		if (sql.slice(-1) !== ';')
-			throw new Error('Invalid query, needs trailing ;');
+			throw new Error('Query needs trailing ;');
 
-		// Execute as non-prepared if no params are supplied
-		if (typeof params === 'undefined') {
-			const [ data ] =
-				await this.#connectionPool.execute(sql);
-			return data;
+		const expectedParams = sql.split('?').length - 1;
+		const prepared = expectedParams > 0;
+
+		if (!prepared && typeof params !== 'undefined') {
+			throw new Error('Can not pass in parameters ' +
+				'for a non-prepared statement');
+		} else if (prepared && params.length !== expectedParams) {
+			throw new Error('Number of params does not equal ' +
+				'expected number');
+		}
+
+		return sql;
+	}
+
+	/**
+	 * Run a query against the database connection
+	 *
+	 * @param {string} sql The query to be executed
+	 * @param {(Array<string|number>)} Values to replace prepared statement
+	 *
+	 * @return {(Array<object>|object)} Data returned from the database
+	 */
+	async runQuery(sql, params) {
+		sql = this.validateQuery(sql, params);
+		const prepared = sql.includes('?');
+
+		let data;
+		if (!prepared) {
+			[ data ] = await this.#connectionPool.execute(sql);
+		} else {
+			[ data ] =
+				await this.#connectionPool.execute(sql, params);
 		}
 
-		const [ data ] =
-			await this.#connectionPool.execute(sql, params);
 		return data;
 	}
 
diff --git a/lib/DatabaseConnectionPool.test.js b/lib/DatabaseConnectionPool.test.js
index 6e5f023..e732347 100644
--- a/lib/DatabaseConnectionPool.test.js
+++ b/lib/DatabaseConnectionPool.test.js
@@ -47,11 +47,46 @@ describe('DatabaseConnectionPool', () => {
 	test('Query with whitespace after ; should not fail', ()=> {
 		const dbp = new DatabaseConnectionPool();
 
-		const sql = `SELECT * FROM class;`;
-		dbp.runQuery(sql);
+		const sql = `SELECT * FROM class;		`;
 
-		expect(dbp.runQuery.mock.results[0].value).toEqual({
-			sql: sql
-		});
+		expect(() => dbp.runQuery(sql)).not.toThrow();
+	});
+
+	test('Prepared query should fail if no params are given', () => {
+		const dbp = new DatabaseConnectionPool();
+
+		const sql = `SELECT * FROM class where name = ?;`;
+
+		expect(() => dbp.runQuery(sql)).toThrow();
+	});
+
+	test('Prepared query should fail if too many params given', () => {
+		const dbp = new DatabaseConnectionPool();
+
+		const sql = `SELECT * FROM class where name = ?;`;
+		const params = [ 'bob', 'jim' ];
+
+		expect(() => dbp.runQuery(sql, params)).toThrow();
+	});
+
+	test('Prepared query should fail if too few params given', () => {
+		const dbp = new DatabaseConnectionPool();
+
+		const sql = `SELECT * FROM class where name = ?
+			and classId = ? and subjectId = ?;`;
+
+		const params = [ 'bob' ];
+
+		expect(() => dbp.runQuery(sql, params)).toThrow();
+	});
+
+	test('Non-prepared query should fail if params given', () => {
+		const dbp = new DatabaseConnectionPool();
+
+		const sql = `SELECT * FROM class;`;
+
+		const params = [ 'bob', 'jim' ];
+
+		expect(() => dbp.runQuery(sql, params)).toThrow();
 	});
 });
diff --git a/lib/__mocks__/DatabaseConnectionPool.js b/lib/__mocks__/DatabaseConnectionPool.js
index 6d539f3..fdb20e1 100644
--- a/lib/__mocks__/DatabaseConnectionPool.js
+++ b/lib/__mocks__/DatabaseConnectionPool.js
@@ -2,23 +2,51 @@
 
 const DatabaseConnectionPool = require('./DatabaseConnectionPool');
 
-const mockRunQuery = jest.fn((sql, params) => {
+/**
+ * Sanitise and validate an sql query
+ *
+ * @param {string} sql The query to be executed
+ * @param {(Array<string|number>)} Values to replace prepared statement
+ *
+ * @return {string} Sanitised and validated sql query
+ */
+function validateQuery(sql, params) {
 	sql = sql.trim();
 
 	if (sql.slice(-1) !== ';')
-		throw new Error('Invalid query, needs trailing ;');
+		throw new Error('Query needs trailing ;');
 
-	// Execute as non-prepared if no params are supplied
-	if (typeof params === 'undefined') {
-		return {
+	const expectedParams = sql.split('?').length - 1;
+	const prepared = expectedParams > 0;
+
+	if (!prepared && typeof params !== 'undefined') {
+		throw new Error('Can not pass in parameters ' +
+			'for a non-prepared statement');
+	} else if (prepared && params.length !== expectedParams) {
+		throw new Error('Number of params does not equal ' +
+			'expected number');
+	}
+
+	return sql;
+}
+
+const mockRunQuery = jest.fn((sql, params) => {
+	sql = validateQuery(sql, params);
+	const prepared = sql.includes('?');
+
+	let data;
+	if (!prepared) {
+		data = {
 			sql: sql
 		};
+	} else {
+		data = {
+			sql: sql,
+			params: params
+		};
 	}
 
-	return {
-		sql: sql,
-		params: params
-	};
+	return data;
 });
 
 const mockClose = jest.fn();