fix(security): log a constant verb label, never a slice of the outgoing line - #28
Merged
Merged
Conversation
…ng line Closes the third report of py/clear-text-logging-sensitive-data in avicbotwikimedia.py (CodeQL #1 fixed, #9 dismissed as a false positive, #12 raised again once the line moved). The dismissal reasoning was sound and I verified it independently: auth commands ARE redacted, and the else branch logged only the first token. But `message.split(" ", 1)[0]` keeps the logged value data-dependent on the outgoing line, so the finding is structurally un-dismissable — it comes back with a new alert number every time the line number shifts, and each round costs somebody a fresh triage. Three fixes in, the right move is to break the dependency rather than argue with it again. The logged label now comes from _LOGGABLE_IRC_VERBS, a constant table whose values are literals, so the log can only ever receive one of those literals or "UNKNOWN" — never a substring of `message`. That is also a real improvement, not just taint-breaking. Auth verbs are deliberately absent from the table, so a PASS/NICKSERV line logs "UNKNOWN" even if the redaction branch above is later changed or reordered; previously that branch was the only thing standing between a credential and the debug log. An unrecognised or malformed command now logs "UNKNOWN" instead of echoing an arbitrary token. Tests written first and confirmed red (4 failures), each carrying an explicit negative control per house style — every case asserts the naive `split()[0]` DID echo the token, so none can pass vacuously. Verified with the exact ruff CI pins (0.15.22, not my local 0.15.20): check and format --check both clean, 38 tests pass.
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
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 the third report of
py/clear-text-logging-sensitive-datainavicbotwikimedia.py:Why the dismissal keeps not sticking
The dismissal reasoning was correct — auth commands are redacted, and the else branch logged only the first token. I verified that independently before changing anything.
But
message.split(" ", 1)[0]keeps the logged value data-dependent on the outgoing line, so the finding is structurally un-dismissable: it returns with a new alert number every time the line number shifts, and each round costs somebody a fresh triage. Three fixes in (f83d467,74e4bc9,0c13ada— the last explicitly "to satisfy CodeQL taint analysis"), the right move is to break the dependency rather than argue with it a fourth time.The change
The logged label now comes from
_LOGGABLE_IRC_VERBS, a constant table whose values are literals. The log can only receive one of those literals or"UNKNOWN"— never a substring ofmessage.This is a real improvement, not only taint-breaking:
PASS/NICKSERVline logs"UNKNOWN"even if the redaction branch above is later changed or reordered. Previously that branch was the only thing standing between a credential and the debug log."UNKNOWN"rather than echoing an arbitrary token.Verification
Tests written first and confirmed red (4 failures) before implementing. Each carries an explicit negative control per house style — every case asserts the naive
split()[0]did echo the token, so none can pass vacuously.Verified against the exact CI pin (ruff 0.15.22, not my local 0.15.20 — they differ, and only CI’s counts):
ruff checkandruff format --checkclean, 38 tests pass.Tests are synchronous by design: CI installs only
pytest python-dotenv, so an async test ofsend_rawwould pass locally and never run in CI.