Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 49 additions & 3 deletions avicbotwikimedia.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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:
"""
Expand Down
55 changes: 55 additions & 0 deletions tests/test_irc.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading