test(security): non-superuser installer invariant

[NikolayS]

What invariant

The partition-keys SECURITY DEFINER co-ownership invariant (blueprints/partition-keys/SPEC.md §6, T-security in §9): receive_partitioned reaches the admin-only pgque.get_batch_cursor(4) trusted-SQL hook not via any grant but because both functions share an OWNER — the role that ran devel/sql/pgque.sql. The invariant is "installed by the pgque install owner", explicitly NOT requiring superuser or pgque_admin. Every existing test runs against a superuser-owned install, where superuser privilege masks ownership — this is the first test to exercise the invariant as designed. (SPEC §9 marks T-security; the spec file is being edited on the PR-336 branch, so this PR deliberately makes no spec edit — a one-line "implemented" note can ride that branch.)

Why a shell harness

Role and database bootstrap needs a superuser, but the point of the test is that the install itself must not: the harness (superuser DSN) creates a pgque_nsu_installer_* role (CREATEROLE, no superuser, no pgque_admin) plus bare reader/writer app roles and two installer-owned databases, then runs \i devel/sql/pgque.sql under set role as the installer — set role applies that role's privileges to every permission/ownership check, so the install runs with genuinely non-superuser rights and the harness needs no passwords. That bootstrap/step split doesn't fit tests/run_all.sql (one database, one role), hence a standalone tests/security_nonsuperuser_install.sh mirroring tests/two_session_slot_claim.sh.

What it asserts, in order:

1. devel/sql/pgque.sql installs cleanly as a non-superuser owner (no findings — it does, including the conditional role-membership grants).
2. Ownership sanity: all partition functions + get_batch_cursor co-owned by the installer.
3. End-to-end as bare app roles: writer does keyed sends; reader does subscribe_slot → claim_slot → receive_partitioned → ack_partitioned over n=2, with hash-routing and per-key single-slot assertions.
4. The reader calling get_batch_cursor directly still fails 42501 (both overloads, mirroring tests/test_security_get_batch_cursor.sql).
5. partition_consumer / partition_slot unreadable by reader and writer (42501).

The negative control (the "red" that proves teeth)

In a second, identical install: after proving the reader flow works, the superuser reassigns receive_partitioned's OWNER to a foreign role that is deliberately given every other privilege the function body needs (pgque_reader membership for next_batch, EXECUTE on _slot_guard/_slot_batch/_slot_name) — everything except co-ownership. The previously-working reader flow must then fail with 42501 specifically on get_batch_cursor (asserted on sqlstate and message). If the flow still worked, the harness raises NEGATIVE CONTROL HAS NO TEETH and fails. Co-ownership is load-bearing, and this harness detects its absence.

CI wiring — follow-up

Not wired into tests/run_all.sql (needs the superuser bootstrap + throwaway databases + cluster roles). Follow-up: a dedicated CI job invoking the harness with PGQUE_TEST_SUPERUSER_DSN across PG 14–18.

Verification (local, PG 18.3)

$ PGQUE_TEST_SUPERUSER_DSN='dbname=postgres user=postgres' tests/security_nonsuperuser_install.sh
ok: 00_bootstrap
ok: 10_install_pgque_nsu_main_56293 (non-superuser install succeeded)
ok: 10_install_pgque_nsu_negctl_56293 (non-superuser install succeeded)
ok: 15_grants
ok: 20_ownership
ok: 30_flow_main
ok: 40_negctl_pre
ok: 50_negctl_flip
ok: 55_negctl_post
... NOTICE:  PASS: bare pgque_reader end-to-end (subscribe/claim/receive_partitioned/ack) under a non-superuser install owner
... NOTICE:  PASS: reader blocked from get_batch_cursor/3 and /4 (42501)
... NOTICE:  PASS: partition tables not readable by pgque_reader
... NOTICE:  PASS: partition tables not readable by pgque_writer
... NOTICE:  PASS: negctl pre-flip reader flow works
... NOTICE:  PASS: negative control -- ownership flip breaks the reader flow with 42501 on get_batch_cursor
PASS: security_nonsuperuser_install -- co-ownership invariant holds under a non-superuser, non-pgque_admin install owner (and the harness detects its absence)
exit=0

shellcheck: clean. Cleanup verified: no leftover pgque_nsu_* roles or databases.

🤖 Generated with Claude Code

[NikolayS]

samorev Code Review Report

