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/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.) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 25ccb358..93c7e389 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. @@ -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/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_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" + ) 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 ---------------------------------------------- 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. # --------------------------------------------------------------------------------------------------