From 558f0c0713e6d01cb142ab73151c5805f457be0b Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 3 Aug 2026 22:09:26 -0500 Subject: [PATCH 1/6] 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. --- tests/test_at_rest_assertion_control.py | 106 ++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 tests/test_at_rest_assertion_control.py diff --git a/tests/test_at_rest_assertion_control.py b/tests/test_at_rest_assertion_control.py new file mode 100644 index 00000000..36d6abaf --- /dev/null +++ b/tests/test_at_rest_assertion_control.py @@ -0,0 +1,106 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Negative control for the PHI-at-rest assertion FORM itself (BACKLOG #347 follow-up). + +**Why this file exists.** The at-rest tests assert that a stored body is ciphertext by checking the +`mfenc:` marker and the absence of the plaintext. When one of those assertions flakes, the cheap fix is +to weaken it until it stops failing — and a weakened assertion is indistinguishable, on a green run, +from a working one. That is the whole argument of BACKLOG #1000: a control nobody has watched fail is +an assumption wearing a green tick. + +So the *form* gets its own control. These tests do not exercise the store; they exercise the predicate +the store tests rely on, against a body that is deliberately NOT enciphered. If someone later relaxes +`assert PLAINTEXT not in stored` back to a short-substring check, the last test here goes red and says +why. + +This is deliberately a separate file from `test_store_encryption.py`: it is about the assertion, not +about the cipher, and keeping it separate means a sweep of the store tests cannot quietly take it with +them. +""" + +from __future__ import annotations + +import base64 +import os + +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher + +# A synthetic ADT — never real PHI. Carries '|' and CR, which base64 cannot emit; that is precisely +# what makes whole-plaintext absence a DETERMINISTIC assertion rather than a probabilistic one. +ADT = "MSH|^~\\&|S|F|R|RF|20260101||ADT^A01|MSG1|P|2.5.1\rPID|1||100^^^H^MR||DOE^JANE\r" + + +def _leaking_at_rest(plaintext: str) -> str: + """A stored value that carries the marker but was NEVER enciphered. + + This 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. In + every case the marker is present and the body is readable. + """ + return MARKER_PREFIX + "v1:deadbeef:" + plaintext + + +def test_the_marker_check_alone_does_not_detect_a_leak() -> None: + """Establishes the baseline: `startswith(MARKER_PREFIX)` proves nothing about confidentiality. + + Stated as a passing test rather than a comment, because the marker half is genuinely load-bearing + for "is this row enciphered at all" and must NOT be removed — it just cannot carry the PHI claim + on its own. Both halves are needed, for different reasons. + """ + leaked = _leaking_at_rest(ADT) + assert leaked.startswith(MARKER_PREFIX) # the marker is happy... + assert ADT in leaked # ...while the entire body sits there in cleartext + + +def test_whole_plaintext_absence_detects_a_leak() -> None: + """THE CONTROL. The form used at every at-rest call site must fail on an unenciphered body. + + If this test ever needs changing to accommodate a "fix" to a flaky at-rest assertion, that is the + signal the fix weakened the property rather than sharpened it. + """ + leaked = _leaking_at_rest(ADT) + detected = not (leaked.startswith(MARKER_PREFIX) and ADT not in leaked) + assert detected, ( + "the whole-plaintext-absence assertion PASSED on a body that was never enciphered — " + "the at-rest PHI check is no longer able to detect the thing it exists for" + ) + + +def test_whole_plaintext_absence_does_not_flake_on_real_ciphertext() -> None: + """The other direction: the form must not red on correct encryption. + + A control that catches leaks by being trigger-happy would be swapped out within a week. Real + tokens, real key, many draws — the deterministic form has no false-positive rate to measure. + """ + cipher = make_cipher(generate_key()) + for _ in range(200): + token = cipher.encrypt(ADT) + assert token.startswith(MARKER_PREFIX) and ADT not in token + + +def test_a_short_substring_check_is_not_a_substitute() -> None: + """Why the retired form was replaced, pinned so it cannot quietly return. + + A 3-character needle against a random base64 body collides at a rate that is small per assertion + and NOT small per CI run — measured at ~1 in 2,222 per assertion here, and it did fire in CI. The + point is not the exact rate; it is that the rate is nonzero and unbounded by anything the test + controls, while the whole-plaintext form's is zero by construction. + + Both properties are asserted over the SAME draws, so this cannot pass by sampling luck: whatever + the short needle does, the deterministic form is clean across every one of them. + """ + cipher = make_cipher(generate_key()) + short_needle_hits = 0 + for _ in range(2000): + token = cipher.encrypt(ADT) + assert ADT not in token # deterministic form: never fires on correct encryption + if "DOE" in token: + short_needle_hits += 1 + + # Independent of the cipher, to keep this honest about WHY: it is a property of random base64, + # not of this particular token stream. + random_hits = sum(1 for _ in range(20000) if "DOE" in base64.b64encode(os.urandom(96)).decode()) + assert random_hits > 0, ( + "'DOE' never appeared in 20,000 random base64 bodies — that contradicts the measured rate " + "(~1 in 2,222) and means this control is no longer demonstrating the flake it documents" + ) From f2d996175bd9b5954eb251068448dfc2c92af702 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 07:36:21 -0500 Subject: [PATCH 2/6] 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\\proj FIRES c:/users//proj missed c:\users\\proj missed C:\USERS\\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/, /Users/ FIRE (unchanged) /users/ 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. --- docs/BACKLOG.md | 2 +- scripts/security/scan_forbidden.py | 20 +++++++++++-- tests/test_scan_tokens_source.py | 47 ++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 25ccb358..b00492d0 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -2822,7 +2822,7 @@ No test covers it. `tests/test_scan_tokens_source.py:559-583` (`test_absolute_ho 4. Add the regression case to `tests/test_scan_tokens_source.py:559`, alongside the existing canonical fixtures — a lowercased and an upper-cased Windows path must both produce a hit, and the POSIX `/users/…` non-match should be asserted deliberately so the next person does not "fix" it into the 47-false-positive form. -5. **Same fix site, sibling defect:** `_WORKTREE_SLUG` at `scripts/security/scan_forbidden.py:92` is case-blind the same way (`[a-z0-9]+`); `claude/Some-Task-a1b2c3` is MISSED. `scripts/worktree/new.ps1:43,86` passes `-Name` through verbatim with no lowercasing, so an upper-cased worktree name is reachable. Narrower than the home-path case (agent-created slugs are lowercase by convention), but it is a two-character edit in the same block — take it in the same change or say why not. +5. **Same fix site, sibling defect:** `_WORKTREE_SLUG` at `scripts/security/scan_forbidden.py:92` is case-blind the same way (`[a-z0-9]+`); an upper-cased slug — `claude/` followed by `Some-Task-a1b2c3` — is MISSED. (Written split on purpose, for the reason in the note above: once the fix lands, the joined literal trips the very detector it documents, and unlike `_HOME_PATH` the slug pattern has no `<…>` exemption to write it into.) `scripts/worktree/new.ps1:43,86` passes `-Name` through verbatim with no lowercasing, so an upper-cased worktree name is reachable. Narrower than the home-path case (agent-created slugs are lowercase by convention), but it is a two-character edit in the same block — take it in the same change or say why not. **Related:** `scripts/security/scan_forbidden.py` (`_HOME_PATH` :99-106, `_WORKTREE_SLUG` :92, call site :758-759), `tests/test_scan_tokens_source.py:559-583`, `.github/workflows/security.yml:446-493`, `.github/required-contexts.txt`, `scripts/worktree/new.ps1`. Sibling **#321** — same gate, same "green gate that cannot see the class" root cause, but the **opposite mechanism**: #321 is an incomplete *token source* (data, fixed by the owner updating a private secret) and explicitly scopes itself away from scanner defects at `docs/BACKLOG.md:7356`; this is a *structural detector* defect (code, fixed by a regex edit) that is live even with no token source. Also **#322**, and the anonymizer's structural-detector item from this same audit. Note #321's **Related:** line at `docs/BACKLOG.md:7363` cites `tests/test_scan_forbidden.py` for regression tests, but the home-path test actually lives in `tests/test_scan_tokens_source.py` — worth correcting when someone next touches #321. diff --git a/scripts/security/scan_forbidden.py b/scripts/security/scan_forbidden.py index 71ceef8e..f78f2df3 100644 --- a/scripts/security/scan_forbidden.py +++ b/scripts/security/scan_forbidden.py @@ -88,16 +88,30 @@ # # A worktree/branch slug is whatever the task happened to be CALLED, so it can name a prospect segment, # a customer engagement, or a competitor study. That is unbounded: the leak is the project name itself, -# and there is no list to add it to. Matching the shape is the only control that scales. -_WORKTREE_SLUG = re.compile(r"(?:claude/|worktrees/)[a-z0-9]+(?:-[a-z0-9]+)*-[0-9a-f]{6}") +# and there is no list to add it to. Matching the shape is the only control that scales. It is +# case-folded whole: agent slugs are lowercase by convention, but scripts/worktree/new.ps1 +# validates -Name as ^[A-Za-z0-9._-]+$ and hands it to `git worktree add -b` verbatim, so an +# upper-cased slug is reachable -- and unlike _HOME_PATH no common URL shape collides here. +_WORKTREE_SLUG = re.compile(r"(?i:(?:claude/|worktrees/)[a-z0-9]+(?:-[a-z0-9]+)*-[0-9a-f]{6})") # An absolute user-home path carries the OS account name, and inside a worktree path the slug as well. # Exempt: bracket/env placeholders (, $HOME, %USERPROFILE%, {home}), the well-known shared and CI # accounts, and the DOCUMENTATION placeholder names this repo already uses in examples (me, svc, you, # user, username, example). Everything else looks like a real account and fires. That list is the whole # judgement call here: "is this a real person's login" is not decidable by shape, so the pattern trusts # a small, explicit set of conventional stand-ins and treats anything else as a disclosure. +# +# The drive-letter arm folds case INLINE. Windows paths are case-INSENSITIVE, so `C:\Users\`, +# `c:\users\` and `C:\USERS\` are the SAME directory naming the SAME account, and a +# literal `Users` caught only one of those four spellings. (The examples use the `` +# placeholder the lookahead below exempts: a real account segment written here would trip this +# very detector.) Keep the fold SCOPED to that arm -- do NOT lift it to a whole-pattern +# re.IGNORECASE. That also lower-cases the POSIX /Users arm, and `/users/` is an extremely +# common URL segment: measured, it then matches the web console's /ui/users/... routes in 47 +# places on the tracked tree and reds this required context on its first run. The exemption +# list below stays case-SENSITIVE for the inverse reason -- on POSIX `Public` and `public` are +# DIFFERENT accounts, and widening an exemption is the under-detection direction. _HOME_PATH = re.compile( - r"(?:[A-Za-z]:[\\/]Users|/home|/Users)[\\/]" + r"(?:(?i:[A-Za-z]:[\\/]users)|/home|/Users)[\\/]" r"(?!<|\$|%|\{" r"|(?:Public|Default|All|ContainerAdministrator|runner|vsts" r"|me|svc|you|user|username|example)[\\/\s\"'`]" diff --git a/tests/test_scan_tokens_source.py b/tests/test_scan_tokens_source.py index baef6a74..1ae1da1b 100644 --- a/tests/test_scan_tokens_source.py +++ b/tests/test_scan_tokens_source.py @@ -583,6 +583,53 @@ def test_absolute_home_path_is_flagged_but_placeholders_are_not( ), "placeholders / CI / shared accounts must not fire" +def test_home_path_casing_variants_all_fire_but_the_posix_users_route_does_not( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Windows paths are case-INSENSITIVE: all four spellings name the SAME account. + + The ``/users/`` non-match at the end is asserted deliberately, not incidentally -- it pins the + asymmetry that keeps the case-fold scoped to the drive-letter arm, so the next reader cannot + quietly widen it to a whole-pattern ``re.I``. The scanner's ``_HOME_PATH`` comment says why. + """ + mod = _load(None, monkeypatch) + variants = tmp_path / "variants.md" + # Assembled like the fixtures above so no SOURCE line here is itself a match -- this file is + # scanned by the gate it tests. + variants.write_text( + f"c:{_BS}users{_BS}Carol{_BS}Code\n" + f"C:{_BS}USERS{_BS}Dave{_BS}Code\n" + "c:/" + "users/Erin/Code\n", + encoding="utf-8", + ) + hits = mod.scan_file(variants, "docs/variants.md") # type: ignore[attr-defined] + assert sum("absolute user-home path" in h for h in hits) == 3 + assert not any(n in h for h in hits for n in ("Carol", "Dave", "Erin")) + + route = tmp_path / "route.md" + route.write_text("/users/list\nGET /ui/users/{id}/roles\n", encoding="utf-8") + assert not any( + "absolute user-home path" in h + for h in mod.scan_file(route, "docs/route.md") # type: ignore[attr-defined] + ), "a lower-cased POSIX /users/ segment is a REST route, not a home path" + + +def test_worktree_slug_casing_variant_is_flagged( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """An upper-cased slug is reachable, so it must not slip the gate. + + ``scripts/worktree/new.ps1`` validates ``-Name`` as ``^[A-Za-z0-9._-]+$`` and hands it to + ``git worktree add -b`` verbatim, with no lowercasing anywhere on the path. + """ + mod = _load(None, monkeypatch) + f = tmp_path / "notes.md" + # Split for the same reason as the fixtures above. + f.write_text("see .claude/work" + "trees/Some-Task-Name-a1b2c3 for details\n", encoding="utf-8") + hits = mod.scan_file(f, "docs/notes.md") # type: ignore[attr-defined] + assert any("worktree/branch slug" in h for h in hits) + assert not any("Some-Task-Name" in h for h in hits) + + # -------------------------------------------------------------------------------------------------- # Round-3 hardening: parser diagnostics, allowlist breadth, and per-section floors. # -------------------------------------------------------------------------------------------------- From 2fdd1e05f9e9ae7be5aefb7d0c79908fbbabea5e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 07:38:31 -0500 Subject: [PATCH 3/6] 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. --- CHANGELOG.md | 11 ++++ docs/BACKLOG.md | 6 +- ...is-triage-policy-accepted-risk-register.md | 16 ++++-- messagefoundry/logging_setup.py | 40 +++++++++++++- tests/test_logging.py | 55 +++++++++++++++++++ 5 files changed, 120 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b794a21..9ddf65bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,17 @@ All notable changes to MessageFoundry are documented here. The format follows documenting the weakness without changing the gate. ### Fixed +- **A CR/LF inside an exception message could forge a whole log line on the text sink.** + `ControlCharScrubFilter` escaped only the rendered message, and `logging.Formatter` appends a record's + traceback (`exc_text`) and stack dump (`stack_info`) **verbatim** — so a newline-bearing exception + string landed at column 0 on its own physical line, where a payload padded to the record layout was + byte-indistinguishable from a real entry to an operator or a line-oriented SIEM parser. Both fields + are now scrubbed too (ASVS 16.4.1; the residual ADR 0034 §1 disclosed, BACKLOG #335). **Visible + change:** a traceback is *not* collapsed onto one line — its line breaks are kept and every line is + indented with ` | `, so it stays readable while no line of it can start at column 0. A log parser + keyed on `Traceback (most recent call last):` at the start of a line needs that prefix added. The + JSON sink is unchanged in substance (`json.dumps` already escaped these fields); its `exception` + and `stack` values now carry the same indent. - **The DICOM C-STORE SCP's fail-closed refusal named a settings key that does not exist.** It told the operator to set `[inbound].source_ip_allowlist`; `InboundSettings` has no such field and section models ignore unknown keys, so an operator following the engine's **own error message** wrote a key diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index b00492d0..b751693f 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -230,7 +230,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal | 51 | **#124** | Batch-export message bodies from a connection log to a file | 4 | 3 | _fill-in_ | DEMAND-GATE | Console polish now that the capability itself ships — a scripted operator exports today through the audited step-up route, leaving only the save-selected affordance; the JS is already written (`messagefoundry_webconsole/static/app.js:1380`), so the cost is emitting the `data-mf-*` attributes and row checkboxes in `pages/messages.py` and registering `/ui/messages/export` ahead of `/ui/messages/{message_id}` (`routes/core.py:468`) so the path parameter cannot swallow it. | | 52 | **#133** | User-chosen display colour on configuration objects | 4 | 3 | _fill-in_ | DEMAND-GATE | Value 4 ("DX or console polish") is right and the stale-citation finding is right (no messagefoundry/console/ package; the live chrome is _html.py's page() head). But D3→2 rests on "a colour is that same shape [as `flagged`] plus a render", and that is false in a way this codebase enforces. `flagged` is a bool with no rendering sink; a colour is an operator-supplied STRING rendered into console markup, and the /ui CSP is `style-src 'self'` with no 'unsafe-inline' (_security.py:205, _auth.py:141, and app.css:2 states the constraint outright). An inline `style="…"` colour would simply not render, so the build must either bind a fixed palette to CSS classes shipped in app.css or add a nonce'd style mechanism the CSP does not currently grant for styles — a design decision plus value validation on untrusted config input, on top of the config-model → TOML → API → console thread. That is D3 ("a new setting into one connector"-scale work), not D2's "default flip or doc edit"-adjacent band. Quadrant stays fill-in; tier stays DEMAND-GATE. | | 53 | **#234** | Steps view projection refreshes on save only | 4 | 3 | _fill-in_ | P3 | UX latency on an opt-in authoring surface, not a correctness gap — the rows merely lag the buffer while live values stay correctly save-gated (ide/src/stepsView.ts:327); the debounce already exists at :89, but relaxing a deliberate ADR 0076 §5 guardrail means an amendment plus proving `EditLoopGuard` holds when projection races an in-flight `lens rewrite`. | -| 54 | **#335** | Control-char scrub misses `exc_text`/`stack_info` | 4 | 3 | _fill-in_ | P3 | `ControlCharScrubFilter.filter` still translates only `record.getMessage()` while `RedactionFilter` is the sole toucher of `exc_text`/`stack_info` (`logging_setup.py:124-131`), so a CR/LF traceback can forge a record on the text sink — but `JsonFormatter` escapes C0 regardless, the off-box forwarder defaults to json, and the message-path `exc_info` sites are a handful of non-peer-derived guards, so it is log-record integrity on one sink; the filter already runs last, so the cost is the readability call ADR 0034:146 defers plus tests and an ADR amendment. | +| 54 | **#335** | Control-char scrub misses `exc_text`/`stack_info` | 4 | 3 | _fill-in_ | P3 | ✅ **DONE 2026-08-04** — see the item's banner. The row is retained because the rank column and the open-item census above this table are recomputed as one pass, which this commit deliberately does not run; treat the census as one item stale until it does. As filed: `ControlCharScrubFilter.filter` translated only `record.getMessage()` while `RedactionFilter` was the sole toucher of `exc_text`/`stack_info`, so a CR/LF traceback could forge a record on the text sink. | | 55 | **#343** | Sandbox child stderr is inherited unframed into the engine log stream | 4 | 3 | _fill-in_ | P3 | The worker is still spawned `stderr=None` (`pipeline/sandbox.py:266`), so a sandboxed Handler's bytes land in the engine's own log stream unattributed and a `print()` of a body writes PHI at whatever level the operator runs — but the same `print()` under the default `mode=off` reaches the same stream, so the sandbox-specific loss is attribution and the fd-1 framing that survives on luck rather than design; a `stderr=subprocess.PIPE` relay thread through the stdlib logger (inheriting the existing PHI filters) plus a bootstrap redirect of the child's `sys.stdout`, all inside one module. | | 56 | **#346** | The sandbox import boundary is enforced only at runtime, under an off-by-default flag | 4 | 3 | _fill-in_ | P3 | The scorer verified the item's own measurement (`FORBIDDEN_MODULES` appears nowhere under `tests/`, confirmed) and inherited its conclusion — but the conclusion is the part that is false. The item's load-bearing claim is that "a re-violation is invisible to a green suite" because the guard runs only in the child under a non-default flag. `tests/test_sandbox.py` runs REAL `mode=SUBPROCESS` sessions across roughly a dozen tests (`test_subprocess_parity_router_and_handler`, `test_subprocess_marshals_live_store_run_context`, `test_generator_router_routes_under_mode_subprocess`, `test_setstate_tuple_and_nonfinite_values_survive_mode_subprocess`, ...) — the child is genuinely spawned, since the OFF test asserts `off._proc is None` as the distinguishing property. Decisively, `test_response_view_reaches_a_sandboxed_handler` (~:617-645) drives a `CapturedResponse` through a live subprocess round-trip, i.e. the exact violation instance the item is built on would now be caught red by CI. So the compensating control is a live test file, not absent, and the residual narrows to a FUTURE codec type added without an accompanying subprocess-mode test. That is test-coverage hardening = value 4, not "real gap, awkward workaround" = 6. Difficulty 3 stands (an `ast` walker anchored on the constant, falsified against a planted import). At value 4 the tier is P3 (P2 needs value >= 5) and the quadrant is fill-in. | | 57 | **#351** | SQL Server failover test asserts on a 0.35s wall-clock margin across a real DB round-trip | 4 | 3 | _fill-in_ | P3 | One observation on one leg, with the 2022 leg passing the same commit and a sibling PR passing both, bounds this to a marginal test whose red misattributes to whichever PR it fires on — the residual worth is settling whether #348's work at the `_acquire` chokepoint merely spent latency the test had no headroom for or tipped a real delay-predicate regression; the edit is confined to one test file, but it cannot be validated locally by default (the SQL Server leg silently skips) and must not be landed as a wider margin before the question is answered. | @@ -2654,7 +2654,7 @@ Wall time (the cleaner signal — all three still ingest everything up to 300/s) ## 321. Leak gate is blind to the ported-estate site-code and partner-product token class -> 🔢 **Filed 2026-08-01 — not started.** Value **7/10** · Difficulty **3/10** · _quick win_. A required merge context exited 0 on content carrying a real site code and a partner product name, with no compensating control (`scan_forbidden.py:10-12` is explicit that gitleaks finds secrets, not this class) and nothing stopping the next estate-derived identifier landing the same way; `.md` is not in `_SITE_SKIP_SUFFIXES` (`scan_forbidden.py:119`, `{".lock", ".svg"}`) so the file was scanned — the fix is owner-run token data across the private file plus the Actions *and* Dependabot secret stores, a negative test per class, and optionally a structural shape backstop. +> ✅ **DONE (2026-08-04).** `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 sink. The readability call ADR 0034 §1 deferred was taken explicitly and amended there in the same commit: the traceback is **not** collapsed to one line — its line breaks are kept and every line is indented with `_CONTINUATION_PREFIX` (`" | "`), so no traceback line starts at column 0 and none can impersonate `_LOG_FORMAT`. Pinned by the `test_control_char_*` tests in `tests/test_logging.py`. One residual stays open and is recorded in ADR 0034 §1: a handler carrying this filter *without* `RedactionFilter` would still hand the formatter an unrendered `exc_info` — no shipped handler is in that state. Scored **4/10** value · **3/10** difficulty when filed 2026-08-01. > ⚠️ **AMENDED 2026-08-03 — the "no negative test" premise is false; the detector-coverage half of Proposed 2 is already in the tree.** The item says "Today no test asserts the detectors can see a site code at all, which is why the hole was invisible", but `tests/test_scan_forbidden.py` carries per-class hit tests for at least the site code (`:126`), a customer name (`:83`), a case-sensitive code (`:91`) and a routable IP (`:107`), plus the boundary and skip-suffix controls at `:136` and `:152`, with the structural classes covered separately in `tests/test_scan_tokens_source.py` (`:539`, `:559`). ⚠️ **What those tests cannot prove is exactly what this item is about.** They monkeypatch a **synthetic** site-code pattern over `SITE_CODE_RE` / `_SITE_CODE_FILE` (`:50-52`, `:66-67`), so they exercise the machinery and never the loaded token set — and with no prefix loaded both detectors fall back to the always-failing sentinel `_NEVER` (`scripts/security/scan_forbidden.py:111`, `:453-454`). **Both remaining halves stand untouched:** the owner-run token data (Proposed 1) and the prefix-free estate-identifier shape backstop (Proposed 3) — the committed structural detectors are at least the routable-IPv4 pattern, `_WORKTREE_SLUG` (`:92`) and `_HOME_PATH` (`:99`), none of which match that shape. The item's own two anchors still resolve exactly (`scan_forbidden.py:10-12`, `:119`). @@ -3290,7 +3290,7 @@ What is *not* covered is the thing that will grow: `.semgrep/messagefoundry.yml` ## 335. Control-char scrub misses `exc_text`/`stack_info` -> 🔢 **Filed 2026-08-01 — not started.** Value **4/10** · Difficulty **3/10** · _fill-in_. `ControlCharScrubFilter.filter` still translates only `record.getMessage()` while `RedactionFilter` is the sole toucher of `exc_text`/`stack_info` (`logging_setup.py:124-131`), so a CR/LF traceback can forge a record on the text sink — but `JsonFormatter` escapes C0 regardless, the off-box forwarder defaults to json, and the message-path `exc_info` sites are a handful of non-peer-derived guards, so it is log-record integrity on one sink; the filter already runs last, so the cost is the readability call ADR 0034:146 defers plus tests and an ADR amendment. +> ✅ **DONE (2026-08-04).** `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 sink. The readability call ADR 0034 §1 deferred was taken explicitly and amended there in the same commit: the traceback is **not** collapsed to one line — its line breaks are kept and every line is indented with `_CONTINUATION_PREFIX` (`" | "`), so no traceback line starts at column 0 and none can impersonate `_LOG_FORMAT`. Pinned by the `test_control_char_*` tests in `tests/test_logging.py`. One residual stays open and is recorded in ADR 0034 §1: a handler carrying this filter *without* `RedactionFilter` would still hand the formatter an unrendered `exc_info` — no shipped handler is in that state. Scored **4/10** value · **3/10** difficulty when filed 2026-08-01. **Cluster:** Security / Logging. **Priority:** P3. **Verdict:** build (small). **Severity:** low. diff --git a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md index 4dfc96d5..f4b1c6d4 100644 --- a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md +++ b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md @@ -141,10 +141,18 @@ the class rationale **must not be inherited** by a future finding on a `log.exce `exc_info=True` site — the engine has many (the delivery/router/transform catches, the `_on_*_worker_done` callbacks, the pollers). `JsonFormatter` escapes `exc_text` through `json.dumps`, so the off-box forwarder (JSON by default) is unaffected; the residual is the human-readable stdout/NSSM text log. -**Open hardening (not done):** apply `_CTRL_TRANSLATION` to `exc_text`/`stack_info` in -`ControlCharScrubFilter` — it runs after `RedactionFilter`, so `exc_text` is already populated. It is a -few lines, but it collapses every traceback to one physical line, which is an operator-facing -readability change and wants an explicit decision rather than a drive-by edit. +**Open hardening — CLOSED 2026-08-04 (BACKLOG #335).** `ControlCharScrubFilter` now scrubs +`record.exc_text` and `record.stack_info` as well as the rendered message. The readability decision +deferred above was taken explicitly, and it is **not** the collapse-to-one-line this paragraph feared: +the traceback keeps its line breaks and every line is indented with `_CONTINUATION_PREFIX`, so no +traceback line begins at column 0 and none can impersonate the `_LOG_FORMAT` record prefix. Why the +block's *first* line is indented too, and why re-application is idempotent (every handler carries its +own filter chain, so one record is scrubbed once per sink), is recorded at `_scrub_block`; the property +is pinned by the `test_control_char_*` tests in `tests/test_logging.py`. **One residual survives, so the +register line at `:40` still reads wider than the code:** a handler carrying `ControlCharScrubFilter` +*without* `RedactionFilter` hands the formatter an unrendered `exc_info` that no filter has touched. No +shipped handler is in that state — `_install_phi_filters` installs both — but that is a construction +guarantee, not a scrub. The paragraph above stands as the record of what was true before. **2. `PinnedDependenciesID` — the blanket rationale was applied too widely.** "CI installs editably (`pip install -e .[extras]`), which cannot use `--require-hashes`" is true of the editable installs and diff --git a/messagefoundry/logging_setup.py b/messagefoundry/logging_setup.py index 5fb3c095..2a3d78ef 100644 --- a/messagefoundry/logging_setup.py +++ b/messagefoundry/logging_setup.py @@ -68,6 +68,30 @@ _CTRL_TRANSLATION[_i] = f"\\x{_i:02x}" _CTRL_TRANSLATION[0x7F] = "\\x7f" +#: Stamped on every physical line of a record's ``exc_text``/``stack_info`` (BACKLOG #335). A traceback +#: is multi-line by nature, so collapsing it the way the rendered message is collapsed would cost the +#: operator the readability an incident depends on. Its line breaks are kept and every line is indented +#: instead, so no traceback line starts at column 0 and none can impersonate the ``_LOG_FORMAT`` record +#: prefix (ASVS 16.4.1 — the readability call ADR 0034 §1 deferred). +_CONTINUATION_PREFIX = " | " + + +def _scrub_block(text: str) -> str: + """Escape control characters in a multi-line block (``exc_text``/``stack_info``) while KEEPING its + line breaks, indenting every line with :data:`_CONTINUATION_PREFIX`. + + The **first** line is indented too, so the guarantee does not rest on it being the stdlib + ``Traceback (most recent call last):`` header: ``Formatter.formatException`` emits no header at all + when the exception carries no ``__traceback__``, and that first line is then peer-derived text. + + Idempotent — the prefix is stripped before it is re-applied — because every handler carries its own + filter chain, so a record dispatched to stdout *and* the off-box forwarder is scrubbed twice and the + two sinks must not disagree.""" + return "\n".join( + _CONTINUATION_PREFIX + line.removeprefix(_CONTINUATION_PREFIX).translate(_CTRL_TRANSLATION) + for line in text.split("\n") + ) + class ControlCharScrubFilter(logging.Filter): """Neutralize CR/LF and other control characters in the rendered log message to prevent log @@ -76,7 +100,13 @@ class ControlCharScrubFilter(logging.Filter): Untrusted MLLP peer data and HL7-derived exception text reach the general log; without this a crafted value containing a newline could inject a forged log line into NSSM's captured stdout. We render the message (applying ``%`` args) once, escape any control characters, and only then - replace ``record.msg`` — clean messages keep their lazy ``msg``/``args`` untouched.""" + replace ``record.msg`` — clean messages keep their lazy ``msg``/``args`` untouched. + + ``record.exc_text`` and ``record.stack_info`` are covered too (BACKLOG #335, ADR 0034 §1), via + :func:`_scrub_block`. This filter is installed **last** (see :func:`_install_phi_filters`), so + :class:`RedactionFilter` has already rendered ``exc_info`` into ``exc_text`` and cleared it; a + handler carrying this filter *without* that one would leave an unrendered ``exc_info`` for the + formatter to expand unscrubbed.""" def filter(self, record: logging.LogRecord) -> bool: message = record.getMessage() @@ -84,6 +114,14 @@ def filter(self, record: logging.LogRecord) -> bool: if scrubbed != message: record.msg = scrubbed record.args = () + # The rendered message is only half the record: ``Formatter.format`` appends ``exc_text`` and + # ``stack_info`` VERBATIM, so a CR/LF inside an exception message forged a whole line on the + # text sink (BACKLOG #335). ``RedactionFilter`` is installed first and renders ``exc_info`` + # into ``exc_text``, so both fields are already populated when this filter runs. + if record.exc_text: + record.exc_text = _scrub_block(record.exc_text) + if record.stack_info: + record.stack_info = _scrub_block(record.stack_info) return True diff --git a/tests/test_logging.py b/tests/test_logging.py index 02da8193..3065cd1f 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -19,9 +19,11 @@ from messagefoundry import __main__ from messagefoundry.logging_setup import ( ControlCharScrubFilter, + CredentialQueryScrubFilter, JsonFormatter, RedactionFilter, SyslogForward, + _make_formatter, configure_logging, ) @@ -248,6 +250,59 @@ def test_redaction_filter_residual_bare_name_not_caught() -> None: assert "DOE^JANE" in out # accepted residual: a single-delimiter bare name passes through +# --- BACKLOG #335: the control-char scrub covers exc_text / stack_info ------- + +#: A payload shaped exactly like a real record under ``_LOG_FORMAT`` (level padded to eight columns). +_FORGED_RECORD = "2026-08-01T00:00:00Z INFO messagefoundry.auth: FORGED admin login ok" +#: Matches a line that OPENS with the production record prefix (a UTC stamp at column 0). +_RECORD_PREFIX_RE = re.compile(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z ") + + +def _production_lines(record: logging.LogRecord) -> list[str]: + """Render ``record`` the way a text sink does: the production filter chain, in the order + ``_install_phi_filters`` installs it, then the production text formatter.""" + for scrub in (RedactionFilter(), CredentialQueryScrubFilter(), ControlCharScrubFilter()): + scrub.filter(record) + return _make_formatter("text").format(record).split("\n") + + +def test_control_char_filter_scrubs_exception_traceback() -> None: + # ADR 0034 §1: ``Formatter.format`` appends exc_text VERBATIM, so a CR/LF inside an exception + # message used to land a forged record at column 0 on the text sink (stdout/NSSM, and a + # forward_format="text" collector). Exactly ONE line may open with the record prefix. + try: + raise ValueError(f"boom\n{_FORGED_RECORD}") + except ValueError: + rec = logging.LogRecord( + "mefor.demo", logging.ERROR, __file__, 1, "delivery failed", (), sys.exc_info() + ) + lines = _production_lines(rec) + assert _RECORD_PREFIX_RE.match(lines[0]) # the real record — proves the matcher can SEE one + assert [ln for ln in lines[1:] if _RECORD_PREFIX_RE.match(ln)] == [] + assert "FORGED admin login ok" in "\n".join(lines) # neutralized, not dropped + assert len(lines) > 3, "the traceback must stay multi-line — readability is the deferred call" + + +def test_control_char_filter_scrubs_stack_info() -> None: + # The same vector via stack_info, which the formatter also appends verbatim. + rec = logging.LogRecord("t", logging.ERROR, __file__, 1, "stack dump", (), None) + rec.stack_info = f"Stack (most recent call last):\n{_FORGED_RECORD}" + lines = _production_lines(rec) + assert [ln for ln in lines[1:] if _RECORD_PREFIX_RE.match(ln)] == [] + + +def test_control_char_block_scrub_is_idempotent() -> None: + # Every handler carries its OWN chain, so a record dispatched to stdout AND the off-box forwarder + # is scrubbed twice; a second pass must not re-indent an already-indented block, or the two sinks + # would print different text for the same record. + rec = logging.LogRecord("t", logging.ERROR, __file__, 1, "x", (), None) + rec.exc_text = f"Traceback (most recent call last):\n{_FORGED_RECORD}" + ControlCharScrubFilter().filter(rec) + once = rec.exc_text + ControlCharScrubFilter().filter(rec) + assert rec.exc_text == once + + # --- C2: prod-DEBUG serve guard ---------------------------------------------- From 70e3c43896bbbb6053142b6e4c4f63673719bc31 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 09:49:03 -0500 Subject: [PATCH 4/6] 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. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index b751693f..52c211ca 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -2654,7 +2654,7 @@ Wall time (the cleaner signal — all three still ingest everything up to 300/s) ## 321. Leak gate is blind to the ported-estate site-code and partner-product token class -> ✅ **DONE (2026-08-04).** `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 sink. The readability call ADR 0034 §1 deferred was taken explicitly and amended there in the same commit: the traceback is **not** collapsed to one line — its line breaks are kept and every line is indented with `_CONTINUATION_PREFIX` (`" | "`), so no traceback line starts at column 0 and none can impersonate `_LOG_FORMAT`. Pinned by the `test_control_char_*` tests in `tests/test_logging.py`. One residual stays open and is recorded in ADR 0034 §1: a handler carrying this filter *without* `RedactionFilter` would still hand the formatter an unrendered `exc_info` — no shipped handler is in that state. Scored **4/10** value · **3/10** difficulty when filed 2026-08-01. +> 🔢 **Filed 2026-08-01 — not started.** Value **7/10** · Difficulty **3/10** · _quick win_. A required merge context exited 0 on content carrying a real site code and a partner product name, with no compensating control (`scan_forbidden.py:10-12` is explicit that gitleaks finds secrets, not this class) and nothing stopping the next estate-derived identifier landing the same way; `.md` is not in `_SITE_SKIP_SUFFIXES` (`scan_forbidden.py:119`, `{".lock", ".svg"}`) so the file was scanned — the fix is owner-run token data across the private file plus the Actions *and* Dependabot secret stores, a negative test per class, and optionally a structural shape backstop. > ⚠️ **AMENDED 2026-08-03 — the "no negative test" premise is false; the detector-coverage half of Proposed 2 is already in the tree.** The item says "Today no test asserts the detectors can see a site code at all, which is why the hole was invisible", but `tests/test_scan_forbidden.py` carries per-class hit tests for at least the site code (`:126`), a customer name (`:83`), a case-sensitive code (`:91`) and a routable IP (`:107`), plus the boundary and skip-suffix controls at `:136` and `:152`, with the structural classes covered separately in `tests/test_scan_tokens_source.py` (`:539`, `:559`). ⚠️ **What those tests cannot prove is exactly what this item is about.** They monkeypatch a **synthetic** site-code pattern over `SITE_CODE_RE` / `_SITE_CODE_FILE` (`:50-52`, `:66-67`), so they exercise the machinery and never the loaded token set — and with no prefix loaded both detectors fall back to the always-failing sentinel `_NEVER` (`scripts/security/scan_forbidden.py:111`, `:453-454`). **Both remaining halves stand untouched:** the owner-run token data (Proposed 1) and the prefix-free estate-identifier shape backstop (Proposed 3) — the committed structural detectors are at least the routable-IPv4 pattern, `_WORKTREE_SLUG` (`:92`) and `_HOME_PATH` (`:99`), none of which match that shape. The item's own two anchors still resolve exactly (`scan_forbidden.py:10-12`, `:119`). From 7de774a7fb8f110ac25d1e098523980af2e6efd9 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 10:12:13 -0500 Subject: [PATCH 5/6] 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/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 52c211ca..93c7e389 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -230,7 +230,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal | 51 | **#124** | Batch-export message bodies from a connection log to a file | 4 | 3 | _fill-in_ | DEMAND-GATE | Console polish now that the capability itself ships — a scripted operator exports today through the audited step-up route, leaving only the save-selected affordance; the JS is already written (`messagefoundry_webconsole/static/app.js:1380`), so the cost is emitting the `data-mf-*` attributes and row checkboxes in `pages/messages.py` and registering `/ui/messages/export` ahead of `/ui/messages/{message_id}` (`routes/core.py:468`) so the path parameter cannot swallow it. | | 52 | **#133** | User-chosen display colour on configuration objects | 4 | 3 | _fill-in_ | DEMAND-GATE | Value 4 ("DX or console polish") is right and the stale-citation finding is right (no messagefoundry/console/ package; the live chrome is _html.py's page() head). But D3→2 rests on "a colour is that same shape [as `flagged`] plus a render", and that is false in a way this codebase enforces. `flagged` is a bool with no rendering sink; a colour is an operator-supplied STRING rendered into console markup, and the /ui CSP is `style-src 'self'` with no 'unsafe-inline' (_security.py:205, _auth.py:141, and app.css:2 states the constraint outright). An inline `style="…"` colour would simply not render, so the build must either bind a fixed palette to CSS classes shipped in app.css or add a nonce'd style mechanism the CSP does not currently grant for styles — a design decision plus value validation on untrusted config input, on top of the config-model → TOML → API → console thread. That is D3 ("a new setting into one connector"-scale work), not D2's "default flip or doc edit"-adjacent band. Quadrant stays fill-in; tier stays DEMAND-GATE. | | 53 | **#234** | Steps view projection refreshes on save only | 4 | 3 | _fill-in_ | P3 | UX latency on an opt-in authoring surface, not a correctness gap — the rows merely lag the buffer while live values stay correctly save-gated (ide/src/stepsView.ts:327); the debounce already exists at :89, but relaxing a deliberate ADR 0076 §5 guardrail means an amendment plus proving `EditLoopGuard` holds when projection races an in-flight `lens rewrite`. | -| 54 | **#335** | Control-char scrub misses `exc_text`/`stack_info` | 4 | 3 | _fill-in_ | P3 | ✅ **DONE 2026-08-04** — see the item's banner. The row is retained because the rank column and the open-item census above this table are recomputed as one pass, which this commit deliberately does not run; treat the census as one item stale until it does. As filed: `ControlCharScrubFilter.filter` translated only `record.getMessage()` while `RedactionFilter` was the sole toucher of `exc_text`/`stack_info`, so a CR/LF traceback could forge a record on the text sink. | +| 54 | **#335** | Control-char scrub misses `exc_text`/`stack_info` | 4 | 3 | _fill-in_ | P3 | `ControlCharScrubFilter.filter` still translates only `record.getMessage()` while `RedactionFilter` is the sole toucher of `exc_text`/`stack_info` (`logging_setup.py:124-131`), so a CR/LF traceback can forge a record on the text sink — but `JsonFormatter` escapes C0 regardless, the off-box forwarder defaults to json, and the message-path `exc_info` sites are a handful of non-peer-derived guards, so it is log-record integrity on one sink; the filter already runs last, so the cost is the readability call ADR 0034:146 defers plus tests and an ADR amendment. | | 55 | **#343** | Sandbox child stderr is inherited unframed into the engine log stream | 4 | 3 | _fill-in_ | P3 | The worker is still spawned `stderr=None` (`pipeline/sandbox.py:266`), so a sandboxed Handler's bytes land in the engine's own log stream unattributed and a `print()` of a body writes PHI at whatever level the operator runs — but the same `print()` under the default `mode=off` reaches the same stream, so the sandbox-specific loss is attribution and the fd-1 framing that survives on luck rather than design; a `stderr=subprocess.PIPE` relay thread through the stdlib logger (inheriting the existing PHI filters) plus a bootstrap redirect of the child's `sys.stdout`, all inside one module. | | 56 | **#346** | The sandbox import boundary is enforced only at runtime, under an off-by-default flag | 4 | 3 | _fill-in_ | P3 | The scorer verified the item's own measurement (`FORBIDDEN_MODULES` appears nowhere under `tests/`, confirmed) and inherited its conclusion — but the conclusion is the part that is false. The item's load-bearing claim is that "a re-violation is invisible to a green suite" because the guard runs only in the child under a non-default flag. `tests/test_sandbox.py` runs REAL `mode=SUBPROCESS` sessions across roughly a dozen tests (`test_subprocess_parity_router_and_handler`, `test_subprocess_marshals_live_store_run_context`, `test_generator_router_routes_under_mode_subprocess`, `test_setstate_tuple_and_nonfinite_values_survive_mode_subprocess`, ...) — the child is genuinely spawned, since the OFF test asserts `off._proc is None` as the distinguishing property. Decisively, `test_response_view_reaches_a_sandboxed_handler` (~:617-645) drives a `CapturedResponse` through a live subprocess round-trip, i.e. the exact violation instance the item is built on would now be caught red by CI. So the compensating control is a live test file, not absent, and the residual narrows to a FUTURE codec type added without an accompanying subprocess-mode test. That is test-coverage hardening = value 4, not "real gap, awkward workaround" = 6. Difficulty 3 stands (an `ast` walker anchored on the constant, falsified against a planted import). At value 4 the tier is P3 (P2 needs value >= 5) and the quadrant is fill-in. | | 57 | **#351** | SQL Server failover test asserts on a 0.35s wall-clock margin across a real DB round-trip | 4 | 3 | _fill-in_ | P3 | One observation on one leg, with the 2022 leg passing the same commit and a sibling PR passing both, bounds this to a marginal test whose red misattributes to whichever PR it fires on — the residual worth is settling whether #348's work at the `_acquire` chokepoint merely spent latency the test had no headroom for or tipped a real delay-predicate regression; the edit is confined to one test file, but it cannot be validated locally by default (the SQL Server leg silently skips) and must not be landed as a wider margin before the question is answered. | From 4fbcee2b95f7dc47cb5ae0d332bfd9e60ac43e23 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 10:16:10 -0500 Subject: [PATCH 6/6] 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. --- CLAUDE.md | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index cf1c76a4..9498dd91 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,6 +14,43 @@ stops matching the code, fix the doc. --- +## 0. Deployment status — read this before writing any severity claim + +> ⛔ **MessageFoundry is a NOT-DEPLOYED beta. There are ZERO production instances. Nobody is +> running it.** **Published to PyPI is *not* deployed** — a release artifact on an index is not a +> running instance, and the two get conflated constantly. Distinguish **shipped** (on `main`, on +> PyPI), **deployable**, and **deployed**: only the first two are true today. + +This is load-bearing because the wrong premise silently corrupts severity, urgency, and prose +across the repo. **Two consequences, and they pull in opposite directions — apply both:** + +1. **Present-tense impact claims are factually false.** *"PHI is exposed"*, *"customers are + affected"*, *"operators rely on this today"*, *"live feeds are shipping X"*, *"this needs an + incident response"* — none of these are true of anything here. Write beta defects in the + conditional: **"would expose X on first deployment"**, *"a deploying site would hit Y"*, *"is + wrong in the shipped code"*. False present tense does not stay local; it propagates into + security scorecards, review registers, BACKLOG banners and public docs, and a security record + asserting a live exposure that does not exist is exactly the *"compensating control resting on + a false premise"* defect §11 forbids. +2. **Hypothetical migration costs are vacuous.** *"breaks a running deployment on upgrade"*, + *"operators need notice / a migration window / a deprecation period"*, *"backward compatibility + with what sites have configured"* — there is nothing to break and nobody to notify, so the cost + of a breaking change is currently **zero**. Prefer the simple, correct end state over a staged + migration or compatibility shim; those are real costs paid to protect users who do not exist. + +⛔ **It cuts one way only — never cite "not deployed" to relax a rule.** It removes false urgency +and vacuous costs. It does **not** downgrade a fix, justify skipping a gate, weaken a control, or +make a finding unimportant. The security, PHI (§9) and leak-gate rules exist so the **first** +deployment is safe; zero deployments is why there is still time to get them right, not permission +to lower the bar. Note that §9's *"this engine carries PHI"* is a statement about the design and +intended use — **not** evidence of a live PHI-carrying instance. + +This is an **owner-stated fact**, repeatedly. Do not re-derive it, do not go looking for +deployments to confirm it, and do not soften it to "as far as I can tell". If an adopter ever goes +live, this section must be revised first — check with the owner before assuming it still holds. + +--- + ## 1. Project Overview MessageFoundry routes, transforms, and validates HL7 v2.x messages between **connections**, @@ -370,6 +407,10 @@ python samples/send_mllp.py samples/messages/adt_a01.hl7 This engine carries PHI. The full PHI map — threat model, data-at-rest inventory, redaction rules, and the retention/encryption roadmap + secure-ops checklist — is [`docs/PHI.md`](docs/PHI.md). Treat these as hard rules: + +> "Carries PHI" describes the **design and intended use** — it is not a claim that a live instance is +> holding PHI today (§0: zero deployments). That changes how you word a *finding*, never whether these +> rules apply: they are what make the first deployment safe, so none of them relax. - **Never log full message bodies at INFO or above.** Full payloads go only to the secured store, never to the general log. (Logging is stdlib today; structlog + redaction is planned — until then, don't raise the service to `DEBUG` in production.)