• PR: test(security): non-superuser installer invariant #349 - test(security): non-superuser installer invariant
• Author: @NikolayS
• AI-Assisted: Yes

Pipeline	Coverage
PASS	Not reported

---

REVIEW FINDINGS (10)

HIGH MR/PR state - Review target is draft

The review target is still marked as draft.
Fix: Mark it ready for review before merge.

HIGH [bugs] On PG16+ against a cluster where the pgque_* roles do NOT pre-exist, the post-install assertions are guaranteed to fail. Since PG16, a non-superuser CREATEROLE role that creates a role is automatically granted membership in it WITH ADMIN OPTION (createrole_self_grant only controls whether INHERIT/SET ride along — the pg_auth_members row always exists). pg_has_role(..., 'member') is documented as membership "without regard to what specific privileges are conferred", so it returns true. The script's own bootstrap comment states that on a fresh cluster "the installer CREATES the roles itself (that is what CREATEROLE is for)" — exactly the path that trips this. The local PG 18.3 verification in the PR ran the other branch (pgque_admin pre-existed), so this path was never exercised, and it is precisely the path a fresh CI container on the proposed PG 14–18 matrix takes. Result: green on PG 14/15, hard-fail on PG 16/17/18.

Fix: The substantive invariant is "the installer does not need pgque_admin's privileges", which is usage mode, not member mode. Either switch the three checks to pg_has_role(current_user, '<role>', 'usage'), or make the bootstrap deterministic by having the superuser pre-create pgque_admin/pgque_reader/pgque_writer unconditionally (and grant pgque_reader/pgque_writer to pgque_admin) before the install, so the installer never creates them and both cluster states collapse into one code path. The latter also removes the CREATEROLE-creator grant entirely and fixes the cleanup leak below.

MEDIUM [bugs] Cleanup leaks cluster-wide state and the leak is only a warning, so the run still exits 0. pgque_reader/pgque_writer/pgque_admin are never dropped, so a run against a fresh cluster permanently mutates it; a second run then takes the other bootstrap branch, making the harness non-idempotent in a way that silently changes what it tests. Worse, on PG16+ fresh clusters the installer holds the creator ADMIN OPTION grants on those roles, so drop role ${installer} fails — the comment anticipates exactly this ("can block DROP ROLE — warn, don't fail") but the consequence is a leaked pgque_nsu_installer_<pid> role on every first run, reported only on stderr while the script prints PASS and returns 0.

Fix: Before dropping the installer, run drop owned by ${installer} / revoke ... granted by ${installer} (or reassign owned) against the bootstrap database so the dependent membership grants go away; also record whether the bootstrap created pgque_reader/pgque_writer/pgque_admin and drop them in that case. Suppressing output with 2>&1 hides the actual server error — capture it into ${workdir} and print it with the warning so the leak is diagnosable.

MEDIUM [tests] Nothing in this PR executes the harness. It is not in tests/run_all.sql and no CI job is added (the PR states the wiring is a follow-up). A security-invariant regression test that no automated run invokes provides zero regression protection and will bit-rot — and given the PG16+ issue above, the first thing CI wiring would surface is a failure. The invariant is described as first-of-its-kind coverage for T-security, which makes landing it unexecuted the highest-cost case for deferral.

# NOT wired into tests/run_all.sql: run_all runs inside one database as one role ... CI wiring is a follow-up. — and changed_files contains only tests/security_nonsuperuser_install.sh.
Fix: Add the CI job in this PR rather than deferring it. The harness already takes everything it needs from one env var, so a matrix step that sets PGQUE_TEST_SUPERUSER_DSN and invokes the script (skipping cleanly when the var is unset) is small, and it is what proves the harness works on more than the author's PG 18.3.

MEDIUM [tests] Every assertion in the harness except the two explicit raise exception guards depends on plpgsql.check_asserts, which is a settable GUC. If it is off in the target cluster (or in ALTER DATABASE/ALTER ROLE settings inherited by the throwaway databases), all assert statements become no-ops: the ownership check, the 42501 sqlstate checks, the message check on get_batch_cursor, the count and hash-routing checks, and the non-superuser precondition all silently pass, and the script prints its final PASS line. For a harness whose stated purpose is proving a security invariant has teeth, the teeth are GUC-dependent.

Fix: Add set plpgsql.check_asserts = on; at the top of each generated SQL step (or once in the bootstrap via alter database ... set plpgsql.check_asserts = on), and self-check it — e.g. a do $$ begin assert false; end $$; in a block that must error, failing the run if it does not.

