Follow-on to #18: run application-level tests as a genuine non-superuser - #30
Open
jnasbyupgrade wants to merge 8 commits into
Open
Conversation
Adds the Claude Code GitHub Actions workflows: `claude.yml` (@claude mentions) and `claude-code-review.yml` (auto-review on PRs from the jnasbyupgrade fork via pull_request_target, gated to that fork). Requires the `CLAUDE_CODE_OAUTH_TOKEN` secret on this repo and the Claude GitHub App granted access. Part of enabling Claude Code review across the postgresql-extensions repos. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ruser As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled membership in the new role, so the install's `SET ROLE test_factory__owner` fails unless the current role is a superuser (which bypasses the check). This surfaces only on non-superuser installs (e.g. RDS/Aurora). Grant the role back to the installing role WITH SET, gated on PG16+ (pre-16 GRANT already permits SET ROLE). Unconditional, so it also covers a pre-existing role where CREATE ROLE was a no-op. Fixes Postgres-Extensions#14 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Postgres-Extensions#14) The failure can't be reproduced under pg_regress, which runs as a superuser that bypasses the SET ROLE check. Instead assert the state the fix establishes: after install the installing role holds a SET-enabled membership in test_factory__owner (PG16+; skipped with identical output pre-16). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # .github/workflows/claude-code-review.yml # pgxntool/pgtle.sh
pg_regress always connects as a superuser, so CREATE EXTENSION itself can't be exercised as non-superuser here (test_factory isn't marked trusted, and making it so is a real production behavior change, not a test-harness concern). But the application-level testing that runs after install doesn't need to stay superuser, and testing it that way misses exactly the class of bug issue Postgres-Extensions#14 was. Two changes: - test/helpers/create.sql now uses SET SESSION AUTHORIZATION instead of SET ROLE to switch into test_role. SET ROLE only changes current_user; a further SET ROLE's own permission check (like the one test_factory's install performs, and like issue Postgres-Extensions#14's bug) is based on session_user, which SET ROLE leaves untouched. Under pg_regress's superuser connection, that means SET ROLE alone silently leaves this whole class of check bypassed for the rest of the file -- SET SESSION AUTHORIZATION actually drops it. - New test/sql/security.sql proves the public tf.* API needs nothing beyond what a freshly-created, unprivileged role gets by default (no owned schema, no explicit grants, not a member of test_factory__owner): register/get work end to end, and the role still can't SET ROLE into test_factory__owner. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ions#18 test gap Mark both control files `superuser = false` (the pre-PG13 mechanism, not `trusted` -- `trusted` is an unrecognized control-file key on PG10-12, which this project's CI still tests, and errors out entirely there, not just for non-superuser attempts). Verified empirically against this container's PG12 and PG17 clusters. test/sql/install.sql now creates a disposable NOSUPERUSER + CREATEROLE role (mirroring what a real RDS/Aurora master user has) and installs through it via SET SESSION AUTHORIZATION, replacing the indirect pg_auth_members proxy check from Postgres-Extensions#18 with a genuine end-to-end repro: before the Postgres-Extensions#18 fix this fails with "must be able to SET ROLE test_factory__owner"; after the fix it succeeds. Two extra grants were needed beyond CREATEROLE, found by actually running this rather than reasoning about it: USAGE on the tap schema (a pgtap test-harness necessity, unrelated to what's under test) and CREATE on the current database (never granted to PUBLIC by default -- only CONNECT/TEMP are -- unlike what I'd assumed). Also fixed a real local-iteration flakiness this surfaced: test_factory__owner is deliberately left behind by DROP EXTENSION so a real install/uninstall cycle by the same installer keeps working, but this test creates a fresh disposable installer role every run, so an orphaned owner role from a previous run of this file belongs to an installer that no longer exists, breaking the GRANT ... WITH SET. install.sql now drops both roles at start and end. Verified stable across many repeated `make test` runs against the same cluster. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
3 tasks
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jnasbyupgrade
marked this pull request as ready for review
August 12, 2026 23:19
jnasbyupgrade
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #18: run more of the application-level test suite as a genuine non-superuser.
Background
While updating #18, the question came up: could we run more of this test suite as a non-superuser, given #18 was exactly the kind of bug (
SET ROLEpermission check) that a superuser-run test suite can't naturally catch?test/helpers/create.sqlswitches into a non-superusertest_rolefor most ofbase.sql/pgtap.sql-- but viaSET ROLE, notSET SESSION AUTHORIZATION. That distinction matters:SET ROLEonly changescurrent_user; Postgres's own permission check for a furtherSET ROLE(the exact class of check #18's bug was in) is based onsession_user, whichSET ROLEleaves untouched. Under pg_regress's superuser connection, that silently bypassed this whole class of check for the rest of the file. Fixed by switching toSET SESSION AUTHORIZATION. Also addedtest/sql/security.sql, proving the publictf.*API needs nothing beyond a freshly-created, unprivileged role.Scope note: this PR originally also added a separate non-superuser
CREATE EXTENSIONrepro (a disposable installer role intest/sql/install.sql). That's dropped now --test/install/load.sql's fresh/update branch (landed as part of #18's own review iterations, now onmaster) already does exactly this as the real install for the whole suite, making the separate repro redundant;test/sql/install.sqlitself was deleted upstream for the same reason. This PR is rebased onto currentmasterand now contains only the still-missing piece.Test plan
make test--load/base/pgtappass;securityruns correctly (4/4 pgtap assertions pass) but needsmake resultsto accept its expected output, per this repo's human-verifies-expected-output conventionmake test-update-- same resulttest/expected/base.out/pgtap.outunchanged🤖 Generated with Claude Code