diff options
| -rw-r--r-- | abusectl/report.py | 53 | ||||
| -rw-r--r-- | tests/test_report.py | 112 |
2 files changed, 132 insertions, 33 deletions
diff --git a/abusectl/report.py b/abusectl/report.py index cf839f3..06a3cb5 100644 --- a/abusectl/report.py +++ b/abusectl/report.py @@ -26,6 +26,8 @@ thing in a report disclosed deliberately, and a module that reaches for it itself is a module that can disclose it in a code path nobody reviewed. """ +import hashlib + def _group_key(address: str) -> str: """The key two spellings of one desk must share, and no more than that. @@ -52,6 +54,32 @@ def _group_key(address: str) -> str: return f"{local}@{domain.lower()}" +def email_destination_id(address: str) -> str: + """The id of the destination that reports to this desk. + + Derived from the ADDRESS, so an id names a desk rather than a position + in whatever list this run happened to build. + + That distinction is the whole point. `report` may run again on a case, + and `contacts` may have resolved a new indicator since; a positional id + then renumbers every desk after the newcomer. Bodies are written to + bodies/<id>.xarf and each body's SHA-256 is recorded against its id, so + after a renumber the file on disk belongs to a DIFFERENT desk than the + manifest entry sharing its id. The edit check would compare one desk's + body against another's, reporting an edit nobody made, or, if the two + happened to match, missing one that was. + + It hashes the same normalised form the grouping uses, so two spellings + of one desk get one id. Deriving it from the raw text instead would let + the spelling RDAP published first decide a body's filename. + """ + digest = hashlib.sha256(_group_key(address).encode("utf-8")).hexdigest() + # ponytail: 8 hex chars, a case has a handful of desks; widen if a + # collision is ever observed. A short id keeps a case directory + # readable to the human reviewing it, which is what it is for. + return f"email-{digest[:8]}" + + def email_destinations(contacts: list[dict]) -> list[dict]: """Group contacts into one destination per abuse ADDRESS. @@ -60,18 +88,15 @@ def email_destinations(contacts: list[dict]) -> list[dict]: common case. One mail per address rather than per contact is what stops a desk receiving two mails about one incident. - Ids are numbered over the DESTINATIONS produced, not over the contacts - read, so a contact that resolved to no desk leaves no hole: a reviewer - who sees "email-2" and "email-4" reasonably reads two reports as - missing. - - Insertion order carries the numbering, so the same contacts in the same - order always produce the same ids. That matters downstream: bodies are - written to bodies/<id>.xarf and hashed against that id, so an id that - wandered between two runs over one input would compare one desk's body - against another's. It is NOT a promise that an id survives a change in - the contacts themselves; adding a desk earlier in the list renumbers - every desk after it. + Each id is derived from its own target address, so it survives both a + reordering and a change in the contacts: a desk keeps its id when a new + indicator resolves to a new desk ahead of it. See email_destination_id + for why that matters more than it first appears. + + The returned ORDER is still first-seen, because the destinations are a + list a human reads during review and the order the indicators were + found in is the most explicable one available. Nothing downstream may + key off that order; the id is what identifies a destination. """ by_address: dict[str, dict] = {} @@ -88,12 +113,12 @@ def email_destinations(contacts: list[dict]) -> list[dict]: return [ { - "id": f"email-{index}", + "id": email_destination_id(destination["target"]), "kind": "email", "target": destination["target"], "iocs": destination["iocs"], "body": None, "status": "pending", } - for index, destination in enumerate(by_address.values(), start=1) + for destination in by_address.values() ] diff --git a/tests/test_report.py b/tests/test_report.py index 6a0dc51..c72974e 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -22,17 +22,36 @@ class Grouping(unittest.TestCase): "abuse": ["a@host.invalid", "b@host.invalid"], "source": "rdap"}, ] destinations = report.email_destinations(contacts) - self.assertEqual( - sorted(d["target"] for d in destinations), - ["a@host.invalid", "b@host.invalid"], - ) + self.assertEqual([d["target"] for d in destinations], + ["a@host.invalid", "b@host.invalid"]) + # Both desks carry the indicator, and each gets its own id: a + # destination that reached only one desk, or two rows sharing an + # id, would pass an assertion on the sorted targets alone. + self.assertEqual([d["iocs"] for d in destinations], + [["ioc-1"], ["ioc-1"]]) + self.assertEqual(len({d["id"] for d in destinations}), 2) + for destination in destinations: + self.assertEqual(destination["id"], + report.email_destination_id( + destination["target"])) def test_a_contact_with_no_address_creates_no_destination(self): + """The contact that resolved must still produce its destination. + + Asserted alongside one that DOES resolve, because "no destination + for this contact" is also what returning nothing at all looks + like, and that is not the behaviour being described. + """ contacts = [ {"iocs": ["ioc-1"], "query": "example.invalid", "abuse": [], "source": "rdap", "error": "no abuse role published"}, + {"iocs": ["ioc-2"], "query": "198.51.100.7", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, ] - self.assertEqual(report.email_destinations(contacts), []) + destinations = report.email_destinations(contacts) + self.assertEqual([d["target"] for d in destinations], + ["abuse@host.invalid"]) + self.assertEqual(destinations[0]["iocs"], ["ioc-2"]) def test_destinations_carry_stable_ids_and_pending_status(self): contacts = [ @@ -40,17 +59,26 @@ class Grouping(unittest.TestCase): "abuse": ["abuse@host.invalid"], "source": "rdap"}, ] destination = report.email_destinations(contacts)[0] - self.assertEqual(destination["id"], "email-1") + self.assertEqual(destination["id"], + report.email_destination_id("abuse@host.invalid")) self.assertEqual(destination["kind"], "email") self.assertEqual(destination["status"], "pending") - def test_ids_are_numbered_per_destination_not_per_contact(self): - """A skipped contact must not leave a hole in the numbering. + def test_an_id_is_the_literal_shape_a_reviewer_will_read(self): + """Pin the shape, since it becomes a filename in bodies/. + + Computed by hand rather than by calling the code under test, so + this fails if the derivation changes rather than following it. + """ + self.assertEqual(report.email_destination_id("abuse@host.invalid"), + "email-bc50e369") + + def test_ids_are_derived_per_destination_not_per_contact(self): + """A contact that resolved to no desk must not shift another's id. - The obvious implementation enumerates the contacts, and a contact - with no abuse address then burns an id: the desks come back as - "email-2" and "email-4", which reads to a reviewer as two reports - having gone missing. + The obvious implementation numbers destinations by position, and a + skipped contact then either burns an id or renumbers the rest. + Both are wrong for the same reason: an id names a desk. """ contacts = [ {"iocs": ["ioc-1"], "query": "example.invalid", "abuse": [], @@ -62,7 +90,8 @@ class Grouping(unittest.TestCase): ] destinations = report.email_destinations(contacts) self.assertEqual([d["id"] for d in destinations], - ["email-1", "email-2"]) + [report.email_destination_id("a@host.invalid"), + report.email_destination_id("b@host.invalid")]) self.assertEqual([d["target"] for d in destinations], ["a@host.invalid", "b@host.invalid"]) @@ -95,6 +124,16 @@ class Grouping(unittest.TestCase): self.assertEqual(destinations[0]["iocs"], ["ioc-1", "ioc-2"]) self.assertEqual(destinations[0]["target"], "abuse@Host.Invalid") + def test_two_spellings_of_one_desk_share_an_id(self): + """The id derives from the same normalised form the grouping uses. + + Otherwise the spelling RDAP happened to publish first would decide + a body's filename, and a re-run that saw the other spelling first + would look like a different desk. + """ + self.assertEqual(report.email_destination_id("abuse@Host.Invalid"), + report.email_destination_id("abuse@host.invalid")) + def test_two_local_part_cases_stay_two_destinations(self): """Only the receiving host knows whether its local parts fold. @@ -109,6 +148,7 @@ class Grouping(unittest.TestCase): destinations = report.email_destinations(contacts) self.assertEqual(sorted(d["target"] for d in destinations), ["Abuse@host.invalid", "abuse@host.invalid"]) + self.assertNotEqual(destinations[0]["id"], destinations[1]["id"]) def test_a_destination_starts_with_no_body(self): contacts = [ @@ -118,12 +158,7 @@ class Grouping(unittest.TestCase): self.assertIsNone(report.email_destinations(contacts)[0]["body"]) def test_the_same_contacts_produce_the_same_ids_twice(self): - """Ids must not depend on dict iteration luck or set ordering. - - Task 8 writes each body to bodies/<id>.xarf and records its hash - against that id, so an id that moved between two runs over the - same input would compare one desk's body against another's. - """ + """Ids must not depend on dict iteration luck or set ordering.""" contacts = [ {"iocs": ["ioc-1"], "query": "198.51.100.7", "abuse": ["b@host.invalid", "a@host.invalid"], "source": "rdap"}, @@ -138,6 +173,45 @@ class Grouping(unittest.TestCase): ["b@host.invalid", "a@host.invalid", "c@host.invalid"]) + def test_a_desks_id_survives_another_desk_appearing(self): + """An id names a DESK, not a position in this run's list. + + Task 8 writes each body to bodies/<id>.xarf and records its hash + against that id. With a positional id, re-running contacts on a + case that gained an indicator renumbers every desk after the new + one, so bodies/<id>.xarf on disk belongs to a different desk than + the manifest's entry of that id, and the edit check compares one + desk's body against another's. + """ + established = {"iocs": ["ioc-1"], "query": "198.51.100.7", + "abuse": ["b@host.invalid"], "source": "rdap"} + first = report.email_destinations([established]) + + # A later contacts run finds an indicator whose desk sorts ahead. + newcomer = {"iocs": ["ioc-2"], "query": "example.invalid", + "abuse": ["a@new.invalid"], "source": "rdap"} + second = report.email_destinations([newcomer, established]) + + by_target = {d["target"]: d["id"] for d in second} + self.assertEqual(by_target["b@host.invalid"], first[0]["id"]) + self.assertNotEqual(by_target["a@new.invalid"], first[0]["id"]) + + def test_an_ids_position_does_not_leak_into_it(self): + """The same desk alone and third in a list gets one id.""" + alone = report.email_destinations([ + {"iocs": ["ioc-1"], "query": "198.51.100.7", + "abuse": ["desk@host.invalid"], "source": "rdap"}, + ]) + crowded = report.email_destinations([ + {"iocs": ["ioc-2"], "query": "198.51.100.8", + "abuse": ["one@host.invalid", "two@host.invalid"], + "source": "rdap"}, + {"iocs": ["ioc-1"], "query": "198.51.100.7", + "abuse": ["desk@host.invalid"], "source": "rdap"}, + ]) + self.assertEqual(crowded[2]["target"], "desk@host.invalid") + self.assertEqual(crowded[2]["id"], alone[0]["id"]) + if __name__ == "__main__": unittest.main() |