[habdelra]

What this changes

Three migrations withhold secret-bearing tables from readonly_role and its direct members: the ones that seal proxy_endpoints.api_key and incoming_webhooks.signing_secret, and the one that adds user_keys / key_write_tickets. A plain REVOKE, run as the table's owner, removes only grants the owner made, which leaves two gaps:

• A grant from another role survives. When role A holds SELECT … WITH GRANT OPTION and grants SELECT to a read-only member, the member can still read the table after up().
• A member that passed its grant on blocks the migration. The member's REVOKE fails with "dependent privileges exist", and up() aborts.

In all three migrations, up() repeatedly picks a SELECT grant that the read-only role or a member still holds on the table or one of its columns. It revokes that grant acting as the role that made it (SET ROLE <grantor>, since Postgres only accepts GRANTED BY naming the current user), with CASCADE, until none are left. If the migrating role can't act as that grantor, up() raises an error naming the grant and its grantor, rather than leaving the table readable. As a backstop, a grant that is still there after its REVOKE also raises.

Who counts as a read-only member

A reader is readonly_role itself or any role that reaches it through memberships granted with INHERIT or SET, directly or through other roles. The table's owner never counts. On Postgres 16, a role with CREATEROLE that creates readonly_role (the RDS master user is one) is automatically made a member of it with ADMIN alone. Such a role can manage the membership, but it can't inherit the role or SET ROLE to it, and it is often the tables' owner. If every direct member counted as a reader, the migration would revoke that owner's own SELECT, and a server connecting as the owner could no longer read the sealed tables. Production and staging don't have that membership, so neither database is affected.

Keeping down() exact

down() in the two sealing migrations still restores exactly what up() removed:

• up() takes every SELECT grant on the table and its columns before revoking, and afterwards records the grants that disappeared. That includes grants CASCADE took from roles outside the read-only role. Each recorded entry carries its grantor; an owner grant has a null grantor, and a grant to PUBLIC has a null grantee.
• down() re-grants each entry acting as its grantor. It works in passes, restoring each grant once the grantor's own ACL entry carries the grant option again, so a member's own grant comes back before the grant it passed on. An option the grantor only inherits through another role doesn't count, because granting on it would record that other role as the grantor.
• A record with no grantor field is read as grants from the owner. That is the shape the record has on databases where up() already ran.

The user-keys migration's down() drops both tables, so that migration records nothing.

Test plan

• proxy-endpoint-keys-test / incoming-webhook-secrets-test, the migration-down module:
  • A member's SELECT granted by another role is revoked by up(): the member gets "permission denied", and the grantor keeps its own grant. down() restores the grant from the original grantor.
  • A member that passed SELECT on to an outside role doesn't block up(), and the passed-on grant goes too. down() restores both.
  • A member that also inherits the grant option from the read-only role: the grant it passed on comes back from the member, not the role.
  • A record with no grantors restores as owner grants.
  • The existing exact-restore, column-grant, idempotence, dropped-role and other-record tests still pass.
• Reader rule, in the two sealing suites:
  • A table owner that is an ADMIN-only member of readonly_role keeps its own ACL entries through up() and down(). The user-keys suite covers this through up().
  • An owner that also reads through readonly_role (INHERIT and SET) keeps its own SELECT, which pins the owner exclusion.
  • A non-owner role with ADMIN alone keeps its own grant, which pins the INHERIT-or-SET check.
  • A role that reaches readonly_role through a group is denied, and down() restores its grant. The user-keys suite covers this too.
• user-keys-test: neither table is readable by a member granted SELECT by another role, nor by a member that passed its grant on.
• Ran all three modules locally: 74 passed.
• Checked against a scratch Postgres 16.3: a mixed scenario (owner, delegated, passed-on and column grants) round-trips through up and down with an identical ACL. A non-superuser owner that can't SET ROLE to the grantor gets the named error.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-16T22:07:14.782685Z	0a87986	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a87986f2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Ship the privilege revocation as a new migration

In any environment that has already applied these three migrations, this new loop will never execute: the deploy script runs node-pg-migrate ... up against the existing migrations tracking table, and node-pg-migrate records applied migrations in that table rather than replaying modified files (migration-history documentation). Consequently, existing staging/production databases can retain the exact delegated SELECT grants this commit is intended to remove, leaving secret-bearing tables readable; the tests only pass because they invoke the edited migration against fresh state. Preserve the historical files and put the repair in a new timestamped migration so deployed databases see it as pending.

