diff --git a/CHANGELOG.md b/CHANGELOG.md index 46049bc9..779450b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -425,6 +425,45 @@ true until the next version shipped. ### Fixed +- A conftest can no longer switch a vacuity rule off by rebinding a name the + layer reads (#924). + + pytest imports `conftest.py` from the directory it is policing, into the + policing interpreter, before collection. Every module-level name in + `pgc_vacuity.py` is therefore writable by the code it judges. + + #958 closed the datum one exploit used. The three scans that read such data + are module-level names one frame further out, and each was a two-line conftest + away from being a no-op. Measured with the pinned runner: + + ``` + GUARD no conftest with the scan rebound to a no-op + order collapse REFUSED rc=4 PASSED rc=0 + broad except REFUSED rc=4 PASSED rc=0 + raises not pinned REFUSED rc=4 PASSED rc=0 + ``` + + Plugging a fourth name would reopen this again: the transitive closure from + the eight hooks is 31 of the module's 47 names, `ast` among them. So the layer + snapshots its own bindings at import, holds the snapshot in a closure, and + refuses a run in which any of them changed. Names added later are covered + without being listed anywhere. + + No allowlist is needed: the module contains no `global` statement, so every + module-level binding is constant after import. + + The bindings are restored before the refusal is raised. `pytester` runs its + inner session in-process on the same module object, so without that an inner + conftest's rebind stays made for every test that follows. + + This is a cost guard, not a lock. Reaching into the hook's `__defaults__` still + reaches the closure. The criterion is that silencing a rule must cost more than + stating a reason, which `expect.cannot_run(REASON, detail)` does. + + The shell harness needs no equivalent, and not because bash is simpler: its + policing runs in a different process from the code it polices. `selftest/260` + reads `lib.sh` with grep and awk and never sources the file it judges. + - The sentinel sweep no longer excludes an assertion by accident of naming (#938). `_comparisons()` selected on the first two parameter names, so `wrote(cur, want, diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 4f8e4b30..06ac6703 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -261,6 +261,32 @@ the general case was hand-rolled. | `test_the_inequality_scan_finds_a_planted_offence` | the AST scan fires on both spellings | | `test_the_inequality_scan_does_not_flag_honest_code` | five shapes it must not flag | | `test_no_test_in_this_corpus_hand_rolls_an_inequality` | the population is zero, across 17 files | +| `test_a_conftest_cannot_switch_off_the_order_collapse_scan` | #924, the route still open after #958 | +| `test_a_conftest_cannot_switch_off_the_broad_except_scan` | the same hatch, a second scan | +| `test_a_conftest_cannot_switch_off_the_raises_scan` | and a third | +| `test_the_refusal_names_the_binding_that_changed` | an honest run, refused, naming the conftest's name | +| `test_a_new_attribute_on_the_layer_is_not_a_rebind` | the control: a check that fires on anything is not a check | +| `test_the_rebind_does_not_leak_into_this_session` | the binding is restored, so `pytester` does not poison the outer run | + +**A conftest is loaded from the directory being policed, into the policing +interpreter, before collection.** So every module-level name in `pgc_vacuity` is +writable by the code it judges. #958 closed the datum one exploit used; the three +scans that read such data are module-level names one frame further out, and each was +a two-line conftest away from being a no-op: + +``` +GUARD no conftest with `pgc_vacuity. = lambda p: []` +order collapse REFUSED rc=4 PASSED rc=0 +broad except REFUSED rc=4 PASSED rc=0 +raises not pinned REFUSED rc=4 PASSED rc=0 +``` + +The layer now snapshots its own bindings at import and refuses a run in which any of +them changed, which covers names added after it was written. It is a cost guard, not +a lock: the criterion #924 set is that silencing a rule must cost more than stating a +reason. **The shell harness needs no equivalent** — `selftest/260` greps `lib.sh` from +a separate process and never sources the file it judges. + **Two failed queries are not two observable arms.** `query_error()` produces a value unique per occurrence precisely so two failures cannot compare **equal** and pass an diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 99598ad6..ccc06e89 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -1622,12 +1622,102 @@ def _raises_sites(path): return out -def pytest_collection_modifyitems(config, items): +# THE LAYER'S OWN BINDINGS, AND WHY THEY ARE CHECKED RATHER THAN HIDDEN (#924). +# +# pytest imports `conftest.py` FROM THE DIRECTORY IT IS POLICING into the policing +# interpreter, before collection, with no opt-out. Every module-level name here is +# therefore writable by the code this layer judges. That is what conftest is for; it +# is a defect here only because this layer's job is to refuse, and a refusal that can +# be deleted in two lines is a suggestion. +# +# THE SHELL HARNESS HAS NO EQUIVALENT, and not because bash is simpler: its policing +# runs in a DIFFERENT PROCESS from the code it polices. `selftest/260` reads +# `$TESTDIR/lib.sh` with grep and awk and never sources the file it judges, so +# nothing a policed suite writes can reach the judge's namespace. +# +# #958 closed the datum one exploit used, by binding the killer list in a default +# argument. The three scans that READ such data are module-level names themselves, +# one frame further out. Measured on main 226f805 with the pinned runner, each was a +# two-line conftest away from being a no-op: +# +# GUARD no conftest with `pgc_vacuity. = lambda p: []` +# order collapse REFUSED rc=4 PASSED rc=0 +# broad except REFUSED rc=4 PASSED rc=0 +# raises not pinned REFUSED rc=4 PASSED rc=0 +# +# SO THE FIX IS NOT A FOURTH NAME MOVED OUT OF REACH. Plugging names one at a time is +# what reopened #924 after #958, and the transitive closure from the eight hooks is +# 31 of this module's 47 names -- `ast` among them. The layer notices instead that one +# of its own bindings CHANGED, which covers the names added after this was written. +# +# THE SNAPSHOT IS EXACT WITH NO ALLOWLIST because this module contains no `global` +# statement anywhere: after import, every module-level binding here is constant. +# Verified rather than assumed, and it is the property that makes a bare identity +# comparison correct. +# +# A NEW ATTRIBUTE IS NOT A REBIND. `pgc_vacuity._something_new = 1` changes no +# behaviour, and refusing it would make this a tripwire on the mere act of importing +# the module -- the false-positive engine this layer's own budget forbids. The arm +# for that is a control in test_layer.py. +# +# WHAT IT STILL DOES NOT STOP, stated because a guard's blind spots are part of its +# meaning. Anything sharing the interpreter can eventually win: reaching into +# `pytest_collection_modifyitems.__defaults__` reaches the closure below. The +# criterion #924 set is COST, not impossibility -- the hatch must cost more than +# stating a reason, and that shape is unmistakably deliberate where two lines of +# assignment are not. It also checks at COLLECTION only, so a test that rebinds a +# name inside its own body and restores it is untouched; two arms in +# test_failed_query_sentinel.py do exactly that, legitimately. +def _binding_guard(): + snapshot = {} + missing = object() + + def arm(ns): + snapshot.update({k: v for k, v in ns.items() if not k.startswith("__")}) + + def changed(ns): + return sorted(k for k, v in snapshot.items() if ns.get(k, missing) is not v) + + # RESTORED BEFORE THE REFUSAL, not after it, and this is load-bearing rather than + # tidiness. `pytester` runs its inner session IN-PROCESS on this same module + # object, so a rebind made by an inner conftest stays made for every test that + # follows in the outer run -- the hazard `_RunShape` already paid for once. The + # arms that prove this refusal would otherwise poison the rest of their own file. + def restore(ns): + ns.update(snapshot) + + return arm, changed, restore + + +_arm_bindings, _changed_bindings, _restore_bindings = _binding_guard() + + +def pytest_collection_modifyitems(config, items, + _changed=_changed_bindings, + _restore=_restore_bindings): """Refuse a bare skip, which exits 0 and reads as success. Measured: two skipped tests report `2 skipped` and exit 0. A skip is allowed only through expect.cannot_run(), which names a reason from a closed list. """ + # FIRST, because every rule below is read through a name a conftest can write. + # Captured in this signature at DEFINITION time, so rebinding `_changed_bindings` + # or `_restore_bindings` on the module does not reach what runs here. + _rebound = _changed(globals()) + if _rebound: + _restore(globals()) + raise pytest.UsageError( + "the pgColumnar vacuity layer refuses this run: a conftest or plugin " + "rebound the layer's own " + + ("names " if len(_rebound) > 1 else "name ") + + ", ".join(_rebound) + + " -- a rule this layer enforces is read through that binding, so the " + "run would have reported on rules that were switched off. The bindings " + "have been restored. If a check is wrong for your case, say so where the " + "run records it: expect.cannot_run(REASON, detail), which names a reason " + "from a closed list, or fix the test the rule is objecting to." + ) + offenders = [] seen_files = set() for item in items: @@ -1725,3 +1815,10 @@ def pytest_collection_modifyitems(config, items): raise pytest.UsageError( "the pgColumnar vacuity layer refuses this run: " + ". ".join(parts) + "." ) + + +# ARMED HERE, AT THE BOTTOM, because a snapshot taken earlier would miss every name +# defined after it -- including this hook. Import order is what makes this safe: the +# module is fully executed before pytest imports any conftest, so nothing the policed +# tree writes can be in the snapshot. +_arm_bindings(globals()) diff --git a/test/pytest/test_layer.py b/test/pytest/test_layer.py index 3e6b7772..f153f4fe 100644 --- a/test/pytest/test_layer.py +++ b/test/pytest/test_layer.py @@ -540,3 +540,196 @@ def test_no_test_in_this_corpus_hand_rolls_an_inequality(expect): for f in files: offences += _hand_rolled_inequalities(f.read_text(encoding="utf-8"), f.name) expect.text(repr(offences), "[]", "no test hand-rolls an inequality") + + +# ---- the layer's own names are reachable from the tree it polices (#924) ---- +# +# pytest imports `conftest.py` FROM THE POLICED DIRECTORY into the policing +# interpreter, before collection, with no opt-out. So every module-level name in +# pgc_vacuity is writable by the code it judges. That is not a bug in pytest; it +# is what conftest is for. It is a defect here because this layer's whole job is +# to refuse, and a refusal that can be deleted in two lines is a suggestion. +# +# #958 closed the datum one exploit used (`_ORDER_KILLERS`) by binding it in a +# default argument. The three scans that READ such data are module-level names +# themselves, one frame further out, and each is a two-line conftest away from +# being a no-op. Measured on main 226f805 with the pinned runner: +# +# GUARD no conftest with the rebind +# order collapse REFUSED rc=4 PASSED rc=0 +# broad except REFUSED rc=4 PASSED rc=0 +# raises not pinned REFUSED rc=4 PASSED rc=0 +# +# The fix is not another name moved out of reach. It is the layer noticing that +# one of its own bindings changed, which covers the names added after this was +# written as well as the 47 present when it was. + + +def _rebind(name): + """A conftest that switches one scan off, and nothing else. Two lines.""" + return ( + "import pgc_vacuity\n" + f"pgc_vacuity.{name} = lambda path: []\n" + ) + + +def test_a_conftest_cannot_switch_off_the_order_collapse_scan(pytester, expect): + """#924, the route still open after #958.""" + pytester.makepyfile( + """ + def test_order_collapsed(expect): + got = ["b", "a"] + g = sorted(got) + expect.ordered_rows(g, ["a", "b"], "rows in order") + """ + ) + plain = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(plain, "premise: the collapse is refused when nobody rebinds") + plain.stderr.fnmatch_lines(["*order-killed*"]) + + pytester.makeconftest(_rebind("_sorted_ordered_sites")) + hatched = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(hatched, "and it is still refused after the scan is rebound") + hatched.stderr.fnmatch_lines(["*_sorted_ordered_sites*"]) + + +def test_a_conftest_cannot_switch_off_the_broad_except_scan(pytester, expect): + """The same hatch, a different scan. Named separately because a fix that + closed only the scan #924 happens to name would leave this one open, which + is how #924 came back after #958.""" + pytester.makepyfile( + """ + def test_broad(expect): + try: + x = 1 + except Exception: + pass + expect.num(x, 1, "x is one") + """ + ) + plain = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(plain, "premise: the broad except is refused") + plain.stderr.fnmatch_lines(["*catches Exception broadly*"]) + + pytester.makeconftest(_rebind("_broad_except_sites")) + hatched = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(hatched, "and it is still refused after the scan is rebound") + hatched.stderr.fnmatch_lines(["*_broad_except_sites*"]) + + +def test_a_conftest_cannot_switch_off_the_raises_scan(pytester, expect): + """The third. The body holds a compound statement rather than a broad + exception class, because that arm needs no driver installed and this file + runs in the database-free job. + + My first version of this premise used `pytest.raises(ValueError)` around one + statement, which the rule does not refuse -- so the PREMISE printed `1 passed` + and the arm could not have shown anything being switched off. + """ + pytester.makepyfile( + """ + import pytest + + def test_raises_compound(expect): + with pytest.raises(ValueError): + for i in [1]: + raise ValueError("boom") + expect.num(1, 1, "ran") + """ + ) + plain = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(plain, "premise: an unpinned compound raises block is refused") + plain.stderr.fnmatch_lines(["*not pinned*"]) + + pytester.makeconftest(_rebind("_raises_sites")) + hatched = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(hatched, "and it is still refused after the scan is rebound") + hatched.stderr.fnmatch_lines(["*_raises_sites*"]) + + +def test_the_refusal_names_the_binding_that_changed(pytester, expect): + """A refusal that does not say what was rebound sends the reader to the + wrong file. The offending test here is HONEST -- the only thing wrong with + the run is the conftest -- so nothing but the tamper check can refuse it.""" + pytester.makepyfile( + """ + def test_honest(expect): + expect.num(1, 1, "one is one") + """ + ) + plain = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(plain, "premise: an honest test passes", passed=1, failed=0) + + pytester.makeconftest(_rebind("_broad_except_sites")) + hatched = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(hatched, "a rebound layer name refuses a run of honest tests") + hatched.stderr.fnmatch_lines(["*_broad_except_sites*"]) + + +def test_a_new_attribute_on_the_layer_is_not_a_rebind(pytester, expect): + """The control, and it is LOAD-BEARING rather than hygiene (@OffgridwithJD). + + A guard that fired on anything touching the module would be the + false-positive engine this layer's own budget forbids -- but it would also + turn four EXISTING tests red, because each writes a name that has never been + a module attribute: + + _ORDER_KILLERS test_ordered.py:148, :169 (removed by #958) + _BROAD_RAISES test_raises_sqlstate.py:584 + broad_families test_raises_sqlstate.py:585 + BROAD_RAISES test_raises_sqlstate.py:586 + + A snapshot comparison cannot flag any of them, because there is no prior + binding to differ from. So this arm is what keeps an ADD from being tightened + into tampering by someone who reads the check as incomplete. Verified: only + `QUERY_ERROR` (test_failed_query_sentinel.py:321, :352) rebinds a name that + exists, and it restores it in a `finally` without driving an inner collection. + """ + pytester.makepyfile( + """ + def test_honest(expect): + expect.num(1, 1, "one is one") + """ + ) + pytester.makeconftest( + "import pgc_vacuity\n" + "pgc_vacuity._a_name_the_layer_never_had = 1\n" + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a NEW attribute is not a changed binding", + passed=1, failed=0) + + +def test_the_rebind_does_not_leak_into_this_session(pytester, expect): + """pytester runs the inner session IN-PROCESS, on the same module object, so + a conftest that rebinds a scan leaves it rebound for every test that follows + -- including the ones in this file. The layer already paid for that once + (see `_RunShape`: 'AN INSTANCE PER CONFIG, NOT MODULE GLOBALS'). + + So the refusal restores the binding before raising, and this asserts it by + USING the scan afterwards rather than by comparing identities: a no-op lambda + returns [] for everything, and the real scan finds the planted offence. + """ + pytester.makepyfile( + """ + def test_honest(expect): + expect.num(1, 1, "one is one") + """ + ) + pytester.makeconftest(_rebind("_broad_except_sites")) + expect.run_failed(pytester.runpytest("-p", "pgc_vacuity"), + "the inner run is refused") + + import pgc_vacuity + planted = pytester.makepyfile( + planted=""" + def test_planted(expect): + try: + x = 1 + except Exception: + pass + expect.num(x, 1, "x is one") + """ + ) + expect.num(len(pgc_vacuity._broad_except_sites(str(planted))), 1, + "the real scan is back and still finds the offence") diff --git a/test/pytest/test_raises_sqlstate.py b/test/pytest/test_raises_sqlstate.py index 102b7510..f0621e43 100644 --- a/test/pytest/test_raises_sqlstate.py +++ b/test/pytest/test_raises_sqlstate.py @@ -577,6 +577,19 @@ def test_a_conftest_cannot_switch_the_broad_family_list_off(pytester, expect): The list is therefore bound inside `_raises_sites` rather than at module level. The conftest below writes BOTH plausible spellings of the name onto the module and then presents the refused shape. The run must still be refused. + + WHAT THIS ARM DOES NOT COVER, and the sentence above was read as covering it + (#924, @OffgridwithJD). Binding the list inside the reader protects the DATA. + `_raises_sites` -- the reader itself -- is a module-level name, and + `pgc_vacuity._raises_sites = lambda p: []` switched this exact rule off, + measured. None of the three names this conftest writes has ever existed on the + module, so this arm cannot fail on that: it pins that GUESSING the data name + does not work, which is a smaller claim than it reads as. + + The reader is covered by the binding snapshot instead, and by + `test_a_conftest_cannot_switch_off_the_raises_scan` in test_layer.py. It is not + re-asserted here: two files claiming the same thing is how one of them stops + being read. """ pytester.makeconftest(''' import pgc_vacuity