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