AGENTS.md reference: AGENTS.md:L175-L179

Useful? React with 👍 / 👎.

[habdelra]

[Claude Code 🤖] Editing these migrations in place is deliberate, and nothing changes in this PR. Production has never had a SELECT grant made directly to a readonly_role member, whether by the owner or another role, so a deployed production database holds no grant that a re-run of the hardened up() would remove. The edit gives every database that runs these migrations from here on the hardened up(), and down() still reads the records the earlier up() wrote. A new migration would only repeat the revoke where there's nothing to revoke, and it would need its own record-and-restore down(). Whether staging is in the same state is for a human reviewer to confirm. If it isn't, a one-off check of the sealed tables' ACLs there closes the gap.

[habdelra]

[Claude Code 🤖] This review covered the grant and revoke logic in the three migrations and the migration tests. It checked whether up() removes every direct grant, whether down() restores the exact ACL, and whether the tests would catch either going wrong. I checked the SQL against a scratch Postgres 16.3. I didn't run the realm-server suite.

No blocking issues. In every case I tried, up() removed every direct SELECT grant to the read-only role and its members. down() restored the exact ACL except when a grantor inherits its grant option, which needs a small change.

1. down() checks a grantor's grant option including inherited ones, so a grant can come back under the wrong grantor. Regression, non-blocking. See the comment on the has_table_privilege check in the proxy-endpoints migration.
2. The passed-on tests never reach down()'s second pass. Test gap, non-blocking. See the comment on RECIPIENT in proxy-endpoint-keys-test.ts.
3. Question: have the two sealing migrations or the user-keys migration already run on staging? The handling of records with no grantor key suggests some database ran the earlier up(). Editing up() in place doesn't make it run again, so on such a database a SELECT another role granted to a member is still there. If staging has run them, a new migration or a one-off check of those tables' ACLs would close the gap. Non-blocking for this change.
4. A member can still read the tables through PUBLIC or through another role it belongs to. Pre-existing, follow-up. See the comment on the revoke loop in the proxy-endpoints migration.

[habdelra]

[Claude Code 🤖] has_table_privilege and has_column_privilege also count a grant option the grantor only inherits. So down() can run a recorded grant before the grantor's own grant is back, and Postgres then records the grant as coming from the role the grantor inherits the option from. I reproduced this on PG 16.3:

• readonly_role holds SELECT WITH GRANT OPTION from the owner.
• Its member zz_member holds its own SELECT WITH GRANT OPTION and passes SELECT on to rx.
• up() revokes all three.
• The record is ordered by grantee, so down() restores readonly_role first, then rx, then zz_member. When it reaches rx, zz_member passes the check through readonly_role.
• After down(), rx's SELECT comes from readonly_role instead of zz_member.

Checking the grantor's own ACL entry fixes this. The grantor Postgres records always holds the option directly, so a direct check never rejects a grant that should go through. I put this in place of the CASE and re-ran three scenarios: the one above, the passed-on case, and a grant that has to wait for a later pass. All three came back with an identical ACL:

IF NOT EXISTS (
  SELECT 1 FROM pg_class tbl
  CROSS JOIN LATERAL aclexplode(tbl.relacl) acl
  WHERE tbl.oid = 'proxy_endpoints'::regclass
    AND acl.grantee = (revoked->>'grantor')::regrole
    AND acl.privilege_type = 'SELECT' AND acl.is_grantable
  UNION ALL
  SELECT 1 FROM pg_attribute col
  CROSS JOIN LATERAL aclexplode(col.attacl) acl
  WHERE col.attrelid = 'proxy_endpoints'::regclass
    AND col.attname = revoked->>'column'
    AND acl.grantee = (revoked->>'grantor')::regrole
    AND acl.privilege_type = 'SELECT' AND acl.is_grantable
) THEN

The same block is in 1791999900813_seal-incoming-webhook-signing-secrets.js.

Regression, non-blocking. It only happens when a grantor inherits the grant option from a role whose grant down() restores earlier in the same pass.

