fix(checks): handler-security lint missed getattr access and undecorated helpers (BACKLOG #337)

docs/BACKLOG.md

@@ -3066,7 +3066,7 @@ Honestly bounded: **this is build-time only.** No PHI path, no running-engine su
 
 ## 337. handler-security lint: `getattr` indirection and the undecorated helper
 
-> 🔢 **Filed 2026-08-01 — not started.** Value **3/10** · Difficulty **3/10** · _fill-in_. `_AMBIENT_BARE_NAMES` (`checks.py:476`) still matches a literal name chain and `checks.py` contains no `getattr` resolution at all, and the rule loop still bails on `_message_fn_decorator(node) is None` (`:937`) so the `_<feed>_transforms.py` helper CONNECTIONS.md steers PHI handling into is never opened — but the lint is advisory unless an adopter opts into `--strict-handler-security`, and evading it reaches neither the DEK nor the audit chain in either sandbox posture; ~15 lines splicing a constant into `_dotted_call_name` plus a `phi-to-log` widening that must be recalibrated against the two shipped sample helpers before it lands.
+> ✅ **Done 2026-08-05 (#337).** Value **3/10** · Difficulty **3/10**. Both recall gaps in `_check_handler_security` (`checks.py`) are closed and pinned. A constant `getattr(mod, "name")` indirection now resolves in `_dotted_call_name`, so `getattr(os, "system")(...)` is flagged for `ambient-authority` (the shared resolver also flags a `getattr(time, "time")()` wall-clock read for `impure-transform`); and `phi-to-log` now scans undecorated `_*` transform helpers keyed on the first positional parameter, while `impure-transform` stays decorated-scope so the shipped `_pdf_mdm_transforms.py` ingest-time timestamp fallback stays clean. New `tests/test_checks_handler_security.py` cases cover both, and the change was proven green against `samples/config` before landing. Still an advisory-by-default filter — an evasion reaches neither the DEK nor the audit chain in either sandbox posture; ADR 0144 amended, to be re-scored upward when ADR 0147 (OS-level default-deny) lands.
 
 **Cluster:** Security & Compliance. **Priority:** P3. **Verdict:** build (small). **Severity:** low.
 

docs/adr/0144-security-lint-gate-over-admin-authored-router-handler-config.md

@@ -66,7 +66,10 @@ the `ruff`/`mypy` run-if-installed convention in escalation.
     logger-shaped receiver** (`log`/`logger`/`logging`/`getLogger(...)`; `.debug` excluded) — the
     CLAUDE.md §9 "never log full bodies at INFO+" rule. The logger-receiver gate keeps a FHIR/ACK/
     validation builder (`outcome.error(...)`, `warnings.warn(...)`) from being mistaken for a sink.
-    Scoped to `@router`/`@handler` **bodies** (not the signature/defaults).
+    Scans **every** function body — a decorated `@router`/`@handler` or an undecorated `_*` transform
+    helper — keying on the first positional parameter as the message symbol (not the signature/defaults).
+    Widened from decorated-scope by BACKLOG #337 (see the 2026-08-05 amendment); `impure-transform`
+    stays `@router`/`@handler`-scoped.
   - **`unsafe-db-lookup`** — a **non-constant** interpolation (an f-string with a value, `+`/`%` with a
     variable operand, `.format(...)`) flowing into the `db_lookup`/`fhir_lookup` **statement/query**
     argument (2nd positional or `statement=`/`query=`). A pure-literal concat folds to a constant and is
@@ -187,12 +190,24 @@ not a boundary) and recorded here rather than chased into diminishing-returns pr
   log.info(body)` — **recovered by the opt-in Semgrep taint leg (Increment 3, Control B)**; still a
   false-negative of the stdlib `check` itself.
 - **Aliased-import evasion** (`from subprocess import run as r; r(...)`) is out of scope of the AST scan —
-  **recovered by the Semgrep leg (Increment 3)**, which resolves the import alias. `getattr(os, "system")`
-  remains a false-negative; the lint is a fallible-author guardrail, not a malicious-bypass boundary
-  (that is ADR 0087's job).
-- **Decorated-scope only** for `phi-to-log`/`impure-transform`: PHI logged or a wall-clock read inside an
-  **undecorated** helper is not scanned (the trade that keeps the shipped `_pdf_mdm_transforms.py`
-  timestamp fallback clean).
+  **recovered by the Semgrep leg (Increment 3)**, which resolves the import alias. A *constant*
+  `getattr(mod, "name")` indirection is now resolved in `_dotted_call_name` (BACKLOG #337), so
+  `getattr(os, "system")(...)` **is** flagged (and the same splice flags a `getattr(time, "time")()`
+  wall-clock read). The remaining false-negatives are a *dynamic* `getattr(os, name)`, a
+  `globals()["os"]` / subscript indirection, and the opt-in Semgrep leg (which still carries no `getattr`
+  pattern). The lint is a fallible-author guardrail, not a malicious-bypass boundary (that is ADR 0087's job).
+- **`phi-to-log` reaches undecorated `_*` helpers** (BACKLOG #337): the decompose-by-role convention
+  (`docs/CONNECTIONS.md` §"Decomposing by role", #226) steers field-level PHI handling into undecorated
+  `_<feed>_transforms.py` helpers, so `phi-to-log` scans **every** function body — decorated or not —
+  keying on the first positional parameter as the message symbol. Residuals: an `async def` handler is
+  still scanned by neither rule (`_message_fn_decorator` is `FunctionDef`-only — a third gap this lane
+  reported but did not scope), and a helper whose first parameter is *not* the message is an accepted
+  advisory FP/FN tail.
+- **`impure-transform` stays decorated-scope only**: a wall-clock read inside an **undecorated** helper is
+  not scanned — the trade that keeps the shipped `_pdf_mdm_transforms.py` ingest-time timestamp fallback
+  clean (an undecorated `time.gmtime(ingest_time or time.time())`). This, not `phi-to-log`, is the actual
+  justification for the decorated-scope gate; widening `impure-transform` would re-introduce that false
+  positive and red the samples gate.
 - **Non-recursive** (`glob("*.py")`, no subdirs) — mirrors the existing `_check_raise_fstring` convention.
 - **Trusted-identifier concat** (`"select … from " + TABLE`, PHI parameterized) still nudges — SQL cannot
   parameterize an identifier, so the concatenation reminder is intentional (silence it with a literal
@@ -211,6 +226,23 @@ not a boundary) and recorded here rather than chased into diminishing-returns pr
   resolve the distribution behind an import. Distinct typosquat/hallucinated names are caught; same-name
   substitution is ADR 0087's boundary, not this lint's.
 
+> **AMENDED 2026-08-05 (BACKLOG #337) — two recall gaps closed; the severity of a miss is unchanged.**
+> A *constant* `getattr(mod, "name")` indirection now resolves in `_dotted_call_name`, so
+> `getattr(os, "system")(...)` is flagged for `ambient-authority` (the shared resolver also flags a
+> `getattr(time, "time")()` wall-clock read for `impure-transform`), and `phi-to-log` now reaches
+> undecorated `_*` transform helpers. The severity framing is deliberately conservative and holds in
+> **both** sandbox postures: the lint is advisory and pre-deployment, and an evasion reaches only **host**
+> actions the ADR 0087 sandbox does not confine. `DEFAULT_FORBIDDEN_MODULES` (`pipeline/sandbox.py`) blocks
+> `socket`/`ssl`/`asyncio`/`multiprocessing`, the I/O-bearing `messagefoundry.*` subpackages and
+> `cryptography`, but **not `os` or `subprocess`** — ADR 0087 confines the **address space**, not the
+> **host**. So under `[sandbox].mode=off` the author already holds in-process execution, and under
+> `mode=subprocess` an evasion still reaches neither the DEK nor the audit chain: no PHI-exposure path and
+> no runtime-behaviour change in either posture ("a filter, not a fix"). This should be **re-scored upward
+> when [ADR 0147](0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md)
+> lands** (OS-level default-deny; *Proposed*, no code today), at which point the lint becomes load-bearing
+> for exactly the class OS confinement is meant to close. MessageFoundry is a not-deployed beta, so a miss
+> "would slip past a deploying site's CI", never "PHI is exposed".
+
 ## Increment 3 (built)
 
 All four Increment-3 follow-ons shipped:

docs/testing/master-test-plan/16-security-phi-and-supply-chain.md

@@ -152,7 +152,7 @@ spaces collide, so every foreign ID below is prefixed.
 | `tests/test_last_resort.py` (104) | ASVS 16.5.4 at unit level: the asyncio loop handler **and** `sys.excepthook` both route an unhandled exception through `safe_exc` — exception type preserved, the planted PHI fragment absent from the log record — `KeyboardInterrupt` passes to `sys.__excepthook__` untouched, and a framed MLLP listener survives a raising handler (connection drops, server stays up). What it does **not** prove: that the handlers are installed in a real serving process, or that the redacted record stays clean across *every* configured sink (SEC-73). |
 | `tests/test_cert_cli.py` (446) + `tests/test_cert_expiry.py:308-312` | `messagefoundry/pki.py` exercised through the `cert` CLI: in-memory `.pfx` bundles built with `pkcs12.serialize_key_and_certificates`, `read_cert_facts` staying best-effort on an unparseable extension rather than sinking the cert, and `pki._SECONDS_PER_DAY` pinned equal to `pipeline/cert_expiry.py`'s. No adversarial PKCS#12 corpus and no key-material-egress assertion (SEC-75/SEC-76). |
 | `tests/test_anon_core.py` (326) + `test_anon_integration.py` + `test_anon_parity.py` | ADR 0030 end to end: salt-keyed determinism + weak-salt reject, the two-layer rule model, structure-preserving surrogates, FREETEXT blunt redact + OBX-2 gating, MSH-separator whole-field write, fail-closed no-MSH refusal, field-anchored site-code scrub, engine↔tee byte-identical parity. |
-| `tests/test_checks_handler_security.py` (987) | ADR 0144 lint: all five rule families, decorated-scope vs whole-file scoping, the operator allow-root escape, and the advisory-vs-strict `CheckResult` shape. |
+| `tests/test_checks_handler_security.py` (1,079) | ADR 0144 lint: all five rule families across their three scoping regimes (`impure-transform` decorated-scope, `phi-to-log` every-function-body per BACKLOG #337, the rest whole-module), the operator allow-root escape, and the advisory-vs-strict `CheckResult` shape. |
 | `.github/workflows/security.yml` `semgrep` job + `scripts/ci/assert_semgrep_handler_taint.py` | The packaged `handler-security.yml` rules are syntactically valid, the two recovered false-negatives fire exactly as annotated, `# ok` cases stay clean, and `samples/config` scans clean under `--error`. |
 | `tests/test_sandbox.py` (293) | ADR 0087: off-mode byte-identical in-process call, worker bootstrap, forbidden-import guard, resource caps, fail-closed `SandboxError` routing to ERROR/dead-letter post-ACK, and the `db_lookup`/`fhir_lookup` refusal. |
 | `tests/test_secrets_dpapi.py` (125) + `test_secretprovider.py` + `test_backup_crypto.py` | DPAPI machine- and user-scope round-trip (Windows-only), SecretProvider `none`/`env`/`vault` dispatch with fail-closed ref-without-provider and empty-value, and `.mfbak` chunked-AEAD tamper detection. |

messagefoundry/checks.py

@@ -574,14 +574,50 @@
     return None
 
 
+def _const_getattr_target(node: ast.expr) -> tuple[str, ast.expr] | None:
+    """For a ``getattr(receiver, "<str literal>")`` call, return ``(attr_name, receiver)`` so the chain
+    resolver can splice the constant attribute in — turning ``getattr(os, "system")`` into ``os.system``.
+
+    Returns None for anything else: a non-``getattr`` call, a *dynamic* attribute
+    (``getattr(os, name)`` — a variable, not a str constant), or a ``*args`` splat in the first two
+    positions (which makes positional indexing meaningless). Only a statically-known constant attribute
+    name is spliced; a dynamic indirection stays unresolvable, so benign reflection is not flagged."""
+    if not (
+        isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "getattr"
+    ):
+        return None
+    if len(node.args) < 2 or any(isinstance(a, ast.Starred) for a in node.args[:2]):
+        return None
+    attr = node.args[1]
+    if isinstance(attr, ast.Constant) and isinstance(attr.value, str):
+        attr_name: str = attr.value  # bound locally for mypy-strict narrowing
+        return attr_name, node.args[0]
+    return None
+
+
 def _dotted_call_name(func: ast.expr) -> str | None:
     """Reconstruct a dotted Name/Attribute chain (``os.path.join``) from a call's ``func``, or None
-    when it is not a pure Name/Attribute chain (e.g. the receiver is itself a call or subscript)."""
+    when the chain does not bottom out in a plain Name (e.g. the receiver is a subscript or a
+    non-``getattr`` call).
+
+    A *constant* ``getattr`` indirection is spliced: ``getattr(os, "system")`` resolves to ``os.system``
+    (and ``getattr(pkg, "sub").fn`` to ``pkg.sub.fn``). A *dynamic* ``getattr(os, name)``, a subscript
+    (``globals()["os"]``), or any other call-shaped receiver stays unresolvable → None. Each iteration
+    strictly descends a finite AST (Attribute → its value, or a getattr splice → its receiver), so the
+    loop always terminates."""
     parts: list[str] = []
     cur: ast.expr = func
-    while isinstance(cur, ast.Attribute):
-        parts.append(cur.attr)
-        cur = cur.value
+    while True:
+        if isinstance(cur, ast.Attribute):
+            parts.append(cur.attr)
+            cur = cur.value
+            continue
+        target = _const_getattr_target(cur)
+        if target is None:
+            break
+        attr_name, receiver = target
+        parts.append(attr_name)
+        cur = receiver
     if isinstance(cur, ast.Name):
         parts.append(cur.id)
         return ".".join(reversed(parts))
@@ -909,9 +945,10 @@
     that wants a hard gate on its own CI. The runtime half is the opt-in ADR 0087 sandbox. Mirrors
     :func:`_check_raise_fstring`: static ``ast`` only (never imports/executes the config), globs
     ``*.py`` under ``config_dir`` (helpers included), and skips a broken/unreadable file (``validate``
-    reports those) so it never crashes the gate. ``phi-to-log`` and ``impure-transform`` are scoped to
-    ``@router``/``@handler`` bodies (so an undecorated helper's wall-clock fallback is not a false
-    positive); the other three scan the whole module."""
+    reports those) so it never crashes the gate. ``impure-transform`` is scoped to ``@router``/``@handler``
+    bodies (so an undecorated helper's wall-clock fallback is not a false positive); ``phi-to-log`` scans
+    every function body — decorated or an undecorated ``_*`` transform helper — keying on the first
+    positional parameter as the message symbol (BACKLOG #337); the other three scan the whole module."""
     base = Path(config_dir)
     if not base.is_dir():
         return CheckResult(
@@ -946,19 +983,25 @@
                 file_hits.append((node.lineno, "unsafe-db-lookup"))
             if _ambient_authority_hit(node):
                 file_hits.append((node.lineno, "ambient-authority"))
-        # Decorated-scope rules — phi-to-log + impure-transform, over each router/handler's own body
+        # Per-FunctionDef body rules — phi-to-log + impure-transform, over each function's own body
         # only (not nested defs, not the signature), so each call is scanned once with its own message
-        # symbol and an import-time default arg is never mistaken for per-message impurity.
+        # symbol and an import-time default arg is never mistaken for per-message impurity. phi-to-log
+        # runs over EVERY function body — a decorated @router/@handler AND an undecorated `_*` transform
+        # helper (BACKLOG #337, CLAUDE.md §9; the decompose-by-role convention steers PHI handling into
+        # `_*` helpers, so a lint that only saw decorated bodies skipped the file the convention names),
+        # keying on the first positional parameter as the message symbol. impure-transform stays
+        # decorated-scope only, so the shipped `_pdf_mdm_transforms.py` ingest-time wall-clock fallback
+        # in an undecorated helper is not a false positive (the trade ADR 0144 records).
         for node in ast.walk(tree):
-            if _message_fn_decorator(node) is None:
+            if not isinstance(node, ast.FunctionDef):
                 continue
-            assert isinstance(node, ast.FunctionDef)  # narrowed by _message_fn_decorator
+            decorated = _message_fn_decorator(node) is not None
             params = node.args.posonlyargs + node.args.args
             msg_sym = params[0].arg if params else None
             for sub in _body_calls(node):
                 if msg_sym is not None and _phi_to_log_hit(sub, msg_sym):
                     file_hits.append((sub.lineno, "phi-to-log"))
-                if _impure_transform_hit(sub, imported):
+                if decorated and _impure_transform_hit(sub, imported):
                     file_hits.append((sub.lineno, "impure-transform"))
         hits += [f"{path.name}:{lineno} [{rule}]" for lineno, rule in sorted(file_hits)]
     if not hits: