approval_airlock's new _redact_output (4933ff) fails OPEN for list/string outputs when phi_guard is unavailable — worse than the dict case it was written alongside (v1.4.0)
Reproduction steps:
- v1.4.0 adds workbench/approval_airlock.py's _redact_output() (commit
4933ff, attributed to dinkarshweta — closing bugs_found_v0.99.txt finding
#17, "make_receipt redacts only dict outputs; a list/string handler
output is sealed into the receipt with PHI/secrets unredacted"):
def _redact_output(output):
if isinstance(output, dict):
return redact(output)
if _phi is None:
return output
strict = bool(_phi.phi_mode_enabled())
return _phi.scrub(output, strict=strict)
- phi_guard is imported at module top with a try/except that degrades to
_phi = None on failure — the file's own comment calls this scope
explicit: "Absence degrades to the historical credential-only redaction
rather than blocking the airlock." For the DICT path (redact()), that
promise holds: redact()'s FIRST pass (credential scrubbing by field name,
SECRET_HINT) runs UNCONDITIONALLY, independent of _phi:
if any(h in str(k).lower() for h in SECRET_HINT):
out[k] = "••••••"
- The NEW non-dict branch has no equivalent fallback: `if _phi is None:
return output` returns the value completely UNCHANGED — not even the
credential-hint scrub the dict path retains.
- Run repro_redact_output_fails_open_no_phi_v140.py against a clean v1.4.0
extraction. It drives the REAL, unmodified _redact_output with phi_guard
simulated unavailable (AA._phi = None, the same "broken/partial
install" trigger condition already documented elsewhere in this codebase
for the historical guard_egress_messages ImportError bug):
list output: [{'ssn': '123-45-6789'}, 'Patient Jane Doe, ... api_key=sk_live_SECRET123'] (UNCHANGED)
string output: 'call me at api_key=sk_live_ANOTHERSECRET or SSN 123-45-6789' (UNCHANGED)
dict output (control): {'api_key': '••••••', 'notes': 'SSN 123-45-6789'} (partially scrubbed)
CONFIRMED
Expected:
The same "historical credential-only redaction, never nothing" floor the
file's own comment promises for the dict case must hold for list/string
outputs too — they are exactly the shapes 4933ff exists to protect, so they
should get AT LEAST as much protection under phi_guard's absence as a dict
gets, not less.
Actual:
A list or string handler output is sealed into the signed, persisted, and
web-displayed receipt completely in the clear whenever phi_guard fails to
import — worse than the dict case fixed in the SAME commit, and a fresh
instance of the fail-open-on-unavailable-guard pattern this platform has
elsewhere explicitly committed to avoiding ("Fails CLOSED: if a policy is
configured and the guard raises, we deny rather than leak" — the
guard_egress_messages fix this same contributor's prior report closed).
Suggested fix:
Give the non-dict branch the same credential-hint floor the dict branch
gets, independent of _phi:
def _redact_output(output):
if isinstance(output, dict):
return redact(output)
if isinstance(output, (list, str)):
if _phi is not None:
strict = bool(_phi.phi_mode_enabled())
return _phi.scrub(output, strict=strict)
# No phi_guard: still scrub obvious secret-shaped substrings by
# the same SECRET_HINT vocabulary the dict path always applies,
# rather than returning the value untouched.
return _scrub_secret_hint_recursive(output)
return output
A single shared helper that walks any JSON shape applying the SECRET_HINT
credential pass — independent of phi_guard's availability — would let both
branches genuinely share one floor instead of the dict path accidentally
having one and the non-dict path having none.