[habdelra]

[Claude Code 🤖] Fixed in 8a74cbf, using your query. In both sealing migrations, down() now restores a recorded grant only once the grantor's own ACL entry carries SELECT with the grant option, on the table or on the grant's column. A new migration-down test pins it: the read-only role holds the grant option, and its member zz_… holds its own and passes SELECT on to x_…. With the previous check, that recipient's grant came back from readonly_role; it now comes back from the member, matching the ACL from before up().

[habdelra]

[Claude Code 🤖] The passed-on test never runs down()'s second pass. up() records grants in grantee-name order, and readonly_direct_member_… sorts before recipient_…. So the member's own grant comes back before the recipient's in the first pass, and the waiting branch never runs. The test would still pass if down() made only one pass.

Give the recipient a name that sorts first, for example a_recipient_for_proxy_endpoints_test. Its grant then has to wait for its grantor. Make the same change for RECIPIENT in incoming-webhook-secrets-test.ts.

Test gap in this change, non-blocking.

[habdelra]

[Claude Code 🤖] Fixed in 8a74cbf. The recipient is now a_recipient_for_<table>_test in both suites, so its grant comes ahead of the member's in the record and down() only restores it on a later pass.

[habdelra]

[Claude Code 🤖] After up(), the read-only role and its members can still read the table through PUBLIC, and a member can also read it through another role it belongs to. The loop only looks for grants made to those roles by name. I checked both on PG 16.3:

• After GRANT SELECT (api_key) ON proxy_endpoints TO PUBLIC, up() leaves api_key readable by readonly_role and every member.
• A member that also belongs to a role holding SELECT can still read the whole table.

The header comment says the role and its members lose SELECT, which reads as covering both cases. A check after the loop would pin the result itself. For example, it could raise (or at least warn) when has_column_privilege is still true on any column for readonly_role or any direct member. The same applies to the incoming-webhooks and user-keys migrations.

Pre-existing: the plain REVOKE had the same reach. Follow-up, not this PR. Non-blocking.

[habdelra]

[Claude Code 🤖] Agreed, that's a follow-up and not this PR. I've filed it in the tracker: after the revoke loop, all three migrations check has_column_privilege for the read-only role and each direct member, and either raise or warn if one of them can still read a column. Revoking from PUBLIC or from an unrelated role isn't the migration's call, which is why the follow-up is a check rather than a broader revoke.

[github-actions]

Host Test Results

    1 files      1 suites   1h 47m 19s ⏱️
5 703 tests 5 692 ✅ 11 💤 0 ❌
5 718 runs  5 707 ✅ 11 💤 0 ❌

Results for commit 826b1d2.

Realm Server Test Results

    1 files    343 suites   2h 1m 41s ⏱️
5 807 tests 5 807 ✅ 0 💤 0 ❌
5 870 runs  5 870 ✅ 0 💤 0 ❌

Results for commit 826b1d2.

[habdelra]

[Claude Code 🤖] Pushed 7c6e7ac. The revoke loop no longer revokes the table owner's own SELECT: a read-only member is a direct member of readonly_role granted it with INHERIT or SET, and never the table's owner. On Postgres 16, a CREATEROLE role that creates readonly_role becomes an ADMIN-only member of it, and the version of these migrations on main revokes that owner's SELECT. The description has the details. These migrations have already run on deployed databases, so editing them doesn't reach those. Whether to add a repair migration is an open question for the PR author.

[habdelra]

[Claude Code 🤖] Pushed 23a40bb, which adds a repair migration for databases that already ran the sealing migrations. It gives the table owner back the SELECT the earlier member check revoked, and revokes every SELECT a reader still holds on the four withheld tables, whoever granted it. The description has the details.

[habdelra]

[Claude Code 🤖] This review covered the read-only member predicate shared by the two sealing migrations and the user-keys migration, along with the tests added for it. I checked each clause against a scratch Postgres 16.3, migrating as a non-superuser CREATEROLE owner. The repair migration is being removed from this PR, so this review doesn't cover it. I didn't run the realm-server suite.

No blocking issues. In every case I built, the predicate left alone an owner that is an ADMIN-only member and still found every direct reader. The new tests don't pin either clause on its own.

1. Each new owner test still passes with either clause of the predicate removed. Test gap, non-blocking. See the comment on the owner test in proxy-endpoint-keys-test.ts.
2. A role that reaches readonly_role through another role keeps its own SELECT. Pre-existing, follow-up, non-blocking. See the comment on the membership predicate in the proxy-endpoints migration.

[habdelra]

[Claude Code 🤖] This test still passes with either half of the new member predicate removed, so neither half is pinned. An owner that is an ADMIN-only member is excluded by both (inherit_option OR set_option) and member <> relowner. On a scratch Postgres 16.3, I dropped each clause in turn from the proxy-endpoints up() and ran this setup. Both times, the owner kept SELECT.

Each clause is needed only in a case the suites don't build:

• The owner clause: an owner that also holds INHERIT or SET on readonly_role. On Postgres 16, a CREATEROLE role running with createrole_self_grant = 'inherit, set' gets that membership row as well as the ADMIN-only one. Without the clause, up() revokes the owner's SELECT.
• The INHERIT/SET clause: a role that isn't the owner, administers readonly_role with ADMIN alone, and holds its own SELECT. Without the clause, up() revokes that grant.

Please add a test for each:

• For the first, run GRANT readonly_role TO <owner> WITH INHERIT TRUE, SET TRUE on top of this setup, then assert that the owner's ACL entries are unchanged.
• For the second, set up a non-owner ADMIN-only member with its own GRANT SELECT, then assert that its grant survives up().

Add the same tests to incoming-webhook-secrets-test.ts, and the owner one to user-keys-test.ts.

Test gap in this change, non-blocking.

[habdelra]

[Claude Code 🤖] Fixed in 61ef0a7. Each half of the reader rule now has a test that fails without it, in both sealing suites. One test makes the table's owner a member of readonly_role with INHERIT and SET; the owner check is all that keeps its own SELECT. The other gives a non-owner role ADMIN alone and its own SELECT on the table; the INHERIT-or-SET check is all that keeps that grant.

[habdelra]

[Claude Code 🤖] This predicate counts only direct members, so a role that reaches readonly_role through another role keeps any SELECT granted to it directly. On Postgres 16.3, I ran GRANT readonly_role TO grp; GRANT grp TO lg; GRANT SELECT ON proxy_endpoints TO lg. After up(), lg still holds its grant and can read api_key. The header comment's "every role that is a member of it" reads as covering lg. A post-revoke has_column_privilege check that follows INHERIT/SET memberships through intermediate roles would stop on this grant instead of removing it.

There are two ways to close the gap:

• Have up() remove the grant. Replace the one-level IN (...) with a recursive walk of pg_auth_members that follows INHERIT or SET edges and drops the table's owner at the end. Make the same change in the incoming-webhooks and user-keys migrations.
• Keep the current reach. Narrow the header comment to say "direct members".

Pre-existing: the base-branch loop also looks only one level deep. Follow-up, non-blocking.

[habdelra]

[Claude Code 🤖] Done in 61ef0a7. I made the rule follow memberships rather than narrowing the comment. In all three migrations, a reader is readonly_role or any role that reaches it through memberships granted with INHERIT or SET, directly or through other roles, and never the table's owner. A login that reaches readonly_role through a group now has its direct grant revoked. That's the same reach the check-only migration uses, so it no longer trips on such a grant. Tests in all three suites cover a role that reaches readonly_role through a group.

[habdelra]

[Claude Code 🤖] Pushed 61ef0a7. It removes the repair migration, because production and staging were checked and neither needs it. Readers now also include roles that reach readonly_role through other roles. The description reflects the current shape.

[FadhlanR]

[Claude Code 🤖] I reviewed the revoke loop and the restore passes in the three migrations as SQL, checking Postgres grantor-scoped REVOKE, CASCADE and SET ROLE semantics and the reader rule. I also checked that the two sealing migrations stay twins and that the tests pin each case. I didn't run the migrations against a database.

No blocking issues. up() closes both gaps it targets, and down() restores the exact grants in the cases I traced, including records written in the older shape that has no grantor.

