fix(checks): handler-security lint missed getattr access and undecorated helpers (BACKLOG #337) - #224
Merged
Merged
Conversation
…ed in undecorated helpers (BACKLOG #337)
Two execution-verified recall gaps in `_check_handler_security` (ADR 0144), closed and pinned by tests.
Gap 1 -- ambient-authority / impure-transform saw only a literal Name/Attribute chain, so a
`getattr(os, "system")("id")` indirection (the outer call's func is itself a Call) was never resolved
and never flagged. `_dotted_call_name` now splices a *constant* `getattr(mod, "name")` into the chain
(`getattr(os, "system")` -> `os.system`), so the ambient-authority deny-list sees it; the same shared
resolver flags a `getattr(time, "time")()` wall-clock read for impure-transform. A *dynamic*
`getattr(os, name)`, a `globals()["os"]` subscript, and any other call-shaped receiver stay unresolved
(None), so benign reflection is not flagged. The opt-in Semgrep leg still carries no getattr pattern
(recorded as a residual, not fixed here).
Gap 2 -- phi-to-log was decorated-scope only, so it never opened the undecorated
`_<feed>_transforms.py` helpers the decompose-by-role convention (docs/CONNECTIONS.md, #226) steers
field-level PHI handling into. phi-to-log now scans every function body -- a decorated @router/@handler
OR an undecorated helper -- keyed on the first positional parameter as the message symbol.
impure-transform stays decorated-scope ONLY: widening it too would re-flag the shipped
`_pdf_mdm_transforms.py` ingest-time wall-clock fallback and red the read-only samples/config
calibration gate (the trade ADR 0144 records). Proven both directions against the real samples before
landing.
Severity is unchanged and deliberately conservative -- it holds in BOTH sandbox postures. The lint is
advisory by default (required only under --strict-handler-security) and pre-deployment. 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. So under 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, no runtime-behaviour change ("a filter, not a fix"). To be re-scored
upward when ADR 0147 (OS-level default-deny; Proposed, no code) lands. MessageFoundry is a not-deployed
beta, so a miss "would slip past a deploying site's CI", never "PHI is exposed".
Docs in this commit describe the same change: ADR 0144's Decision-section phi-to-log bullet is
corrected to the widened every-function-body scope (it still asserted decorated-scope, contradicting
the amendment and the code) and its amendment blockquote is added; the master-test-plan row for this
test file is refreshed (line count 987 -> 1,079 and the three scoping regimes). The BACKLOG #337 banner
flip is a separate follow-on commit.
Tests (tests/test_checks_handler_security.py): amb_pos_getattr_os_system, amb_neg_getattr_dynamic_attr,
impure_pos_getattr_time_time, phi_pos_undecorated_helper_logs_msg, plus
test_widened_phi_to_log_does_not_widen_impure_transform and
test_phi_to_log_undecorated_helper_non_message_local_is_clean. Each was falsified by reverting the
fix: the two getattr positives red when the splice is removed; the phi positive reds when the loop
reverts to decorated-scope; the impure-transform guard reds when impure-transform is widened (which
also reds the real samples calibration); the discrimination test reds when phi-to-log ignores the
message symbol; the dynamic-getattr negative reds when the splice over-fires on a non-constant attr.
…lper recall gaps closed) Banner-only edit of docs/BACKLOG.md item #337: the open `[filed]` banner becomes a closed `[done]` banner recording the resolution. Isolated from the code+tests+docs commit per the banner-flip rule. Only #337's banner line changed. The ranked table and the four census distribution lines are untouched, and the census was NOT recomputed -- the coordinator reconciles it.
wshallwshall
enabled auto-merge (squash)
August 6, 2026 00:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes BACKLOG #337. Two recall gaps in the handler-security lint: attribute access via
getattrwas invisible to it, and helpers that are not decorated were never walked.Both are false-negative gaps -- the lint reported clean on code it could not see, which is the failure mode that matters for a security lint. A lint that misses a class is worse than no lint, because it converts "I should check this" into "already checked".
Per-lane notes, owner decisions and merge notes are in
.git/mefor-coord/handoff/LANE-HANDOVER-backlog-session-2e228e.md(not published).Verified: 5 files;
merge-treeagainstorigin/mainreports 0 conflicts; working tree clean at handoff.