From 4eabced8f41fce8b65341e1bfb234c59d33438d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9on=20Avic=20Simmons?= Date: Sat, 22 Aug 2026 10:21:42 -0400 Subject: [PATCH] fix(security): log a constant verb label, never a slice of the outgoing line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- avicbotwikimedia.py | 52 +++++++++++++++++++++++++++++++++++++++--- tests/test_irc.py | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/avicbotwikimedia.py b/avicbotwikimedia.py index 9328258..5fb0213 100755 --- a/avicbotwikimedia.py +++ b/avicbotwikimedia.py @@ -459,6 +459,52 @@ class BotConfig: # ============================================================================= +# Log labels for outgoing IRC lines. Values are literals, so anything looked +# up here is provably NOT a substring of the line being sent. +# +# Redacting auth commands and then logging `message.split(" ", 1)[0]` still +# left the logged value data-dependent on the outgoing line, and CodeQL +# reported it three times on that basis (alerts #1, #9, #12 — fixed, +# dismissed as a false positive, then raised again once the line moved). +# Auth verbs are deliberately absent: if the redaction branch in send_raw is +# ever changed, a PASS/NICKSERV line still logs "UNKNOWN" rather than leaking. +_LOGGABLE_IRC_VERBS: dict[str, str] = { + v: v + for v in ( + "PRIVMSG", + "NOTICE", + "JOIN", + "PART", + "PING", + "PONG", + "MODE", + "NICK", + "USER", + "QUIT", + "WHO", + "WHOIS", + "WHOWAS", + "TOPIC", + "KICK", + "INVITE", + "NAMES", + "LIST", + "AWAY", + "CAP", + ) +} + + +def _loggable_verb(message: str) -> str: + """Return a constant, log-safe label for an outgoing IRC line. + + The result is always one of ``_LOGGABLE_IRC_VERBS``' literal values or + ``"UNKNOWN"`` — never a slice of ``message``. + """ + verb = message.split(" ", 1)[0].upper() + return _LOGGABLE_IRC_VERBS.get(verb, "UNKNOWN") + + class IRCBot: """ Asynchronous IRC Bot implementation. @@ -566,9 +612,9 @@ async def send_raw(self, message: str) -> None: if upper.startswith("PASS") or "IDENTIFY" in upper or "NICKSERV" in upper: logger.debug(">>> [REDACTED AUTH COMMAND]") else: - # Log only the IRC verb to avoid leaking message content. - verb = message.split(" ", 1)[0] - logger.debug(">>> %s ...", verb) + # Log a literal from a constant table, never a slice of the + # outgoing line — see _LOGGABLE_IRC_VERBS. + logger.debug(">>> %s ...", _loggable_verb(message)) async def send_message(self, target: str, message: str) -> None: """ diff --git a/tests/test_irc.py b/tests/test_irc.py index a294fb8..2f88941 100644 --- a/tests/test_irc.py +++ b/tests/test_irc.py @@ -95,3 +95,58 @@ def test_no_match(self, bot): assert match is None match = bot._pattern_after.search("unrelated message") assert match is None + + +class TestLoggableVerb: + """`send_raw` must never write any slice of an outgoing line to the log. + + Redacting auth commands and then logging `message.split(" ", 1)[0]` + leaves the logged value data-dependent on the line itself, which is why + this was reported three times (CodeQL alerts #1, #9, #12 — fixed, + dismissed, then raised again when the line moved). The logged label now + comes from a constant table, so no substring of an outgoing line can + reach the log regardless of what the redaction branch does. + + Each test carries a NEGATIVE CONTROL asserting the old naive approach + DID echo the token, per the house style. + """ + + @staticmethod + def _naive(message: str) -> str: + """The previous implementation, kept as the negative control.""" + return message.split(" ", 1)[0] + + def test_known_verb_maps_to_a_literal(self): + from avicbotwikimedia import _loggable_verb + + assert _loggable_verb("PRIVMSG #chan :hello") == "PRIVMSG" + assert _loggable_verb("privmsg #chan :hello") == "PRIVMSG" + + def test_unknown_verb_is_not_echoed(self): + from avicbotwikimedia import _loggable_verb + + line = "SUPERSECRET hunter2" + # negative control: the old approach echoed the raw token + assert self._naive(line) == "SUPERSECRET" + assert _loggable_verb(line) == "UNKNOWN" + + def test_auth_verbs_are_never_loggable(self): + from avicbotwikimedia import _loggable_verb + + # Excluded from the table deliberately, so that even if the + # redaction branch in send_raw is later changed, these cannot leak. + for line in ("PASS hunter2", "NICKSERV IDENTIFY hunter2"): + assert self._naive(line) != "UNKNOWN" # negative control + assert _loggable_verb(line) == "UNKNOWN" + + def test_no_substring_of_the_line_survives(self): + from avicbotwikimedia import _LOGGABLE_IRC_VERBS, _loggable_verb + + payload = "hunter2" + allowed = set(_LOGGABLE_IRC_VERBS.values()) | {"UNKNOWN"} + # negative control: a bare credential with no space leaked verbatim + assert self._naive(payload) == payload + for line in (f"PASS {payload}", f"WEIRD {payload}", payload): + out = _loggable_verb(line) + assert payload not in out + assert out in allowed