Remove password from stringified outputs (#2066)

* Remove password from stringified outputs

Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password.  To widen the pit of success I'm making that field non-enumerable.  You can still get at it...it just wont show up "by accident" when you're logging things now.

The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0.

* Implement feedback

* Fix more whitespace the autoformatter changed

* Simplify code a bit

* Remove password from stringified outputs (#2070)

* Keep ConnectionParameters’s password property writable

`Client` writes to it when `password` is a function.

* Avoid creating password property on pool options

when it didn’t exist previously.

* Allow password option to be non-enumerable

to avoid breaking uses like `new Pool(existingPool.options)`.

* Make password property definitions consistent

in formatting and configurability.

Co-authored-by: Charmander <[email protected]>

Author: brianc

.gitignore

@@ -7,3 +7,4 @@ package-lock.json
 *.swp
 dist
 .DS_Store
+.vscode/

packages/pg-pool/index.js

@@ -60,6 +60,18 @@ class Pool extends EventEmitter {
   constructor (options, Client) {
     super()
     this.options = Object.assign({}, options)
+
+    if (options != null && 'password' in options) {
+      // "hiding" the password so it doesn't show up in stack traces
+      // or if the client is console.logged
+      Object.defineProperty(this.options, 'password', {
+        configurable: true,
+        enumerable: false,
+        writable: true,
+        value: options.password
+      })
+    }
+
     this.options.max = this.options.max || this.options.poolSize || 10
     this.log = this.options.log || function () { }
     this.Client = this.options.Client || Client || require('pg').Client

packages/pg/lib/client.js

@@ -30,7 +30,16 @@ var Client = function (config) {
   this.database = this.connectionParameters.database
   this.port = this.connectionParameters.port
   this.host = this.connectionParameters.host
-  this.password = this.connectionParameters.password
+
+  // "hiding" the password so it doesn't show up in stack traces
+  // or if the client is console.logged
+  Object.defineProperty(this, 'password', {
+    configurable: true,
+    enumerable: false,
+    writable: true,
+    value: this.connectionParameters.password
+  })
+
   this.replication = this.connectionParameters.replication
 
   var c = config || {}

packages/pg/lib/connection-parameters.js

@@ -54,7 +54,16 @@ var ConnectionParameters = function (config) {
   this.database = val('database', config)
   this.port = parseInt(val('port', config), 10)
   this.host = val('host', config)
-  this.password = val('password', config)
+
+  // "hiding" the password so it doesn't show up in stack traces
+  // or if the client is console.logged
+  Object.defineProperty(this, 'password', {
+    configurable: true,
+    enumerable: false,
+    writable: true,
+    value: val('password', config)
+  })
+
   this.binary = val('binary', config)
   this.ssl = typeof config.ssl === 'undefined' ? useSsl() : config.ssl
   this.client_encoding = val('client_encoding', config)

packages/pg/lib/native/client.js

@@ -43,7 +43,15 @@ var Client = module.exports = function (config) {
   // for the time being. TODO: deprecate all this jazz
   var cp = this.connectionParameters = new ConnectionParameters(config)
   this.user = cp.user
-  this.password = cp.password
+
+  // "hiding" the password so it doesn't show up in stack traces
+  // or if the client is console.logged
+  const hiddenPassword = cp.password
+  Object.defineProperty(this, 'password', {
+    enumerable: false,
+    writable: true,
+    value: hiddenPassword
+  })
   this.database = cp.database
   this.host = cp.host
   this.port = cp.port

packages/pg/test/integration/gh-issues/2064-tests.js

@@ -0,0 +1,32 @@
+
+"use strict"
+const helper = require('./../test-helper')
+const assert = require('assert')
+const util = require('util')
+
+const suite = new helper.Suite()
+
+const password = 'FAIL THIS TEST'
+
+suite.test('Password should not exist in toString() output', () => {
+  const pool = new helper.pg.Pool({ password })
+  const client = new helper.pg.Client({ password })
+  assert(pool.toString().indexOf(password) === -1);
+  assert(client.toString().indexOf(password) === -1);
+})
+
+suite.test('Password should not exist in util.inspect output', () => {
+  const pool = new helper.pg.Pool({ password })
+  const client = new helper.pg.Client({ password })
+  const depth = 20;
+  assert(util.inspect(pool, { depth }).indexOf(password) === -1);
+  assert(util.inspect(client, { depth }).indexOf(password) === -1);
+})
+
+suite.test('Password should not exist in json.stringfy output', () => {
+  const pool = new helper.pg.Pool({ password })
+  const client = new helper.pg.Client({ password })
+  const depth = 20;
+  assert(JSON.stringify(pool).indexOf(password) === -1);
+  assert(JSON.stringify(client).indexOf(password) === -1);
+})