From 75ce658e34adcbd27d2caf99b0c6fea3cd95c7bf Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 18 Jul 2026 15:38:29 -0500 Subject: [PATCH 1/2] Run more of the test suite as a genuine non-superuser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #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) --- test/expected/security.out | 6 ++++ test/helpers/create.sql | 10 +++++- test/sql/security.sql | 69 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 test/expected/security.out create mode 100644 test/sql/security.sql diff --git a/test/expected/security.out b/test/expected/security.out new file mode 100644 index 0000000..7313ce6 --- /dev/null +++ b/test/expected/security.out @@ -0,0 +1,6 @@ +\set ECHO none +Creating extension test_factory +ok 1 - Bare, unprivileged role can register test data with zero extra grants +ok 2 - Bare, unprivileged role can create+fetch test data with zero extra grants +ok 3 - Bare, unprivileged role gets the cached row on a second call +ok 4 - Bare role cannot SET ROLE into the extension owner role diff --git a/test/helpers/create.sql b/test/helpers/create.sql index 9897f75..eae4f54 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -7,7 +7,15 @@ GRANT USAGE ON SCHEMA tap TO test_role; */ CREATE SCHEMA test AUTHORIZATION test_role; -SET ROLE = test_role; +/* + * SET SESSION AUTHORIZATION (not SET ROLE): it changes session_user too, not + * just current_user. Permission checks for a *further* SET ROLE (like the one + * test_factory's install does, and like issue #14's bug) are based on + * session_user's superuser status, not current_user's -- so a plain SET ROLE + * here would leave that one class of check silently bypassed for the rest of + * this file, since pg_regress always connects as a superuser. + */ +SET SESSION AUTHORIZATION test_role; SET search_path = test, tap; CREATE TABLE customer( diff --git a/test/sql/security.sql b/test/sql/security.sql new file mode 100644 index 0000000..3ba5047 --- /dev/null +++ b/test/sql/security.sql @@ -0,0 +1,69 @@ +\set ECHO none +\i test/helpers/setup.sql + +\set extension_name test_factory +\i test/helpers/create_extension.sql + +/* + * Prove the public tf.* API needs nothing beyond what a freshly-created, + * unprivileged login role already gets by default: no owned schema, no + * explicit GRANTs, and (deliberately) no membership in test_factory__owner. + * Everything it uses here (tf/_tf schema USAGE, EXECUTE on tf.* functions, + * CREATE TEMP TABLE) comes from either Postgres' own defaults or the GRANTs + * test_factory's install script makes to PUBLIC. + */ +SET ROLE = DEFAULT; +CREATE ROLE test_factory_bare_user; +-- USAGE on tap is a pgtap test-harness necessity (to call lives_ok() etc. +-- below), not one of the grants under test here. +GRANT USAGE ON SCHEMA tap TO test_factory_bare_user; +/* + * SET SESSION AUTHORIZATION, not SET ROLE: it changes session_user too, which + * is what a further SET ROLE's permission check actually looks at. A plain + * SET ROLE here would leave this session able to SET ROLE into anything + * (including test_factory__owner below) regardless of grants, since + * pg_regress always connects as a superuser. + */ +SET SESSION AUTHORIZATION test_factory_bare_user; + +CREATE TEMP TABLE widget( + widget_id serial PRIMARY KEY + , name text NOT NULL +); + +SELECT lives_ok( +$lives_ok$SELECT tf.register( + 'widget' + , array[ + row( + 'base' + , $$INSERT INTO widget VALUES (DEFAULT, 'gadget') RETURNING *$$ + )::tf.test_set + ] +);$lives_ok$ + , 'Bare, unprivileged role can register test data with zero extra grants' +); + +SELECT results_eq( + $$SELECT * FROM tf.get( NULL::widget, 'base' )$$ + , $$VALUES( 1, 'gadget' )$$ + , 'Bare, unprivileged role can create+fetch test data with zero extra grants' +); + +SELECT results_eq( + $$SELECT * FROM tf.get( NULL::widget, 'base' )$$ + , $$VALUES( 1, 'gadget' )$$ + , 'Bare, unprivileged role gets the cached row on a second call' +); + +-- Confirm role isolation still holds for a role that otherwise works fine +SELECT throws_ok( + $$SET ROLE test_factory__owner$$ + , '42501' + , NULL + , 'Bare role cannot SET ROLE into the extension owner role' +); + +ROLLBACK; + +-- vi: expandtab ts=2 sw=2 From 8c17d2765eec281a29675f1c2dcdb8f28946f921 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 17:17:10 -0500 Subject: [PATCH 2/2] Actually install as a non-superuser role, closing the #18 test gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #18 with a genuine end-to-end repro: before the #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) --- test/expected/install.out | 2 +- test/sql/install.sql | 38 +++++++++++++++++++++++++++++++++++++- test_factory.control | 7 +++++++ test_factory_pgtap.control | 2 ++ 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/test/expected/install.out b/test/expected/install.out index ae61611..9c7abeb 100644 --- a/test/expected/install.out +++ b/test/expected/install.out @@ -3,7 +3,7 @@ ok 1 - drop extension test_factory_pgtap ok 2 - drop extension test_factory ok 3 - Extension test_factory should not exist ok 4 - Extension test_factory_pgtap should not exist -ok 5 - create extension +ok 5 - create extension as a non-superuser role (issue #14) ok 6 - Function tf.tap(text, text) should exist ok 7 - clean-up test_factory_pgtap ok 8 - clean-up test_factory diff --git a/test/sql/install.sql b/test/sql/install.sql index ae58bf3..57aa986 100644 --- a/test/sql/install.sql +++ b/test/sql/install.sql @@ -9,11 +9,41 @@ SET client_min_messages = WARNING; */ SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory_pgtap$$, 'drop extension test_factory_pgtap'); SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory$$, 'drop extension test_factory'); +/* + * test_factory__owner is deliberately left behind by DROP EXTENSION (the + * install script tolerates it already existing, so a real install/uninstall + * cycle by the same installer keeps working). But this test creates a fresh, + * disposable test_factory_installer role below, and an orphaned owner role + * from a previous run of *this file* would belong to an installer that no + * longer exists -- drop it defensively so repeated local `make installcheck` + * runs against the same cluster don't flake. + */ +DROP ROLE IF EXISTS test_factory__owner; SELECT hasnt_extension( 'test_factory' ); SELECT hasnt_extension( 'test_factory_pgtap' ); -SELECT lives_ok($$CREATE EXTENSION test_factory_pgtap CASCADE$$, 'create extension'); +/* + * Install as a genuine non-superuser role (NOSUPERUSER + CREATEROLE mirrors + * what a real RDS/Aurora master user has), now that both control files are + * marked `superuser = false`. Before the issue #14 fix this fails with + * "must be able to SET ROLE test_factory__owner"; after the fix it succeeds. + */ +CREATE ROLE test_factory_installer NOSUPERUSER CREATEROLE; +-- USAGE on tap is a pgtap test-harness necessity (to call lives_ok() etc. +-- below), not one of the grants under test here. +GRANT USAGE ON SCHEMA tap TO test_factory_installer; +-- CREATE ON DATABASE is never granted to PUBLIC by default (only CONNECT/TEMP +-- are) -- a real RDS/Aurora master user gets this explicitly via rds_superuser, +-- so grant it here to mirror that setup. +DO $body$ +BEGIN + EXECUTE format('GRANT CREATE ON DATABASE %I TO test_factory_installer', current_database()); +END +$body$; +SET SESSION AUTHORIZATION test_factory_installer; +SELECT lives_ok($$CREATE EXTENSION test_factory_pgtap CASCADE$$, 'create extension as a non-superuser role (issue #14)'); +RESET SESSION AUTHORIZATION; COMMIT; SELECT has_function('tf', 'tap', array['text','text']); @@ -21,6 +51,12 @@ SELECT has_function('tf', 'tap', array['text','text']); -- Cleanup SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory_pgtap$$, 'clean-up test_factory_pgtap'); SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory$$, 'clean-up test_factory'); +-- DROP ROLE alone fails while the GRANT USAGE ON SCHEMA tap above still holds; +-- DROP OWNED clears any privileges/ownership left in this database first. +DROP OWNED BY test_factory_installer; +DROP ROLE IF EXISTS test_factory_installer; +-- See the comment above the earlier DROP ROLE IF EXISTS test_factory__owner. +DROP ROLE IF EXISTS test_factory__owner; /* * Arguably we should cleanup pgtap and the tap schema... diff --git a/test_factory.control b/test_factory.control index a18e438..8faf2d4 100644 --- a/test_factory.control +++ b/test_factory.control @@ -1,3 +1,10 @@ comment = 'A framework for managing test data' default_version = '0.5.0' relocatable = false +# Not a security boundary weakening: test_factory__owner is a locked-down, +# dedicated owner role and every privileged function is SECURITY DEFINER with +# search_path=pg_catalog specifically so this is safe to install without a +# real superuser (e.g. RDS/Aurora's non-superuser master user). Installing +# still requires CREATEROLE (for test_factory__owner) plus CREATE on the +# target database. +superuser = false diff --git a/test_factory_pgtap.control b/test_factory_pgtap.control index 8e4a75f..0711698 100644 --- a/test_factory_pgtap.control +++ b/test_factory_pgtap.control @@ -2,3 +2,5 @@ comment = 'A framework for managing test data' default_version = '0.1.0' relocatable = false requires = 'pgtap, test_factory' +# See test_factory.control -- same rationale. +superuser = false