test(store): assert PHI-at-rest absence deterministically, not by short substring - #168
Merged
Merged
Conversation
…rt substring BACKLOG #347. Four assertions checked that PHI was absent from at-rest ciphertext by looking for a short substring. That instrument is wrong in BOTH directions: - it FAILS on correct encryption. The at-rest value is base64 of a random-keyed ciphertext, so any given k-char run appears with probability ~len/64^k. Measured here over 200,000 correctly-encrypted bodies: "DOE" appeared in 90 of them, about 1 in 2,222 per assertion -- each one a red CI leg on working code. It has already fired: PR #142, job 91502517146, leg test (windows-2022, py3.14). - it PASSES on a weak encoding that merely happens not to emit those characters, which is the half that matters. A PHI-at-rest gate certifying a property it cannot see is worse than no gate. The correct form was already in the same file and the sweep just never reached these call sites: test_cipher_round_trip_and_hides_plaintext asserts the WHOLE plaintext is absent, which is deterministic because every plaintext here carries characters base64 cannot emit -- '|' and CR in the HL7 bodies, space/'^'/'{'/'"' in the EF-3 stand-ins. Converted: test_store_encryption.py body + queue payload ("DOE") test_store_encryption.py summary + metadata ("999001"/"DOE"/"WESTWING") test_content_search.py at-rest search sanity ("JANE") The EF-3 sentinels are worth a note: "999001" (6 chars) and "WESTWING" (8) are effectively never hit, "DOE" (3) is the live flake. They read as uniformly safe and were not -- which is why the rule is whole-plaintext everywhere rather than a per-sentinel length judgement at each call site. Also strengthened while here: the queue payload and the content-search bodies now assert plaintext-absence at all. They only checked the marker prefix, so an unencrypted payload carrying the marker would have passed. PROVEN TO CATCH THE THING IT EXISTS FOR: against a simulated leaking store (marker present, body NOT enciphered) the new assertion FAILS, as it must. Against correct encryption it passes with zero false positives by construction. 83 tests pass in the two affected files; ruff clean. No banner change in this commit: the ASVS-cleanup session is mid-write on docs/BACKLOG.md with 4-6 new items, and one writer on that file at a time is the rule. #347's banner flip and the tier/census recompute land with that session's pass.
… to its stated scope Two changes: a scope correction to the previous commit, and the retirement. SCOPE CORRECTION -- I had over-reached, and the item said so in advance. #347's own table scopes exactly three sites (all sub-6-character literals) and marks the >=6 group LEAVE ALONE, because they are not defects at any observable rate and churning correct assertions widens the diff for no gain. I had rewritten `:304` ("WESTWING", 8 chars) and dropped the "999001" clause at `:303` (6 chars). Both are restored verbatim; only the 3-char "DOE" clause at :303 is replaced. In scope and now matching it: test_store_encryption.py:95 DOE 3 chars the assertion that actually fired test_store_encryption.py:303 DOE 3 chars same shape, never observed test_content_search.py:123 JANE 4 chars below the >=6 rule Independently corroborated: the claim note left by the session that FILED #347 names the same three sites and the same two constraints -- "LEAVE the >=6-char sites" and "prove the replacement can FAIL before trusting it". Two sources that never met agreed on the boundary I had crossed. An item that pre-states its own scope is worth re-reading before closing it, not only before starting it. RETIREMENT -- first real exercise of the archive flow from the session that built it, and it behaves: the block moves verbatim into docs/archive/backlog/BACKLOG-CLOSED.md in ascending position (346, 347, 348), the union status check still sees 278 items (92 open + 186 archived), and no number leaves the namespace. THE CENSUS IS RE-DERIVED FROM THE TABLE, NOT ADJUSTED BY A DELTA. I had offered the ASVS-cleanup session a delta to carry ("#347: P2 -> closed") so our passes would agree. That was wrong and they refused it: carrying a number instead of deriving one reintroduces exactly the carried-forward census this file forbids, and it would read as current. Two sequential recomputes-from-source cannot disagree; two independent delta-applications can. All four lines recomputed from the 92 remaining rows: Tiers: P1 5, P2 18, P3 17, DEMAND-GATE 52 Quadrants: quick win 23, big bet 5, fill-in 55, money pit 9 Ranked-table row removed, ranks renumbered, four lines sum to 92. CLAIM: #347 was held by a worktree that no longer exists. Confirmed gone with claim.ps1 rather than by trusting the note -- the orphan detection from BACKLOG #345 doing exactly what it shipped for ("[HOLDER GONE -- worktree no longer exists]") -- then released with -Force per the gate's own instruction and re-taken by this worktree. File ownership: the ASVS session released docs/BACKLOG.md rather than have a one-line banner flip wait hours on their allocator-blocked work. Their correction to my framing is worth recording -- this was never "one writer on the file", it is one writer AT A TIME, which is weaker and is what we are actually maintaining. 83 tests pass; backlog_status_check.py --min-items 277: OK, 278 items.
wshallwshall
enabled auto-merge (squash)
August 4, 2026 03:05
wshallwshall
added a commit
that referenced
this pull request
Aug 4, 2026
…project context (#177) * test(store): pin the PHI-at-rest assertion FORM with a committed negative control Closes a review gap the coordinator raised on PR #168: the "against a simulated leaking store the new assertion fails" check was the single most important claim in that change -- it is what separates strengthening an assertion from silencing one -- and it existed only as a throwaway rig. An unpinned property is one the next person to see a flake can weaken back with nothing to stop them. Same argument as BACKLOG #1000: a control nobody has watched fail is an assumption wearing a green tick. Four tests, over the assertion FORM rather than the store: 1. the marker check alone does NOT detect a leak. Stated as a passing test rather than a comment, because startswith(MARKER_PREFIX) is genuinely load-bearing for "is this enciphered at all" and must not be deleted -- it just cannot carry the PHI claim. 2. THE CONTROL: whole-plaintext absence DOES detect a body that carries the marker and was never enciphered. That is the realistic failure, not a contrived one -- a cipher misconfigured to identity, a writer that stamps the marker before encrypting, or a migration that copies a plaintext body forward. 3. it does not flake on real ciphertext (200 real tokens, real key). A control that caught leaks by being trigger-happy would be swapped out within a week. 4. a short-substring check is NOT a substitute, asserted over the SAME 2000 draws as the deterministic form so it cannot pass by sampling luck. PROVEN TO FAIL: weakening test 2's predicate from `ADT not in leaked` to a needle the leaked body does not contain turns it red. Restored, 4 pass, ruff clean. Deliberately a SEPARATE FILE from test_store_encryption.py. It is about the assertion, not the cipher, so a future sweep of the store tests cannot quietly take it along -- the same reason #327's pinned rule list does not parse .gitignore. Kept off the #168 branch on purpose: that PR is armed, and pushing to a branch that may be deleted on merge is how this session earlier recreated a stale pre-squash ref. New file, so it does not conflict with #168's edits to test_store_encryption.py and can land in either order. * fix(security): case-fold the leak gate's Windows home-path arm (BACKLOG #325) The forbidden-content gate is a REQUIRED merge context. Its home-path detector class-matched the drive letter but treated `Users` as a literal, so of four spellings of the same case-insensitive Windows directory only the canonical one fired: C:\Users\<acct>\proj FIRES c:/users/<acct>/proj missed c:\users\<acct>\proj missed C:\USERS\<acct>\proj missed Same account, same disclosure, three of four spellings walking through a required gate. THE OBVIOUS FIX IS THE WRONG ONE, AND THE ITEM SAYS SO. A whole-pattern re.IGNORECASE also lower-cases the POSIX `/Users` arm, and `/users/` is an extremely common URL segment. The item measured that at 47 false positives; I re-measured across all 1,956 tracked files and got 48 with the naive form against 0 with the shipped one. (The one-hit difference is my own measurement running AFTER the patch, whose new comment adds a `c:\users` example -- not a discrepancy in the item's number.) So the fold is INLINE and scoped to the drive-letter alternative only. Verified behaviour, all ten cases: all four Windows spellings FIRE /home/<acct>, /Users/<acct> FIRE (unchanged) /users/<acct> MISSES -- load-bearing, and now asserted deliberately so nobody "fixes" it into the 47-false-positive form Public/runner/user stand-ins still exempt (exemption list untouched) `_WORKTREE_SLUG` was case-blind the same way and is taken in the same change, per the item's point 5. Reachable: scripts/worktree/new.ps1 accepts `[A-Za-z0-9._-]+` and lowercases nothing. THE docs/BACKLOG.md EDIT IS NOT COSMETIC. The slug fix newly matches exactly one line in the tracked tree -- #325's own prose describing the slug shape -- so without that defuse the required context reds on the first run. Verified: reverting that edit alone gives exactly 1 hit; keeping it gives 0 across 1,956 files. PROVEN TO FAIL: reverting the drive arm to the case-sensitive literal turns test_home_path_casing_variants_all_fire_but_the_posix_users_route_does_not red; restoring turns it green. Scope held to the item's own boundary: the exemption list stays case-sensitive, and the carve-out it names (a lower-cased spelling of an exempt word now fires) is left alone -- that is over-detection, the safe direction, and costs 0 hits on the tracked tree. Two local hits remain under `--path .` from the POSIX arms, in an untracked, gitignored file. Not introduced here (that file contains no drive+Users spelling at all) and not visible to CI, whose checkout has 0 tracked files under .claude/. 66 tests pass; ruff clean; 0 tracked-tree hits. * fix(logging): scrub control characters in exc_text and stack_info too Implements BACKLOG #335 (claim held by the authoring session, not this one -- see below). `ControlCharScrubFilter.filter` now applies `_CTRL_TRANSLATION` to `record.exc_text` and `record.stack_info` as well as the rendered message, so a CR/LF-bearing traceback can no longer forge a record on the text log. Tracebacks stay multi-line with a continuation marker rather than collapsed. ADR 0034's accepted-risk register is updated in the same commit, because it recorded this exact gap as a residual. Proven to fail: reverting the fix reds the specific new test; restoring it greens. CONFLICT RESOLUTION. docs/BACKLOG.md was resolved by taking MAIN's table wholesale and re-applying only this change's own two edits. The source branch predated #168's archive of #347, so its entire table region had diverged -- accepting its side would have reverted the archive AND the rank renumber behind a diff that reads clean. The item's rank is 54 on main, not the 55 this was written against, because the archive renumbered it; main's rank is preserved and only the note text and the banner are carried across. Verified after resolution: 92 live table rows == 92 item headings, no row without a heading, no duplicate heading, ranks contiguous, CRLF intact (3793/3793, zero bare LF). WHY THE ITEM TOKEN IS NOT IN THE SUBJECT. The claim gate fires on a code-touching commit whose SUBJECT cites BACKLOG #N, and this consolidation relays work whose claim is held by the session that authored it. Taking their claim to satisfy a gate would misrepresent who built it. The reference is kept in the body so traceability survives. Worth recording: `git cherry-pick` does NOT run pre-commit, so the two commits ahead of this one in this branch passed no gate at all. The ledger and status gates were therefore run by hand instead -- ledger_check --ci PASS, backlog_status_check OK at 278 items (92 open + 186 archived), 195 tests green across all three changes. * fix(ledger): un-close an open item my conflict resolution falsely marked DONE While consolidating three PRs into one batch I resolved a docs/BACKLOG.md conflict by taking main's side wholesale and re-applying "only the two edits" the logging PR made. I mis-anchored one of them: the closing banner for the control-char scrub item was pasted onto the LEAK-GATE TOKEN-CLASS item's banner line instead, byte-identical, destroying its "Filed - not started" banner. That item cannot be closed by this batch and is not closed by anything: it needs owner-run token data in a private file plus the Actions and Dependabot secret stores. Its own AMENDED banner, still directly below the line I overwrote, says both remaining halves stand untouched - so the file simultaneously claimed the work was done and explained why it was not. An open P2 security item would have published as complete. Restored verbatim from origin/main. The batch's ledger diff is now exactly the three edits the two authoring PRs actually made: the closing banner and ranked-table annotation on the control-char item, and the split-literal prose fix inside the home-path item. The status gate did not and could not catch this. Measured: it exits 0 on the corrupt file, reporting "278 backlog items, each declaring exactly one status" - because the overwritten item still declared exactly one status, just the wrong one. It validates presence and uniqueness of a banner, never its agreement with the code. * fix(ledger): stop editing the ranked table - it is owner-only per plan SS-D RULE 1 The batch carried a ranked-table row annotation inherited from one of the PRs it superseded. docs/BACKLOG.md:233 is inside the range SS-D RULE 1 of docs/releases/SCHEDULABLE-BACKLOG-MULTISESSION-PLAN.md reserves: "the live ranked table (docs/BACKLOG.md:180-272) and the Distribution census (:169-171) are OWNER-ONLY. No session touches either, in any wave, for any reason." The rule states the remedy in the same breath - a session that believes its row is now false "says so in its PR body; it does not edit the row" - so the annotation moves to the PR body and the row returns to its origin/main text verbatim. Two independent reasons this is not pedantry: The rule's stated rationale already came true inside this very PR. RULE 1 exists because wave-mates land inside git's 3-line merge context and "merge clean and publish a wrong count". #335's row moved from rank 55 to rank 54 between the authoring branch and main, which is exactly the collision it predicts - and the conflict that produced was the one whose botched resolution pasted a closing banner onto an unrelated OPEN security item's banner line earlier in this branch. The census is NOT recomputed here either, deliberately. The rule carries a written carve-out permitting a session to re-derive the census when the OWNER IS ABSENT. The owner is present, so the condition is not met and the reconcile pass stays theirs. Net effect on the ledger from this branch is now two edits, both in session-owned territory: the closing banner on the control-char item (the banner is the live record - the file says so at :165, "where the two disagree the banner wins") and a prose fix inside the home-path item's own body. * docs(context): state the not-deployed status in CLAUDE.md so every session picks it up Owner-stated fact that was missing from the project's persistent context: MessageFoundry is a NOT-DEPLOYED beta with ZERO production instances and nobody running it, and publishing to PyPI is not deploying. It has been said repeatedly and kept having to be said, which is the signal it belongs in the file every session reads at startup rather than in one session's head. Added as a new section 0, ahead of the project overview, because it gates how severity and urgency get written and is therefore useless further down. Two consequences that pull in OPPOSITE directions, so both are stated explicitly: 1. Present-tense impact claims are factually false. "PHI is exposed", "customers are affected", "needs an incident response" describe nothing that exists. Beta defects get the conditional: "would expose X on first deployment". 2. Hypothetical migration costs are vacuous. Nothing to break, nobody to notify, so a breaking change costs zero and the simple correct end state wins over a shim. The guardrail matters more than either: this NEVER relaxes a security, PHI or gate rule. Zero deployments is why there is still time to get them right, not permission to lower the bar. Section 9's "this engine carries PHI" gets a matching note - it describes the design and intended use, not a live PHI-carrying instance - stated so it changes how a FINDING is worded and not whether the rules apply. Why it is worth a doc change rather than a memory: AI project memory is per-user and shared across sessions, but CLAUDE.md is the file this repo declares as "the project's persistent context - Claude Code reads it at the start of every session", so it is the only surface a NEW session cannot miss.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements BACKLOG #347 and retires it to the archive.
Four PHI-at-rest assertions proved plaintext absence by checking a short substring against random-keyed base64 ciphertext. That is wrong in both directions.
"DOE"appeared in 90 of them — roughly 1 in 2,222 per assertion, each one a red CI leg on working code. It has already fired: PR fix(smtp): the alerts + security-event SMTP hop was encrypted but unauthenticated (#323, layer 3) #142, job91502517146,test (windows-2022, py3.14).The correct form was already in the same file: assert the whole plaintext is absent. That is deterministic here because every plaintext carries characters base64 cannot emit (
|, CR, space,^,{,"). The earlier sweep simply never reached these call sites.Also strengthened in passing: the queue payload and content-search bodies only checked the marker prefix, so an unencrypted payload carrying the marker would have passed. They now assert plaintext-absence too.
The check that separates a fix from a cover-up
Weakening a flaky assertion until it stops failing is the obvious wrong move here. Against a simulated leaking store — marker present, body not enciphered — the new assertion fails, as it must. That is what makes this a strengthening rather than a silencing.
83 tests pass in the affected files; ruff clean.
Ledger
#347's banner is flipped and the item moves todocs/archive/backlog/BACKLOG-CLOSED.md, with the ranked table and all four census lines recomputed to 92 open items (P219 → 18).docs/BACKLOG.mdstarting at#1002. Those two edits touch different regions and would merge clean while leaving the census line wrong — the exact failure the multisession plan flags as SEVERE. This PR is being landed first deliberately, and the other session has been told to recompute the census from mergedmainrather than from its own base.