Re: [PATCH] Fixed malformed error message on malformed SCRAM message.

On 06/02/2017 08:57 AM, Noah Misch wrote:
> On Tue, May 30, 2017 at 04:39:55PM -0700, Michael Paquier wrote:
>> diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
>> index 99feb0ce94..366a11feb8 100644
>> --- a/src/backend/libpq/auth-scram.c
>> +++ b/src/backend/libpq/auth-scram.c
>> @@ -283,11 +283,13 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
>> if (inputlen == 0)
>> ereport(ERROR,
>> (errcode(ERRCODE_PROTOCOL_VIOLATION),
>> - (errmsg("malformed SCRAM message (empty message)"))));
>> + errmsg("malformed SCRAM message"),
>> + errdetail("The message is empty.")));
>> if (inputlen != strlen(input))
>> ereport(ERROR,
>> (errcode(ERRCODE_PROTOCOL_VIOLATION),
>> - (errmsg("malformed SCRAM message (length mismatch)"))));
>> + errmsg("malformed SCRAM message"),
>> + errdetail("Input length does not match.")));
>
> auth.c uses COMMERROR for this sort of thing; why does auth-scram.c use ERROR?

Yeah, COMMERROR would be more consistent. But I wonder why auth.c uses
COMMERROR rather than ERROR for these. It would seem more user-friendly.
Or developer-friendly; the most likely time you see these messages is
when you're developing a driver that speaks the protocol.

And pqformat.c uses ERROR for protocol violation errors. So we're not
very consistent. I think it would be best to convert all or most of the
ERRCODE_PROTOCOL_VIOLATION messages to ERROR. COMMERROR is appropriate
when you you think that the client has already disconnected, or is so
thoroughly confused that sending an error would just make things worse.
Or if you the message contains information that you don't want to reveal
to a non-authenticated client. I don't think that applies to any of the
COMMERRORs in auth.c.

- Heikki