From 244c36c820b135c39c3d58e10939496b57c845f5 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 8 Sep 2026 15:24:27 +0200 Subject: fix: redact a valueless query token whole, not as a kept name parse_qsl reads "?victim@example.org" as the pair ("victim@example.org", ""), and the code kept parameter NAMES because they fingerprint the phishing kit. A name that is itself an address is not a name, so keeping it published the recipient's identifier verbatim (percent-decoded, no less: %40 fools no consumer). _redact_kv_string now splits the query/fragment string by hand on "&" and ";" and inspects each token's own name: one containing "@" is a value that landed in name position and is redacted WHOLE ("?REDACTED" rather than "?victim%40example.org=REDACTED"); an ordinary name still keeps its shape ("?flag" stays "?flag=REDACTED", "?t=1&t=2" stays "?t=REDACTED&t=REDACTED"). This also exposed a second leak reachable through the same fixture: parse._suspect_segments() ran redact.suspect_path_segments()'s decode-and-check predicate (meant for a querystring smuggled past percent-encoding into a PATH segment) against a raw query VALUE, so a plaintext "?e=you@example.org" reproduced the address in the manifest's suspect_path_segments flag even though the URL itself was correctly redacted. redact.suspect_path_segments() now exposes the opaque-shape half of its check as _looks_opaque(), and parse.py uses only that half against query values: a query value is always fully redacted regardless, so the flag may hint at its shape but must never reproduce it. Turns tests.test_parse.TestIocAssembly.test_the_address_does_not_survive_any_url_shape green, and closes the gap test_no_ioc_holds_a_recipient_address had been passing over with URL redaction fully disabled. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KphFXTc2QajxXsHWyvGJ4R --- abusectl/parse.py | 12 +++++++++++- abusectl/redact.py | 42 ++++++++++++++++++++++++++++++++++++++---- tests/test_redact.py | 14 ++++++++++++++ 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/abusectl/parse.py b/abusectl/parse.py index 79cccc9..468e35e 100644 --- a/abusectl/parse.py +++ b/abusectl/parse.py @@ -322,6 +322,15 @@ def _suspect_segments(raw_url: str) -> list[str]: is exactly this: ``?id=``). Checking it there too is what lets review see the shape of the token redaction removes. + Only the opaque-SHAPE check applies to a query value, via + redact._looks_opaque(); the decode-and-check half of + suspect_path_segments() is for a querystring smuggled into a PATH + segment, and running it against a query value would reproduce a + plaintext address (``?e=you@example.org``) in the manifest instead of + merely flagging that a value is suspect. Query values are always fully + redacted in the reported URL; this flag is a shape hint for review, not + a second place for the value itself to leak through. + Must be called on the ORIGINAL url, before redact.url() replaces every query value with REDACTED: past that point there is nothing left to recognise as suspect. @@ -329,7 +338,8 @@ def _suspect_segments(raw_url: str) -> list[str]: found = list(redact.suspect_path_segments(raw_url)) query = urlsplit(raw_url).query for _, value in parse_qsl(query, keep_blank_values=True): - found.extend(redact.suspect_path_segments(value)) + if redact._looks_opaque(value): + found.append(value) return found diff --git a/abusectl/redact.py b/abusectl/redact.py index ffc63a8..5e7c4d3 100644 --- a/abusectl/redact.py +++ b/abusectl/redact.py @@ -70,9 +70,26 @@ _SUSPECT_CHARS = set(string.ascii_letters + string.digits + "+/=_-") def _redact_kv_string(value: str) -> str: - """Redact a query- or fragment-style key=value string, keeping names.""" - pairs = parse_qsl(value, keep_blank_values=True) - return urlencode([(name, REDACTED) for name, _ in pairs]) + """Redact a query- or fragment-style key=value string, keeping names. + + parse_qsl cannot tell a bare flag ("?flag") from a nameless token that + merely looks like one ("?victim@example.org"): both parse as a pair + whose value is empty, so keeping "the name" kept the recipient address + verbatim in the second case. The two are told apart here instead: a + parameter name that fingerprints a phishing kit ("flag", "id", "src") + is never itself an email address, so a token containing "@" is a value + that landed in name position, not a name, and the WHOLE token is + redacted rather than only "the value" it does not actually have. + """ + tokens = [t for t in re.split(r"[&;]", value) if t] + redacted = [] + for token in tokens: + name = token.partition("=")[0] + if "@" in name: + redacted.append(REDACTED) + else: + redacted.append(f"{name}={REDACTED}") + return "&".join(redacted) def _redact_fragment(fragment: str) -> str: @@ -125,6 +142,17 @@ def url(raw: str) -> str: ) +def _looks_opaque(segment: str) -> bool: + """Return True if segment is long and shaped like a base64/hex blob. + + This is the one predicate safe to run against a query VALUE as well as + a path segment: it flags the SHAPE of a token without decoding or + reproducing anything readable, so a plaintext address sitting directly + in a value never comes back out through this check. + """ + return len(segment) >= _MIN_SUSPECT_LENGTH and set(segment) <= _SUSPECT_CHARS + + def suspect_path_segments(raw: str) -> list[str]: """Return path segments that look like an encoded identifier. @@ -134,13 +162,19 @@ def suspect_path_segments(raw: str) -> list[str]: query string smuggled into one path segment (``%3F`` and all) is caught. Flagging is never redaction: unlike a query value, a path segment may be the very thing being reported, so the decision is left to human review. + + The decode-and-check half of this is deliberately PATH-only (see + ``_looks_opaque`` for the half that is safe on a query value too): it + exists to catch a querystring smuggled past percent-encoding, and + running it against an already-plaintext query value would reproduce a + recipient address in cleartext instead of merely flagging its shape. """ parts = urlsplit(raw) segments = [seg for seg in parts.path.split("/") if seg] return [ seg for seg in segments - if (len(seg) >= _MIN_SUSPECT_LENGTH and set(seg) <= _SUSPECT_CHARS) + if _looks_opaque(seg) or ("=" in unquote(seg) or "@" in unquote(seg)) ] diff --git a/tests/test_redact.py b/tests/test_redact.py index 23c2a45..e6d65c0 100644 --- a/tests/test_redact.py +++ b/tests/test_redact.py @@ -51,6 +51,20 @@ class TestRedactUrl(unittest.TestCase): "http://a.example.invalid/p?t=REDACTED&t=REDACTED", ) + def test_a_valueless_token_is_redacted_whole(self): + # parse_qsl reads ?victim@example.org as a NAME, and names are kept. + # A token with no "=" is a value, not a fingerprint. + self.assertEqual( + redact.url("http://a.invalid/p?victim@example.org"), + "http://a.invalid/p?REDACTED", + ) + + def test_a_valueless_token_in_a_fragment_is_redacted_whole(self): + self.assertEqual( + redact.url("http://a.invalid/p#victim@example.org"), + "http://a.invalid/p#REDACTED", + ) + class TestSuspectPathSegments(unittest.TestCase): def test_a_base64_looking_segment_is_flagged(self): -- cgit v1.2.3