[habdelra]

Background and Goal

readonly_role, used by dashboards and read-only database access, is meant to be unable to read four tables: proxy_endpoints and incoming_webhooks (vendor keys and webhook signing secrets), and user_keys and key_write_tickets. The migrations that create or seal those tables revoke the SELECT grants made to the role and its members by name. Two paths still let those roles read:

• PUBLIC. After GRANT SELECT (api_key) ON proxy_endpoints TO PUBLIC, the role and every member can read api_key.
• Another role. A member that also belongs to a role holding SELECT reads the table through it.

Revoking SELECT from PUBLIC or from an unrelated role isn't a migration's call. This PR adds one migration, 1792193305492_check-readonly-role-cant-read-withheld-tables, that checks the outcome. It asks has_column_privilege about every column of the four tables for each read-only role, and raises an error naming the role, table and column if any of them can read one. The hint tells the operator to revoke the grant or role membership responsible. The migration grants and revokes nothing, and down() is a no-op.

Key decisions and non-obvious mechanics

• A new migration, not edits to the old ones. Staging and production have already recorded the migrations that create and seal these tables, so node-pg-migrate would skip a check added to them. A new migration runs on existing databases, and on fresh ones after the tables exist.
• Which roles count as read-only. readonly_role, plus every role reachable from it through memberships that carry inherit_option or set_option, including transitive ones (readonly_role → group → login). Two kinds of role are excluded:
  • A member holding only the ADMIN option reads nothing through the role. On Postgres 16 and later, a non-superuser that runs CREATE ROLE readonly_role (an RDS master user, for example) automatically holds exactly that membership.
  • A table's owner reads the table because it owns it.
• Raise, not warn. The role's provisioned members read only through the role, so on a correctly provisioned database the check finds nothing, and a positive result is a real exposure. A warning would be lost in the migrate job's log while the table stays readable.
• What the check covers. It checks what each role can read as itself. A member that can SET ROLE to an unrelated role reads as that role, and that is outside the check.
• Schema snapshot. The migration changes no schema, so packages/host/config/schema/ gets the existing snapshot renamed to the new timestamp rather than a re-dump.

Tests

readonly-role-withheld-tables-test stops up() in three cases: a PUBLIC column grant, a member reading through another role, and a login that inherits the role through a group. It lets up() through in three: a role outside the read-only role holding SELECT, an ADMIN-only member reading through another role, and a member that owns a table.

🤖 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-16T23:03:41.507070Z	3e9af22	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: 3e9af22f45

ℹ️ 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]

Put the privilege guard in a new migration

For any staging or production database that already records this migration filename in the migrations table, node-pg-migrate skips the file, so this newly added check never runs. These three migrations were introduced by earlier changes, and the same issue affects the edits to the incoming-webhook and user-key migrations; a new timestamped migration is needed to validate databases where the tables and grants already exist.

Useful? React with 👍 / 👎.

[habdelra]

[Claude Code 🤖] Agreed. Fixed in d5a7c2d. The check now lives in a new migration, 1792192933698_check-readonly-role-cant-read-withheld-tables, and the three applied migrations are untouched. It checks all four withheld tables, so it runs on databases that already recorded those migrations as well as on fresh ones.

[chatgpt-codex-connector]

Traverse transitive read-only role memberships

If readonly_role is granted to an intermediate group and a login inherits that group, this subquery returns only the intermediate group, not the login. A leaf login that also has direct or separately inherited SELECT therefore retains access without triggering this guard—for example, readonly_role -> reporting_group -> alice, with alice receiving SELECT elsewhere. The same direct-members-only query is duplicated in all three migrations, so the membership traversal needs to include transitive members.

Useful? React with 👍 / 👎.

[habdelra]

[Claude Code 🤖] Fixed in d5a7c2d. The check now walks memberships recursively from readonly_role, following every edge that carries inherit_option or set_option, so a login under readonly_role → group → login is checked. The new test stops when a role that inherits the read-only role through a group reads a table covers that chain.

[habdelra]

[Claude Code 🤖] This review covers the post-revoke readability check in the three sealing migrations: which roles it examines, what has_column_privilege does and doesn't count, and how it behaves when it fires. I tested each case against a throwaway postgres:16.3. I didn't run the realm-server suite, and I didn't look at the base branch's revoke and record logic beyond where this check depends on it.

Nothing in the diff itself blocks. One pre-existing question needs an answer from prod/staging before this stack deploys: does the migrating role hold an ADMIN-only membership in readonly_role? PG 16 creates one automatically when a non-superuser runs CREATE ROLE. If it does, up() strips the table owner's own SELECT, and the check only passes because of that.

