Stop credential fragments reaching logs and error messages (#62) - #70
Merged
Conversation
Both repositories are public now, so this moved from a hygiene item to a live
disclosure path: the realistic route is someone pasting a stack trace into an
issue.
_safe_neo4j_url() correctly redacts our own message, but every caller then
appended `Original error: {str(e)}`, and py2neo's exception text embeds the
ConnectionProfile — which is precisely the mis-parse that redactor was written
to avoid. With NEO4J_URL=bolt://neo4j:p@ssSECRET@host the log line read:
Failed to connect to Neo4j at bolt://host.
Original error: Cannot open connection to ConnectionProfile('bolt://ssSECRET@host')
so the password fragment survived, in the same line as the redaction. The 14
`exc_info=True` calls carried it too, via
`WireError: Cannot connect to IPv4Address(('ssSECRET@localhost', 9999))`, into
debug_log.txt (mode 0664) and stdout.
_safe_exception() renders the exception TYPE, which is the part with
diagnostic value, and the message only when it is demonstrably free of
credential material — no "://", no ConnectionProfile or IPv4Address repr, no
"@", and not containing NEO4J_PASSWORD. Over-redaction would make failures
undiagnosable, so a benign message passes through intact:
ValueError: column 'foo' not found in results (kept)
ConnectionUnavailable (message withheld: ...) (withheld)
Tracebacks are now opt-in via LNG_DEBUG_TRACEBACKS=1 rather than always on,
because a traceback through py2neo carries the same profile the message did.
Also closes the fragment gap in _safe_neo4j_url() itself: it split on "/" and
"?" but not "#", so bolt://neo4j:pw@host:7687#TOPSECRET kept the fragment.
Verified end to end against a real failing connection with an "@" password and
with a "/" password: neither leaks. 13 regression tests cover the redactor
across 7 adversarial URLs, the exception formatter in both directions, and the
traceback gate.
Closes #62.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An adversarial review of the first attempt showed the PR's central claim —
"nothing credential-bearing reaches debug_log.txt or stdout" — was false.
Gating the fourteen exc_info=True calls inside neo4j_connector did not close
the leak, because the secret escapes through paths that module does not own:
- src/reaction_generator.py:219 logs the same exception with an UNGATED
exc_info=True, putting ConnectionProfile('bolt://ssSECRET@localhost:7687')
into debug_log.txt with LNG_DEBUG_TRACEBACKS off. That path swallows the
error and continues, so it leaks on runs that otherwise succeed. Same shape
at logic_network_generator.py:150 and pathway_generator.py:405,414.
- scripts/validate_logic_network.py and bin/validate-against-mpbiopath.py
construct a Graph with no handler at all, so a first-connection failure —
the most likely failure — printed the whole chain to stderr.
- `raise ConnectionError(...) from e` cleans only str(); __cause__ still holds
the raw py2neo exception, so anything formatting the chain re-discloses it.
- Seven bare `raise` sites propagate the raw exception, and
pathway_generator.py:415 interpolates it with str(e).
Per-call-site redaction cannot cover paths nobody enumerated. New
src/credential_redaction.py scrubs where text is EMITTED: a logging filter on
every handler (rendering and scrubbing exc_info, then clearing it so the
handler cannot re-render the original) plus an excepthook for anything
uncaught. Installed from configure_logging and from the two bare entrypoints.
Scrubbing is structural — URL userinfo, ConnectionProfile and IPv4Address
reprs — rather than value-based, so ordinary text is untouched.
Verified against the reviewer's own reproductions, all with the gate OFF:
ungated exc_info path SECRET in stdout/stderr/debug_log.txt: 0/0/0
validate_logic_network SECRET in stderr: 0
shows ConnectionProfile(<redacted>), IPv4Address(<redacted>)
chained __cause__ raw chain leaks: True -> after scrub: False
bare re-raise raw chain leaks: True -> after scrub: False
Also fixes over-withholding that the review measured. The substring test on
NEO4J_PASSWORD discarded whole messages, and the documented dev passwords are
ordinary words ("test" in README.md, "reactome" in practice), so
"Cannot find /opt/reactome/data/graph.db" and "KeyError: 'reactome_release'"
vanished entirely. A password substring is now scrubbed rather than
withheld; structural markers still withhold:
RuntimeError: Cannot find file /opt/<redacted>/data/graph.db
RuntimeError (message withheld: may contain credentials)
Passwords under four characters are ignored, since substituting them would
corrupt every line for no benefit.
Tests: 22, up from 13, covering the logging filter with a real traceback, the
excepthook path, structural scrubbing, and the scrub-not-withhold behaviour.
The review also found two of the four denylist markers were unexercised —
those now have cases.
Suite 943 passed, ruff clean.
Refs #62.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Both repos are public now, so this moved from hygiene to a live disclosure path — the realistic route is someone pasting a stack trace into an issue.
The leak
_safe_neo4j_url()redacts our own message correctly. But every caller then appendedOriginal error: {str(e)}, and py2neo's exception text embeds theConnectionProfile— precisely the mis-parse that redactor was written to avoid:The password fragment survives, in the same line as the redaction. The 14
exc_info=Truecalls carried it too, intodebug_log.txt(mode 0664) and stdout:Secret scanning does not catch this — it scans commits, not runtime output.
The fix
_safe_exception()renders the exception type, which is the part with diagnostic value, and the message only when demonstrably free of credential material (no://, noConnectionProfile/IPv4Addressrepr, no@, and not containingNEO4J_PASSWORD).Over-redaction would make failures undiagnosable, so this stays precise:
Tracebacks become opt-in via
LNG_DEBUG_TRACEBACKS=1, since a py2neo traceback carries the same profile the message did.Also closes a gap in
_safe_neo4j_url()itself — it split on/and?but not#, sobolt://neo4j:pw@host:7687#TOPSECRETkept the fragment.Verification
End to end against a real failing connection with an
@password and a/password — neither leaks. 13 regression tests cover 7 adversarial URLs, the exception formatter in both directions, and the traceback gate.Closes #62.
🤖 Generated with Claude Code