From f21fab8c943dd3ba5ea149596ed2161148cc388b Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 17 Aug 2026 15:07:11 -0500 Subject: [PATCH 1/2] 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/2] 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