Raise vs. warn: raising is the right call. A migration runs once, so a warning would be the only signal, it would sit in the migrate job's logs, and the table would stay readable after the migration is marked done. Raising is only safe if the role set is tight and the hint gives an operator a fix that works. A false positive halts every deploy until someone with DB access steps in. Comments 1 and 3 are about those two conditions.

1. Narrow "direct member" to memberships that carry inherit_option or set_option, and exclude the table owner, in the revoke loop and in this check. Run the pg_auth_members query in that thread first. Pre-existing, now load-bearing; blocks only if the query finds such a row. (Thread on the member subquery in 1791993196828_seal-proxy-endpoint-api-keys.js.)
2. A NOINHERIT member that can SET ROLE to a role with SELECT gets past the check. Change the wording, or also check SET-able roles. Non-blocking. (Thread on has_column_privilege.)
3. The hint can't fix superuser or pg_read_all_data members. Mention removing the membership. Non-blocking. (Thread on the HINT.)

Adjacent, not for this PR: each test file now defines createRole twice and builds up() statements in three or four places. A shared helper in tests/helpers would keep them from drifting apart.

[habdelra]

[Claude Code 🤖] Before this ships, please check staging and prod for an ADMIN-only membership in readonly_role, and narrow this "direct member" set (and the revoke loop's) so that kind of membership doesn't count.

Here is why it matters. On PG 16+, when a non-superuser with CREATEROLE runs CREATE ROLE, the new role gets an automatic pg_auth_members row: the creator becomes a member with admin_option = t, inherit_option = f, set_option = f. An RDS master user is exactly that kind of role. If it ran 1751981407344_setup-grafana-db-user.js and also runs these migrations, it is both a "direct member" and the table owner. I reproduced this on postgres:16.3 with a non-superuser CREATEROLE master that created the table and readonly_role. up() then revoked the owner's own SELECT on proxy_endpoints (the owner's grant shows up in the aclexplode snapshot under its own name), recorded it in readonly_role_revoked_select_grants, and afterward master got permission denied for table proxy_endpoints. I haven't checked whether the realm server connects as that same role. If it does, that is an outage.

Your check isn't the cause. It passes in that setup only because the owner's SELECT is already gone. But the check now depends on the same over-broad set. If someone fixes the revoke to leave the owner alone, this check will flag the owner and stop every deploy.

Fix:

• Count a member only when it actually takes on the role's privileges: inherit_option OR set_option. Those are PG 16 columns, so first confirm the RDS major version.
• Leave out tbl.relowner.
• Apply this to all three places the set appears in each migration (the revoke loop, this check, and the user-keys twin).

Query to run against prod and staging: SELECT m.rolname, a.admin_option, a.inherit_option, a.set_option FROM pg_auth_members a JOIN pg_roles m ON m.oid = a.member WHERE a.roleid = 'readonly_role'::regrole;

Classification: pre-existing (main already revokes from every member by name), and this PR now depends on it. Whether it blocks depends on that query: if it returns an ADMIN-only row for the migrating role, it blocks deploying this stack.

[habdelra]

[Claude Code 🤖] Confirmed on postgres:16.3, and fixed for this check in d5a7c2d. A membership counts only when it carries inherit_option OR set_option, so the creator's automatic ADMIN-only membership is skipped, and each table's relowner is excluded. Tests pin both cases: an ADMIN-only member reading through another role, and a member that owns a table. Production RDS runs postgres_engine_version = "16", so the PG 16 columns are available. The owner losing its own SELECT comes from the revoke loop in the already-applied sealing migrations, and that fix and any repair belong with that loop, not this check. The pg_auth_members query has gone to someone with production access.

[habdelra]

[Claude Code 🤖] has_column_privilege only counts privileges a role inherits, so it misses a member that has to SET ROLE to read. I verified this on 16.3 with a NOINHERIT member of readonly_role that also belongs to a role holding SELECT on proxy_endpoints. up() succeeded, has_column_privilege('<member>', 'proxy_endpoints', 'api_key', 'SELECT') returned false, and SET ROLE <other>; SELECT api_key FROM proxy_endpoints worked.

Two ways to handle it. Either word the header comment and error text as "can read without SET ROLE", or also check every role the member can SET ROLE to (pg_has_role(reader, r, 'SET') on 16+, or 'MEMBER'). The provisioned users are INHERIT, so I'd take the wording change.

Classification: regression in what the comment claims, not in behavior. Non-blocking. Applies to all three migrations.

[habdelra]

[Claude Code 🤖] Took the wording route in d5a7c2d. The migration header says the check covers what each role reads as itself, and that a member that can SET ROLE to some other role reads as that role, which is outside the check. The provisioned members are INHERIT.

