Repository: knex/knex (js)

Issue:
DB passwords can be accidentally leaked through logs
**Disclaimer:** I'm reporting this problem here because there are no public policies on how to report security problems. I would have preferred doing this privately.

# Environment

Knex version: 2.4.2
Database + version: MySQL, PostgreSQL, MSSQL (I didn't test with others. DB version does not have any effect)
OS: Mac OS X, Linux

# Bug

1. Explain what kind of behaviour you are getting and how you think it should do:
   ```typescript
   const query = knex('myTable').select('*');
   logger.debug({ query }) // passwords are leaked here
   
   // For a more realistic case:
   try {
     const result = await query
     return result
   } catch (err) {
     logger.error({ err, query }, 'Unknown Error while performing query') // passwords are leaked here
   }
   ```
   The previous code should not leak passwords through the logs, but it does 😢 .
   In our case, the logger was Pino, this problem doesn't necessarily happen with other loggers (as they might use different
   serialisation methods).



3. Error message: There is no error message

4. Reduced test code:
   ```typescript
   // This new test could be placed in `test/integration2/query/misc/additional.spec.js`
   it('should not leak passwords when logged', async function () {
      const util = require('util')
      const query = knex('test_table_two').select('*');
      const fakeLog = util.inspect(query, { depth: null });
      const passwordMatches = fakeLog.match(
        /(knextest|testpassword|S0meVeryHardPassword|testrootpassword)/g
      );

      expect(passwordMatches).to.be.null;
   });
   ```





Pull request:
fix(security): avoid password leaks on query logs
## Motivation:
We detected that under certain conditions, passwords could be leaked through logs, when executing code such as:
```typescript
const query = knex('myTable').select('*');
logger.debug({ query }) // passwords are leaked here
```
In our case, the logger was `Pino`, this problem doesn't necessarily happen with other loggers (as they might use different
serialisation methods).

## Fixes:
- password fields are not leaked anymore. Fixes #5560 .
- accepts configuration objects where the password field exists but it is non-enumerable (this might happen when lib users are trying to prevent the same previous problem, but at a higher level).

## Changes:
- marks the password field as non-enumerable when possible, to avoid potential leaks through logs.
- adapts tests
- add new regression test


Patch:
diff --git a/lib/client.js b/lib/client.js
index 1a8ec2f5..00ee2893 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -31,6 +31,7 @@ const { POOL_CONFIG_OPTIONS } = require('./constants');
 const ViewBuilder = require('./schema/viewbuilder.js');
 const ViewCompiler = require('./schema/viewcompiler.js');
 const isPlainObject = require('lodash/isPlainObject');
+const { setHiddenProperty } = require('./util/security.js');
 
 const debug = require('debug')('knex:client');
 
@@ -43,6 +44,10 @@ class Client extends EventEmitter {
     this.config = config;
     this.logger = new Logger(config);
 
+    if (this.config.connection && this.config.connection.password) {
+      setHiddenProperty(this.config.connection);
+    }
+
     //Client is a required field, so throw error if it's not supplied.
     //If 'this.dialect' is set, then this is a 'super()' call, in which case
     //'client' does not have to be set as it's already assigned on the client prototype.
@@ -69,6 +74,9 @@ class Client extends EventEmitter {
       this.connectionConfigExpirationChecker = () => true; // causes the provider to be called on first use
     } else {
       this.connectionSettings = cloneDeep(config.connection || {});
+      if (config.connection && config.connection.password) {
+        setHiddenProperty(this.connectionSettings, config.connection);
+      }
       this.connectionConfigExpirationChecker = null;
     }
     if (this.driverName && config.connection) {
@@ -305,6 +313,18 @@ class Client extends EventEmitter {
     try {
       const connection = await this.pool.acquire().promise;
       debug('acquired connection from pool: %s', connection.__knexUid);
+      if (connection.config) {
+        if (connection.config.password) {
+          setHiddenProperty(connection.config);
+        }
+        if (
+          connection.config.authentication &&
+          connection.config.authentication.options &&
+          connection.config.authentication.options.password
+        ) {
+          setHiddenProperty(connection.config.authentication.options);
+        }
+      }
       return connection;
     } catch (error) {
       let convertedError = error;
diff --git a/lib/dialects/better-sqlite3/index.js b/lib/dialects/better-sqlite3/index.js
index 18cf1576..85a5ba69 100644
--- a/lib/dialects/better-sqlite3/index.js
+++ b/lib/dialects/better-sqlite3/index.js
@@ -13,6 +13,7 @@ class Client_BetterSQLite3 extends Client_SQLite3 {
 
     return new this.driver(this.connectionSettings.filename, {
       nativeBinding: options.nativeBinding,
+      readonly: !!options.readonly,
     });
   }
 
diff --git a/lib/dialects/mssql/index.js b/lib/dialects/mssql/index.js
index 27654fd8..67fba028 100644
--- a/lib/dialects/mssql/index.js
+++ b/lib/dialects/mssql/index.js
@@ -12,6 +12,7 @@ const TableCompiler = require('./schema/mssql-tablecompiler');
 const ViewCompiler = require('./schema/mssql-viewcompiler');
 const ColumnCompiler = require('./schema/mssql-columncompiler');
 const QueryBuilder = require('../../query/querybuilder');
+const { setHiddenProperty } = require('../../util/security');
 
 const debug = require('debug')('knex:mssql');
 
@@ -66,6 +67,10 @@ class Client_MSSQL extends Client {
       },
     };
 
+    if (cfg.authentication.options.password) {
+      setHiddenProperty(cfg.authentication.options);
+    }
+
     // tedious always connect via tcp when port is specified
     if (cfg.options.instanceName) delete cfg.options.port;
 
diff --git a/lib/dialects/mysql2/index.js b/lib/dialects/mysql2/index.js
index 63bfa4f0..a3c71394 100644
--- a/lib/dialects/mysql2/index.js
+++ b/lib/dialects/mysql2/index.js
@@ -14,6 +14,26 @@ class Client_MySQL2 extends Client_MySQL {
   _driver() {
     return require('mysql2');
   }
+
+  initializeDriver() {
+    try {
+      this.driver = this._driver();
+    } catch (e) {
+      let message = `Knex: run\n$ npm install ${this.driverName}`;
+
+      const nodeMajorVersion = process.version.replace(/^v/, '').split('.')[0];
+      if (nodeMajorVersion <= 12) {
+        message += `@3.2.0`;
+        this.logger.error(
+          'Mysql2 version 3.2.0 is the latest version to support Node.js 12 or lower.'
+        );
+      }
+      message += ` --save`;
+      this.logger.error(`${message}\n${e.message}\n${e.stack}`);
+      throw new Error(`${message}\n${e.message}`);
+    }
+  }
+
   validateConnection(connection) {
     return (
       connection &&
diff --git a/lib/execution/runner.js b/lib/execution/runner.js
index 8bb23216..ed016529 100644
--- a/lib/execution/runner.js
+++ b/lib/execution/runner.js
@@ -72,6 +72,24 @@ class Runner {
       this.client.releaseConnection(this.connection);
     });
 
+    // If the stream is manually destroyed, the close event is not
+    // propagated to the top of the pipe chain. We need to manually verify
+    // that the source stream is closed and if not, manually destroy it.
+    stream.on('pipe', (sourceStream) => {
+      const cleanSourceStream = () => {
+        if (!sourceStream.closed) {
+          sourceStream.destroy();
+        }
+      };
+
+      // Stream already closed, cleanup immediately
+      if (stream.closed) {
+        cleanSourceStream();
+      } else {
+        stream.on('close', cleanSourceStream);
+      }
+    });
+
     const connectionAcquirePromise = this.ensureConnection(
       ensureConnectionStreamCallback,
       {
diff --git a/lib/knex-builder/make-knex.js b/lib/knex-builder/make-knex.js
index 55f9acd3..7345708c 100644
--- a/lib/knex-builder/make-knex.js
+++ b/lib/knex-builder/make-knex.js
@@ -7,6 +7,7 @@ const QueryInterface = require('../query/method-constants');
 const merge = require('lodash/merge');
 const batchInsert = require('../execution/batch-insert');
 const { isObject } = require('../util/is');
+const { setHiddenProperty } = require('../util/security');
 
 // Javascript does not officially support "callable objects".  Instead,
 // you must create a regular Function and inject properties/methods
@@ -208,6 +209,10 @@ function initContext(knexFn) {
         knexClone.client = Object.create(this.client.constructor.prototype); // Clone client to avoid leaking listeners that are set on it
         merge(knexClone.client, this.client);
         knexClone.client.config = Object.assign({}, this.client.config); // Clone client config to make sure they can be modified independently
+
+        if (this.client.config.password) {
+          setHiddenProperty(knexClone.client.config, this.client.config);
+        }
       }
 
       redefineProperties(knexClone, knexClone.client);
diff --git a/lib/util/security.js b/lib/util/security.js
new file mode 100644
index 00000000..d4b4edb9
--- /dev/null
+++ b/lib/util/security.js
@@ -0,0 +1,26 @@
+/**
+ * Sets a hidden (non-enumerable) property on the `target` object, copying it
+ * from `source`.
+ *
+ * This is useful when we want to protect certain data from being accidentally
+ * leaked through logs, also when the property is non-enumerable on the `source`
+ * object and we want to ensure that it is properly copied.
+ *
+ * @param {object} target
+ * @param {object} source - default: target
+ * @param {string} propertyName - default: 'password'
+ */
+function setHiddenProperty(target, source, propertyName = 'password') {
+  if (!source) {
+    source = target;
+  }
+
+  Object.defineProperty(target, propertyName, {
+    enumerable: false,
+    value: source[propertyName],
+  });
+}
+
+module.exports = {
+  setHiddenProperty,
+};
diff --git a/types/index.d.ts b/types/index.d.ts
index 5281d0ea..d17d1122 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -2647,6 +2647,7 @@ export declare namespace Knex {
       constraintName?: string
     ): ColumnBuilder;
     checkRegex(regex: string, constraintName?: string): ColumnBuilder;
+    collate(collation: string): ColumnBuilder;
   }
 
   interface ForeignConstraintBuilder {
@@ -3076,6 +3077,7 @@ export declare namespace Knex {
     filename: string;
     options?: {
       nativeBinding?: string;
+      readonly?: boolean;
     };
   }
 


Test patch:
diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml
index dd744034..a57dfd69 100644
--- a/.github/workflows/unit-tests.yml
+++ b/.github/workflows/unit-tests.yml
@@ -37,6 +37,10 @@ jobs:
       - name: Run npm install
         run: npm install
 
+      - name: Run npm install mysql2@3.2.0 if Node 12.x
+        run: npm install mysql2@3.2.0
+        if: matrix.node-version == '12.x'
+
       - run: npm run build
 
       - name: Run Tests
diff --git a/test/db-less-test-suite.js b/test/db-less-test-suite.js
index 8cac5955..647d83af 100644
--- a/test/db-less-test-suite.js
+++ b/test/db-less-test-suite.js
@@ -10,6 +10,7 @@ describe('Util Tests', function () {
   require('./unit/util/nanoid');
   require('./unit/util/save-async-stack');
   require('./unit/util/comma-no-paren-regex');
+  require('./unit/util/security');
 });
 
 describe('Query Building Tests', function () {
@@ -50,6 +51,10 @@ if (config.mysql) {
   require('./unit/dialects/mysql');
 }
 
+if (config['better-sqlite3']) {
+  require('./unit/dialects/better-sqlite3');
+}
+
 describe('CLI tests', function () {
   this.timeout(process.env.KNEX_TEST_TIMEOUT || 5000);
   require('./cli/help.spec');
diff --git a/test/integration/connection-config-provider.js b/test/integration/connection-config-provider.js
index 37006e90..0725fe13 100644
--- a/test/integration/connection-config-provider.js
+++ b/test/integration/connection-config-provider.js
@@ -4,6 +4,7 @@ const { expect } = require('chai');
 
 const _ = require('lodash');
 const makeKnex = require('../../knex');
+const { setHiddenProperty } = require('../../lib/util/security');
 
 module.exports = function (config) {
   describe('Connection configuration provider', function () {
@@ -13,6 +14,9 @@ module.exports = function (config) {
 
     this.beforeEach(() => {
       configWorkingCopy = _.cloneDeep(config);
+      if (config.connection && config.connection.password) {
+        setHiddenProperty(configWorkingCopy.connection, config.connection);
+      }
       configWorkingCopy.pool.min = 1;
       configWorkingCopy.pool.max = 2;
       providerInvocationCount = 0;
diff --git a/test/integration2/query/misc/additional.spec.js b/test/integration2/query/misc/additional.spec.js
index 52002b62..f05d4d2a 100644
--- a/test/integration2/query/misc/additional.spec.js
+++ b/test/integration2/query/misc/additional.spec.js
@@ -1,6 +1,7 @@
 /*eslint no-var:0, max-len:0 */
 'use strict';
 
+const util = require('util');
 const chai = require('chai');
 chai.use(require('chai-as-promised'));
 chai.use(require('sinon-chai'));
@@ -34,6 +35,7 @@ const {
 const logger = require('../../../integration/logger');
 const { insertAccounts } = require('../../../util/dataInsertHelper');
 const sinon = require('sinon');
+const { setHiddenProperty } = require('../../../../lib/util/security');
 
 describe('Additional', function () {
   getAllDbs().forEach((db) => {
@@ -128,7 +130,7 @@ describe('Additional', function () {
           });
         });
 
-        it('should close the connection when a stream query iteration errors', async function () {
+        it('should release the connection when a stream query iteration errors', async function () {
           const spy = sinon.spy(knex.client, 'releaseConnection');
 
           const stream = knex.raw('VALUES (1), (2), (3)').stream();
@@ -146,6 +148,26 @@ describe('Additional', function () {
           spy.restore();
         });
 
+        it('should close the db connection when prematurely closing a stream', async function () {
+          await knex('accounts').truncate();
+          await insertAccounts(knex, 'accounts');
+
+          // We limit to only one row at a time, to keep the cursor open
+          const stream = knex('accounts').stream({ highWaterMark: 1 });
+
+          // eslint-disable-next-line no-unused-vars
+          for await (const _ of stream) {
+            stream.destroy();
+            break;
+          }
+
+          await new Promise((res) => setTimeout(res, 50));
+
+          // Will timeout if the connection is in the pool, but not closed
+          this.timeout(1000);
+          await knex('accounts').limit(1);
+        });
+
         it('should process response done through a stream', async () => {
           await knex('accounts').truncate();
           await insertAccounts(knex, 'accounts');
@@ -986,6 +1008,16 @@ describe('Additional', function () {
           // To make this test easier, I'm changing the pool settings to max 1.
           // Also setting acquireTimeoutMillis to lower as not to wait the default time
           const knexConfig = _.cloneDeep(knex.client.config);
+          if (
+            knex.client.config.connection &&
+            knex.client.config.connection.password
+          ) {
+            setHiddenProperty(
+              knexConfig.connection,
+              knex.client.config.connection
+            );
+          }
+
           knexConfig.pool.min = 0;
           knexConfig.pool.max = 1;
           knexConfig.pool.acquireTimeoutMillis = 100;
@@ -1103,6 +1135,16 @@ describe('Additional', function () {
           // To make this test easier, I'm changing the pool settings to max 1.
           // Also setting acquireTimeoutMillis to lower as not to wait the default time
           const knexConfig = _.cloneDeep(knex.client.config);
+          if (
+            knex.client.config.connection &&
+            knex.client.config.connection.password
+          ) {
+            setHiddenProperty(
+              knexConfig.connection,
+              knex.client.config.connection
+            );
+          }
+
           knexConfig.pool.min = 0;
           knexConfig.pool.max = 2;
           knexConfig.pool.acquireTimeoutMillis = 100;
@@ -1400,6 +1442,17 @@ describe('Additional', function () {
             expect(knexDb.client.pool.destroyed).to.equal(false);
           });
         });
+
+        it('should not leak passwords when logged', async function () {
+          const query = knex('test_table_two').select('*');
+          const fakeLog = util.inspect(query, { depth: null });
+
+          // These passwords come from `scripts/docker-compose.yml`
+          const passwordMatches = fakeLog.match(
+            /(knextest|testpassword|S0meVeryHardPassword|testrootpassword)/g
+          );
+          expect(passwordMatches).to.be.null;
+        });
       });
     });
   });
diff --git a/test/unit/dialects/better-sqlite3.js b/test/unit/dialects/better-sqlite3.js
index dbf3dfe2..4b690cb9 100644
--- a/test/unit/dialects/better-sqlite3.js
+++ b/test/unit/dialects/better-sqlite3.js
@@ -1,6 +1,7 @@
 'use strict';
 const path = require('path');
 const expect = require('chai').expect;
+const fs = require('fs');
 const knex = require('../../../knex');
 
 describe('better-sqlite3 unit tests', () => {
@@ -12,12 +13,18 @@ describe('better-sqlite3 unit tests', () => {
         connection: {
           filename: ':memory:',
           options: {
-            nativeBinding: path.resolve(__dirname, '../../../node_modules/better-sqlite3/build/Release/better_sqlite3.node'),
+            nativeBinding: path.resolve(
+              __dirname,
+              '../../../node_modules/better-sqlite3/build/Release/better_sqlite3.node'
+            ),
           },
         },
       });
 
-      const result = await knexInstance.select(knexInstance.raw('2 + 2 as answer')).first().catch(err => err);
+      const result = await knexInstance
+        .select(knexInstance.raw('2 + 2 as answer'))
+        .first()
+        .catch((err) => err);
       expect(result.answer).to.equal(4);
     });
 
@@ -33,10 +40,68 @@ describe('better-sqlite3 unit tests', () => {
         },
       });
 
-      const result = await knexInstance.select(knexInstance.raw('2 + 2 as answer')).first().catch(err => err);
+      const result = await knexInstance
+        .select(knexInstance.raw('2 + 2 as answer'))
+        .first()
+        .catch((err) => err);
       console.dir(result);
       expect(result).to.be.an('error');
       expect(result.code).to.equal('MODULE_NOT_FOUND');
     });
   });
+
+  describe('readonly', () => {
+    const dbPath = path.resolve(__dirname, '../test.sqlite3');
+
+    before(() => {
+      // Read-only access requires the DB file to exist.
+      fs.writeFileSync(dbPath, '');
+    });
+
+    it('should initialize the DB with the passed-in `readonly` option', async () => {
+      const knexInstance = knex({
+        client: 'better-sqlite3',
+        useNullAsDefault: true,
+        connection: {
+          filename: dbPath,
+          options: {
+            readonly: true,
+          },
+        },
+      });
+
+      const connection = await knexInstance.client.acquireConnection();
+      expect(connection.readonly).to.equal(true);
+    });
+
+    it('should prevent writing to the DB', async () => {
+      const knexInstance = knex({
+        client: 'better-sqlite3',
+        useNullAsDefault: true,
+        connection: {
+          filename: dbPath,
+          options: {
+            readonly: true,
+          },
+        },
+      });
+
+      await expect(
+        knexInstance.raw('create table shouldFail (x integer)')
+      ).to.eventually.be.rejectedWith('attempt to write a readonly database');
+    });
+
+    it('should fall back on `readonly` = `false`', async () => {
+      const knexInstance = knex({
+        client: 'better-sqlite3',
+        useNullAsDefault: true,
+        connection: {
+          filename: dbPath,
+        },
+      });
+
+      const connection = await knexInstance.client.acquireConnection();
+      expect(connection.readonly).to.equal(false);
+    });
+  });
 });
diff --git a/test/unit/util/security.js b/test/unit/util/security.js
new file mode 100644
index 00000000..c2846b12
--- /dev/null
+++ b/test/unit/util/security.js
@@ -0,0 +1,39 @@
+const { expect } = require('chai');
+const { setHiddenProperty } = require('../../../lib/util/security');
+
+describe('setHiddenProperty', () => {
+  it('hides password property when only target object is passed', () => {
+    const target = { password: 'xyz' };
+    expect(Object.keys(target)).to.contain('password'); // sanity check
+
+    setHiddenProperty(target);
+    expect(Object.keys(target)).to.not.contain('password');
+
+    // Although not enumerable, it is still present
+    expect(target).to.have.property('password');
+    expect(target.password).to.equal('xyz');
+  });
+
+  it('sets property from source even when it is not enumerable', () => {
+    const source = Object.create(
+      {},
+      {
+        password: {
+          enumerable: false,
+          value: 'xyz',
+        },
+      }
+    );
+
+    expect(Object.keys(source)).to.not.contain('password'); // sanity check
+    expect(source).to.have.property('password'); // sanity check
+    expect(source.password).to.equal('xyz'); // sanity check
+
+    const target = {};
+    setHiddenProperty(target, source);
+
+    expect(Object.keys(target)).to.not.contain('password');
+    expect(target).to.have.property('password');
+    expect(target.password).to.equal('xyz');
+  });
+});