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

Handle SASL SCRAM server error responses

Open v0idpwn opened this issue 5 months ago • 3 comments

Add proper error handling for SCRAM-SERVER-FINAL-MESSAGE error attribute. The SCRAM specification allows servers to return error messages via the 'e' attribute in the server final message. Currently, these errors are ignored and authentication fails later during signature verification.

Postgres typically doesn't return this error (see here on why), but poolers, or other applications using the postgres protocol might, and it's part of the SCRAM spec, so it probably makes sense for node-postgres to handle it.

Aligns behaviour with psql, postgrex, and somewhat with pgJDBC (pgJDBC is stricter with scram errors).

For reference:

  • libpq handling it: https://github.com/postgres/postgres/blob/2047ad068139f0b8c6da73d0b845ca9ba30fb33d/src/interfaces/libpq/fe-auth-scram.c#L708

v0idpwn avatar Jul 23 '25 19:07 v0idpwn

Hi, Brian. I'd be happy to, but I'm not sure about how, since Postgres itself doesn't return this error. Do you suggest some approach to do it?

For the record, I discovered this because Supavisor returns scram errors. I sent a PR aligning it's behaviour with Postgres, but I decided to open this one too to align node-postgres scram implementation with the others.

v0idpwn avatar Jul 24 '25 12:07 v0idpwn

oh i see - what returns this error? or how can you get this error to trigger?

brianc avatar Jul 24 '25 13:07 brianc

what was worrying to me about this is throwing errors, depending on where they're thrown, might not be caught by the client and then emitted as an error event & instead result in uncaughtExceptions - I audited the code and it looks like all this SASL code is wrapped in try/catches & uses throwing to abort in the event of bad, malformed, unexpected responses and the like, so this is actually okay...and if its impossible to write an integration test for I'm okay to merge it like this. I don't love merging code that doesn't actually have an integration test around it with a real backed, but if there's no way to force/trick postgres into returning an e attribute, we can do it.

brianc avatar Jul 24 '25 13:07 brianc