Postgraphile: Postgraphile logs password on connection loss

[conord33]

I'm submitting a ...

• [x] bug report
• [ ] feature request
• [ ] question

When the connection to postgres is dropped the db connection object is logged which contains everything you would need to connect to the db such as host, user, and password... The password should be hidden at minimum.

PostGraphile version: 4.3.1, 4.3.3

Steps to reproduce:

Spin up a project with postgraphile then kill the database; you should see the log.

Current behavior:

This is the current log you receive when the connection is lost:

PostgreSQL client generated error:  { Error: Connection terminated unexpectedly
     at Connection.con.once (/project/path/node_modules/postgraphile/node_modules/pg/lib/client.js:199:9)
     at Object.onceWrapper (events.js:313:30)
     at emitNone (events.js:106:13)
     at Connection.emit (events.js:208:7)
     at Socket.<anonymous> (/project/path/node_modules/postgraphile/node_modules/pg/lib/connection.js:129:10)
     at emitNone (events.js:111:20)
     at Socket.emit (events.js:208:7)
     at endReadableNT (_stream_readable.js:1064:12)
     at _combinedTickCallback (internal/process/next_tick.js:138:11)
     at process._tickCallback (internal/process/next_tick.js:180:9)
   client: 
    Client {
      domain: null,
      _events: { notification: [AsyncFunction: listener], error: [Function] },
      _eventsCount: 2,
      _maxListeners: undefined,
      connectionParameters: 
       ConnectionParameters {
         user: 'postgres',
         database: 'db',
         port: 5432,
         host: 'localhost',
         password: null, <---------------------------------------------- PASSWORD
         binary: false,
         ssl: false,
         client_encoding: '',
         replication: undefined,
         isDomainSocket: false,
         application_name: undefined,
         fallback_application_name: undefined,
         statement_timeout: false },
      user: 'postgres',
      database: 'db',
      port: 5432,
      host: 'localhost',
      password: null, <---------------------------------------------- PASSWORD
      replication: undefined,
      _types: TypeOverrides { _types: [Object], text: {}, binary: {} },
      _ending: true,
      _connecting: false,
      _connected: true,
      _connectionError: false,
      _queryable: false,
      connection: 
       Connection {
         domain: null,
         _events: [Object],
         _eventsCount: 19,
         _maxListeners: undefined,
         stream: [Object],
         _keepAlive: false,
         lastBuffer: false,
         lastOffset: 0,
         buffer: null,
         offset: 1,
         encoding: 'utf8',
         parsedStatements: {},
         writer: [Object],
         ssl: false,
         _ending: true,
         _mode: 0,
         _emitMessage: false,
         _reader: [Object] },
      queryQueue: [],
      binary: false,
      processID: 45,
      secretKey: -332921639,
      ssl: false,
      release: [Function: bound release],
      activeQuery: null,
      readyForQuery: true,
      hasExecuted: true } }

Expected behavior:

The password should not be logged.

[benjie]

Great spot 👍

[benjie]

Interestingly I fixed this literally 5 hours ago: https://github.com/graphile/postgraphile/commit/35c3e985264fe5f384f9d2916f2ad2dec9c27f91

[conord33]

You must be psychic 🔮 Thanks! Ill keep an eye out for the next release.

[benjie]

It’s available now: postgraphile@next (including subscriptions and live queries features!)

[richardscarrott]

@conord33 @benjie I don't suppose either of you know, in general, why a connection might be terminated? We're seeing this quite frequently recently -- we're using the nodejs postgraphile API so I wonder if there's something we should be doing to reconnect in these instances; currently we see the error propagated to the our global error handler where we kill the process / restart the pod...

[richardscarrott]

@benjie Okay figured it out I think, we're on a very old version so I can see newer versions bind to .on('error') to prevent it from propagating to the global error handler -- https://github.com/graphile/postgraphile/blob/main/src/postgraphile/postgraphile.ts#L263

I guess it's expected that connections will terminate over time and the pgPool manages this behind the scenes so we can still safely keep the server alive?

[benjie]

This should only happen when the PostgreSQL server terminates the connection, which should in general only be when it's rebooting or when you specifically close them. Pool will auto-close connections when they've been idle for a certain period of time (I think 8 seconds by default?). But yes, adding the error handler is vital so that the Node process doesn't exit in these circumstances; event emitters with unhandled error events cause the node process to exit.

[richardscarrott]

Thanks for the support, I've added the error handler to our pgpool (as an interim until we upgrade postgraphile) so at least it's not restarting our server every time; then we just need to figure out why the DB is closing the connections so often.