From f6391827c42c2cdd4e674fe62b61025dafe4fbc0 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 14:22:20 +0200 Subject: [PATCH] Filter every OpenTagViewer escrow record, not just this session's Joining the trust circle writes an escrow record beside the user's real hardware, and the recovery picker then asks for "the screen-lock passcode of one of your Apple devices" about a program that has no screen. There is no answer: that passcode was generated, never shown, and was never the user's to know. The app already dropped one such record. It dropped exactly one. The filter compared each record against the serial the current session presents, which was correct only while every install presented the same constant. Serials are drawn per install now, so the equality test hides this install's record and leaves every other one of ours on the list: the record from before a reinstall, the desktop exporter's, and one more for every trust-circle join. All equally unusable, and there are more of them than there was of the one being filtered. So the match moves to the prefix, which is the part that is stable by design - SERIAL_PREFIX exists precisely so an entry is identifiable as ours. Both legacy constants fall out for free, since 0PENTAGVIEWR and 0PENTAGXPORT begin with their own prefixes. written_by_opentagviewer lives in exporter/identity.py because that module is one of the handful shipped into the APK, so it is the one place the app bridge and the desktop exporter can share a decision rather than keeping two that drift. The exporter had no filter at all before this; its wizard and CLI now use the same one, applied before the "nothing to recover from" check so that message stays true for an account holding only our records. Tests both directions, because they fail in opposite ways: too lax leaves unusable tiles, too eager hides a real Mac and presents as "the app cannot see my device". A record with no serial is kept rather than guessed at - the escrow schema is unstable enough that the field is genuinely sometimes absent. The duplicated prefix literal is pinned against Java on a device, since a copy nothing compares is a copy that drifts silently. Verified the key test fails against the old equality check. Co-Authored-By: Claude Opus 5 (1M context) --- .../python/PythonPackagingTest.java | 39 ++++++++++ app/src/main/python/icloud_bridge.py | 18 +++-- app/src/test/python/test_icloud_bridge.py | 61 +++++++++++++++ python/exporter/cli.py | 19 ++++- python/exporter/identity.py | 46 +++++++++++ python/exporter/wizard.py | 18 ++++- python/test/test_our_own_serials.py | 78 +++++++++++++++++++ 7 files changed, 267 insertions(+), 12 deletions(-) create mode 100644 python/test/test_our_own_serials.py diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java index fbd6b38b..dd880331 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java @@ -244,6 +244,45 @@ public void theappsIcloudBridgeIsPackagedAndKnowsWhoItIs() { assertEquals("OpenTagViewer", identity.get("device_name").toString()); } + /** + * The serial prefixes Python filters on are the ones Java actually presents. + * + *

This literal is duplicated, and that is the whole reason for this test. Python + * needs both prefixes to keep this project's escrow records out of the recovery picker - + * its own and the exporter's - and it cannot read the Java constant at import time. So + * {@code exporter.identity.APP_SERIAL_PREFIX} is a second copy of + * {@link AdiDeviceIdentity#SERIAL_PREFIX}, and a copy nothing compares is a copy that + * drifts. + * + *

Drift here is silent and nasty: change the Java prefix alone and every serial this app + * draws stops being recognised as its own, so its escrow records reappear in the picker + * asking the user for a passcode that was generated and never shown. Nothing fails, and the + * only symptom is on an account nobody testing has. + * + *

Asserted in both directions against the drawn serial rather than only as string + * equality, so a prefix that is right and a generator that stopped using it also fails. + */ + @Test + public void thepythonSideFiltersOnThePrefixJavaActuallyPresents() { + final PyObject identity = Python.getInstance().getModule("exporter.identity"); + + assertEquals("Python's copy of the app's serial prefix has drifted from Java's", + AdiDeviceIdentity.SERIAL_PREFIX, + identity.get("APP_SERIAL_PREFIX").toString()); + + // And a serial this app actually draws is recognised by the Python predicate. + final String drawn = AdiDeviceIdentity.generate().serial(); + assertTrue("a drawn serial must start with the prefix it is filtered by", + drawn.startsWith(AdiDeviceIdentity.SERIAL_PREFIX)); + assertTrue("Python does not recognise a serial this app draws as its own", + identity.callAttr("written_by_opentagviewer", drawn).toBoolean()); + + // The legacy constant too, since installs predating drawn serials still present it. + assertTrue("Python must still recognise the pre-drawn constant", + identity.callAttr("written_by_opentagviewer", + AdiDeviceIdentity.LEGACY_SERIAL).toBoolean()); + } + /** * The shared package's own tests are not shipped either. * diff --git a/app/src/main/python/icloud_bridge.py b/app/src/main/python/icloud_bridge.py index 5f39d6e2..371fe07b 100644 --- a/app/src/main/python/icloud_bridge.py +++ b/app/src/main/python/icloud_bridge.py @@ -43,6 +43,7 @@ import identity as app_identity from exporter import icloud +from exporter.identity import written_by_opentagviewer from findmy.errors import AppleServiceUnavailableError, InvalidCredentialsError from findmy.keychain.enrolment import DeviceDescription from findmy.keychain.join import JoinedPeer @@ -363,16 +364,21 @@ def recoveryOptions(self) -> str: # that has no screen and no lock. There is no answer to that question: the escrow # passcode was generated, never shown, and is not the user's to know. # - # Matched on the serial this session presents, which is the one place this app's - # identity is written (rule 11) - not on the name, which Apple does not carry for this - # entry, nor on a literal, which would be a second copy of the identity to keep in step. + # **Matched on the prefix, not on the serial this session happens to present.** It was + # the latter, and that was only ever correct while every install presented one constant. + # Serials are drawn per install now, so an equality test drops this install's record and + # leaves every other record this project wrote sitting in the picker: the one from before + # a reinstall, the desktop exporter's, and one more per trust-circle join. They are + # exactly as unusable as the one that was being filtered, and there are more of them. + # + # Not matched on the name, which Apple does not carry for this entry. # # Filtered here rather than in the screen so the count below is honest: an account whose - # only recoverable record is this app's has nothing the user can recover from, and - # should be told so rather than shown one unusable tile. + # only recoverable records are this project's has nothing the user can recover from, and + # should be told so rather than shown unusable tiles. self._records = [ record for record in options.recoverable - if record.serial != self._identity.serial + if not written_by_opentagviewer(record.serial) ] if not self._records: diff --git a/app/src/test/python/test_icloud_bridge.py b/app/src/test/python/test_icloud_bridge.py index 6eff5a90..4819ace9 100644 --- a/app/src/test/python/test_icloud_bridge.py +++ b/app/src/test/python/test_icloud_bridge.py @@ -318,6 +318,67 @@ def test_this_apps_own_record_is_not_offered_to_unlock_with(self, session): assert [d["serial"] for d in answer["devices"]] == ["F2LX9Q", "C02XK"] + def test_records_from_other_installs_of_this_project_are_dropped_too(self, session): + """ + **The case an equality check missed, and the reason this matches the prefix.** + + The filter compared against the serial the current session presents. That was correct + only while every install presented one constant; serials are drawn per install now, so + it dropped this install's record and left every other one of ours on the list - the one + from before a reinstall, the desktop exporter's, and one more per trust-circle join. + + All of them are exactly as unusable as the record that was being filtered, and there + are more of them. None has a passcode anybody has ever seen. + """ + made = session(FakeClient(FakeOptions([ + FakeRecord("F2LX9Q"), + FakeRecord(FakeAsyncAccount.serial), # this session's own + FakeRecord("0PENTAGVQ4WM"), # the app, before a reinstall + FakeRecord("0PENTAGXR7KD"), # the desktop exporter + FakeRecord("0PENTAGVIEWR"), # the app, before serials were drawn + FakeRecord("0PENTAGXPORT"), # the exporter, likewise + FakeRecord("C02XK"), + ]))) + + answer = json.loads(made.recoveryOptions()) + + assert [d["serial"] for d in answer["devices"]] == ["F2LX9Q", "C02XK"] + + def test_a_real_device_is_not_dropped_for_looking_a_bit_like_ours(self, session): + """ + The other direction, which matters more than it looks. + + A filter that is too eager hides hardware the user *can* unlock with, and the symptom + is "this app cannot see my Mac" - far worse than an extra unusable tile. Only the two + eight-character prefixes count, and nothing shorter or merely similar. + """ + made = session(FakeClient(FakeOptions([ + FakeRecord("0PENTAG"), # short of the prefix + FakeRecord("0PENTAHV1234"), # one letter off + FakeRecord("PENTAGV1234"), # missing the leading zero + FakeRecord("X0PENTAGV123"), # prefix present, but not at the start + ]))) + + answer = json.loads(made.recoveryOptions()) + + assert [d["serial"] for d in answer["devices"]] == [ + "0PENTAG", "0PENTAHV1234", "PENTAGV1234", "X0PENTAGV123", + ] + + def test_a_record_with_no_serial_is_kept_rather_than_guessed_at(self, session): + """ + The escrow schema is unstable enough that a record can carry no serial at all. + + Dropping those would hide a real device on the strength of a missing field. Keeping one + costs an entry the user can look at and decide about; dropping it costs them the only + recovery path they had. + """ + made = session(FakeClient(FakeOptions([FakeRecord(None), FakeRecord("F2LX9Q")]))) + + answer = json.loads(made.recoveryOptions()) + + assert [d["serial"] for d in answer["devices"]] == [None, "F2LX9Q"] + def test_it_cannot_be_unlocked_with_either(self, session): """ Dropped from the records, not merely from the listing. diff --git a/python/exporter/cli.py b/python/exporter/cli.py index e3ce181b..49ab15fb 100644 --- a/python/exporter/cli.py +++ b/python/exporter/cli.py @@ -49,6 +49,7 @@ suggested_name, ) from exporter.icloud import Candidate, ExportSourceError, not_a_terms_problem +from exporter.identity import written_by_opentagviewer from exporter.certs import ensure_ca_bundle from exporter.version import EXPORT_VIA_CLI, GITHUB_ISSUES_LINK, VERSION, describe_build from opentagviewer_export import ( @@ -567,7 +568,19 @@ async def unlock(client, arguments: argparse.Namespace) -> bool: """ options = await client.recovery_options() - if not options.recoverable: + # **This project's own records are dropped, not offered.** Joining the trust circle writes an + # escrow record beside the user's real hardware, and asking for "the screen-lock passcode" of + # a program that has no screen has no answer - the passcode was generated and never shown. + # + # Filtered once, here, rather than at the picker: every count and message below has to be + # about what the user can actually recover from, or an account holding nothing but our own + # records reports options it cannot use. + recoverable = [ + record for record in options.recoverable + if not written_by_opentagviewer(record.serial) + ] + + if not recoverable: print("\nNo record on this account can currently be recovered from.", file=sys.stderr) if not options.viability_is_trustworthy: print("Nothing was reported usable at all, which reads as a service having a bad day", file=sys.stderr) @@ -577,10 +590,10 @@ async def unlock(client, arguments: argparse.Namespace) -> bool: print("\nUnlocking needs the screen-lock passcode of one of your Apple devices -", file=sys.stderr) print("its PIN or login password, not your Apple ID password.\n", file=sys.stderr) - chosen = _pick_device(options.recoverable, arguments.device) or options.recoverable[ + chosen = _pick_device(recoverable, arguments.device) or recoverable[ await ask_choice( "Which device's passcode do you have?", - [record.describe() for record in options.recoverable], + [record.describe() for record in recoverable], ) ] diff --git a/python/exporter/identity.py b/python/exporter/identity.py index 33ab2a4f..57e41411 100644 --- a/python/exporter/identity.py +++ b/python/exporter/identity.py @@ -140,3 +140,49 @@ def serial_from(stored: dict | None) -> str: CLOUDKIT_DEVICE_NAME = DEVICE_NAME """Older spelling, kept for anything still importing it.""" + + +APP_SERIAL_PREFIX = "0PENTAGV" +""" +What the Android app's serials begin with, mirroring :data:`SERIAL_PREFIX` for this program. + +**Here rather than only in Java**, because the two programs have to recognise *each other's* +records and not merely their own. This module is one of the handful shipped into the APK +(`app/build.gradle.kts` lists it by name), so it is the one place both can read the pair from. +Duplicating the literal is not free, which is why +`PythonPackagingTest.thepythonSideFiltersOnThePrefixJavaActuallyPresents` asserts it against +`AdiDeviceIdentity.SERIAL_PREFIX` on a device rather than trusting this comment. +""" + +OUR_SERIAL_PREFIXES = (SERIAL_PREFIX, APP_SERIAL_PREFIX) +"""Both prefixes this project presents to Apple, for :func:`written_by_opentagviewer`.""" + + +def written_by_opentagviewer(serial: str | None) -> bool: + """ + Whether a serial belongs to this project rather than to a device the user owns. + + **For filtering escrow records out of a recovery picker, and that is the whole use.** Joining + the trust circle registers this project as a device and writes an escrow record beside the + user's real hardware. The picker then asks for "the screen-lock passcode of one of your Apple + devices" for something with no screen and no lock, and there is no answer: that passcode was + generated, never shown, and was never the user's to know. + + **Matching the prefix rather than one serial is the point.** The app used to compare against + the serial the current session presents, which worked only while every install presented the + same constant. Serials are drawn per install now (:data:`EXPORTER_SERIAL` explains why), so an + exact match filters this install's record and leaves every other one of ours on the list - a + reinstall's, the other program's, and one per trust-circle join. The prefixes are the stable + part by design: see :data:`SERIAL_PREFIX`, which exists so an entry is identifiable as ours. + + Both legacy constants are caught without being named, since `0PENTAGVIEWR` and `0PENTAGXPORT` + begin with their own prefixes. + + :param serial: The serial off an escrow record, which may be absent or a non-string - the + record schema is unstable enough that every field can be missing. + :return: True when this project wrote it, and the user cannot be asked about it. + """ + if not isinstance(serial, str): + return False + + return serial.startswith(OUR_SERIAL_PREFIXES) diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index df0c40b1..5cc0ca68 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -59,6 +59,7 @@ from findmy.keychain.recovery import RecoveryError from exporter.icloud import Candidate, ExportSourceError, not_a_terms_problem +from exporter.identity import written_by_opentagviewer from exporter.version import ( APP_TITLE, EXPORT_VIA_WIZARD, @@ -524,7 +525,18 @@ async def _read_icloud(self, asker: Asker): async with await icloud.open_client(account) as client: options = await client.recovery_options() - if not options.recoverable: + + # This project's own escrow records are dropped rather than offered: joining the + # trust circle writes one beside the user's real hardware, and its passcode was + # generated and never shown, so "which device's passcode do you have?" has no + # answer for it. Filtered before the count, so an account holding nothing but our + # own records says there is nothing to recover from - which is the truth. + recoverable = [ + record for record in options.recoverable + if not written_by_opentagviewer(record.serial) + ] + + if not recoverable: raise ExportSourceError( "No device on this account can currently be recovered from, so its keychain" " cannot be unlocked.", @@ -535,9 +547,9 @@ async def _read_icloud(self, asker: Asker): "Unlock", "Which device's screen-lock passcode do you have?\n" "That is its PIN or login password, not your Apple ID password.", - [record.describe() for record in options.recoverable], + [record.describe() for record in recoverable], )) - chosen = options.recoverable[index] + chosen = recoverable[index] async def ask_passcode(attempt: int, chosen=chosen) -> str: again = "\n\nThat last one was not accepted." if attempt > 1 else "" diff --git a/python/test/test_our_own_serials.py b/python/test/test_our_own_serials.py new file mode 100644 index 00000000..0a1a7eaa --- /dev/null +++ b/python/test/test_our_own_serials.py @@ -0,0 +1,78 @@ +""" +Recognising this project's own serials, so its escrow records stay out of a recovery picker. + +Joining the trust circle writes an escrow record beside the user's real hardware. The picker +then asks "which device's screen-lock passcode do you have?" about a program with no screen, and +there is no answer: that passcode was generated, never shown, and was never the user's to know. + +**Both directions are load-bearing, and they fail in opposite ways.** Too lax leaves unusable +tiles the user tries to guess a PIN for. Too eager hides real hardware, and presents as "it +cannot see my Mac" - which costs somebody the only recovery path they had. +""" + +from __future__ import annotations + +import pytest + +from exporter.identity import ( + APP_SERIAL_PREFIX, + EXPORTER_SERIAL, + OUR_SERIAL_PREFIXES, + SERIAL_PREFIX, + written_by_opentagviewer, +) + +LEGACY_APP_SERIAL = "0PENTAGVIEWR" +"""What the app presented before serials were drawn. Named here so the test reads as evidence.""" + + +class TestOurOwnRecordsAreRecognised: + @pytest.mark.parametrize("serial", [ + "0PENTAGVQ4WM", # the app, drawn + "0PENTAGXR7KD", # this program, drawn + LEGACY_APP_SERIAL, + EXPORTER_SERIAL, + ]) + def test_a_serial_this_project_presents_is_ours(self, serial: str) -> None: + assert written_by_opentagviewer(serial) + + def test_the_legacy_constants_need_no_special_case(self) -> None: + # They begin with their own prefixes, so matching the prefix catches them for free. + # Worth pinning: a later reader may be tempted to add them as literals, or to remove + # them believing they are handled separately. + assert LEGACY_APP_SERIAL.startswith(APP_SERIAL_PREFIX) + assert EXPORTER_SERIAL.startswith(SERIAL_PREFIX) + + def test_every_prefix_is_covered(self) -> None: + # So adding a third prefix without adding it to the tuple fails here rather than in a + # picker on somebody's account. + for prefix in OUR_SERIAL_PREFIXES: + assert written_by_opentagviewer(prefix + "0000") + + +class TestRealHardwareIsLeftAlone: + @pytest.mark.parametrize("serial", [ + "F2LX9Q4RNB", # an actual-shaped Apple serial + "C02XK1ABCDEF", + "0PENTAG", # short of either prefix + "0PENTAHV1234", # one letter off + "PENTAGV1234", # missing the leading zero + "X0PENTAGV123", # ours, but not at the start + "0pentagv1234", # lowercase: Apple serials are uppercase, so this is not one of ours + "", + ]) + def test_a_serial_that_is_not_ours_is_not_claimed(self, serial: str) -> None: + assert not written_by_opentagviewer(serial) + + +class TestRecordsThatSayNothing: + """ + The escrow schema is genuinely unstable - of twelve records on one account, one had no + serial, build or bottle id at all. So the absent cases are ordinary, not defensive padding. + """ + + @pytest.mark.parametrize("value", [None, 12345, b"0PENTAGV1234", ["0PENTAGV1234"]]) + def test_anything_that_is_not_a_string_is_not_ours(self, value: object) -> None: + # Kept rather than dropped: hiding a record because a field was missing would hide real + # hardware on the strength of nothing at all. + assert not written_by_opentagviewer(value) # type: ignore[arg-type]