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