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]