diff --git a/python/exporter/redact.py b/python/exporter/redact.py new file mode 100644 index 00000000..068df8e2 --- /dev/null +++ b/python/exporter/redact.py @@ -0,0 +1,145 @@ +""" +Take the obvious personal identifiers out of a log, so posting one is less costly. + +**Best effort, and the wording everywhere says so.** This cannot promise a clean file: the text +comes from a library reading Apple's structures, and a field that is innocuous on one account may +not be on another. What it can do is remove the things that are *known* to appear and are +recognisable by shape, which is most of what a person would otherwise have to find by hand. + +**A library was considered and rejected.** `scrubadub` pulls scikit-learn, textblob, dateparser +and faker; `presidio-analyzer` pulls spacy, numpy and pydantic. Both are large enough to matter in +a frozen desktop bundle, and neither knows what an Apple device serial, a Cuttlefish peer hash or +a keychain `acct` field looks like - they are built for prose. Everything worth removing here has +a known shape, and a known shape is a regex. + +**Values are numbered rather than blanked**, so `` is the same serial everywhere it +appears. A log where every identifier became `***` loses the one thing that makes it a log: that +this record and that record are about the same device. Redaction that destroys the diagnosis +defeats the reason for sending the file. +""" + +from __future__ import annotations + +import re +from collections import Counter +from dataclasses import dataclass + + +@dataclass(frozen=True) +class Rule: + """One kind of identifier, and how to find it.""" + + name: str + pattern: re.Pattern[str] + group: int = 0 + """Which capture group holds the value. 0 means the whole match.""" + + +# Order matters where two rules could claim the same text. Paths run before names so that a +# username inside a home directory is taken as a path rather than left for something looser, and +# the labelled fields run before the loose shapes so their placeholder names stay meaningful. +RULES: tuple[Rule, ...] = ( + # An Apple ID, wherever it turns up - an error message, a prompt echoed back, a keychain + # attribute. The one identifier here that names a person rather than a device. + Rule("email", re.compile(r"\b[\w.+-]+@[\w-]+\.[\w.-]+\b")), + + # `/Users/paula/…`, `/home/amity/…`, `C:\Users\paula\…`. Tracebacks are full of these, and the + # account name is very often the person's actual name. + Rule("user", re.compile(r"(?i)(?:/Users/|/home/|[A-Z]:\\Users\\)([^/\\\s'\"]+)"), group=1), + + # Cuttlefish peers, as `Peer SHA256:=`. Pseudonymous, stable, one per device on the + # account - so a set of them fingerprints the account even without a name attached. + Rule("peer", re.compile(r"SHA256:[A-Za-z0-9+/]{20,}={0,2}")), + + # How escrow records describe themselves: "…, serial F2LX9Q…, escrowed 2024-03-11". + Rule("serial", re.compile(r"\bserial\s+([A-Z0-9]{6,20})\b"), group=1), + + # The device name an escrow record carries, which is whatever the user called their phone - + # very often their own name. Both of the lines FindMy.py logs it on. + # + # **MULTILINE is not decoration.** Without it `$` matches only at the end of the whole file, so + # this fired on the last line and nothing else - it reported zero devices on a real log full of + # them, which is the failure mode where redaction looks like it ran. + Rule( + "device", + re.compile(r"(?:escrow recovery for|escrow record)\s+(.+?)(?=,|$)", re.MULTILINE), + group=1, + ), + + # How `EscrowRecord.describe()` renders: ", , serial …, escrowed …". The name is + # whatever somebody called their phone, and is frequently their own name; the model beside it + # is not personal and is worth keeping, since it is often the point of the line. + # + # Anchored on a model followed by `serial` so it cannot run away with a whole sentence. + # The anchor allows a list marker, because this is most often read off a numbered menu of + # recovery options - " 1. Paula's iPhone, iPhone15,2, serial …" - and anchoring on the bare + # line start missed every one of them. + Rule( + "device", + re.compile( + r"(?:^[ \t]*(?:\d+\.[ \t]*)?|\?[ \t]|:[ \t])" + r"([^,\n]{1,60}?),[ \t]+" + # Bounded rather than comma-free: an Apple model identifier contains a comma of its + # own - `iPhone15,2` - so a lookahead that stops at the first comma never reaches the + # `serial` it is anchored on, and matched only the models that happen not to have one. + r"(?=(?:iPhone|iPad|iPod|Mac|MacBook|iMac|Watch).{0,40}?,[ \t]+serial\b)", + re.MULTILINE, + ), + group=1, + ), + + # `describe_item` prints keychain attributes verbatim: acct='…', labl='…'. What is in them is + # Apple's choice, so the safe assumption is that it identifies somebody. + # + # The quotes are matched but *not* captured, so an empty attribute stays `acct=''` instead of + # becoming `acct=` - a placeholder standing in for nothing, which reads as though + # something was hidden and makes the count wrong. + Rule("item", re.compile(r"\b(?:acct|labl|srvr|agrp)=(['\"])([^'\"]*)\1"), group=2), + + # Record and beacon identifiers. Not secret, but unique to one account's accessories. + Rule("uuid", re.compile(r"\b[0-9a-fA-F]{8}-(?:[0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}\b")), +) + + +def redact(text: str) -> tuple[str, Counter[str]]: + """ + Replace recognised identifiers with stable placeholders. + + :returns: The cleaned text, and how many *distinct* values were replaced per kind - which is + what a person needs to sanity-check the result. Ten devices found in a log from somebody + who owns two is a rule matching too much, and no serials found at all is a rule that has + stopped matching, and both are worth seeing. + """ + seen: dict[str, dict[str, str]] = {rule.name: {} for rule in RULES} + + for rule in RULES: + def substitute(match: re.Match[str], rule: Rule = rule) -> str: + value = match.group(rule.group) + + # Nothing to do, and replacing it would turn an empty field into a fake identifier. + if not value or not value.strip(): + return match.group(0) + + known = seen[rule.name] + if value not in known: + known[value] = f"<{rule.name}-{len(known) + 1}>" + + # Only the captured part is replaced, so the surrounding text - which is what makes + # the line readable - survives. "serial F2LX9Q" becomes "serial ", not + # "". + start, end = match.span(rule.group) + return match.group(0)[: start - match.start()] + known[value] + match.group(0)[end - match.start():] + + text = rule.pattern.sub(substitute, text) + + return text, Counter({name: len(values) for name, values in seen.items() if values}) + + +def summarise(counts: Counter[str]) -> str: + """One line naming what was taken out, for showing to the person about to send the file.""" + if not counts: + return "Nothing recognisable was found to remove." + + parts = [f"{count} {name}{'s' if count != 1 else ''}" for name, count in sorted(counts.items())] + + return "Replaced " + ", ".join(parts) + "." diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index 0e45ef51..9ac77d0d 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -39,6 +39,7 @@ is_verification_code, verification_code, ) +from exporter.redact import redact, summarise from exporter.tkutil import centre_on_screen, centre_over from exporter.custom_tags import ( CustomTagError, @@ -199,10 +200,73 @@ def _build(self) -> None: help_label.grid(row=0, column=2) help_label.bind("", lambda _event: webbrowser.open(WIKI_LINK, new=2, autoraise=True)) - ttk.Button(buttons, text="Cancel", command=self.destroy).grid(row=0, column=3, padx=(0, 8)) + # **Where Cancel used to be**, which did exactly what the window's own close button does + # and so earned none of that space. This does something nothing else here can. + # + # A button rather than a link, because being findable is the whole point: somebody asked + # for "the logs" who cannot find them attaches one of two wrong things instead - a + # screenshot of an error dialog, which never contains the useful part, or their export zip, + # which holds the keys to their tags and cannot be un-shared once it is posted. + ttk.Button(buttons, text="Save logs…", command=self._save_logs).grid( + row=0, column=3, padx=(0, 8), + ) self.confirm_button = ttk.Button(buttons, text="Export…", command=self._export, state="disabled") self.confirm_button.grid(row=0, column=4) + def _save_logs(self) -> None: + """ + Copy the log somewhere the user can find, under a name nothing else here could be. + + **The point is the name, as much as the copy.** The two files this program produces are a + bundle of tag keys and a log, and one of them can be handed out safely. Somebody asked for + "the file" attaches whichever they can find, and the wrong answer publishes the keys to + their tags permanently - so the log is written as `.txt`, with `logs` in its name, and the + dialog afterwards says which is which rather than assuming it is obvious. + """ + log = log_file() + + if not log.is_file() or log.stat().st_size == 0: + messagebox.showinfo( + "Save logs", + "There is nothing in the log yet.\n\n" + "It is written as the exporter runs, so try again after the step that went wrong.", + parent=self, + ) + return + + stamp = datetime.datetime.now().strftime("%Y%m%d-%H%M%S") + chosen = asksaveasfilename( + parent=self, + title="Save logs", + # Deliberately not .zip. The extension is the fastest way to tell this from an export, + # and a text file is also one somebody can read before sending, which is the whole ask. + defaultextension=".txt", + initialfile=f"OpenTagViewer-logs-{stamp}.txt", + filetypes=[("Text files", "*.txt"), ("All files", "*.*")], + ) + + if not chosen: + return + + try: + cleaned, counts = redact(log.read_text(encoding="utf-8", errors="replace")) + Path(chosen).write_text(cleaned, encoding="utf-8") + except OSError as e: + logger.exception("Could not save the log") + messagebox.showerror("Save logs", f"Could not write that file:\n\n{e}", parent=self) + return + + messagebox.showinfo( + "Save logs", + f"Saved to:\n{chosen}\n\n" + f"{summarise(counts)} The copy on disk is untouched.\n\n" + "This is the log, not your tags. Your tags are the .zip the Export… button writes.\n\n" + "**Read it before you post it.** Identifiers were removed by pattern-matching, which" + " cannot promise it caught everything - the text comes from a library reading Apple's" + " structures, and a field that is harmless on one account may not be on another.", + parent=self, + ) + def _read_label(self) -> str: """What the button that reads the account says, which is not the same on both routes.""" return "Read this Mac's Find My files" if self.route.is_local else "Sign in to Apple…" diff --git a/python/test/test_redact.py b/python/test/test_redact.py new file mode 100644 index 00000000..961bdafd --- /dev/null +++ b/python/test/test_redact.py @@ -0,0 +1,140 @@ +""" +Taking personal identifiers out of a log before somebody posts it. + +Two properties, and the second is the one that is easy to forget. + +**It removes what it claims to.** Every rule here exists because that identifier was observed in a +real log, so a rule that stops matching is a regression with a privacy cost. + +**And it leaves the diagnosis intact.** Redaction that eats timestamps, module names, byte counts +or model numbers produces a file nobody can debug from, and the person sending it has no way to +tell. A log stripped to `***` is not safer in any way that matters - it just gets a second request +for the unredacted one. +""" + +from __future__ import annotations + +import pytest + +from exporter.redact import redact, summarise + + +def clean(text: str) -> str: + return redact(text)[0] + + +class TestWhatItRemoves: + def test_an_apple_id(self): + assert "someone@example.com" not in clean("Attempting authentication for user someone@example.com") + + def test_a_home_directory_names_a_person(self): + # Tracebacks are full of these and the account name is very often somebody's real name. + out = clean('File "/home/kmvaesp/OpenTagViewer/python/exporter/cli.py", line 893') + + assert "kmvaesp" not in out + assert "OpenTagViewer/python/exporter/cli.py" in out, "the useful half of the path stays" + + @pytest.mark.parametrize("path", [ + "/Users/kmvaesp/Library/Logs/x.log", + "/home/kmvaesp/x.log", + r"C:\Users\kmvaesp\AppData\x.log", + ]) + def test_every_platform_spelling_of_a_home_directory(self, path): + assert "kmvaesp" not in clean(path) + + def test_a_peer_hash(self): + line = "Peer SHA256:7Yt2Rg90+i3fSELB8nD25LePIq+Op/V3P18oT8t7qPI= carries no signing key" + + out = clean(line) + + assert "7Yt2Rg90+i3fSEL" not in out + assert "carries no signing key" in out + + def test_a_device_serial(self): + assert "BWBPC2K1ZR" not in clean("iPad, iPad Pro, serial BWBPC2K1ZR, escrowed 2024-03-11") + + def test_the_name_somebody_gave_their_phone(self): + # The model beside it is not personal and is often the point of the line, so it stays. + out = clean(" 1. Sam's iPhone, iPhone15,2, serial BWBPC2, escrowed 2024-03-11") + + assert "Sam" not in out + assert "iPhone15,2" in out + + def test_keychain_attributes_printed_verbatim(self): + # What Apple puts in these is Apple's choice, so the safe assumption is that it identifies + # somebody. + out = clean("Service key item: class='genp', acct='someone@example.com', labl='Home WiFi'") + + assert "someone@example.com" not in out + assert "Home WiFi" not in out + + def test_record_identifiers(self): + assert "725A989D" not in clean("Record 725A989D-D871-49A7-B2FE-948C24F356AB carries a field") + + +class TestWhatItKeeps: + """ + A log that cannot be read is not a safer log, it is a second round trip. + """ + + @pytest.mark.parametrize("line", [ + "INFO findmy.keychain.session: Reading the Manatee view with classA: 64 bytes", + "DEBUG findmy.cloudkit.client: Zone ProtectedCloudStorage fully synced after page 1", + "INFO findmy.keychain.items: View holds 61 item(s) and 29 pointer(s)", + "2026-08-16 12:09:48,557 WARNING exporter.privacy: ====", + "DerError: Truncated DER: element claims 109 bytes, 61 remain", + "The Manatee view holds 141 elliptic-curve key(s)", + ]) + def test_the_lines_that_make_a_log_worth_having(self, line): + assert clean(line) == line + + def test_a_traceback_still_points_at_code(self): + out = clean(' File "/home/kmvaesp/x/findmy/keychain/servicekey.py", line 297, in service_keys_from_der') + + assert "servicekey.py" in out + assert "line 297" in out + assert "service_keys_from_der" in out + + +class TestTheSameValueGetsTheSameName: + """ + Numbered rather than blanked, because "these two lines are about one device" is often the + whole diagnosis - and blanket `***` destroys exactly that. + """ + + def test_one_value_twice_is_one_placeholder(self): + out = clean("serial ABC123 said no\nserial ABC123 said no again\n") + + assert out.count("") == 2 + + def test_two_values_are_told_apart(self): + out = clean("serial ABC123 here\nserial XYZ789 there\n") + + assert "" in out + assert "" in out + + def test_the_count_is_of_distinct_values(self): + # What a person checks the result against: two devices on a two-device account is right, + # and ten is a rule matching too much. + _, counts = redact("serial ABC123 x\nserial ABC123 y\nserial XYZ789 z\n") + + assert counts["serial"] == 2 + + +class TestSayingWhatHappened: + def test_it_names_what_was_replaced(self): + _, counts = redact("user a@b.com and serial ABC123") + + assert summarise(counts) == "Replaced 1 email, 1 serial." + + def test_finding_nothing_is_said_plainly(self): + # Not silence: "nothing was found" and "the redactor did not run" look identical otherwise, + # and only one of them is fine. + assert "Nothing recognisable" in summarise(redact("View holds 61 item(s)")[1]) + + +class TestItDoesNotInventIdentifiers: + def test_an_empty_field_is_left_alone(self): + out = clean("Service key item: acct='', labl='x'") + + assert "acct=''" in out, "an empty attribute must not become a fake identifier" diff --git a/python/test/test_save_logs_button.py b/python/test/test_save_logs_button.py new file mode 100644 index 00000000..7b1fa693 --- /dev/null +++ b/python/test/test_save_logs_button.py @@ -0,0 +1,209 @@ +""" +The "Save logs…" button, driven through the actual window. + +**Separate from `test_redact.py` on purpose.** That file proves the patterns work on text. This one +proves the button is wired to them, which is the part that breaks silently: a window that writes an +unredacted file looks exactly like one that writes a redacted file. + +Every identifier below is randomly generated and belongs to nobody. That is deliberate - the first +draft of this file used values lifted from a real user's bug report, which is precisely what the +feature under test exists to prevent, and would have committed them to the history permanently. +""" + +from __future__ import annotations + +from unittest import mock + +import pytest + +# **Before anything that imports tkinter, `exporter.wizard` included.** A `pytest.skip` inside a +# fixture is too late: the module is imported during collection, so a Python built without Tk +# fails the whole run before any fixture can decline. Not hypothetical - CI runs a matrix, and +# 3.12 resolved to a framework interpreter with no `_tkinter` while 3.13 had one, so this took +# down four jobs and reported the failure against the three it cancelled. +tk = pytest.importorskip("tkinter", reason="needs a Python built with Tk") + +from exporter import wizard # noqa: E402 - has to follow the importorskip above + +# Random, and shaped like the real thing: a peer hash is base64 of a SHA-256, a serial is ten +# uppercase alphanumerics, a home directory carries somebody's login name. +PEER = "7Yt2Rg90+i3fSELB8nD25LePIq+Op/V3P18oT8t7qPI=" +SERIAL = "BWBPC2K1ZR" +USER = "kmvaesp" + +# One of everything the redactor should find, in the shapes FindMy.py actually writes them - so +# this fails if the wiring is right and the patterns have rotted. +LOG = f"""\ +2026-08-16 12:09:48 WARNING exporter.privacy: NOT safe to publish +2026-08-16 12:09:49 INFO findmy.reports.account: Attempting authentication for user someone@example.com +2026-08-16 12:09:50 INFO findmy.keychain.recovery: Beginning escrow recovery for Sam's iPhone +2026-08-16 12:09:51 WARNING findmy.keychain.peers: Peer SHA256:{PEER} carries no signing key +2026-08-16 12:09:52 INFO findmy.keychain.items: Service key item: acct='someone@example.com', labl='Home WiFi' + 1. Sam's iPhone, iPhone15,2, serial {SERIAL}, escrowed 2024-03-11 + File "/home/{USER}/OpenTagViewer/python/exporter/cli.py", line 893, in run +2026-08-16 12:09:53 INFO findmy.keychain.session: The Manatee view holds 141 elliptic-curve key(s) +""" + +SECRETS = [ + "someone@example.com", + "Sam's iPhone", + PEER[:15], + SERIAL, + f"/home/{USER}/", + "Home WiFi", +] + + +@pytest.fixture(scope="module") +def window(): + """ + One window for the whole module. + + **Built once rather than per test.** Repeatedly creating and destroying a `Tk` in one process + is unstable on macOS - the first attempt at this crashed the interpreter during collection, + before a single assertion ran. Nothing here mutates window state, so one is enough. + """ + try: + app = wizard.WizardApp() + except tk.TclError as e: # pragma: no cover - depends on the machine, not on the code + pytest.skip(f"needs a display to build a window: {e}") + + app.withdraw() + try: + yield app + finally: + app.destroy() + + +@pytest.fixture +def log(tmp_path): + """A log file this test owns.""" + path = tmp_path / "exporter.log" + path.write_text(LOG, encoding="utf-8") + + return path + + +def click_save_logs(window, log, saving_to): + """Press the button, answering its save dialog with this path. None cancels the dialog.""" + with mock.patch.object(wizard, "log_file", return_value=log), \ + mock.patch.object(wizard, "asksaveasfilename", + return_value=str(saving_to) if saving_to else ""), \ + mock.patch.object(wizard.messagebox, "showinfo") as info, \ + mock.patch.object(wizard.messagebox, "showerror") as error: + window._save_logs() + + return info, error + + +def button_labels(widget) -> list[str]: + found: list[str] = [] + + for child in widget.winfo_children(): + try: + label = child.cget("text") + except tk.TclError: + label = "" + if label: + found.append(str(label)) + found.extend(button_labels(child)) + + return found + + +class TestTheButtonExists: + def test_it_is_in_the_window(self, window): + assert "Save logs…" in button_labels(window) + + def test_cancel_is_not(self, window): + # It did exactly what the window's own close button does, and this took its place. + assert "Cancel" not in button_labels(window) + + +class TestWhatItWrites: + def test_no_identifier_survives_into_the_saved_file(self, window, log, tmp_path): + """ + The whole reason the button exists. + + Asserted per identifier rather than by comparing whole files, so a failure names which one + leaked instead of only saying that something did. + """ + target = tmp_path / "out.txt" + + click_save_logs(window, log, target) + + saved = target.read_text(encoding="utf-8") + for secret in SECRETS: + assert secret not in saved, f"{secret!r} reached a file a user is about to post" + + def test_the_log_is_still_worth_reading(self, window, log, tmp_path): + # Redaction that eats the diagnosis just produces a second request for the original. + target = tmp_path / "out.txt" + + click_save_logs(window, log, target) + + saved = target.read_text(encoding="utf-8") + assert "The Manatee view holds 141 elliptic-curve key(s)" in saved + assert "carries no signing key" in saved + assert "iPhone15,2" in saved, "a model is not personal and is often the point of the line" + assert "line 893, in run" in saved + + def test_the_original_on_disk_is_left_alone(self, window, log, tmp_path): + # The copy the user keeps stays complete; only what leaves the machine is redacted. + click_save_logs(window, log, tmp_path / "out.txt") + + assert log.read_text(encoding="utf-8") == LOG + + def test_it_says_what_it_took_out(self, window, log, tmp_path): + info, _ = click_save_logs(window, log, tmp_path / "out.txt") + + body = info.call_args[0][1] + assert "Replaced" in body + assert "serial" in body + + def test_it_does_not_promise_the_file_is_clean(self, window, log, tmp_path): + # The one claim that must never be made. Pattern-matching cannot know what Apple put in a + # field, and somebody told "this is safe now" will not read it. + info, _ = click_save_logs(window, log, tmp_path / "out.txt") + + body = info.call_args[0][1].lower() + assert "read it before you post it" in body + assert "cannot promise" in body + + def test_it_distinguishes_the_log_from_the_tags(self, window, log, tmp_path): + info, _ = click_save_logs(window, log, tmp_path / "out.txt") + + assert "not your tags" in info.call_args[0][1] + + +class TestWhenThereIsNothingToSave: + def test_a_missing_log_does_not_open_a_save_dialog(self, window, log): + log.unlink() + + with mock.patch.object(wizard, "log_file", return_value=log), \ + mock.patch.object(wizard, "asksaveasfilename") as save, \ + mock.patch.object(wizard.messagebox, "showinfo") as info: + window._save_logs() + + assert not save.called, "asking where to save nothing wastes the one action they took" + assert "nothing in the log yet" in info.call_args[0][1].lower() + + def test_an_empty_log_is_treated_the_same(self, window, log): + log.write_text("", encoding="utf-8") + + with mock.patch.object(wizard, "log_file", return_value=log), \ + mock.patch.object(wizard, "asksaveasfilename") as save, \ + mock.patch.object(wizard.messagebox, "showinfo"): + window._save_logs() + + assert not save.called + + +class TestWhenTheUserBacksOut: + def test_cancelling_the_dialog_writes_nothing(self, window, log, tmp_path): + before = set(tmp_path.iterdir()) + + info, error = click_save_logs(window, log, None) + + assert set(tmp_path.iterdir()) == before + assert not info.called and not error.called