From 1d8ead05f9c0d2fccb0e7a58d2effb1c8baf0c9f Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 12 Sep 2026 15:52:32 -0600 Subject: [PATCH 1/2] test/pytest: port stats_privilege, asserting the SQLSTATE (#432) Second of the four suites that decide a refusal by TEXT with no SQLSTATE anywhere. THE SCOPE IS BOUNDED AND MEASURED, which is the useful half. Across the corpus: assert SQLSTATE only 50 suites assert both 3 assert TEXT only, no SQLSTATE 4 native_ownership, stats_privilege, projection_privilege, rls_direct_storage So this is not "the corpus greps text". It is four suites, two of them now ported, and the class closes at four. WHAT THE PORT ASSERTS THAT THE BASH SUITE CANNOT. stats_privilege.sh decides the refusal with `grep -c 'permission denied for table'`. CLAUDE.md names the rule: 42501 comes only from aclcheck_error, and a grep for "permission denied" is also satisfied by other refusals. `permission denied for schema` matches it too, which is not hypothetical -- it is the confusion measured while porting native_ownership, where a missing schema grant produced a refusal that looked like the one under test. This port asserts 42501 AND that the message names the table. REAL LOGINS, NOT `SET ROLE`. The ownership port used SET ROLE deliberately, to avoid conflating refusal with login. Here the opposite is right: "the owner can open a session" is one of this suite's own premises, so SET ROLE would assert it away. Each role connects. A HELPER TURNED A DRIVER DETAIL INTO A PRODUCT CLAIM. psycopg3 returns the FIRST statement's result for a multi-statement execute, so `SET search_path ...; SELECT` hands back the SET's empty result and raises "the last operation didn't produce records". The first version collapsed that into a 0 through `0 if err else rows[0][0]` and the arm reported "the OWNER of the table can read its stats: got 0 want 1" -- a product failure, from a driver behaviour, with the real error swallowed by the helper. The SET is now its own execute and every call site asserts `err is None` rather than folding an error into a value. guard half 277 passed, 679 checks, count enforced cluster half 169 passed, 456 checks, count enforced (166 -> 169) Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- CHANGELOG.md | 23 +++++ test/pytest/TESTS.md | 42 ++++++++ test/pytest/expected_tests.txt | 4 +- test/pytest/test_stats_privilege.py | 147 ++++++++++++++++++++++++++++ 4 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 test/pytest/test_stats_privilege.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ec129fe..5591044d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,29 @@ true until the next version shipped. ### Added +- `stats_privilege.sh` has a pytest twin, and it asserts the SQLSTATE (#432). + + The bash suite decides the refusal with `grep -c 'permission denied for table'`. + `CLAUDE.md` names the rule: 42501 comes only from `aclcheck_error`, and a grep for + "permission denied" is also satisfied by other refusals -- `permission denied for + schema` among them, which is the exact confusion measured while porting + `native_ownership`. The port asserts 42501 AND that the message names the table. + + Real logins rather than `SET ROLE`, because session-opening is a property this + suite tests and `SET ROLE` would assert it away. + + SCOPE, MEASURED. Across the corpus, 50 suites already assert a refusal by SQLSTATE + and 3 assert both. Only FOUR assert by text with no SQLSTATE anywhere: + `native_ownership`, `stats_privilege`, `projection_privilege` and + `rls_direct_storage`. Two are now ported; the class closes at four, not at the + whole corpus. + + A HELPER TURNED A DRIVER DETAIL INTO A PRODUCT CLAIM. psycopg3 returns the FIRST + statement's result for a multi-statement execute, so `SET search_path ...; SELECT` + hands back the SET's empty result. The first version collapsed that into a 0 and + the arm reported "the OWNER cannot read its stats". Every call site now asserts the + error is None rather than folding it into a value. + - The mutation ledger records WHICH MAJORS each check exists on (#1010). suitepartnamemajorslast-redmutations diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index c18afb96..e0819688 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -76,6 +76,7 @@ behaviour, the source of that number is named. - [28. test_docs_join_clustering.py: the runtime filter's layout precondition](#28-test_docs_join_clusteringpy-the-runtime-filters-layout-precondition) - [29. test_join_vector_agg.py: ungrouped fold over a unique-key join](#29-test_join_vector_aggpy-ungrouped-fold-over-a-unique-key-join) - [30. test_differential.py: the heap oracle, type matrix](#30-test_differentialpy-the-heap-oracle-type-matrix) +- [31. test_stats_privilege.py: stats is readable only by a caller who may read the table](#31-test_stats_privilegepy-stats-is-readable-only-by-a-caller-who-may-read-the-table) ## 1. How to read a test in here @@ -2936,3 +2937,44 @@ arms while asserting nothing. Four columns of four types in one WHERE, where a per-column arm cannot reach: the scan combines their skip decisions, and a predicate right alone can be wrong in conjunction. + + +## 31. test_stats_privilege.py: stats is readable only by a caller who may read the table + +Port of `test/stats_privilege.sh` (#560, ported for #432). + +`stats()` is SECURITY DEFINER and does its own privilege check, because GRANTing +SELECT on `pgcolumnar.zone_map` to PUBLIC would publish per-column minimum, maximum +and sum for every columnar table. + +**The trap the suite exists to catch.** Inside a SECURITY DEFINER function the +effective user is the function owner, so `pg_class_aclcheck(relid, GetUserId(), ...)` +checks the superuser who installed the extension and returns `ACLCHECK_OK` for every +relation. It looks like a correct check and refuses nobody. + +### What the port asserts that the bash suite cannot + +`stats_privilege.sh` decides the refusal with `grep -c 'permission denied for table'`. +`CLAUDE.md` names the rule: 42501 comes only from `aclcheck_error`, while a grep for +"permission denied" is also satisfied by other refusals. **`permission denied for +schema` matches it too**, which is not hypothetical — it is the confusion measured +while porting `native_ownership`. This port asserts 42501 **and** that the message +names the table, so neither half carries the arm alone. + +**Real logins, not `SET ROLE`.** Unlike the ownership port, session-opening is a +property this suite tests, so `SET ROLE` would assert it away. Each role connects. + +| test | what it pins | +| --- | --- | +| `test_the_premises_each_role_is_what_the_suite_assumes` | each premise run BY the role it is about | +| `test_who_may_read_the_stats` | owner, GRANTed reader and superuser all succeed | +| `test_a_role_with_no_privilege_is_refused` | the bar: 42501 **and** the table named | + +### A helper that turned a driver detail into a product claim + +psycopg3 returns the **first** statement's result for a multi-statement execute, so +`SET search_path ...; SELECT ...` hands back the SET's empty result. The first +version collapsed that into a 0 and the arm reported *"the OWNER cannot read its +stats"* — a product failure, from a driver behaviour. The helper now issues the SET +as its own execute, and every call site asserts the error is `None` rather than +folding it into a value. diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index f1da3f3e..07f9f355 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -50,4 +50,6 @@ guard_tests 277 # # The rate is the point. The ungated half grew by 67 tests in the time this took to review, # which is the argument for gating it rather than a detail about it. -cluster_tests 166 +# 166 -> 169 when test_stats_privilege.py landed: the premises, who may read, and +# the no-privilege refusal. +cluster_tests 169 diff --git a/test/pytest/test_stats_privilege.py b/test/pytest/test_stats_privilege.py new file mode 100644 index 00000000..d1c588f3 --- /dev/null +++ b/test/pytest/test_stats_privilege.py @@ -0,0 +1,147 @@ +"""`pgcolumnar.stats` is readable by the table's owner, and only by a caller who +may read the table (#560, ported for #432). + +`stats()` is SECURITY DEFINER and does its own privilege check, because the +alternative -- GRANTing SELECT on `pgcolumnar.zone_map` to PUBLIC -- would publish +per-column minimum, maximum and sum for every columnar table. That is a larger +disclosure than the usability bug it fixes. + +THE TRAP THIS SUITE EXISTS TO CATCH. Inside a SECURITY DEFINER function the +effective user is the function OWNER, so `pg_class_aclcheck(relid, GetUserId(), ...)` +checks the superuser who installed the extension and returns ACLCHECK_OK for every +relation in the database. It looks like a correct check and refuses nobody. The +refusal arm below is what fails if `GetOuterUserId()` is ever changed back. + +WHAT THIS PORT ASSERTS THAT THE BASH SUITE CANNOT. `stats_privilege.sh` decides the +refusal with `grep -c 'permission denied for table'`. `CLAUDE.md` names the rule: + + Assert SQLSTATE, not error text: 42501 comes only from aclcheck_error, while a + grep for "permission denied" is also satisfied by a login FATAL, a missing + function (42883), a bad argument (22023), or a transaction-block refusal (25001). + +`permission denied for schema` also matches `permission denied`, which is not +hypothetical: it is the exact confusion measured while porting `native_ownership`, +where a missing schema grant produced a refusal that looked like the one under test. +This port asserts 42501 AND that the message names the table, so neither half can +carry the arm alone. + +REAL LOGINS, NOT `SET ROLE`. Unlike the ownership port, session-opening is a property +this suite tests -- "the owner can open a session" is one of its premises -- and +`SET ROLE` would assert it away. Each role connects. +""" + +import pytest + +ROWS = 500 +OWNER, NONE, SEL = "t_stowner", "t_stnone", "t_stsel" + + +def _as(cluster, role, sql, schema=None): + """Run one statement on a fresh connection as `role`, returning (rows, error). + + THE SET IS ITS OWN EXECUTE. psycopg3 returns the FIRST statement's result for a + multi-statement execute, so `SET search_path ...; SELECT ...` hands back the + SET's empty result and raises "the last operation didn't produce records". The + first version of this helper collapsed that into a 0 and the arm reported + "the OWNER cannot read its stats" -- a product claim, from a driver detail. + """ + import psycopg + + dsn = f"host=127.0.0.1 port={cluster.port} user={role} dbname=postgres" + try: + with psycopg.connect(dsn, autocommit=True) as conn: + with conn.cursor() as cur: + if schema: + cur.execute(f'SET search_path TO "{schema}", pgcolumnar, public') + cur.execute(sql) + return cur.fetchall(), None + except psycopg.Error as exc: + return None, exc + + +def _fixture(cur): + for r in (OWNER, NONE, SEL): + cur.execute(f"SELECT 1 FROM pg_roles WHERE rolname = '{r}'") + if cur.fetchone() is None: + cur.execute(f"CREATE ROLE {r} NOSUPERUSER LOGIN") + cur.execute(f"GRANT USAGE ON SCHEMA pgcolumnar TO {r}") + cur.execute("DROP TABLE IF EXISTS st_t") + cur.execute("CREATE TABLE st_t (id int, v text) USING pgcolumnar") + cur.execute(f"INSERT INTO st_t SELECT g, 'v'||g FROM generate_series(1,{ROWS}) g") + cur.execute("SELECT current_schema()") + schema = cur.fetchone()[0] + for r in (OWNER, NONE, SEL): + cur.execute(f'GRANT USAGE ON SCHEMA "{schema}" TO {r}') + cur.execute(f"ALTER TABLE st_t OWNER TO {OWNER}") + cur.execute("REVOKE ALL ON st_t FROM PUBLIC") + cur.execute(f"GRANT SELECT ON st_t TO {SEL}") + return schema + + +def test_the_premises_each_role_is_what_the_suite_assumes(pgc_cluster, pgc_conn, expect): + """Each premise is run BY the role it is about, which is why real logins matter.""" + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + for r in (OWNER, NONE, SEL): + rows, err = _as(pgc_cluster, r, 'SELECT 1', schema) + assert err is None, f"{r} could not connect: {err}" + expect.num(rows[0][0], 1, f"premise: {r} can open a session") + + with pgc_conn.cursor() as cur: + cur.execute("SELECT relowner::regrole::text FROM pg_class WHERE relname = 'st_t'") + expect.text(cur.fetchone()[0], OWNER, "premise: t_stowner really owns the table") + cur.execute( + "SELECT count(*) FROM pg_roles WHERE rolname = ANY(%s) AND rolsuper", + ([OWNER, NONE, SEL],), + ) + expect.num(cur.fetchone()[0], 0, "premise: none of these roles is a superuser") + + rows, err = _as(pgc_cluster, OWNER, 'SELECT count(*) FROM st_t', schema) + assert err is None, f"owner read failed: {err}" + expect.num(rows[0][0], ROWS, + "premise: the owner can read its own table by ordinary SQL") + + _, err = _as(pgc_cluster, NONE, 'SELECT count(*) FROM st_t', schema) + expect.sqlstate(err, "42501", + "premise: the no-privilege role cannot read it by ordinary SQL") + + _, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM pgcolumnar.row_group") + expect.sqlstate(err, "42501", + "premise: the catalog tables are NOT readable by these roles, " + "so a GRANT is not the fix") + + +def test_who_may_read_the_stats(pgc_cluster, pgc_conn, expect): + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + q = "SELECT count(*) FROM pgcolumnar.stats('st_t')" + + rows, err = _as(pgc_cluster, OWNER, q, schema) + assert err is None, f"owner call failed: {type(err).__name__}: {err}" + expect.num(rows[0][0], 1, "the OWNER of the table can read its stats") + + rows, err = _as(pgc_cluster, SEL, q, schema) + assert err is None, f"granted role failed: {err}" + expect.num(rows[0][0], 1, "a role merely GRANTed select can read them too") + + with pgc_conn.cursor() as cur: + cur.execute("SELECT count(*) FROM pgcolumnar.stats('st_t')") + expect.num(cur.fetchone()[0], 1, "a superuser still reads stats") + + +def test_a_role_with_no_privilege_is_refused(pgc_cluster, pgc_conn, expect): + """THE BAR. With GetUserId() the check tests the superuser who owns the function, + passes, and this role reads the stats of a table it cannot read. + + Two assertions, because neither carries it alone: `42501` says a privilege check + refused rather than something else failing, and the table name says it was THIS + table's check rather than a catalog table the caller never asked about. The bash + suite has only the second, as text. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, NONE, + "SELECT count(*) FROM pgcolumnar.stats('st_t')", schema) + expect.sqlstate(err, "42501", "a role with no privilege on the table is refused") + expect.num(int("st_t" in str(err)), 1, + "and the refusal names the TABLE, not a catalog table it never asked about") From 946e280c227f1d5a63e29a255996ea7cd0bea563 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 12 Sep 2026 16:53:46 -0600 Subject: [PATCH 2/2] docs: name which arms are loose, and correct the class to five (#432) Two rounds of review narrowed this twice and the entry said neither thing. WHICH ARMS. The defect arm at stats_privilege.sh:82 greps `permission denied for table`, which the schema message does not match, so it is NOT confusable -- @OffgridwithJD measured that and I had claimed otherwise. The two BARE greps are at :68 and :70 and both are premises. So the risk is a premise satisfied for the wrong reason, weakening what the suite rests on, rather than a defect slipping through. That is a smaller and checkable claim; the one it replaces was neither. THE CLASS IS FIVE, not four. import_export_privilege.sh:76 greps `permission denied for (table|relation)` with no SQLSTATE anywhere. My sweep missed it because the flag class `grep -[qic]*i?` does not cover the E in `grep -qiE`, so the line never matched and a fifth member stayed invisible while the output looked complete. FOUR SWEEPS IN ONE DAY across two sessions have failed this way, each keyed on how something was NAMED or SPELLED rather than on content. The rule that would have caught all four: key on content, and when a sweep returns a tidy number, grep for one known-present member and check the sweep found it. Verified as its own command before pushing rather than chained to it: 277 passed, 680 checks. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- CHANGELOG.md | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fbe63b69..ef68c24d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,20 +47,39 @@ true until the next version shipped. #432's parity the tool can certify, and it is a false red rather than a false green. - `stats_privilege.sh` has a pytest twin, and it asserts the SQLSTATE (#432). - The bash suite decides the refusal with `grep -c 'permission denied for table'`. - `CLAUDE.md` names the rule: 42501 comes only from `aclcheck_error`, and a grep for - "permission denied" is also satisfied by other refusals -- `permission denied for - schema` among them, which is the exact confusion measured while porting - `native_ownership`. The port asserts 42501 AND that the message names the table. + The bash suite decides the refusal with a grep on the message. `CLAUDE.md` names + the rule: 42501 comes only from `aclcheck_error`, while a grep for "permission + denied" is also satisfied by other refusals. + + WHICH ARMS ARE LOOSE, MEASURED RATHER THAN ASSERTED, because two rounds of review + narrowed this twice. The defect arm at `:82` uses `permission denied for table`, + which the schema message does NOT match -- so that arm is not confusable, and an + earlier version of this entry claiming otherwise was wrong. The two BARE greps are + at `:68` and `:70`, and both are premises: + + :68 premise: the no-privilege role cannot read it by ordinary SQL + :70 premise: the catalog tables are NOT readable by these roles + + So the risk is a premise satisfied for the wrong reason, which weakens what the + suite rests on, rather than a defect slipping through. The port asserts 42501 on + the premises and on the refusal, and that the refusal names the table. Real logins rather than `SET ROLE`, because session-opening is a property this suite tests and `SET ROLE` would assert it away. - SCOPE, MEASURED. Across the corpus, 50 suites already assert a refusal by SQLSTATE - and 3 assert both. Only FOUR assert by text with no SQLSTATE anywhere: - `native_ownership`, `stats_privilege`, `projection_privilege` and - `rls_direct_storage`. Two are now ported; the class closes at four, not at the - whole corpus. + SCOPE, MEASURED AND THEN CORRECTED. Across the corpus, 50 suites already assert a + refusal by SQLSTATE and 3 assert both. FIVE assert by text with no SQLSTATE + anywhere: `native_ownership`, `stats_privilege`, `projection_privilege`, + `rls_direct_storage` and `import_export_privilege`. Two are now ported; the class + closes at five, not at the whole corpus. + + My first sweep said four. It missed `import_export_privilege.sh:76` because the + flag class `grep -[qic]*i?` does not cover the `E` in `grep -qiE`, so the line + never matched and a fifth member stayed invisible while the output looked complete. + Found by @OffgridwithJD. Four sweeps in one day across two sessions have now failed + this way, each keyed on how something was NAMED or SPELLED rather than on content: + key on content, and when a sweep returns a tidy number, grep for one known-present + member and check the sweep found it. A HELPER TURNED A DRIVER DETAIL INTO A PRODUCT CLAIM. psycopg3 returns the FIRST statement's result for a multi-statement execute, so `SET search_path ...; SELECT`