Subject: unclear OAuth error message

From: Álvaro Herrera <[email-masked]>
Date: Sat, 24 Jan 2026 15:50:28 +0100

Hello,

While updating the translation, I noticed this code

    /*
     * Log any authentication results even if the token isn't authorized; it
     * might be useful for auditing or troubleshooting.
     */
    if (ret->authn_id)
        set_authn_id(port, ret->authn_id);

    if (!ret->authorized)
    {
        ereport(LOG,
                errmsg("OAuth bearer authentication failed for user \"%s\"",
                       port->user_name),
                errdetail_log("Validator failed to authorize the provided token."));

        status = false;
        goto cleanup;
    }

I'm not sure I understand the errdetail() part of it.  At first it made
me wonder if it was about a user-supplied module that had an internal
failure preventing it from deciding whether the user was authorized or
not (which would have been something like "Validator failed while ...").
But the code suggests that the module worked fine and made the
determination not to authorize the user.  If that's so, then why do we
have the errdetail at all?  Can't we just get rid of it and let the
errmsg stand on its own merit?

There is one more case for this exact errmsg to be given:

    /* Make sure the validator authenticated the user. */
    if (ret->authn_id == NULL || ret->authn_id[0] == '\0')
    {
        ereport(LOG,
                errmsg("OAuth bearer authentication failed for user \"%s\"",
                       port->user_name),
                errdetail_log("Validator provided no identity."));

Here it seems the validator did indeed have an internal problem of some
sort, because while it did declare that the user was authorized, it did
not provide what we were expecting from it.  Should in this case the
errmsg() be different?

(Actually, there's also auth_failed() giving the same message.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
 than society gathers wisdom."  (Isaac Asimov)

---

From: Jacob Champion <[email-masked]>
Date: Mon, 26 Jan 2026 14:17:08 -0800

> errmsg stand on its own merit?

For that code path I suspect we could get rid of the entire message,
because of what you mentioned later: auth_failed() is already going to
give us that. The validator can log what's important if needed, or
not. We could add some DEBUGs, maybe, so that you can still figure out
what's going on if a validator fails silently?

> not provide what we were expecting from it.  Should in this case the

Yeah, I think so. The errdetail should probably become the errmsg,
essentially (but with more context).

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Wed, 18 Mar 2026 09:47:14 -0700

> what's going on if a validator fails silently?

I only just remembered that this is exactly what logdetail is designed
to do. It's passed in from CheckSASLAuth, but OAuth doesn't make use
of it. (My original patchset carried a TODO for this for a long time,
but I lost it at some point...)

I have a parallel patchset that also needs logdetail, so this fix can
piggyback on some of that work.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 20 Mar 2026 10:52:10 -0700

> piggyback on some of that work.

Here is (the confusingly named) v3-0001, which I hope fixes both
upthread complaints. It comes from [1].

--Jacob

[1] https://postgr.es/m/CAOYmi%2BnTXGcroZD_Mnkc8LYWYFbfDYNR4ML_yQ5sF9%2BDY2amcg%40mail.gmail.com

---

From: Zsolt Parragi <[email-masked]>
Date: Mon, 23 Mar 2026 21:21:31 +0000

This is definitely a nice improvement, I only have two minor questions:

- errmsg("internal error in OAuth validator module"));
+ errmsg("internal error in OAuth validator module"),
+ ret->error_detail ? errdetail_log("%s", ret->error_detail) : 0);
+

Isn't including the detail for both the warning and the fatal error
still overly verbose?

+ res->error_detail = error_detail; /* only relevant for failures */
+ if (internal_error)
+ return false;
+

Shouldn't the oauth code include a sanity check to ensure validators
return no error_detail on success instead of silently ignoring it?

---

From: Jacob Champion <[email-masked]>
Date: Fri, 27 Mar 2026 10:01:59 -0700

> Isn't including the detail for both the warning and the fatal error

I'm not too worried about verbosity for an internal error situation;
users shouldn't see it. If they do, I don't mind being very loud about
whose fault it is.

(I'm also influenced by some recent support work on clusters that have
huge log volumes. If someone is focused on the internal error, they
should be able to see at a glance what caused that error, and if
someone is focused on the authentication failure, they should be able
to see at a glance what caused that. The more logs you have to
correlate in a "help! no one can log in" panic situation, the less
likely you are to succeed.)

> return no error_detail on success instead of silently ignoring it?

IMO, no. I don't want error_detail to add semantics to the API, just
descriptive power. Plus, I think a design that sets a possible error
message before entering a complex operation, knowing that it will be
ignored on success, is perfectly valid. libpq-oauth, and to a lesser
extent libpq, make use of that pattern.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 27 Mar 2026 10:03:11 -0700

On Mon, Mar 23, 2026 at 10:18 PM Chao Li <[email-masked]> wrote:
> I also have a small comment on the doc change. The documentation already mentions the memory requirements for error_detail, so I wonder if it also makes sense to mention its style. Since it is emitted as a DETAIL message, I think it should probably follow the usual style rules for detail messages, i.e., start with a capital letter and end with a period.

Sure, I think it makes sense to add a quick link to our style guide as
part of the "best practices" section.

Thanks,
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 27 Mar 2026 23:24:40 +0100

> On 27 Mar 2026, at 18:01, Jacob Champion <[email-masked]> wrote:
> 

Agreed.

> extent libpq, make use of that pattern.

Callsites can also clear the error message on success and not even rely on it
being ignored.

+	 * This string may be either of static duration or palloc'd.
+	 */
+	char	   *error_detail;

I'm not a big fan of "either static or allocated" and prefer if we just require
one or the other.  We have this pattern in other places so it's not a blocker
for going it, but.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 27 Mar 2026 16:20:53 -0700

> Callsites can also clear the error message on success and not even rely on it

Agreed, but are you saying that as an argument for my approach, or for Zsolt's?

> one or the other.  We have this pattern in other places so it's not a blocker

I don't think I can enforce either choice, though: I pass the
error_detail into the ereport(FATAL), so the process is about to go
down, and I'm never going to pfree() it.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Wed, 1 Apr 2026 15:57:10 -0700

> down, and I'm never going to pfree() it.

(I also think it's reasonable to do something like

    res->error_detail = "Help! it's again.";

if there's nothing to format, without mandating a superfluous pstrdup().)

v4 rebases, incorporates Chao Li's feedback, rewrites the surrounding
paragraph a bit, and fixes a silly misuse of <xref> when I meant
<link>.

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 2 Apr 2026 23:25:20 +0200

> On 2 Apr 2026, at 00:57, Jacob Champion <[email-masked]> wrote:

> paragraph a bit, and fixes a silly misuse of <xref> when I meant

This version LGTM.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 3 Apr 2026 16:26:42 -0700

> This version LGTM.

Pushed, thanks!

--Jacob