MEDIUM [bugs] Any failure of the install step is reported as a product finding against devel/sql/pgque.sql, including failures that are harness bugs. The generated file also contains the pre-install current_user/rolsuper assertions and the three post-install membership assertions; a psql connection error, a \connect failure, or the PG16 membership problem above all print FAIL: FINDING -- devel/sql/pgque.sql does not install as a non-superuser owner. In a harness whose output is a security verdict, misattributing harness breakage to the product under test is a real failure mode.

Fix: Split the step: run the pre-install preconditions and the post-install membership assertions as their own run_step calls (generic FAIL: step <name> message), and reserve the FINDING -- wording for the file that contains only begin; \i devel/sql/pgque.sql; commit;.

LOW [bugs] The negative control's post-flip phase re-claims a slot that phase A claimed and never released, and discards the return value without asserting it. 40_negctl_pre calls claim_slot(...'w0') and ack_partitioned but no release_slot. If the re-claim in 55_negctl_post returns NULL (or a new epoch that invalidates the intended path), the subsequent failure may not originate where the test believes. It happens to be safe today only because a non-insufficient_privilege error would propagate and fail the step — but if the lease guard itself raises 42501, the block catches it and the run fails on the position('get_batch_cursor' in v_msg) assert with a misleading message instead of on the invariant.

Fix: Mirror 30_flow_main: capture the epoch into a variable and assert v_epoch is not null before proceeding, and add an explicit release_slot at the end of 40_negctl_pre so phase C starts from a known-free slot.

LOW [bugs] The bootstrap's conditional block guards on pgque_admin existing but then references pgque_reader and pgque_writer unguarded. On a cluster in a partial state (pgque_admin present, pgque_reader absent — e.g. left over from an aborted install, which this harness itself can produce per the cleanup finding), pg_has_role('pgque_admin', 'pgque_reader', 'member') raises undefined_object and the bootstrap step fails with no useful message.

Fix: Test each role's existence independently (select 1 from pg_roles where rolname = 'pgque_reader') before calling pg_has_role on it, or drop the conditional entirely by pre-creating all three roles as superuser per the first finding's fix.

LOW [tests] The negative control hard-codes the exact privilege set receive_partitioned's body needs (pgque_reader membership, EXECUTE on _slot_guard/_slot_batch/_slot_name). If that body later calls another owner-gated helper, the flip will fail on the new helper rather than on get_batch_cursor, and the harness will report negctl: expected the denial to be on get_batch_cursor, got: ... — a confusing failure that looks like a product regression but is test drift. There is no mechanism tying the grant list to the function body.

Fix: Grant EXECUTE on all of pgque's internal helpers except get_batch_cursor programmatically (grant execute on all functions in schema pgque to ${other_owner} followed by revoke execute on function pgque.get_batch_cursor(...) from ${other_owner}), so the control isolates exactly one variable and cannot drift as the body changes. Add a comment noting that the revoke list is the intentional coupling point.

LOW [tests] The ownership sanity check runs only against db_main; db_negctl is never verified pre-flip. The negative control's whole argument is "these two installs are identical, and the only difference in phase B is ownership" — but the baseline co-ownership of db_negctl is assumed rather than asserted. Also, nack_partitioned and release_slot appear in the ownership assertion but are never exercised by any flow, and neither flow verifies post-ack state (e.g. that a second receive_partitioned returns zero rows), so ack_partitioned is only proven not to raise.

Fix: Parameterize 20_ownership.sql over both databases (a loop like the install step already uses), and add a post-ack drain assertion plus a release_slot/nack_partitioned leg to 30_flow_main so the ownership list and the exercised surface match.

---

Summary

Area	Findings	Potential	Filtered
CI/Pipeline	0	0	0
Security	0	0	0
Bugs	0	5	0
Tests	1	3	0
Guidelines	0	0	0
Docs	0	0	0
Metadata	1	0	0

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

Review metadata

provider=github
kind=pr
project=NikolayS/PgQue
number=349
target=github:NikolayS/PgQue#349
state=OPEN
draft=true
diff_lines=465
diff_added=459
diff_removed=0
diff_bytes=17488
comments_count=0
commits_count=1
ci_status=success
ci_summary=total=17 success=17 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

---

samorev-assisted review (AI analysis by Tanya301/samorev)