Handle SASL SCRAM server error responses #3521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

v0idpwn wants to merge 1 commit into brianc:master from v0idpwn:master

v0idpwn commented Jul 23, 2025 •

edited

Loading

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


          Handle SASL SCRAM server error responses

7bd5dd4

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](https://github.com/postgres/postgres/blob/2047ad068139f0b8c6da73d0b845ca9ba30fb33d/src/backend/libpq/auth-scram.c#L423)
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 in particular 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 force-pushed the master branch from 5d41d93 to 7bd5dd4 Compare

July 23, 2025 19:05

charmander approved these changes

View reviewed changes

brianc requested changes

View reviewed changes

Owner

brianc left a comment

Is it possible to include an integration test for this? One that hits the server? I want to make sure throwing here doesn't crash the client w/ an uncaught exception.

Author

v0idpwn commented Jul 24, 2025

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.

Owner

brianc commented Jul 24, 2025

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

Owner

brianc commented Jul 24, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet