sails-postgresql prints database credentials to stderr on error

[alxndrsn]

Sails version: 1.1.0
Node version: v10.15.1
NPM version: 6.4.1
DB adapter name: sails-postgresql
DB adapter version: 1.0.2
Operating system: OSX

---

Observed

When sails-postgresql encounters an Error, it prints the database credentials to stderr.

Expected

Database password should not be printed directly to logs.

Recreate

Steps to recreate

1. git clone [email protected]:alxndrsn/sails-postgres-credentials-leak.git && cd sails-postgres-credentials-leak && npm install && DB_PASSWORD=highly_secret NODE_ENV=production sails lift
2. in a separate terminal: curl http://localhost:1337/thingy

This will cause the database to timeout, and output similar to the following:

$ DB_PASSWORD=highly_secret NODE_ENV=production sails lift

 info: Starting app...

debug: Warning: since `sails.config.session.cookie.secure` is not set to `true`, the session
debug: cookie will be sent over non-TLS connections (i.e. with insecure http:// requests).
debug: To enable secure cookies, set `sails.config.session.cookie.secure` to `true`.
debug:
debug: If your app is behind a proxy or load balancer (e.g. on a PaaS like Heroku), you
debug: may also need to set `sails.config.http.trustProxy` to `true`.
debug:
debug: For more help:
debug:  • https://sailsjs.com/config/session#?the-secure-flag
debug:  • https://sailsjs.com/config/session#?do-i-need-an-ssl-certificate
debug:  • https://sailsjs.com/config/sails-config-http#?properties
debug:  • https://sailsjs.com/support
debug:
Warning: connect.session() MemoryStore is not
designed for a production environment, as it will leak
memory, and will not scale past a single process.
debug: -------------------------------------------------------
debug: :: Thu Feb 21 2019 17:22:40 GMT+0300 (East Africa Time)
debug: Environment : production
debug: Port        : 1337
debug: -------------------------------------------------------
Troubleshooting tips:

 -> Is your Postgresql configuration correct?  Maybe your `poolSize` configuration is set too high? e.g. If your Postgresql database only supports 20 concurrent connections, you should make sure you have your `poolSize` set as something < 20 (see http://stackoverflow.com/a/27387928/486547). The default `poolSize` is 10. To override default settings, specify the desired properties on the relevant Postgresql "connection" config object where the host/port/database/etc. are configured. If you're using Sails, this is generally located in `config/datastores.js`, or wherever your environment-specific database configuration is set.

 -> Maybe your `poolSize` configuration is set too high? e.g. If your Postgresql database only supports 20 concurrent connections, you should make sure you have your `poolSize` set as something < 20 (see http://stackoverflow.com/a/27387928/486547). The default `poolSize` is 10.

 -> Do you have multiple Sails instances sharing the same Postgresql database? Each Sails instance may use up to the configured `poolSize` # of connections. Assuming all of the Sails instances are just copies of one another (a reasonable best practice) we can calculate the actual # of Postgresql connections used (C) by multiplying the configured `poolSize` (P) by the number of Sails instances (N). If the actual number of connections (C) exceeds the total # of **AVAILABLE** connections to your Postgresql database (V), then you have problems.  If this applies to you, try reducing your `poolSize` configuration. A reasonable `poolSize` setting would be V/N.

 -> Are you using an SSL-enabled Postgresql host like Heroku? Make sure to set `ssl` to `true` (see http://stackoverflow.com/a/22177218/486547)


error: Sending 500 ("Server Error") response:
 AdapterError: Unexpected error from database adapter: `select` failed ("badConnection").  A connection either could not be obtained or there was an error using the connection.
Additional data:

{ error:
   { Error: connect ETIMEDOUT 1.2.3.4:5678
       at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1104:14)
     errno: 'ETIMEDOUT',
     code: 'ETIMEDOUT',
     syscall: 'connect',
     address: '1.2.3.4',
     port: 5678 },
  meta:
   { adapter: 'sails-postgresql',
     url:
      'postgres://some_username:[email protected]:5678/some_db',
     identity: 'default' } }

As can be seen, the DB_PASSWORD env var is printed to the terminal, via stderr.

[sailsbot]

@alxndrsn Thanks for posting, we'll take a look as soon as possible.

---

For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

[johnabrams7]

@alxndrsn Thanks for shedding light on this - I'll discuss it with the team.

[alxndrsn]

@johnabrams7 any more thoughts?

[johnabrams7]

@alxndrsn At the moment, we've been developing an upcoming sails-sql adapter which doesn't have this behaviour in the error tests. Unfortunately, postgres isn't currently supported in that one -- However, we're open to a PR for sails-postgresql to get this patched. Feel free to submit one and we'll check it out 👍

[alxndrsn]

@johnabrams7 I've submitted a PR to address this

[johnabrams7]

@alxndrsn Thanks a lot for the PR - I'll have the team take a look!

[rachaelshaw]

Thanks for the PR, @alxndrsn! I left one note for your review, open to your feedback on this: https://github.com/balderdashy/sails-postgresql/pull/283#pullrequestreview-240104121

[alxndrsn]

I've released a node module to fix this issue at https://www.npmjs.com/package/sails-postgresql-redacted. To use:

1. replace sails-postgresql with sails-postgresql-redacted in package.json
2. replace sails-postgresql with sails-postgresql-redacted in datastores.*.adapter config (in config/datastores.js and/or in config/env/*.js

[Noitidart]

@alxndrsn did you release that because the PR wasn't getting merged.

I think they should merge the PR. The risk of false positives, is not dangerous, it's just alarming, but a look will confirm everything is fine. But leaving passwords out there is dangerous.

Sails team, let's get that PR merged please.

[johnabrams7]

@Noitidart - Thanks for the follow-up reasoning.
@alxndrsn - Great appreciation for the workaround!
For both, sorry we've been delayed on this; we're going to examine & test out the PR to get it merged this week.

[Noitidart]

Sincerest thanks @johnabrams7!!

[alxndrsn]

sorry we've been delayed on this; we're going to examine & test out the PR to get it merged this week.

@johnabrams7 did you get a chance to look at this in the end?

[johnabrams7]

@alxndrsn Yes, just got it tested and was able to successfully hide/censor postgresql db passwords with asterisks. Merging it in.