test: a conftest cannot switch a vacuity rule off by rebinding the layer's own names (#924) - #964
Conversation
…yer's own names (#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 (pytest 9.1.1, pytest-xdist 3.8.0): GUARD no conftest 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, compares at the top of pytest_collection_modifyitems, 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. Verified. 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 -- the hazard _RunShape already records. A NEW attribute is not a rebind, and that control is load-bearing rather than hygiene: four existing tests write names that have never been module attributes, and treating an add as tampering would redden all four. This is a cost guard, not a lock. Reaching into the hook's __defaults__ still reaches the closure. The criterion #924 set is that silencing a rule must cost more than stating a reason. The shell harness needs no twin, 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. Also corrects a docstring in test_raises_sqlstate.py that argued for a guarantee the code did not provide: binding the family list inside _raises_sites protects the DATA, and the arm below it writes three names that have never existed, so it could not have failed on the reader. Red first: 5 arms, each failing "the inner run exited 0, so nothing refused it" on the rebind half with the premise half passing. Removal proofs, all three parts: check removed 5 red snapshot never taken 5 red restore removed 5 red, a DIFFERENT five, including the control Verified: pytest 228 driver-free and 326 full corpus against a live PG16 cluster; harness_selftest 800/800 on PG16; docs_style 9/9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
|
The rebased-tree selftest number, as promised, rather than the pre-rebase one carried forward. On
Lane: PG16 in One thing worth being explicit about, since it is the kind of green that means less than it looks: CI cannot fail on the defect this PR fixes. Nothing in the corpus rebinds a layer scan, so a build with the snapshot removed entirely would report exactly the same 13 checks. What establishes the fix is the three mutations in the PR body, and what establishes the defect was there is the rebind table — neither of which is a CI signal. The gate proves this change breaks nothing; the removal proofs are what prove it does anything. |
|
Reviewed at The exploits, on your branchThe last row is the control working: an ADD reaches the guard it was always going to reach, and does not trip the tamper check. Gate on my box, against your numbersOn the
|
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving at 6024dbc5 on full green CI (13 of 13), the head I drove.
Verified on your branch rather than from the diff:
premise, no conftest VACUITY (the guard's own refusal)
conftest: _sorted_ordered_sites = lambda "rebound" + refused
conftest: _raises_sites = lambda "rebound" + refused
conftest: _broad_except_sites = lambda "rebound" + refused
conftest: _something_added_later = 1 VACUITY, NOT the tamper message <- control holds
test_layer.py 31 passed corpus 326 passed driver-free 228 passed
global statements in pgc_vacuity.py: 0 non-dunder names: 47
The three cheap routes are closed, the ADD control still reaches the guard it was always going to reach, and your allowlist-free premise holds independently.
The implementation is the part worth keeping. The snapshot in a closure, identity comparison, the restore before the refusal with its reason stated, _arm_bindings(globals()) last with an explanation of why it must be last, and the blind spot written down rather than omitted — pytest_collection_modifyitems.__defaults__ reaches the closure. The criterion #924 set was cost, and reaching into __defaults__ is unmistakably deliberate where two lines of assignment are not.
I withdrew the _raises_sites arm I suggested: test_layer.py:620 already asserts that exact rebind with its premise, and your reason is better than mine — two files claiming the same thing is how one of them stops being read. The pointer you added from test_raises_sqlstate.py is the right shape.
Two things I got wrong in reviewing this, recorded because they cost you nothing only by luck:
I nearly reported that your PR carried unrelated lib.sh and selftest/340 changes and contradicted your "no ledger change" claim. It was my baseline — I had not re-fetched upstream in that worktree, so I was diffing against a main predating #962 and seeing my own merged work as yours. Against real main it is five files, pytest-only, no ledger.
And I doubted the control arm mattered before measuring what it holds up. It keeps four existing test sites green, because all four write names that have never been module attributes and a snapshot has no prior binding to differ from. You documented that after I raised it; the version I raised was weaker than the one you wrote.
Not blocking, for whoever picks up #924 next: the criterion is still cost, and this closes the two-line routes. __defaults__ remains, by design and by argument.
2 participants
Closes #924 — the criterion that issue set is the cost of the hatch, and it was still two lines after #958.
What was open
#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 one frame further out, and each was a two-line conftest away from being a no-op. Measured on
226f805with the pinned runner:_ORDER_KILLERS = ()is still refused, so #958 holds and is not touched here.My first instrument could not tell those cases apart, and it is worth naming because the failure was invisible: the host carries pytest 7.4.4 and no xdist, the layer registers an xdist hook, so every case died in
check_pendingand readrc=3— premise and exploit identical. The table above is from a venv built fromrequirements-test.txt.Why not a fourth name moved out of reach
That is what reopened #924 after #958. The transitive closure from the eight hooks is 31 of this module's 47 names,
astamong them — so the list of things to hide is not a list anyone can keep.The layer snapshots its own bindings at import, holds the snapshot in a closure, compares at the top of
pytest_collection_modifyitems, and refuses. Names added after this was written are covered without being named anywhere.No allowlist is needed, and that rests on a property rather than on care:
pgc_vacuity.pycontains noglobalstatement, so after import every module-level binding is constant. Verified rather than assumed — it is what makes a bare identity comparison exact.The bindings are restored before the refusal is raised.
pytesterruns 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. That is the hazard_RunShape's comment already records.A new attribute is not a rebind, and that control is load-bearing
Thanks to @OffgridwithJD for the sweep that turned this from hygiene into a requirement. Four existing tests write names that have never been module attributes:
A snapshot comparison cannot flag an add — there is no prior binding to differ from — and if anyone ever tightens it into one, all four go red. The control arm says so in its docstring now. Verified independently: only
QUERY_ERROR(test_failed_query_sentinel.py:321,:352) rebinds a name that exists, and it restores it in afinallywithout driving an inner collection, so it is untouched.One docstring corrected, because it argued for a guarantee the code did not give
test_a_conftest_cannot_switch_the_broad_family_list_offreasons that "the list is therefore bound inside_raises_sitesrather than at module level". True of the data; false of the reader, which this PR measures being switched off. And none of the three names its conftest writes has ever existed, so the arm could never have failed on it.The docstring now states what the arm does cover and points at the arm that covers the reader. I did not add a duplicate arm in that file:
test_a_conftest_cannot_switch_off_the_raises_scanintest_layer.pyasserts exactly that, and two files claiming the same thing is how one of them stops being read. @OffgridwithJD suggested the arm and may disagree with that call — it is a one-line difference to make.What this 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. The criterion is cost, not impossibility — the hatch must cost more than stating a reason, andexpect.cannot_run(REASON, detail)is the honest form. It also checks at collection only, so a test that rebinds inside its own body and restores is untouched, which is what keeps the twoQUERY_ERRORarms working.No shell twin, and not for the usual reason
Confirmed from the code by @OffgridwithJD rather than from my argument:
selftest/260drivesgrep -c ... "$_lib"andawk '/^pgc_set_hash\(\)/,/^}/' "$_lib". It reads the file as text and never sources the file it judges, so nothing a policed suite writes can reach the judge's namespace. The shell harness polices from a different process; pytest polices from inside the thing it polices. A twin here would be a test of nothing.Red first, then three removal proofs
Five arms, each failing with
the inner run exited 0, so nothing refused iton the rebind half while the premise half passed.The third is the interesting one: without the restore, an earlier arm's rebind poisons a later arm in the same file. The leak is not theoretical, and the mutation shows it rather than arguing it.
Gate
The selftest and corpus numbers above were taken on the pre-rebase tree; #962 touched only
lib.sh,selftest/340and the ledger, so the pytest figures are unaffected, and the selftest is re-running on the rebased tree — I will post the number rather than carry the old one forward.No ledger or budget change: the mutation ledger's rows are shell suites (
harness_selftest,native_join_runtime_filter), and this adds no shell check.Found while verifying this, filed separately
#963 — under
-n, a collection-time refusal loses its reason:rc4 becomes 1 and the message is replaced by a 35-line pluggyINTERNALERROR. Pre-existing, not introduced here; this change inherits the behaviour exactly. The control that establishes it is not mine is in the issue: an in-testVacuityErrorreports identically in both modes.🤖 Generated with Claude Code
https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw