Filter every OpenTagViewer escrow record, not just this session's

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.
+     *
+     * <p><b>This literal is duplicated, and that is the whole reason for this test.</b> 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.
+     *
+     * <p>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.
+     *
+     * <p>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.
      *

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:

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.

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],
         )
     ]
 

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)

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 ""

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]