1. Revoke grants the owner made before grants that readers passed on to other readers. CASCADE then removes those passed-on grants without needing SET ROLE (see the LIMIT 1 comment in the proxy endpoints migration).
2. Question: should a reader that can read through a non-reader role or through PUBLIC also be covered? A closing per-reader privilege check would catch every path (see the comment on the INSERT in the proxy endpoints migration).

Both are non-blocking.

[FadhlanR]

[Claude Code 🤖] Nothing orders this pick except the snapshot's sort by grantee name. Take a reader M that holds SELECT from the owner WITH GRANT OPTION and has passed it to another reader N whose name sorts first. The loop then picks N's grant first, which needs SET ROLE M. A migrating role that isn't a superuser and lacks SET on M (on Postgres 16, a CREATEROLE role gets ADMIN alone on the roles it creates) raises here. Revoking M's owner-made grant first would have removed N's grant through CASCADE, with no role switch at all. Ordering owner grants first avoids that failure:

SELECT held INTO withheld
  FROM jsonb_array_elements(remaining) WITH ORDINALITY AS r(held, position)
  WHERE ...
  ORDER BY held->>'grantor' IS NOT NULL, position
  LIMIT 1;

The incoming webhooks twin needs the same change. In add-user-keys.js the pick has no ORDER BY at all, though there the tables are new, so only grants from the owner can exist. Non-blocking: it only turns a failure that the error already names into a success.

[habdelra]

[Claude Code 🤖] Done in ecd2d7c. In both sealing migrations the pick now orders owner-made grants first: WITH ORDINALITY AS remaining_grant(held, position) ... ORDER BY held->>'grantor' IS NOT NULL, position LIMIT 1. So CASCADE on M's owner-made grant removes the grant M passed to N, and no SET ROLE M is needed. add-user-keys.js gets ORDER BY held.grantor <> held.relowner so all three stay alike, and the headers describe the ordering. No new test pins it: the suites run migrations as a superuser, which can always SET ROLE, so the old order passed too. The existing passed-on test now goes through CASCADE, and it still checks that down() restores both grants. All three suites pass (74 tests).

[habdelra]

[Claude Code 🤖] A correction to the ordering in ecd2d7c, fixed in 826b1d2. REVOKE ... CASCADE on a table grant doesn't remove the column grants passed on from it. Say M holds SELECT on the table WITH GRANT OPTION and passes SELECT (id) to another reader. Owner-first revoked M's table grant first, which left the column grant behind. The next pick then ran SET ROLE M, and since M by then held nothing on the table, the REVOKE failed with "permission denied". Now column grants go first, then owner-made grants within each level. Nothing is orphaned that way, because a column grant can never lead to a table grant. Each of the three suites gained a test for this case. All three failed under the owner-first order and pass now (76 tests). Another agent porting this loop found the gap.

[FadhlanR]

[Claude Code 🤖] Question about scope: the header says the read-only role's members lose SELECT, but the loop only removes grants held directly by a reader. A reader that is also an INHERIT member of a non-reader role holding SELECT keeps reading after up(), and so does every reader if SELECT was ever granted to PUBLIC (a grant with a null grantee never matches the IN). If the contract is "no reader can read this table", one check here would cover every path, and it raises the same way the SET ROLE failure does:

SELECT reader_role.rolname INTO still_reading
  FROM <reader CTE> JOIN pg_roles reader_role USING (oid)
  WHERE has_any_column_privilege(reader_role.oid, 'proxy_endpoints'::regclass, 'SELECT')
  LIMIT 1;
IF still_reading IS NOT NULL THEN RAISE EXCEPTION ...; END IF;

has_any_column_privilege sees both table and column grants. It would also flag a reader that inherits the owner role, which seems right for a sealed table. If direct grants are the intended scope, a sentence saying so in the header would settle it. Non-blocking: this gap is older than this PR, and production and staging were already checked.

[habdelra]

[Claude Code 🤖] Direct grants are the scope here, so ecd2d7c adds a header sentence to all three migrations: "Only grants a reader holds itself are revoked: a reader that can read the table through PUBLIC or through a role outside the read-only role keeps that access." I left out the closing privilege check because it would make up() fail on any database where a reader inherits a non-reader role that holds SELECT. That would change what the migration does beyond this fix, and production and staging have no such path today. Covering reach through PUBLIC and other roles is tracked as separate follow-up work.