From ecdea9949ed0f5dbc2bb8f5752db72d46d6b5092 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 17:26:46 +0800 Subject: [PATCH] =?UTF-8?q?A=20row=20cannot=20tell=20a=20decoration=20from?= =?UTF-8?q?=20an=20announced=20name=20=E2=80=94=20so=20give=20it=20the=20p?= =?UTF-8?q?eerID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by generalising a review finding on the Android mirror of this patch. The defect is here too, in both directions, and it has been through two rounds of review with me calling this surface finished. `sealAppliesToRow` stripped a trailing "#" plus four hex from the rendered sender unconditionally. But a peer announces whatever string it likes, and "#" plus four hex is a legal thing to announce — `splitSuffix` exists precisely because announced names carry them. So: pinned "ravi#cafe", row shows "ravi#cafe" -> seal LOST with no rename pinned "ravi", row shows "ravi#cafe" -> seal KEPT after a rename The second is the one that matters, and "#cafe" is indistinguishable from the decoration this app generates itself, so the row reads as "ravi, disambiguated" — a better disguise than an unrelated nickname, not a worse one. Trying the raw name before undecorating fixes only the first half; I wrote that version, tested it, and watched the second half sail through. A row string on its own genuinely cannot distinguish the list decorating "ravi" with "#a1b2" from a peer announcing "ravi#a1b2" — the information is not in the string. It is in the peerID. `PeerDisplayNameResolver` builds the decoration as "#" plus the first four hex of the peerID and nothing else does, so `matchesRenderedName(_:decoratedFor:)` removes that exact suffix and no other. Both call sites already had `message.senderPeerID` in hand. The live check is untouched and must stay that way: what a peer announces has no decoration to remove, so it compares directly. That is the same announced-versus-rendered split the Android patch now has, and conflating the two is what produced this. Two cases added, 25 in the suite. 201 suites / 1977 tests, every difference from unmodified main an addition. Mutation-checked: restore the unconditional strip and the new case fails in both directions. Co-Authored-By: Claude Opus 5 --- bitchat/App/ConversationUIModel.swift | 3 +- .../Identity/SecureIdentityStateManager.swift | 14 ++-- bitchat/Utils/String+Nickname.swift | 34 ++++++++ bitchat/ViewModels/ChatMessageFormatter.swift | 3 +- bitchat/ViewModels/ChatViewModel.swift | 6 +- ...tityStateManagerNicknameBindingTests.swift | 80 +++++++++++++++---- 6 files changed, 112 insertions(+), 28 deletions(-) diff --git a/bitchat/App/ConversationUIModel.swift b/bitchat/App/ConversationUIModel.swift index a201eba6..920cbfff 100644 --- a/bitchat/App/ConversationUIModel.swift +++ b/bitchat/App/ConversationUIModel.swift @@ -169,7 +169,8 @@ final class ConversationUIModel: ObservableObject { // whether its current name is. Checking the current name would let a // rename away, a post, and a rename back put a seal beside a name the // key was never trusted under. One call, one lock. - return chatViewModel.sealAppliesToRow(fingerprint, renderedSender: message.sender) + return chatViewModel.sealAppliesToRow(fingerprint, renderedSender: message.sender, + senderPeerID: peerID) } func senderDisplayName(for peerID: PeerID, fallbackMessages: [BitchatMessage]) -> String? { diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index fe6eb9c8..2f0f7b54 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -139,7 +139,7 @@ protocol SecureIdentityStateManagerProtocol { func trustedNicknameMismatch(fingerprint: String) -> Bool /// Verified AND `renderedSender` is the name it was verified under — what /// a seal beside a name frozen on a message row is asserting. - func sealAppliesToRow(fingerprint: String, renderedSender: String) -> Bool + func sealAppliesToRow(fingerprint: String, renderedSender: String, senderPeerID: PeerID?) -> Bool // MARK: Vouching (transitive verification) @discardableResult @@ -166,13 +166,11 @@ extension SecureIdentityStateManagerProtocol { // single lock acquisition — this runs per row, per render, and the format // cache does not shield the call in ChatMessageFormatter because the // answer is part of its cache key. - func sealAppliesToRow(fingerprint: String, renderedSender: String) -> Bool { + func sealAppliesToRow(fingerprint: String, renderedSender: String, senderPeerID: PeerID?) -> Bool { guard isVerified(fingerprint: fingerprint) else { return false } guard let pinned = trustedNickname(fingerprint: fingerprint), !pinned.isEmpty else { return true } - let shown = renderedSender.withoutCollisionSuffix - guard !shown.isEmpty else { return true } - return pinned.nicknameBindingKey == shown.nicknameBindingKey + return pinned.matchesRenderedName(renderedSender, decoratedFor: senderPeerID) } } @@ -802,7 +800,7 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { /// against its own name, which also makes every row self-consistent — a /// historical "ravi ✓" stays sealed even while that key is currently /// renamed, because that row really was ravi. - func sealAppliesToRow(fingerprint: String, renderedSender: String) -> Bool { + func sealAppliesToRow(fingerprint: String, renderedSender: String, senderPeerID: PeerID?) -> Bool { queue.sync { guard cache.verifiedFingerprints.contains(fingerprint) else { return false } guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty @@ -810,9 +808,7 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { // No petname escape here, unlike the live check: the formatter // renders `message.sender`, never a petname, so a petname does not // stand between the reader and a spoofed name on this row. - let shown = renderedSender.withoutCollisionSuffix - guard !shown.isEmpty else { return true } - return pinned.nicknameBindingKey == shown.nicknameBindingKey + return pinned.matchesRenderedName(renderedSender, decoratedFor: senderPeerID) } } diff --git a/bitchat/Utils/String+Nickname.swift b/bitchat/Utils/String+Nickname.swift index e1fedf95..46ef3c70 100644 --- a/bitchat/Utils/String+Nickname.swift +++ b/bitchat/Utils/String+Nickname.swift @@ -7,6 +7,7 @@ // import Foundation +import BitFoundation // PeerID, for the row decoration check extension String { /// Canonical form for nickname storage and comparison (Unicode NFC). @@ -54,6 +55,39 @@ extension String { return String(dropLast(5)) } + /// Does a seal pinned to `self` still apply beside `rendered` — a name as + /// it appears on a message row, which the list may have decorated? + /// + /// `decoration` is the peer's own `#abcd`, built from its peerID exactly as + /// `PeerDisplayNameResolver` builds it. That is what makes this answerable + /// at all: a row string on its own cannot distinguish the list decorating + /// `ravi` with `#a1b2` from the peer announcing the name `ravi#a1b2`. + /// + /// This used to strip any trailing `#` plus four hex, unconditionally, and + /// was wrong in both directions because a peer announces whatever string it + /// likes and `#` plus four hex is a legal thing to announce: + /// + /// pinned `ravi#cafe`, row shows `ravi#cafe` → seal LOST with no rename + /// pinned `ravi`, row shows `ravi#cafe` → seal KEPT after a rename + /// + /// The second is the one that matters, and it survives a "try the raw name + /// first" fix — which is why this takes the peerID instead. `#cafe` is + /// indistinguishable from the decoration this app generates itself, so the + /// row reads as "ravi, disambiguated": a better disguise than an unrelated + /// nickname, not a worse one. Only the suffix this peer would actually be + /// given comes off. + /// + /// The live check must NOT use this. What a peer announces has no + /// decoration to remove. + func matchesRenderedName(_ rendered: String, decoratedFor peerID: PeerID?) -> Bool { + if nicknameBindingKey == rendered.nicknameBindingKey { return true } + guard let peerID else { return false } + let mine = "#" + String(peerID.id.prefix(4)) + guard rendered.count > mine.count, rendered.hasSuffix(mine) else { return false } + let undecorated = String(rendered.dropLast(mine.count)) + return nicknameBindingKey == undecorated.nicknameBindingKey + } + /// Split a nickname into base and a '#abcd' suffix if present func splitSuffix() -> (String, String) { let name = self.replacingOccurrences(of: "@", with: "") diff --git a/bitchat/ViewModels/ChatMessageFormatter.swift b/bitchat/ViewModels/ChatMessageFormatter.swift index 6b5f9899..1a31d591 100644 --- a/bitchat/ViewModels/ChatMessageFormatter.swift +++ b/bitchat/ViewModels/ChatMessageFormatter.swift @@ -451,7 +451,8 @@ private extension ChatMessageFormatter { // Bound to the name ON THIS ROW, which is frozen at receipt — see // `sealAppliesToRow`. This check feeds the format cache key, so the // cache does not shield it; a single call keeps it to one lock. - return viewModel.sealAppliesToRow(fingerprint, renderedSender: message.sender) + return viewModel.sealAppliesToRow(fingerprint, renderedSender: message.sender, + senderPeerID: peerID) } func appendVerifiedSeal( diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index 9e098ff9..3753cb0f 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -1992,8 +1992,10 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, SynchronousMessage /// A seal beside a name frozen on a message row: is THAT name the one this /// key was verified under? See `sealAppliesToRow`. @MainActor - func sealAppliesToRow(_ fingerprint: String, renderedSender: String) -> Bool { - identityManager.sealAppliesToRow(fingerprint: fingerprint, renderedSender: renderedSender) + func sealAppliesToRow(_ fingerprint: String, renderedSender: String, + senderPeerID: PeerID?) -> Bool { + identityManager.sealAppliesToRow(fingerprint: fingerprint, renderedSender: renderedSender, + senderPeerID: senderPeerID) } // MARK: - BitchatDelegate Methods diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index f582041b..a844ea1a 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -213,9 +213,9 @@ struct SecureIdentityStateManagerNicknameBindingTests { #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), "the live name is bound again, which is why the live check alone is not enough") - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic"), + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic", senderPeerID: nil), "the row posted as medic must not be sealed") - #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi"), + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi", senderPeerID: nil), "a row posted as ravi still is — that row really was ravi") } @@ -230,9 +230,9 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "medic") #expect(manager.trustedNicknameMismatch(fingerprint: vouchee), "live name is unbound") - #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi"), + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi", senderPeerID: nil), "but the archived ravi row is still accurate") - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic")) + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic", senderPeerID: nil)) } @Test @@ -244,24 +244,24 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "ravi@hq") manager.setVerified(fingerprint: vouchee, verified: true) - #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi@hq")) - #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi@hq#a1b2")) - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi")) - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#a1b2")) + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi@hq", senderPeerID: nil)) + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi@hq#a1b2", senderPeerID: decorator)) + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi", senderPeerID: nil)) + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#a1b2", senderPeerID: decorator)) } @Test func aRowIsNotSealedForAnUnverifiedKeyAndFailsOpenWithNoBaseline() { let manager = makeManager() announce(manager, vouchee, as: "ravi") - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi"), + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi", senderPeerID: nil), "no verification, no seal") // Verified before any announce: nothing bound, so rows stay sealed as // they did before this existed. let unbound = makeManager() unbound.setVerified(fingerprint: vouchee, verified: true) - #expect(unbound.sealAppliesToRow(fingerprint: vouchee, renderedSender: "anything")) + #expect(unbound.sealAppliesToRow(fingerprint: vouchee, renderedSender: "anything", senderPeerID: nil)) } @Test @@ -280,6 +280,55 @@ struct SecureIdentityStateManagerNicknameBindingTests { "the baseline is what the sheet needs to name") } + @Test + func anAnnouncedSuffixIsNotADecoration() { + // Found by generalising a review finding on the Android mirror of this + // patch. `sealAppliesToRow` stripped a trailing "#abcd" from the + // rendered sender unconditionally — but a peer announces whatever + // string it likes, and "#" plus four hex is a legal thing to announce, + // which is why `splitSuffix` exists at all. So the one rule was wrong + // in both directions. + // + // The half that matters: a key pinned as "ravi" renames to "ravi#cafe" + // and its rows keep the seal. "#cafe" is indistinguishable from the + // decoration this app generates itself, so the row reads as "ravi, + // disambiguated" — a better disguise than an unrelated name, not worse. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi#cafe", senderPeerID: nil), + "a rename onto a hex-looking suffix must not keep the seal") + + // ...and the other half: a key honestly verified under a name that + // simply ends that way must not lose its seal for standing still. + let ends = makeManager() + announce(ends, vouchee, as: "ravi#cafe") + ends.setVerified(fingerprint: vouchee, verified: true) + #expect(ends.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi#cafe", senderPeerID: nil), + "an announced name that ends in a suffix still matches itself") + #expect(ends.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi#cafe#a1b2", senderPeerID: decorator), + "and still matches once the list decorates it") + #expect(!ends.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi", senderPeerID: nil), + "while the undecorated base is a different name") + } + + @Test + func theLiveCheckNeverUndecorates() { + // The live question has no decoration to remove: `peer.nickname` is + // what the peer claims. Stripping there would let exactly the rename + // above through on the peer list as well as on a row. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + announce(manager, vouchee, as: "ravi#cafe") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee), + "announcing ravi#cafe after being verified as ravi is a rename") + } + + /// A peerID whose first four hex are "a1b2", so `#a1b2` is the decoration + /// `PeerDisplayNameResolver` would give this peer and nothing else is. + private var decorator: PeerID { PeerID(str: "a1b2c3d4") } + // MARK: - What counts as the same name @Test @@ -292,7 +341,7 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "ravi") #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) - #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "RAVI")) + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "RAVI", senderPeerID: nil)) } @Test @@ -307,7 +356,7 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "\u{FF2D}edic") #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "\u{FF2D}edic")) + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "\u{FF2D}edic", senderPeerID: nil)) } @Test @@ -319,11 +368,12 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "medic") manager.setVerified(fingerprint: vouchee, verified: true) - #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#a1b2")) + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#a1b2", senderPeerID: decorator)) #expect(!manager.sealAppliesToRow(fingerprint: vouchee, - renderedSender: "medic#\u{FF21}\u{FF22}\u{FF23}\u{FF24}"), + renderedSender: "medic#\u{FF21}\u{FF22}\u{FF23}\u{FF24}", + senderPeerID: decorator), "a fullwidth tail is part of the name, not a suffix this device generates") - #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#zzzz"), + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#zzzz", senderPeerID: nil), "non-hex is part of the name too") }