REVOKE's CASCADE protection doesn't work with INHERITed table owners

[moving to -hackers]

On Wed, Jun 24, 2026 at 2:57 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

TL;DR: The protection in recursive_revoke() against broken GRANT
OPTION chains doesn't seem to work properly when the grantee also
holds the privileges of the grantor.

More accurately: "when an intermediate grantor in the chain only
indirectly holds the ability to grant."

I think the issue is in recursive_revoke()'s usage of aclmask(), which
in turn uses has_privs_of_role(). It doesn't seem like that's what was
wanted in this particular case... thoughts?

I propose changing that to aclmask_direct(), as in the attached, and
backpatching all the way down.

To try to prove to myself that this works, I added tests to pin each
of the three cases that are treated differently by aclmask_direct():
1. the grantor has indirect ownership privileges
2. the grantor has indirect grant options via INHERIT
3. the grantor has indirect grant options via PUBLIC (which is already
disallowed in practice)

I also tried to expand the existing comment, both to point out the
pitfall and to explain why the short-circuit works. But I've rewritten
it at least a dozen times, so if anyone can tell me whether I've made
sense and/or used the terminology appropriately, I'd appreciate it.

I'm pretty sure the following is unintended behavior. It looks
potentially related to [1] as well.

(To fix [1]/messages/by-id/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=tnqOSMz4+mnzA9m0zMg@mail.gmail.com I suspect we need to make a similar tweak to
check_circularity(), but I haven't looked into that yet.)

Thanks!
--Jacob

[1]: /messages/by-id/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=tnqOSMz4+mnzA9m0zMg@mail.gmail.com

Hi,

On Fri, 26 Jun 2026 at 05:43, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

[moving to -hackers]

On Wed, Jun 24, 2026 at 2:57 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

TL;DR: The protection in recursive_revoke() against broken GRANT
OPTION chains doesn't seem to work properly when the grantee also
holds the privileges of the grantor.

More accurately: "when an intermediate grantor in the chain only
indirectly holds the ability to grant."

I think the issue is in recursive_revoke()'s usage of aclmask(), which
in turn uses has_privs_of_role(). It doesn't seem like that's what was
wanted in this particular case... thoughts?

I propose changing that to aclmask_direct(), as in the attached, and
backpatching all the way down.

To try to prove to myself that this works, I added tests to pin each
of the three cases that are treated differently by aclmask_direct():
1. the grantor has indirect ownership privileges
2. the grantor has indirect grant options via INHERIT
3. the grantor has indirect grant options via PUBLIC (which is already
disallowed in practice)

Thanks, this looks right to me. I traced recursive_revoke() and the
aclmask() -> aclmask_direct() switch makes sense to me, since a
grant is only ever attributed to a role that holds the option directly
(or the owner), it seems right that recursive_revoke() should judge
"still has it" the same way, rather than counting inherited/superuser
access? The three new test cases line up with that too.

I also tried to expand the existing comment, both to point out the

pitfall and to explain why the short-circuit works. But I've rewritten
it at least a dozen times, so if anyone can tell me whether I've made
sense and/or used the terminology appropriately, I'd appreciate it.

One tiny comment question: the phrase "granted by any role on the chain"
in the new comment reads a little oddly to me, would something like
"still holds the option directly via another grantor" be closer to what
the code checks? Could just be me misreading it.

Apart from the above, patch LGTM.

I'm pretty sure the following is unintended behavior. It looks

potentially related to [1] as well.

(To fix [1] I suspect we need to make a similar tweak to
check_circularity(), but I haven't looked into that yet.)

[1]
/messages/by-id/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=tnqOSMz4+mnzA9m0zMg@mail.gmail.com

On check_circularity() for [1]: I tried the same aclmask_direct() swap,
but since it runs on every GRANT ... WITH GRANT OPTION, which
pg_dump/restore replays, erroring there could make restore/pg_upgrade of
an existing cluster (one already holding the [1] self-grant) fail. Feels
like it may need a companion dump/restore change first.

Regards,
Ayush

On Mon, Jul 6, 2026 at 4:03 AM Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:

Thanks, this looks right to me.

Thanks for the review!

One tiny comment question: the phrase "granted by any role on the chain"
in the new comment reads a little oddly to me, would something like
"still holds the option directly via another grantor" be closer to what
the code checks?

The first sentence is still "The grantee might still have some grant
options via another grantor," and I don't think adding "direct" would
have helped me understand this any better the first time I read it.
I'm definitely up for more bikeshedding, though, because I don't
really like what I have...

On check_circularity() for [1]: I tried the same aclmask_direct() swap,
but since it runs on every GRANT ... WITH GRANT OPTION, which
pg_dump/restore replays, erroring there could make restore/pg_upgrade of
an existing cluster (one already holding the [1] self-grant) fail.

Well, I think the complaint in [1] is that dump/restore *already*
fails, no? Can you provide an example of a correct (or benignly
incorrect) dump that would start failing?

As an aside, I'm not sure if check_circularity() is correctly
preventing cycles independently of this issue, so that part may end up
spiraling a bit.

Thanks,
--Jacob

Hi,

On Thu, 9 Jul 2026 at 04:24, Jacob Champion <jacob.champion@enterprisedb.com>
wrote:

On Mon, Jul 6, 2026 at 4:03 AM Ayush Tiwari <ayushtiwari.slg01@gmail.com>
wrote:

The first sentence is still "The grantee might still have some grant
options via another grantor," and I don't think adding "direct" would
have helped me understand this any better the first time I read it.
I'm definitely up for more bikeshedding, though, because I don't
really like what I have...

Fair. The part that tripped me up was actually the second sentence,
"granted by any role on the chain", since the code isn't checking whether
some role could grant them, but whether the grantee itself still holds
them directly via another grantor. Maybe something like "if the grantee
still holds a grant option directly through another grantor, that
privilege's chain is intact"? No strong opinion though.

On check_circularity() for [1]: I tried the same aclmask_direct() swap,
but since it runs on every GRANT ... WITH GRANT OPTION, which
pg_dump/restore replays, erroring there could make restore/pg_upgrade of
an existing cluster (one already holding the [1] self-grant) fail.

Well, I think the complaint in [1] is that dump/restore *already*
fails, no? Can you provide an example of a correct (or benignly
incorrect) dump that would start failing?

The already-failing case in [1] is the one where the membership has
been revoked (REVOKE member FROM owner), which leaves a true orphan.
But the self-grant gets created as soon as an inheriting member does
the redundant WITH GRANT OPTION self-grant, and if the membership is
still in place, that cluster dumps and restores fine today.

CREATE ROLE owner_role;
CREATE ROLE member_role LOGIN;
GRANT owner_role TO member_role; -- membership kept
CREATE TABLE t (i int);
ALTER TABLE t OWNER TO owner_role;
GRANT SELECT ON t TO member_role WITH GRANT OPTION;
SET ROLE member_role;
GRANT SELECT ON t TO member_role WITH GRANT OPTION; -- self-grant

relacl = {owner_role=arwdDxtm/owner_role,
member_role=r*/owner_role,
member_role=r*/member_role}

pg_dump emits "SET SESSION AUTHORIZATION member_role; GRANT SELECT ON
t TO member_role WITH GRANT OPTION;". Restoring that into a fresh
cluster works today, but with the check_circularity() change it fails
with "grant options cannot be granted back to your own grantor".

So it's not that [1] is fine today; it's that the fix also breaks a
broader set of clusters that currently round-trip (membership still
present), including via pg_upgrade. That's why I think the
check_circularity() side needs a companion dump/restore change rather
than going in on its own.

As an aside, I'm not sure if check_circularity() is correctly
preventing cycles independently of this issue, so that part may end up
spiraling a bit.

Agreed, I did not check that either.

Regards,
Ayush