From 8d656242cc77603924d852e63e4e5f4af2e96ba1 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 17:30:25 +0800 Subject: [PATCH] On Android the rendered name IS the announced name, so never strip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review finding about suffix stripping. I said I would bring the iOS fix across; checking how this app actually builds a row name says the answer here is different, and simpler. iOS appends "#" plus four hex of the peerID in PeerDisplayNameResolver, so a row there genuinely carries a synthetic decoration and the fix is to remove exactly that suffix and no other — which needs the peerID, since the string alone cannot distinguish a decoration from an announced name. Nothing decorates a mesh nickname here. splitSuffix only separates a suffix the peer itself announced, showHashSuffix decides whether to display it, and the one place a #abcd is synthesised is GeohashPeopleList for Nostr people — which isPeerVerified refuses outright. So a mesh row shows what was announced, and the undecorate step I added was a fail-OPEN: pinned "medic", row shows "medic#cafe", suffix removed, seal kept. Exactly the case the review raised, still live on the rendered path after I had "fixed" it. sealAppliesToRendered now defers to sealAppliesToAnnounced. It stays a separate named function so there is one place to change if a row here ever does start carrying a decoration, and so nobody reads one comparison as the two questions having merged — collapsing them is what caused this in the first place. Mutation-checked: put the stripping back and two cases fail. Co-Authored-By: Claude Opus 5 --- .../android/identity/NicknameBinding.kt | 39 +++++++++++-------- .../android/identity/NicknameBindingTest.kt | 22 +++++------ 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt index 6e561863..3e8ff79e 100644 --- a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt +++ b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt @@ -88,24 +88,29 @@ object NicknameBinding { /** * Does a seal earned under [pinned] still apply beside [rendered] — a name - * as it appears on a row, which the list may have decorated? + * as it appears on a row? * - * Two peers claiming one nickname are told apart with a `#abcd` the UI - * appends, so comparing the decorated string alone would drop the seal of - * the peer being impersonated at exactly the moment it matters. A raw match - * is tried first, so a name that genuinely ends in a suffix still matches - * itself; only then is one suffix removed. + * On this platform that is the same question as [sealAppliesToAnnounced], + * and the answer is the same comparison, because **nothing decorates a mesh + * nickname here.** `splitSuffix` only separates a suffix the peer itself + * announced, and `showHashSuffix` decides whether to display it; the one + * place a `#abcd` is synthesised is `GeohashPeopleList`, for Nostr people, + * and `isPeerVerified` refuses those outright. iOS is the one that appends + * `#` plus four hex of the peerID in `PeerDisplayNameResolver`, and it has + * to remove exactly that suffix and no other. * - * This is deliberately a SECOND function rather than a flag. The first - * version of this patch had one, stripping unconditionally, and it was - * wrong in both directions at once — which is what happens when one - * predicate is asked two questions. + * It is kept as a separate function anyway, named for the question it + * answers, so that the day a row here does start carrying a decoration + * there is one place to change — and so nobody reads the single comparison + * as the two questions having merged. + * + * The first version of this stripped any trailing `#` plus four hex. That + * is a fail-OPEN on this platform: a key pinned as `medic` renaming to + * `medic#cafe` had the suffix removed and kept its seal, and `#cafe` reads + * as the disambiguator the app generates elsewhere, which is a better + * disguise than an unrelated name rather than a worse one. */ - fun sealAppliesToRendered(pinned: String?, rendered: String?): Boolean { - if (pinned.isNullOrEmpty() || rendered.isNullOrEmpty()) return true - if (sameName(pinned, rendered)) return true - val undecorated = withoutCollisionSuffix(rendered) - if (undecorated.isEmpty()) return true - return sameName(pinned, undecorated) - } + fun sealAppliesToRendered(pinned: String?, rendered: String?): Boolean = + sealAppliesToAnnounced(pinned, rendered) + } diff --git a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt index 3de35ce4..9ea1904f 100644 --- a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt +++ b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt @@ -75,7 +75,7 @@ class NicknameBindingTest { // Two peers claiming one nickname render as "medic#a1b2" and // "medic#c3d4". Comparing the decorated string would drop the seal of // the peer being impersonated, at exactly the moment it matters most. - assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", "medic")) assertEquals("medic", NicknameBinding.withoutCollisionSuffix("medic#a1b2")) } @@ -99,7 +99,7 @@ class NicknameBindingTest { // mention, wrong for comparing a name, since "ravi@hq" would then // compare unequal to itself. assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) - assertTrue(NicknameBinding.sealAppliesToRendered("ravi@hq", "ravi@hq#a1b2")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) assertFalse(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi")) } @@ -119,11 +119,10 @@ class NicknameBindingTest { // A row with no name to show yet is not evidence of a rename. assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", null)) assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", "")) - // A row whose name is nothing BUT a decoration leaves nothing to - // compare, so it fails open too. Note this is a RENDERED-name case: a - // peer that actually announces "#a1b2" has renamed, and says so. - assertTrue(NicknameBinding.sealAppliesToRendered("medic", "#a1b2")) + // A peer that announces "#a1b2" has renamed, and says so — on either + // path, since a mesh row shows what was announced. assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "#a1b2")) + assertFalse(NicknameBinding.sealAppliesToRendered("medic", "#a1b2")) } // ---- announced vs rendered: the split that was missing ----------------- @@ -144,11 +143,12 @@ class NicknameBindingTest { } @Test - fun `a rendered row tries the raw name before undecorating it`() { - // On a row the list may have appended #abcd to tell two namesakes - // apart, so that has to come off — but only after the raw name has had - // its chance, or a name that genuinely ends in a suffix loses its seal. - assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2")) + fun `a rendered mesh row carries no decoration to remove`() { + // Nothing decorates a mesh nickname on this platform: splitSuffix only + // separates a suffix the peer announced, and the one synthesised #abcd + // is for Nostr people, which isPeerVerified refuses outright. So a row + // name IS the announced name, and stripping would be a fail-open. + assertFalse(NicknameBinding.sealAppliesToRendered("medic", "medic#cafe")) assertTrue(NicknameBinding.sealAppliesToRendered("medic#cafe", "medic#cafe")) assertFalse(NicknameBinding.sealAppliesToRendered("medic", "zebra#a1b2")) }