[habdelra]

[Claude Code 🤖] In two cases the check raises and the hint's fix ("revoke the SELECT … reads through PUBLIC or another role") won't clear it. I verified both on 16.3:

• a SUPERUSER member of readonly_role
• a member of pg_read_all_data

Both make has_column_privilege true with no SELECT grant anywhere to revoke. Since a raise here halts the pre-deploy migrate job, the hint is the operator's whole runbook. Please make it cover role membership too, e.g. "…or remove %I from readonly_role (or from the role it reads through)". You could also branch on rolsuper to give that case its own message.

Classification: regression. Non-blocking. Applies to all three migrations.

[habdelra]

[Claude Code 🤖] Fixed in d5a7c2d. The hint is now "Revoke the grant or role membership that lets read

., then run the migration again." That covers a superuser member and a pg_read_all_data member, where there's no SELECT grant to revoke.

[github-actions]

Preview deployments

• Host staging preview

• Host production preview

Host Test Results

    1 files  ±0      1 suites  ±0   1h 45m 32s ⏱️ - 21m 11s
5 703 tests +2  5 692 ✅ +2  11 💤 ±0  0 ❌ ±0 
5 718 runs  +2  5 707 ✅ +2  11 💤 ±0  0 ❌ ±0 

Results for commit 7c129d3. ± Comparison against earlier commit 74add02.

Realm Server Test Results

    1 files  ±0    344 suites  ±0   2h 15m 23s ⏱️ + 20m 22s
5 796 tests ±0  5 796 ✅ ±0  0 💤 ±0  0 ❌ ±0 
5 859 runs  ±0  5 859 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7c129d3. ± Comparison against earlier commit 74add02.

[backspace]

[Claude Code 🤖] This pass judges convergence: whether the five earlier threads are resolved on the head commit, and whether anything left is blocking. I read the migration and the test at the head commit and the sibling migrations that use the same reader set. I didn't run the suite.

No blocking issues. All five earlier threads resolve in the code: the check is a new migration, the reader set is recursive and limited to INHERIT or SET edges, the owner is excluded, the header limits the claim to what a role reads as itself, and the hint covers role membership. The tests pin the ADMIN-only exclusion and the owner exclusion: remove either one and its test fails.

1. Merge and deploy order (question, non-blocking for the code). The head commit moves this migration later so that it runs "after the grant repair". No migration in this PR or another open PR sorts between the two timestamps. The open repair changes the three migrations that are already applied, and node-pg-migrate skips those on staging and production. So on those databases, this check is the first migration to meet a SELECT that a non-owner role granted to a member. If one exists, the check raises, and the deploy stops until an operator revokes the grant. Raising is the intended behavior, but please confirm one of two things before this merges: either the repair ships as its own new migration that sorts before this one, or the production and staging grant query (still open in the earlier thread) returned nothing.
2. Comment wording on the ADMIN-only exclusion: see the inline suggestion. Nit, non-blocking.

Percy is red with 57 visual changes. This PR changes no UI, so you can ignore it.

[backspace]

[Claude Code 🤖] Only a non-superuser creator gets this automatic membership. A superuser that runs CREATE ROLE gets none. The comment justifies an exclusion, so it should state the condition exactly:

Suggested change

	// holding only the ADMIN option reads nothing through the role (on Postgres 16
	// and later the role that creates `readonly_role` holds that membership), and
	// holding only the ADMIN option reads nothing through the role (on Postgres 16
	// and later a non-superuser that creates `readonly_role` holds that membership), and

Nit, non-blocking.

[habdelra]

[Claude Code 🤖] Applied in 7c129d3. The comment now says a non-superuser that creates readonly_role holds the ADMIN-only membership on Postgres 16 and later.

[habdelra]

[Claude Code 🤖] On the merge and deploy order: the second condition holds, and the first no longer applies.

• No repair migration is coming. The grant-repair migration that the re-stamp commit message mentions was dropped from the other open PR before it landed. Its only purpose was to restore a stripped owner SELECT, and the deployed databases turned out not to need that. No migration needs to sort before this one.
• Production and staging have no grant for this check to stop on. Both were queried read-only after the sealing migrations ran. On both, proxy_endpoints, incoming_webhooks, user_keys and key_write_tickets have exactly {postgres=arwdDxt/postgres} as their ACL, so the owner is the only grantee and there is nothing from any other grantor. readonly_role's only members are grafana and claude_readonly_user (inherit and set, granted by rdsadmin), and postgres, the owner, isn't a member. The check finds nothing on either database, so it won't stop a deploy.

Acknowledged on Percy: this PR changes no UI.