Fix MSSQL escaping (#3382)

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Master (Unreleased)
 
+### Bug fixes:
+
+- MSSQL: Escape column ids correctly in all cases (reported by Snyk Security Research Team) 
+
 # 0.19.4 - 09 September, 2019
 
 ### New features:

lib/dialects/mssql/index.js

@@ -207,7 +207,11 @@ Object.assign(Client_MSSQL.prototype, {
   },
 
   wrapIdentifierImpl(value) {
-    return value !== '*' ? `[${value.replace(/\[/g, '[')}]` : '*';
+    if (value === '*') {
+      return '*';
+    }
+
+    return `[${value.replace(/[[\]']+/g, '')}]`;
   },
 
   // Get a raw connection, called by the `pool` whenever a new

lib/helpers.js

@@ -65,7 +65,7 @@ function getUndefinedIndices(mixed) {
       if (containsUndefined(mixed[key])) {
         indices.push(key);
       }
-    })
+    });
   } else {
     indices.push(0);
   }

lib/query/compiler.js

@@ -100,7 +100,9 @@ assign(QueryCompiler.prototype, {
       debugBindings(query.bindings);
       throw new Error(
         `Undefined binding(s) detected when compiling ` +
-          `${method.toUpperCase()}. Undefined column(s): [${this.undefinedBindingsInfo.join(', ')}] query: ${query.sql}`
+          `${method.toUpperCase()}. Undefined column(s): [${this.undefinedBindingsInfo.join(
+            ', '
+          )}] query: ${query.sql}`
       );
     }
 
@@ -373,7 +375,7 @@ assign(QueryCompiler.prototype, {
         Object.prototype.hasOwnProperty.call(stmt, 'value') &&
         helpers.containsUndefined(stmt.value)
       ) {
-        this.undefinedBindingsInfo.push(stmt.column)
+        this.undefinedBindingsInfo.push(stmt.column);
         this._undefinedInWhereClause = true;
       }
       const val = this[stmt.type](stmt);

lib/raw.js

@@ -104,7 +104,9 @@ assign(Raw.prototype, {
 
     obj.bindings = obj.bindings || [];
     if (helpers.containsUndefined(obj.bindings)) {
-      const undefinedBindingIndices = helpers.getUndefinedIndices(this.bindings);
+      const undefinedBindingIndices = helpers.getUndefinedIndices(
+        this.bindings
+      );
       debugBindings(obj.bindings);
       throw new Error(
         `Undefined binding(s) detected for keys [${undefinedBindingIndices}] when compiling RAW query: ${obj.sql}`

lib/transaction.js

@@ -221,10 +221,12 @@ class Transaction extends EventEmitter {
       .then(function(connection) {
         connection.__knexTxId = txid;
 
-        return (trx._previousSibling ? trx._previousSibling.reflect() : Promise.resolve())
-          .then(function () {
-            return connection;
-          });
+        return (trx._previousSibling
+          ? trx._previousSibling.reflect()
+          : Promise.resolve()
+        ).then(function() {
+          return connection;
+        });
       })
       .disposer(function(connection) {
         if (!configConnection) {

test/unit/dialects/mssql.js

@@ -0,0 +1,37 @@
+// global it, describe
+
+'use strict';
+const expect = require('chai').expect;
+const knex = require('../../../knex');
+
+describe('MSSQL unit tests', () => {
+  const knexInstance = knex({
+    client: 'mssql',
+    connection: {
+      port: 1433,
+      host: '127.0.0.1',
+      password: 'yourStrong(!)Password',
+      user: 'sa',
+    },
+  });
+
+  it('should escape statements with [] correctly', async () => {
+    const sql = knexInstance('projects')
+      .where('id] = 1 UNION SELECT 1, @@version -- --', 1)
+      .toSQL();
+    console.log(sql);
+    expect(sql.sql).to.equal(
+      'select * from [projects] where [id = 1 UNION SELECT 1, @@version -- --] = ?'
+    );
+  });
+
+  it('should escape statements with "" correctly', async () => {
+    const sql = knexInstance('projects')
+      .where('id]" = 1 UNION SELECT 1, @@version -- --', 1)
+      .toSQL();
+    console.log(sql);
+    expect(sql.sql).to.equal(
+      'select * from [projects] where [id" = 1 UNION SELECT 1, @@version -- --] = ?'
+    );
+  });
+});

test/unit/dialects/oracledb.js

@@ -1,4 +1,4 @@
-// global it, describe, expect
+// global it, describe
 
 'use strict';
 const _ = require('lodash');

test/unit/dialects/postgres.js

@@ -51,6 +51,29 @@ describe('Postgres Unit Tests', function() {
     });
   });
 
+  it('escape statements correctly', async () => {
+    const knexInstance = knex({
+      client: 'postgresql',
+      version: '10.5',
+      connection: {
+        pool: {},
+      },
+    });
+    const sql = knexInstance('projects')
+      .where('id = 1 UNION SELECT 1, version();', 1)
+      .toSQL();
+    expect(sql.sql).to.equal(
+      'select * from "projects" where "id = 1 UNION SELECT 1, version();" = ?'
+    );
+
+    const sql2 = knexInstance('projects')
+      .where('id = 1" UNION SELECT 1, version();', 1)
+      .toSQL();
+    expect(sql2.sql).to.equal(
+      'select * from "projects" where "id = 1"" UNION SELECT 1, version();" = ?'
+    );
+  });
+
   it('resolve client version if not specified explicitly', (done) => {
     const knexInstance = knex({
       client: 'postgresql',