node-postgres icon indicating copy to clipboard operation
node-postgres copied to clipboard

No handlers for SASL errors

Open sehrope opened this issue 6 years ago • 5 comments

Leaving a note here as a reminder to take another look at the SASL error handling. At first glance I don't think the errors would bubble up as they're thrown synchronously in the helper functions and the caller does not catch or emit them up to the client.

https://github.com/brianc/node-postgres/blob/0acaf9d8ff3e96fd2a71c786c8ee883c2b2006d2/lib/sasl.js#L4-L17

https://github.com/brianc/node-postgres/blob/0acaf9d8ff3e96fd2a71c786c8ee883c2b2006d2/lib/client.js#L142-L146

sehrope avatar Jul 19 '19 13:07 sehrope

At first glance I don't think the errors would bubble up as they're thrown synchronously in the helper functions and the caller does not catch or emit them up to the client.

This is still an issue as of pg 8.7.1.

To reproduce, attempt to connect to a PG instance that uses scram and don't specify a password when connecting:

const client = new Client();
try {
  await client.connect();
} catch (err) {
  // this is never reached
}

You'll get an uncaught exception: Error: SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a string. But this really should be raised by client.connect and not be unhandled.

gabegorelick avatar Dec 03 '21 05:12 gabegorelick

I’m building a CLI app that uses node-postgres.

When I leave all connection arguments empty as a user of the app, it crashes with this very confusing error.

As a developer of the app, I don’t see any way to catch this error to provide any context to the user as to why it crashed and exit gracefully.

If it’s possible to prioritise fixing this, I would appreciate it greatly.

rpominov avatar Jun 06 '22 15:06 rpominov

@rpominov Check if it’s missing before making a connection attempt.

charmander avatar Jun 06 '22 22:06 charmander

I think postgres allows to setup the server so that you can connect without a password.

Also checking the password is not trivial, as it can come from at least 4 places that I'm aware of:

  • password option,
  • connectionString option,
  • env variable,
  • .pgpass file.

In general, there seem to be a lot of authentication methods that node-postgres supports that I don't fully understand. Ideally, I want to just pass the options to node-postgres and show any errors that it produces back to the user, while stating in the documentation that they can use whatever authentication method that the library supports.

rpominov avatar Jun 07 '22 08:06 rpominov

We're encountering this issue over at Sequelize after updating our CI to use postgres 14 (PR https://github.com/sequelize/sequelize/pull/14737)

We have a test that checks that an error is raised is the password is invalid (in this case null), but because the error is not sent to Client.connect, we end up with an unhandled exception

ephys avatar Jul 10 '22 12:07 ephys