From f21fab8c943dd3ba5ea149596ed2161148cc388b Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 17 Aug 2026 15:07:11 -0500 Subject: [PATCH 1/4] Run application-level tests as a genuine non-superuser (issue #14 follow-on) 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. The non-superuser CREATE EXTENSION repro this PR originally added separately (a disposable test_factory_installer role in test/sql/install.sql) is dropped: test/install/load.sql's fresh/update branch now does exactly this, as the real install for the whole suite, making that separate repro redundant. test/sql/install.sql itself was already deleted upstream for the same reason. Co-Authored-By: Claude Sonnet 5 --- test/helpers/create.sql | 10 ++++++- test/sql/security.sql | 66 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 test/sql/security.sql diff --git a/test/helpers/create.sql b/test/helpers/create.sql index 775a299..4d80567 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -11,7 +11,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..d216c18 --- /dev/null +++ b/test/sql/security.sql @@ -0,0 +1,66 @@ +\set ECHO none +\i test/helpers/setup.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 548c9b7912ecb16b46a4a8a0b5ce5efea72e7b5f Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 17 Aug 2026 15:50:10 -0500 Subject: [PATCH 2/4] security.sql: use a psql variable for the bare test role, not a literal Matches test_role/installer_role's existing pattern in test/roles.sql: a role name that other files might need should be defined there once, not inlined, so a rename only touches one place. Also converts the resulting/existing 2+-line -- comment stacks in roles.sql and security.sql to /* */, per the comment-stacked-dashes lint rule tightened in the linter submodule after this branch's original commits were written (2026-08-12, well after this branch's last real CI run on 2026-07-30) -- not a rule violation introduced by this change. Co-Authored-By: Claude Sonnet 5 --- test/roles.sql | 8 ++++++++ test/sql/security.sql | 12 +++++++----- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/test/roles.sql b/test/roles.sql index 4ab7010..9f65807 100644 --- a/test/roles.sql +++ b/test/roles.sql @@ -10,4 +10,12 @@ -- test/install/load.sql only -- see its own comment. \set installer_role test_factory_installer +/* + * test/sql/security.sql only -- see its own comment. Deliberately not + * test_role: that role is set up by test/helpers/create.sql for other + * files, and this one exists specifically to have no setup beyond + * Postgres' own role defaults. + */ +\set bare_role test_factory_bare_user + -- vi: expandtab ts=2 sw=2 diff --git a/test/sql/security.sql b/test/sql/security.sql index d216c18..bf8273d 100644 --- a/test/sql/security.sql +++ b/test/sql/security.sql @@ -10,10 +10,12 @@ * 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; +CREATE ROLE :bare_role; +/* + * 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 :bare_role; /* * 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 @@ -21,7 +23,7 @@ GRANT USAGE ON SCHEMA tap TO test_factory_bare_user; * (including test_factory__owner below) regardless of grants, since * pg_regress always connects as a superuser. */ -SET SESSION AUTHORIZATION test_factory_bare_user; +SET SESSION AUTHORIZATION :bare_role; CREATE TEMP TABLE widget( widget_id serial PRIMARY KEY From 02bb9d2ec53148920dc59ccdb91e70e7a8317433 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 16:53:26 -0500 Subject: [PATCH 3/4] Add expected output for test/sql/security.sql Hand-copied from test/results/ rather than via `make results`: that target's verify-results-pgtap.sh safeguard can't accept a brand-new test file's first baseline (any diff from no-baseline-yet trips its regression.diffs fallback check, even with zero real `not ok` lines) -- filed as Postgres-Extensions/pgxntool#119. Content verified directly: 4/4 real pgtap assertions pass, nothing else in the file. Co-Authored-By: Claude Sonnet 5 --- test/expected/security.out | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 test/expected/security.out diff --git a/test/expected/security.out b/test/expected/security.out new file mode 100644 index 0000000..ce72300 --- /dev/null +++ b/test/expected/security.out @@ -0,0 +1,5 @@ +\set ECHO none +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 From 14c070c3a7cb1ca3b2771c21b1af5d762dc7fcdc Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 14:48:49 -0500 Subject: [PATCH 4/4] Fold security.sql's bare-role proof into the existing tests test_role owns nothing test_factory-related (no tf/_tf ownership beyond what PUBLIC already gets, and -- per create.sql's own comment -- deliberately no test_factory__owner membership), so base.sql/pgtap.sql running under test_role already proved the positive claim security.sql existed for (register/get works with nothing beyond default + tap grants). The only gap was that nothing actually asserted the negative claim the comment only stated as intent. Closed with pgtap's isnt_member_of(), which checks the pg_auth_members row directly -- simpler than throws_ok on a live SET ROLE, and version-independent (no PG16 SET-option branching needed). security.sql, its expected output, and the now-unused bare_role are removed as redundant. Co-Authored-By: Claude Sonnet 5 --- test/expected/base.out | 43 ++++++++++++------------ test/expected/pgtap.out | 23 +++++++------ test/expected/security.out | 5 --- test/helpers/create.sql | 5 +++ test/roles.sql | 8 ----- test/sql/security.sql | 68 -------------------------------------- 6 files changed, 39 insertions(+), 113 deletions(-) delete mode 100644 test/expected/security.out delete mode 100644 test/sql/security.sql diff --git a/test/expected/base.out b/test/expected/base.out index 7af44d8..4f9e474 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,22 +1,23 @@ \set ECHO none -ok 1 - Register test customers -ok 2 - Create function customer__add -ok 3 - Register test invoices -ok 4 - Ensure original_role temp table was dropped -ok 5 - Security definer function _tf.get has search_path=pg_catalog -ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 7 - Security definer function _tf.table_create has search_path=pg_catalog -ok 8 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 9 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 10 - customer table is empty -ok 11 - invoice table is empty -ok 12 - invoice factory output -ok 13 - invoice table content -ok 14 - customer table content -ok 15 - invoice factory second call -ok 16 - invoice table content stayed constant -ok 17 - customer table content stayed constant -ok 18 - Test function factory -ok 19 - customer table has new row -ok 20 - truncate invoice -ok 21 - invoice factory get remains the same after truncate +ok 1 - test_role is not a member of test_factory__owner +ok 2 - Register test customers +ok 3 - Create function customer__add +ok 4 - Register test invoices +ok 5 - Ensure original_role temp table was dropped +ok 6 - Security definer function _tf.get has search_path=pg_catalog +ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 8 - Security definer function _tf.table_create has search_path=pg_catalog +ok 9 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 10 - Security definer function _tf.test_factory__set has search_path=pg_catalog +ok 11 - customer table is empty +ok 12 - invoice table is empty +ok 13 - invoice factory output +ok 14 - invoice table content +ok 15 - customer table content +ok 16 - invoice factory second call +ok 17 - invoice table content stayed constant +ok 18 - customer table content stayed constant +ok 19 - Test function factory +ok 20 - customer table has new row +ok 21 - truncate invoice +ok 22 - invoice factory get remains the same after truncate diff --git a/test/expected/pgtap.out b/test/expected/pgtap.out index 1793e6e..3f11a66 100644 --- a/test/expected/pgtap.out +++ b/test/expected/pgtap.out @@ -1,14 +1,15 @@ \set ECHO none ok 1 - test_factory_pgtap depends on test_factory (control file requires is real and enforced) -ok 2 - Register test customers -ok 3 - Create function customer__add -ok 4 - Register test invoices -ok 5 - Ensure original_role temp table was dropped -ok 6 - Security definer function _tf.get has search_path=pg_catalog -ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 8 - Security definer function _tf.table_create has search_path=pg_catalog -ok 9 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 10 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 11 - Get test data set "base" for table invoice +ok 2 - test_role is not a member of test_factory__owner +ok 3 - Register test customers +ok 4 - Create function customer__add +ok 5 - Register test invoices +ok 6 - Ensure original_role temp table was dropped +ok 7 - Security definer function _tf.get has search_path=pg_catalog +ok 8 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 9 - Security definer function _tf.table_create has search_path=pg_catalog +ok 10 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 11 - Security definer function _tf.test_factory__set has search_path=pg_catalog ok 12 - Get test data set "base" for table invoice -ok 13 - Ensure we get sane error for a non-existent table +ok 13 - Get test data set "base" for table invoice +ok 14 - Ensure we get sane error for a non-existent table diff --git a/test/expected/security.out b/test/expected/security.out deleted file mode 100644 index ce72300..0000000 --- a/test/expected/security.out +++ /dev/null @@ -1,5 +0,0 @@ -\set ECHO none -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 4d80567..4f40f43 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -9,6 +9,11 @@ GRANT USAGE ON SCHEMA tap TO :test_role; * DO NOT GRANT test_role TO test_factory__owner; the whole point test_role is * to check for security problems. */ +SELECT isnt_member_of( + 'test_factory__owner' + , :'test_role' + , 'test_role is not a member of test_factory__owner' +); CREATE SCHEMA test AUTHORIZATION :test_role; /* diff --git a/test/roles.sql b/test/roles.sql index 9f65807..4ab7010 100644 --- a/test/roles.sql +++ b/test/roles.sql @@ -10,12 +10,4 @@ -- test/install/load.sql only -- see its own comment. \set installer_role test_factory_installer -/* - * test/sql/security.sql only -- see its own comment. Deliberately not - * test_role: that role is set up by test/helpers/create.sql for other - * files, and this one exists specifically to have no setup beyond - * Postgres' own role defaults. - */ -\set bare_role test_factory_bare_user - -- vi: expandtab ts=2 sw=2 diff --git a/test/sql/security.sql b/test/sql/security.sql deleted file mode 100644 index bf8273d..0000000 --- a/test/sql/security.sql +++ /dev/null @@ -1,68 +0,0 @@ -\set ECHO none -\i test/helpers/setup.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 :bare_role; -/* - * 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 :bare_role; -/* - * 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 :bare_role; - -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