Handle SASL SCRAM server error responses

[v0idpwn]

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

[brianc]

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.

[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.

[brianc]

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

[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.