From 37e02c18514b916bb9f1a9909cb07c2dfe28c091 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Tue, 8 Sep 2026 17:31:16 +0800 Subject: [PATCH 1/8] Bind a trust seal to the nickname it was earned under MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vouched or verified key can rename itself onto a nickname the user trusts and keep rendering the seal beside the new name. `VouchAttestation.signableBytes` covers `voucheeFingerprint | voucheeSigningKey | timestampMs` and says nothing about a name — which is right; the attestation should stay name-free. But the seal is *rendered* beside a self-claimed nickname: `PeerListModel` derives `verifiedBadge`/`vouchedBadge` from the fingerprint and then shows `peer.displayName`, which falls back to the peer-claimed `nickname` whenever `localPetname` is unset. So the seal is read next to a name that nothing binds it to. Earn one genuine vouch as "ravi", announce "medic", and every device that trusts the real medic now shows a second trusted-looking medic. This is the mirror of #1661's case. That one is same nickname, new key; this is same key, new nickname. The binding has to live on the receiver, because the receiver is the only party that knows which name the key was presenting when it decided to trust it. So `IdentityCache` gains `trustedNicknames`, pinned when trust is established, and all three places that render a seal — the peer list, the public-timeline seal and the DM seal — suppress it while the peer presents a different name. Where it is pinned, and why: - On the FIRST vouch only, never moved by a later one. Otherwise an attacker renames and the next voucher silently re-anchors the seal to the new name. - Re-verifying DOES move it: the user just checked this key in person, under whatever name it shows now. - On the first announce seen while trust already stands. A vouch normally arrives for a peer we have not seen announce — it comes over Noise from someone else, and the vouchee may be hops away — so there is no name to bind to at record time. The announce path deliberately does NOT pin before trust exists: binding to whatever name we happened to see first would suppress the seal of a peer who renamed *before* being vouched. - Never on an empty nickname, and a missing baseline never suppresses. Peers trusted by earlier builds have none, and dropping their seals on upgrade would teach users to ignore the signal. Each seal site compares against the name it actually renders, not against the stored `claimedNickname`. That matters: offline favourite rows take their name from the persisted favourite relationship, which can lag the last announce, and a message row shows the sender as of that message. Comparing the rendered string keeps the seal truthful about the name the user is looking at. Message senders can carry a `#abcd` disambiguation suffix that an announced nickname never has, so a `displayedSender` overload strips it with the existing `splitSuffix()`. That overload is a protocol extension rather than a new requirement, so it needs no mock changes and its logic sits in a layer the tests can reach. A local petname already outranks the claimed nickname for display, so there is nothing to spoof there and the seal stands. The known limit, pinned by a test so it is explicit: if we never saw the key under its real name, its first announce IS the baseline, so an impersonating first announce is not caught. Closing that needs the nickname signed into the attestation, which is a wire change and deliberately not attempted here. `FingerprintView`'s vouched panel is left alone: that sheet is an explicit inspection surface keyed by fingerprint, where showing the trust state is the point. No wire or protocol change, and no new user-facing strings. Suppressing the seal is the security fix; explaining it in the UI wants the notice plumbing #1661 is adding, so it is left for a follow-up. `trustedNicknames` decodes with `decodeIfPresent` like the other post-v1 fields, so caches written by older builds still load. Tests: SecureIdentityStateManagerNicknameBindingTests (15) covers the rename attack, the second-voucher laundering attempt, the vouch-before-first-announce path and its residual gap, the no-pin-before-trust rule, suffix handling in both directions, rebinding on re-verification, clearing on unverify only when no vouch remains, both fail-open cases, and persistence compatibility each way. `TestIdentityManager` in UnifiedPeerServiceTests also conforms to the protocol and is updated. --- bitchat/App/ConversationUIModel.swift | 6 +- bitchat/App/PeerListModel.swift | 13 +- bitchat/Identity/IdentityModels.swift | 10 + .../Identity/SecureIdentityStateManager.swift | 79 ++++++ bitchat/ViewModels/ChatMessageFormatter.swift | 5 +- bitchat/ViewModels/ChatViewModel.swift | 23 ++ bitchatTests/Mocks/MockIdentityManager.swift | 8 + ...tityStateManagerNicknameBindingTests.swift | 265 ++++++++++++++++++ .../Services/UnifiedPeerServiceTests.swift | 4 + 9 files changed, 409 insertions(+), 4 deletions(-) create mode 100644 bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift diff --git a/bitchat/App/ConversationUIModel.swift b/bitchat/App/ConversationUIModel.swift index 4883a04c..d1cc595f 100644 --- a/bitchat/App/ConversationUIModel.swift +++ b/bitchat/App/ConversationUIModel.swift @@ -164,7 +164,11 @@ final class ConversationUIModel: ObservableObject { !isSentByCurrentUser(message), let peerID = message.senderPeerID else { return false } guard let fingerprint = chatViewModel.getFingerprint(for: peerID) else { return false } - return chatViewModel.peerIdentityStore.isVerified(fingerprint) + guard chatViewModel.peerIdentityStore.isVerified(fingerprint) else { return false } + // The seal is read next to `message.sender`, so it has to be bound to + // that name: a verified key that renamed itself onto a name the user + // trusts must not carry the seal beside the new one. + return !chatViewModel.trustedNicknameMismatch(fingerprint, displayedSender: message.sender) } func senderDisplayName(for peerID: PeerID, fallbackMessages: [BitchatMessage]) -> String? { diff --git a/bitchat/App/PeerListModel.swift b/bitchat/App/PeerListModel.swift index 4f1fc633..779bff26 100644 --- a/bitchat/App/PeerListModel.swift +++ b/bitchat/App/PeerListModel.swift @@ -232,9 +232,18 @@ final class PeerListModel: ObservableObject { let isMe = peer.peerID == myPeerID let fingerprint = isMe ? nil : chatViewModel.getFingerprint(for: peer.peerID) let isVerifiedFingerprint = fingerprint.map { peerIdentityStore.isVerified($0) } ?? false - let verifiedBadge = !peer.isConnected && isVerifiedFingerprint + // A seal is bound to the name it was earned under. Without that, + // a key that earned one vouch as "ravi" can rename itself to + // "medic" and keep rendering the seal beside the new name. A local + // petname already outranks the claimed nickname for display, so + // there is nothing to spoof and the seal stands. + let hasPetname = !(peer.localPetname ?? "").isEmpty + let renamedSinceTrust = !hasPetname && (fingerprint.map { + chatViewModel.trustedNicknameMismatch($0, claimedNickname: peer.nickname) + } ?? false) + let verifiedBadge = !peer.isConnected && isVerifiedFingerprint && !renamedSinceTrust // Vouched is subordinate to verified: never show both seals. - let vouchedBadge = !isVerifiedFingerprint + let vouchedBadge = !isVerifiedFingerprint && !renamedSinceTrust && (fingerprint.map { chatViewModel.isVouchedFingerprint($0) } ?? false) return MeshPeerRow( diff --git a/bitchat/Identity/IdentityModels.swift b/bitchat/Identity/IdentityModels.swift index 8a72fcba..f82169f6 100644 --- a/bitchat/Identity/IdentityModels.swift +++ b/bitchat/Identity/IdentityModels.swift @@ -202,6 +202,15 @@ struct IdentityCache: Codable { // entries verified before this field exists sort as oldest) var verifiedAt: [String: Date]? = nil + /// The self-claimed nickname each fingerprint was presenting at the moment + /// trust in it was established here — set on verification and on a first + /// accepted vouch. A trust badge means "this key was the Medic I checked", + /// so it has to be pinned to the name it was earned under; without this + /// baseline a vouched key can rename itself onto a name the user trusts and + /// keep rendering the seal beside it. Absent for peers trusted by older + /// builds, which is why a missing entry never suppresses a badge. + var trustedNicknames: [String: String]? = nil + // Stable Noise fingerprints that proved encrypted private-media support // inside an authenticated Noise session. Optional for decoding caches // written before this migration. Entries are monotonic until a panic wipe @@ -240,6 +249,7 @@ struct IdentityCache: Codable { vouchesByVouchee = try container.decodeIfPresent([String: [VouchRecord]].self, forKey: .vouchesByVouchee) vouchBatchSentAt = try container.decodeIfPresent([String: Date].self, forKey: .vouchBatchSentAt) verifiedAt = try container.decodeIfPresent([String: Date].self, forKey: .verifiedAt) + trustedNicknames = try container.decodeIfPresent([String: String].self, forKey: .trustedNicknames) privateMediaCapableFingerprints = try container.decodeIfPresent(Set.self, forKey: .privateMediaCapableFingerprints) authenticatedSigningKeysByFingerprint = try container.decodeIfPresent([String: Data].self, forKey: .authenticatedSigningKeysByFingerprint) cryptographicIdentities = try container.decodeIfPresent([String: CryptographicIdentity].self, forKey: .cryptographicIdentities) ?? [:] diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index 4204e940..f27f4b0d 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -130,6 +130,9 @@ protocol SecureIdentityStateManagerProtocol { func setVerified(fingerprint: String, verified: Bool) func isVerified(fingerprint: String) -> Bool func getVerifiedFingerprints() -> Set + /// Whether this peer is now claiming a different nickname than the one its + /// trust was earned under. + func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool // MARK: Vouching (transitive verification) @discardableResult @@ -150,6 +153,17 @@ protocol SecureIdentityStateManagerProtocol { func hasObservedPrivateMediaCapability(fingerprint: String) -> Bool } +extension SecureIdentityStateManagerProtocol { + /// `trustedNicknameMismatch` for a name as *rendered* in a message row, + /// which may carry a `#abcd` disambiguation suffix that an announced + /// nickname never has. Comparing the displayed string verbatim would read + /// every suffixed sender as a mismatch. + func trustedNicknameMismatch(fingerprint: String, displayedSender: String) -> Bool { + trustedNicknameMismatch(fingerprint: fingerprint, + claimedNickname: displayedSender.splitSuffix().0) + } +} + /// Singleton manager for secure identity state persistence and retrieval. /// Provides thread-safe access to identity mappings with encryption at rest. /// All identity data is stored encrypted in the device Keychain for security. @@ -428,6 +442,15 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { } else if self.cache.socialIdentities[fingerprint] == nil { self.cache.socialIdentities[fingerprint] = identity } + // A vouch usually arrives for a peer we have not seen announce + // — it comes over Noise from someone else, and the vouchee may + // be several hops away. There was no name to bind to then, so + // bind on the first announce we see while the trust stands. + self.pinTrustedNicknameLocked( + fingerprint: fingerprint, + overwrite: false, + requireExistingTrust: true + ) } self.saveIdentityCache() @@ -678,9 +701,15 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { var verifiedAt = self.cache.verifiedAt ?? [:] verifiedAt[fingerprint] = Date() self.cache.verifiedAt = verifiedAt + // Re-verifying overwrites the baseline: the user just checked + // this key again, under whatever name it presents now. + self.pinTrustedNicknameLocked(fingerprint: fingerprint, overwrite: true) } else { self.cache.verifiedFingerprints.remove(fingerprint) self.cache.verifiedAt?.removeValue(forKey: fingerprint) + if self.cache.vouchesByVouchee?[fingerprint]?.isEmpty ?? true { + self.cache.trustedNicknames?.removeValue(forKey: fingerprint) + } } // Update trust level if social identity exists @@ -698,6 +727,52 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { return cache.verifiedFingerprints.contains(fingerprint) } } + + // MARK: - Nickname binding + // + // A vouch signs `voucheeFingerprint | voucheeSigningKey | timestampMs` and + // deliberately says nothing about a name — the attestation is right to stay + // name-free. But the badge is *rendered* next to a self-claimed nickname, + // so the binding has to exist somewhere, and the receiver is the only party + // that can hold it: it is the one that decided to trust this key while it + // was presenting a particular name. + + /// Requires `queue`. Records the peer's currently claimed nickname as the + /// name its trust is bound to. Empty nicknames are not pinned — a key + /// verified before its first announce has no name to bind to, and pinning + /// "" would then read as a mismatch against every later announce. + /// + /// `requireExistingTrust` is for the announce path, which runs for every + /// peer: pinning a name before there is any trust would bind the badge to + /// whatever name we happened to see first, so a peer who renamed *before* + /// being vouched would have its legitimate badge suppressed. + private func pinTrustedNicknameLocked(fingerprint: String, + overwrite: Bool, + requireExistingTrust: Bool = false) { + if requireExistingTrust { + let trusted = cache.verifiedFingerprints.contains(fingerprint) + || !(cache.vouchesByVouchee?[fingerprint] ?? []).isEmpty + guard trusted else { return } + } + guard let claimed = cache.socialIdentities[fingerprint]?.claimedNickname, + !claimed.isEmpty else { return } + var pinned = cache.trustedNicknames ?? [:] + if !overwrite, pinned[fingerprint] != nil { return } + pinned[fingerprint] = claimed + cache.trustedNicknames = pinned + } + + /// True only when a baseline exists AND the peer now claims something else. + /// Fails OPEN on a missing baseline on purpose: peers trusted by builds + /// before this existed have none, and dropping their badges on upgrade + /// would train users to ignore the signal. + func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool { + queue.sync { + guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty, + !claimedNickname.isEmpty else { return false } + return pinned != claimedNickname + } + } func getVerifiedFingerprints() -> Set { queue.sync { @@ -760,6 +835,10 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { var vouches = self.cache.vouchesByVouchee ?? [:] vouches[voucheeFingerprint] = capped self.cache.vouchesByVouchee = vouches + // Pin on the FIRST vouch only. A later vouch for the same key must + // not move the baseline, or an attacker could rename and then have + // a second voucher silently re-anchor the badge to the new name. + self.pinTrustedNicknameLocked(fingerprint: voucheeFingerprint, overwrite: false) self.saveIdentityCache() return true } diff --git a/bitchat/ViewModels/ChatMessageFormatter.swift b/bitchat/ViewModels/ChatMessageFormatter.swift index 1a00315b..51d67d77 100644 --- a/bitchat/ViewModels/ChatMessageFormatter.swift +++ b/bitchat/ViewModels/ChatMessageFormatter.swift @@ -448,7 +448,10 @@ private extension ChatMessageFormatter { let fingerprint = viewModel.getFingerprint(for: peerID) else { return false } - return viewModel.peerIdentityStore.isVerified(fingerprint) + guard viewModel.peerIdentityStore.isVerified(fingerprint) else { return false } + // Bound to the rendered name, for the same reason as the peer list and + // the DM seal: the badge attests to a key but is read as a name. + return !viewModel.trustedNicknameMismatch(fingerprint, displayedSender: message.sender) } func appendVerifiedSeal( diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index 63fba0f8..660c8e6c 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -1973,6 +1973,29 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, SynchronousMessage identityManager.isVouched(fingerprint: fingerprint) } + /// Whether this peer now claims a different nickname than the one its + /// trust was established under. Trust badges are suppressed in that case: + /// the seal attests to a key, but it is read next to a name, and a key that + /// renamed itself onto a name the user trusts is the impersonation this + /// warns about. + @MainActor + func trustedNicknameMismatch(_ fingerprint: String, claimedNickname: String) -> Bool { + identityManager.trustedNicknameMismatch( + fingerprint: fingerprint, + claimedNickname: claimedNickname + ) + } + + /// The same check for a name as *rendered* in a message row. See the + /// `displayedSender` overload on `SecureIdentityStateManagerProtocol`. + @MainActor + func trustedNicknameMismatch(_ fingerprint: String, displayedSender: String) -> Bool { + identityManager.trustedNicknameMismatch( + fingerprint: fingerprint, + displayedSender: displayedSender + ) + } + // MARK: - BitchatDelegate Methods // MARK: - Command Handling diff --git a/bitchatTests/Mocks/MockIdentityManager.swift b/bitchatTests/Mocks/MockIdentityManager.swift index 930d57ff..c5fa97df 100644 --- a/bitchatTests/Mocks/MockIdentityManager.swift +++ b/bitchatTests/Mocks/MockIdentityManager.swift @@ -98,6 +98,14 @@ final class MockIdentityManager: SecureIdentityStateManagerProtocol { func setVerified(fingerprint: String, verified: Bool) {} + var trustedNicknames: [String: String] = [:] + + func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool { + guard let pinned = trustedNicknames[fingerprint], !pinned.isEmpty, + !claimedNickname.isEmpty else { return false } + return pinned != claimedNickname + } + func isVerified(fingerprint: String) -> Bool { true } diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift new file mode 100644 index 00000000..5a76c6eb --- /dev/null +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -0,0 +1,265 @@ +import Foundation +import Testing + +@testable import bitchat + +/// Binds a trust badge to the nickname it was earned under. +/// +/// `VouchAttestation` signs `voucheeFingerprint | voucheeSigningKey | +/// timestampMs` and deliberately says nothing about a name — a name-free +/// attestation is the right wire format. But the badge is *rendered* beside a +/// self-claimed nickname, so the binding has to live on the receiver, which is +/// the only party that knows what name the key was presenting when it decided +/// to trust it. These tests pin that rule. +/// +/// `@MainActor` for the same reason as `SecureIdentityStateManagerVouchTests`: +/// the manager's blocking `queue.sync` reads must stay off the Swift +/// Concurrency cooperative pool, or CI's few-core runners deadlock. +@MainActor +struct SecureIdentityStateManagerNicknameBindingTests { + private let voucher = String(repeating: "0a", count: 32) + private let secondVoucher = String(repeating: "0c", count: 32) + private let vouchee = String(repeating: "0b", count: 32) + + private func makeManager() -> SecureIdentityStateManager { + SecureIdentityStateManager(MockKeychain()) + } + + /// Mirrors the announce path: the peer tells us what it calls itself. + private func announce(_ manager: SecureIdentityStateManager, + _ fingerprint: String, + as nickname: String) { + manager.upsertCryptographicIdentity( + fingerprint: fingerprint, + noisePublicKey: Data(repeating: 0x01, count: 32), + signingPublicKey: nil, + claimedNickname: nickname + ) + } + + // MARK: - The attack this closes + + @Test + func vouch_pinsTheNicknameItWasEarnedUnder() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: voucher, verified: true) + + #expect(manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date())) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi")) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), + "the baseline is pinned: any other name is a mismatch") + } + + @Test + func renamingOntoATrustedNameAfterEarningAVouchIsAMismatch() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + + // Same key, new self-chosen name — the badge must not follow it. + announce(manager, vouchee, as: "medic") + + #expect(manager.isVouched(fingerprint: vouchee), + "the vouch itself is still valid; only its binding to a name broke") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + "the baseline stayed on the name the vouch was earned under") + } + + @Test + func aLaterVoucherCannotReanchorTheBaselineToTheNewName() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + announce(manager, vouchee, as: "medic") + + // A second vouch arriving after the rename must not launder it. + manager.setVerified(fingerprint: secondVoucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: secondVoucher, timestamp: Date()) + + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + "the second vouch did not launder the rename") + } + + // MARK: - Vouches for peers we have not seen yet + + @Test + func aVouchForAnUnseenPeerBindsOnTheirFirstAnnounce() { + // The usual case: the vouch arrives over Noise from someone else and + // the vouchee is not in our announce set yet, so there is no name to + // bind to at record time. + let manager = makeManager() + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + "nothing is bound yet, so nothing can mismatch") + + announce(manager, vouchee, as: "ravi") + announce(manager, vouchee, as: "medic") + + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + "the first name seen while the vouch stood is the binding") + } + + @Test + func theAnnouncePathDoesNotBindBeforeTrustExists() { + // Otherwise a peer who renamed BEFORE being vouched would be bound to + // the name we happened to see first, and lose a legitimate badge. + let manager = makeManager() + announce(manager, vouchee, as: "rav") + announce(manager, vouchee, as: "ravi") + + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + "bound to the name it was vouched under, not the name seen first") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "rav")) + } + + @Test + func aFirstAnnounceThatIsAlreadyTheImpersonatingNameIsNotCaught() { + // The documented limit of receiver-side binding: if we never saw this + // key under its real name, the impersonating name IS the baseline. + // Only signing the nickname into the attestation closes this, which is + // a wire change and deliberately not attempted here. + let manager = makeManager() + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + + announce(manager, vouchee, as: "medic") + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), + "known gap, pinned here so it is explicit rather than a surprise") + } + + // MARK: - Rebinding and clearing + + @Test + func verifyingInPersonRebindsToTheCurrentName() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + announce(manager, vouchee, as: "medic") + + // The user scanned this key themselves, under the name it shows now. + manager.setVerified(fingerprint: vouchee, verified: true) + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + "the baseline moved to the name just checked in person") + } + + @Test + func unverifyingClearsTheBaselineOnlyWhenNoVouchRemains() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + + manager.setVerified(fingerprint: vouchee, verified: true) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + manager.setVerified(fingerprint: vouchee, verified: false) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), + "with no trust left there is no baseline, so nothing to mismatch") + + // With a vouch still standing, the baseline has to survive: it is what + // that vouch's badge is bound to. + manager.setVerified(fingerprint: voucher, verified: true) + manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) + manager.setVerified(fingerprint: vouchee, verified: true) + manager.setVerified(fingerprint: vouchee, verified: false) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), + "the vouch's badge still needs the name it was bound to") + } + + // MARK: - Names as rendered in a message row + + @Test + func aDisambiguationSuffixIsNotMistakenForARename() { + // Message senders render as "ravi#a1b2" when nicknames collide; the + // announced nickname never carries the suffix. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi#a1b2")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "@ravi#a1b2")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi")) + } + + @Test + func aRenameIsStillCaughtThroughTheSuffix() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "medic#a1b2")) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "medic")) + } + + @Test + func aNameEndingInSomethingSuffixLikeIsNotTruncated() { + // "#abcd" only counts as a suffix when those four characters are hex; + // a nickname that merely contains '#' must still compare whole. + let manager = makeManager() + announce(manager, vouchee, as: "ravi#zzzz") + manager.setVerified(fingerprint: vouchee, verified: true) + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi#zzzz")) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi")) + } + + // MARK: - Failing open + + @Test + func aMissingBaselineNeverSuppressesABadge() { + // Verified before the peer ever announced a name — there is nothing to + // bind to, and pinning "" would read as a mismatch against every later + // announce. Peers trusted by builds before this shipped land here too, + // so an upgrade must not silently drop their badges. + let manager = makeManager() + manager.setVerified(fingerprint: vouchee, verified: true) + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi")) + } + + @Test + func anEmptyClaimedNicknameIsNotAMismatch() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "")) + } + + // MARK: - Persistence compatibility + + @Test + func aCacheWrittenBeforeThisFieldExistedStillLoads() throws { + let legacyJSON = Data(""" + {"socialIdentities":{},"nicknameIndex":{},"verifiedFingerprints":["\(vouchee)"],\ + "lastInteractions":{},"blockedNostrPubkeys":[],"cryptographicIdentities":{},"version":1} + """.utf8) + + let decoded = try JSONDecoder().decode(IdentityCache.self, from: legacyJSON) + + #expect(decoded.trustedNicknames == nil) + #expect(decoded.verifiedFingerprints.contains(vouchee)) + } + + @Test + func theBaselineSurvivesAnEncodeDecodeRoundTrip() throws { + var cache = IdentityCache() + cache.trustedNicknames = [vouchee: "ravi"] + + let decoded = try JSONDecoder().decode(IdentityCache.self, from: JSONEncoder().encode(cache)) + + #expect(decoded.trustedNicknames?[vouchee] == "ravi") + } +} diff --git a/bitchatTests/Services/UnifiedPeerServiceTests.swift b/bitchatTests/Services/UnifiedPeerServiceTests.swift index 4bc6f775..7dfae43b 100644 --- a/bitchatTests/Services/UnifiedPeerServiceTests.swift +++ b/bitchatTests/Services/UnifiedPeerServiceTests.swift @@ -289,6 +289,10 @@ private final class TestIdentityManager: SecureIdentityStateManagerProtocol { verified } + // No name binding is exercised here: these tests drive peer-list assembly, + // not trust display. + func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool { false } + // MARK: Vouching (unused by these tests) @discardableResult From a74fa1066e8380a237bc5e7c8974fb117aedf2df Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Wed, 9 Sep 2026 12:02:02 +0800 Subject: [PATCH 2/8] Cover the connected peer, and stop comparing decorated names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the patch fixed the case that matters least and could hurt the peer it protects. Both are addressed here. **The connected peer kept its seal.** `verifiedBadge` already carried `!peer.isConnected`, so gating it on the rename was dead for a connected peer. For that peer the seal comes from somewhere else entirely: `EncryptionStatus.noiseVerified.icon` is the same `checkmark.seal.fill` glyph, rendered straight from `peer.encryptionStatus.icon` in `MeshPeerList` and again in the DM header in `ContentSheetViews`. Neither was gated, so a verified key that renamed while holding a Noise session — the live impersonation — kept a filled seal beside the new name. The binding now applies at `ChatPeerIdentityCoordinator.getEncryptionStatus`, which both of those read through, demoting `.noiseVerified` to `.noiseSecured`: the session really is encrypted, only the claim about *who* is withdrawn. Nothing branches on `.noiseVerified` beyond the glyph — every behavioural site already treats it and `.noiseSecured` alike — so this changes what is drawn and nothing else. The demotion is computed on read rather than stored, because the cached status is invalidated by verification and session changes but never by a rename; a demotion written into the cache would be stale and sticky. It also has to run on the cache-hit path, which is the common one — the new coordinator test caught that being missed. **The comparison could suppress the victim.** `peer.nickname` is already collision-resolved: `BLEPeerRegistry.transportSnapshots` runs names through `PeerDisplayNameResolver`, which appends `#` plus four hex of the peerID to CONNECTED peers whose nicknames collide. That is precisely the attack — two connected peers both claiming "medic" — so the real medic's `peer.nickname` becomes "medic#c3d4", no longer equals the pinned "medic", and the impersonated peer loses its seal exactly when it matters. So the check no longer takes a name at all. It compares the pinned baseline against the peer's own announced nickname, both in NFC via `normalizedNickname` to match how nicknames are compared everywhere else. That removes three failure modes at once: the collision suffix, the `splitSuffix()` overload that also stripped every `@` from a legal nickname, and the divergence between a message row's historical sender name and the live one. A local petname is handled centrally in the same place, since it outranks the claimed nickname wherever it is displayed. Tests: two coordinator cases for the demotion, including that it is not written into the cache and that it leaves secured and handshaking states alone; a regression guard asserting `PeerDisplayNameResolver` really does decorate and that the binding ignores it; petname and NFC cases. 15 in the binding suite, 10 in the coordinator suite. --- bitchat/App/ConversationUIModel.swift | 7 +- bitchat/App/PeerListModel.swift | 16 +- .../Identity/SecureIdentityStateManager.swift | 45 ++--- bitchat/ViewModels/ChatMessageFormatter.swift | 6 +- .../ChatPeerIdentityCoordinator.swift | 27 ++- bitchat/ViewModels/ChatViewModel.swift | 26 +-- ...tPeerIdentityCoordinatorContextTests.swift | 52 ++++++ bitchatTests/Mocks/MockIdentityManager.swift | 8 +- ...tityStateManagerNicknameBindingTests.swift | 169 ++++++++++-------- .../Services/UnifiedPeerServiceTests.swift | 2 +- 10 files changed, 226 insertions(+), 132 deletions(-) diff --git a/bitchat/App/ConversationUIModel.swift b/bitchat/App/ConversationUIModel.swift index d1cc595f..508ca493 100644 --- a/bitchat/App/ConversationUIModel.swift +++ b/bitchat/App/ConversationUIModel.swift @@ -165,10 +165,9 @@ final class ConversationUIModel: ObservableObject { let peerID = message.senderPeerID else { return false } guard let fingerprint = chatViewModel.getFingerprint(for: peerID) else { return false } guard chatViewModel.peerIdentityStore.isVerified(fingerprint) else { return false } - // The seal is read next to `message.sender`, so it has to be bound to - // that name: a verified key that renamed itself onto a name the user - // trusts must not carry the seal beside the new one. - return !chatViewModel.trustedNicknameMismatch(fingerprint, displayedSender: message.sender) + // A verified key that renamed itself onto a name the user trusts must + // not carry the seal beside the new one. + return !chatViewModel.trustedNicknameMismatch(fingerprint) } func senderDisplayName(for peerID: PeerID, fallbackMessages: [BitchatMessage]) -> String? { diff --git a/bitchat/App/PeerListModel.swift b/bitchat/App/PeerListModel.swift index 779bff26..4cf8bb51 100644 --- a/bitchat/App/PeerListModel.swift +++ b/bitchat/App/PeerListModel.swift @@ -234,13 +234,15 @@ final class PeerListModel: ObservableObject { let isVerifiedFingerprint = fingerprint.map { peerIdentityStore.isVerified($0) } ?? false // A seal is bound to the name it was earned under. Without that, // a key that earned one vouch as "ravi" can rename itself to - // "medic" and keep rendering the seal beside the new name. A local - // petname already outranks the claimed nickname for display, so - // there is nothing to spoof and the seal stands. - let hasPetname = !(peer.localPetname ?? "").isEmpty - let renamedSinceTrust = !hasPetname && (fingerprint.map { - chatViewModel.trustedNicknameMismatch($0, claimedNickname: peer.nickname) - } ?? false) + // "medic" and keep rendering the seal beside the new name. The + // check compares announced names, NOT `peer.nickname`, which is + // already collision-resolved: two connected peers both claiming + // "medic" are rendered "medic#a1b2" and "medic#c3d4", so comparing + // the displayed string would drop the real medic's seal during the + // very attack this defends against. Petnames are handled there too. + let renamedSinceTrust = fingerprint.map { + chatViewModel.trustedNicknameMismatch($0) + } ?? false let verifiedBadge = !peer.isConnected && isVerifiedFingerprint && !renamedSinceTrust // Vouched is subordinate to verified: never show both seals. let vouchedBadge = !isVerifiedFingerprint && !renamedSinceTrust diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index f27f4b0d..91b82624 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -130,9 +130,9 @@ protocol SecureIdentityStateManagerProtocol { func setVerified(fingerprint: String, verified: Bool) func isVerified(fingerprint: String) -> Bool func getVerifiedFingerprints() -> Set - /// Whether this peer is now claiming a different nickname than the one its + /// Whether this peer now announces a different nickname than the one its /// trust was earned under. - func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool + func trustedNicknameMismatch(fingerprint: String) -> Bool // MARK: Vouching (transitive verification) @discardableResult @@ -153,17 +153,6 @@ protocol SecureIdentityStateManagerProtocol { func hasObservedPrivateMediaCapability(fingerprint: String) -> Bool } -extension SecureIdentityStateManagerProtocol { - /// `trustedNicknameMismatch` for a name as *rendered* in a message row, - /// which may carry a `#abcd` disambiguation suffix that an announced - /// nickname never has. Comparing the displayed string verbatim would read - /// every suffixed sender as a mismatch. - func trustedNicknameMismatch(fingerprint: String, displayedSender: String) -> Bool { - trustedNicknameMismatch(fingerprint: fingerprint, - claimedNickname: displayedSender.splitSuffix().0) - } -} - /// Singleton manager for secure identity state persistence and retrieval. /// Provides thread-safe access to identity mappings with encryption at rest. /// All identity data is stored encrypted in the device Keychain for security. @@ -762,15 +751,31 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { cache.trustedNicknames = pinned } - /// True only when a baseline exists AND the peer now claims something else. + /// True only when a baseline exists AND the peer now announces something + /// else. + /// + /// Deliberately compares announced name to announced name rather than to + /// anything on screen. Display strings are decorated: `PeerDisplayNameResolver` + /// appends `#abcd` to CONNECTED peers whose nicknames collide — which is + /// precisely what happens during this attack — so comparing a rendered name + /// would suppress the seal of the peer being impersonated, exactly when it + /// matters most. + /// /// Fails OPEN on a missing baseline on purpose: peers trusted by builds - /// before this existed have none, and dropping their badges on upgrade - /// would train users to ignore the signal. - func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool { + /// before this existed have none, and dropping their seals on upgrade would + /// train users to ignore the signal. + func trustedNicknameMismatch(fingerprint: String) -> Bool { queue.sync { - guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty, - !claimedNickname.isEmpty else { return false } - return pinned != claimedNickname + guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty + else { return false } + let social = cache.socialIdentities[fingerprint] + // A local petname outranks the claimed nickname everywhere it is + // displayed, so there is nothing to spoof and the seal stands. + if let petname = social?.localPetname, !petname.isEmpty { return false } + guard let claimed = social?.claimedNickname, !claimed.isEmpty else { return false } + // NFC, matching `normalizedNickname` everywhere else nicknames are + // compared: a decomposed and a precomposed "café" are one name. + return pinned.normalizedNickname != claimed.normalizedNickname } } diff --git a/bitchat/ViewModels/ChatMessageFormatter.swift b/bitchat/ViewModels/ChatMessageFormatter.swift index 51d67d77..bb661a35 100644 --- a/bitchat/ViewModels/ChatMessageFormatter.swift +++ b/bitchat/ViewModels/ChatMessageFormatter.swift @@ -449,9 +449,9 @@ private extension ChatMessageFormatter { return false } guard viewModel.peerIdentityStore.isVerified(fingerprint) else { return false } - // Bound to the rendered name, for the same reason as the peer list and - // the DM seal: the badge attests to a key but is read as a name. - return !viewModel.trustedNicknameMismatch(fingerprint, displayedSender: message.sender) + // Bound to the name the key announces, for the same reason as the peer + // list and the DM seal: the badge attests to a key but is read as a name. + return !viewModel.trustedNicknameMismatch(fingerprint) } func appendVerifiedSeal( diff --git a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift index e2d0fae8..fbe8338e 100644 --- a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift +++ b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift @@ -76,6 +76,8 @@ protocol ChatPeerIdentityContext: AnyObject { /// falling back to `fallback` when none was stored. Returns the migrated fingerprint. func migrateFingerprintMapping(from oldPeerID: PeerID, to newPeerID: PeerID, fallback: String?) -> String? func isVerifiedFingerprint(_ fingerprint: String) -> Bool + /// Whether the peer renamed itself since its trust was established. + func trustedNicknameMismatch(_ fingerprint: String) -> Bool func setEncryptionStatus(_ status: EncryptionStatus?, for peerID: PeerID) func cachedEncryptionStatus(for peerID: PeerID) -> EncryptionStatus? func setCachedEncryptionStatus(_ status: EncryptionStatus, for peerID: PeerID) @@ -437,7 +439,9 @@ final class ChatPeerIdentityCoordinator { @MainActor func getEncryptionStatus(for peerID: PeerID) -> EncryptionStatus { if let cachedStatus = context.cachedEncryptionStatus(for: peerID) { - return cachedStatus + // Demote on the cache-hit path too — this is the common one, and + // nothing invalidates the cache on a rename. + return demotedIfRenamed(cachedStatus, for: peerID) } // The status must reflect the LIVE session, never history. The old @@ -462,7 +466,26 @@ final class ChatPeerIdentityCoordinator { } context.setCachedEncryptionStatus(status, for: peerID) - return status + return demotedIfRenamed(status, for: peerID) + } + + /// `.noiseVerified` renders the same filled seal as the verified badge, and + /// for a CONNECTED peer it is the only thing that draws one — so the name + /// binding has to apply here too, or the live impersonation keeps its seal. + /// + /// Applied on the way out rather than stored: the cached status is only + /// invalidated by verification and session changes, never by a rename, so a + /// demotion written into the cache would be both stale and sticky. The + /// session really is Noise-secured; only the claim about *who* is weakened, + /// which is exactly the difference between the two cases. Nothing branches + /// on `.noiseVerified` beyond the glyph — every behavioural site treats it + /// and `.noiseSecured` alike. + @MainActor + private func demotedIfRenamed(_ status: EncryptionStatus, for peerID: PeerID) -> EncryptionStatus { + guard status == .noiseVerified, + let fingerprint = getFingerprint(for: peerID), + context.trustedNicknameMismatch(fingerprint) else { return status } + return .noiseSecured } @MainActor diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index 660c8e6c..b2d9f48c 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -1973,27 +1973,13 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, SynchronousMessage identityManager.isVouched(fingerprint: fingerprint) } - /// Whether this peer now claims a different nickname than the one its - /// trust was established under. Trust badges are suppressed in that case: - /// the seal attests to a key, but it is read next to a name, and a key that - /// renamed itself onto a name the user trusts is the impersonation this - /// warns about. + /// Whether this peer now announces a different nickname than the one its + /// trust was established under. Every seal is suppressed in that case: the + /// seal attests to a key, but it is read as a name, and a key that renamed + /// itself onto a name the user trusts is the impersonation this catches. @MainActor - func trustedNicknameMismatch(_ fingerprint: String, claimedNickname: String) -> Bool { - identityManager.trustedNicknameMismatch( - fingerprint: fingerprint, - claimedNickname: claimedNickname - ) - } - - /// The same check for a name as *rendered* in a message row. See the - /// `displayedSender` overload on `SecureIdentityStateManagerProtocol`. - @MainActor - func trustedNicknameMismatch(_ fingerprint: String, displayedSender: String) -> Bool { - identityManager.trustedNicknameMismatch( - fingerprint: fingerprint, - displayedSender: displayedSender - ) + func trustedNicknameMismatch(_ fingerprint: String) -> Bool { + identityManager.trustedNicknameMismatch(fingerprint: fingerprint) } // MARK: - BitchatDelegate Methods diff --git a/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift b/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift index 0bfcd306..d6e69821 100644 --- a/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift +++ b/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift @@ -144,6 +144,12 @@ private final class MockChatPeerIdentityContext: ChatPeerIdentityContext { verifiedFingerprintSet.contains(fingerprint) } + var renamedSinceTrust: Set = [] + + func trustedNicknameMismatch(_ fingerprint: String) -> Bool { + renamedSinceTrust.contains(fingerprint) + } + func setEncryptionStatus(_ status: EncryptionStatus?, for peerID: PeerID) { encryptionStatuses[peerID] = status } @@ -331,6 +337,52 @@ struct ChatPeerIdentityCoordinatorContextTests { #expect(context.unreadPrivateMessages.isEmpty) } + @Test @MainActor + func getEncryptionStatus_demotesToSecuredWhenTheKeyRenamedSinceTrust() async { + // `.noiseVerified` renders the same filled seal as the verified badge, + // and for a CONNECTED peer it is the only thing that draws one. A + // verified key that renames itself onto a trusted name must therefore + // lose it here too, or the live impersonation keeps its seal. + let context = MockChatPeerIdentityContext() + let coordinator = ChatPeerIdentityCoordinator(context: context) + let peerID = PeerID(str: "1122334455667788") + + context.noiseSessionStates[peerID] = .established + context.fingerprintsByPeerID[peerID] = "fp" + context.verifiedFingerprintSet = ["fp"] + #expect(coordinator.getEncryptionStatus(for: peerID) == .noiseVerified) + + // The rename lands. Nothing invalidates the encryption cache on a + // nickname change, so the demotion has to survive the cached value. + context.renamedSinceTrust = ["fp"] + #expect(coordinator.getEncryptionStatus(for: peerID) == .noiseSecured, + "still encrypted, no longer a claim about who") + #expect(context.cachedEncryptionStatuses[peerID] == .noiseVerified, + "the demotion is computed on read, never written into the cache") + + // Re-verifying under the new name restores it. + context.renamedSinceTrust = [] + #expect(coordinator.getEncryptionStatus(for: peerID) == .noiseVerified) + } + + @Test @MainActor + func getEncryptionStatus_leavesUnverifiedStatusesAlone() async { + let context = MockChatPeerIdentityContext() + let coordinator = ChatPeerIdentityCoordinator(context: context) + let peerID = PeerID(str: "1122334455667788") + + // A secured-but-unverified session has no seal to lose, and a rename + // must not disturb the lock it does show. + context.noiseSessionStates[peerID] = .established + context.fingerprintsByPeerID[peerID] = "fp" + context.renamedSinceTrust = ["fp"] + #expect(coordinator.getEncryptionStatus(for: peerID) == .noiseSecured) + + coordinator.invalidateEncryptionCache(for: peerID) + context.noiseSessionStates[peerID] = .handshaking + #expect(coordinator.getEncryptionStatus(for: peerID) == .noiseHandshaking) + } + @Test @MainActor func getEncryptionStatus_computesVerifiedStatusAndCachesIt() async { let context = MockChatPeerIdentityContext() diff --git a/bitchatTests/Mocks/MockIdentityManager.swift b/bitchatTests/Mocks/MockIdentityManager.swift index c5fa97df..f0e1b656 100644 --- a/bitchatTests/Mocks/MockIdentityManager.swift +++ b/bitchatTests/Mocks/MockIdentityManager.swift @@ -100,10 +100,12 @@ final class MockIdentityManager: SecureIdentityStateManagerProtocol { var trustedNicknames: [String: String] = [:] - func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool { + var claimedNicknames: [String: String] = [:] + + func trustedNicknameMismatch(fingerprint: String) -> Bool { guard let pinned = trustedNicknames[fingerprint], !pinned.isEmpty, - !claimedNickname.isEmpty else { return false } - return pinned != claimedNickname + let claimed = claimedNicknames[fingerprint], !claimed.isEmpty else { return false } + return pinned != claimed } func isVerified(fingerprint: String) -> Bool { diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index 5a76c6eb..33945919 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -1,13 +1,14 @@ import Foundation +import BitFoundation import Testing @testable import bitchat -/// Binds a trust badge to the nickname it was earned under. +/// Binds a trust seal to the nickname it was earned under. /// /// `VouchAttestation` signs `voucheeFingerprint | voucheeSigningKey | /// timestampMs` and deliberately says nothing about a name — a name-free -/// attestation is the right wire format. But the badge is *rendered* beside a +/// attestation is the right wire format. But the seal is *rendered* beside a /// self-claimed nickname, so the binding has to live on the receiver, which is /// the only party that knows what name the key was presenting when it decided /// to trust it. These tests pin that rule. @@ -37,18 +38,24 @@ struct SecureIdentityStateManagerNicknameBindingTests { ) } + private func setPetname(_ manager: SecureIdentityStateManager, + _ fingerprint: String, + _ petname: String?) { + guard var identity = manager.getSocialIdentity(for: fingerprint) else { return } + identity.localPetname = petname + manager.updateSocialIdentity(identity) + } + // MARK: - The attack this closes @Test - func vouch_pinsTheNicknameItWasEarnedUnder() { + func vouchPinsTheNicknameItWasEarnedUnder() { let manager = makeManager() announce(manager, vouchee, as: "ravi") manager.setVerified(fingerprint: voucher, verified: true) #expect(manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date())) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi")) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), - "the baseline is pinned: any other name is a mismatch") + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) } @Test @@ -58,14 +65,11 @@ struct SecureIdentityStateManagerNicknameBindingTests { manager.setVerified(fingerprint: voucher, verified: true) manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) - // Same key, new self-chosen name — the badge must not follow it. announce(manager, vouchee, as: "medic") #expect(manager.isVouched(fingerprint: vouchee), "the vouch itself is still valid; only its binding to a name broke") - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), - "the baseline stayed on the name the vouch was earned under") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) } @Test @@ -76,12 +80,10 @@ struct SecureIdentityStateManagerNicknameBindingTests { manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) announce(manager, vouchee, as: "medic") - // A second vouch arriving after the rename must not launder it. manager.setVerified(fingerprint: secondVoucher, verified: true) manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: secondVoucher, timestamp: Date()) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee), "the second vouch did not launder the rename") } @@ -95,21 +97,20 @@ struct SecureIdentityStateManagerNicknameBindingTests { let manager = makeManager() manager.setVerified(fingerprint: voucher, verified: true) manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), "nothing is bound yet, so nothing can mismatch") announce(manager, vouchee, as: "ravi") - announce(manager, vouchee, as: "medic") + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), - "the first name seen while the vouch stood is the binding") + announce(manager, vouchee, as: "medic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) } @Test func theAnnouncePathDoesNotBindBeforeTrustExists() { // Otherwise a peer who renamed BEFORE being vouched would be bound to - // the name we happened to see first, and lose a legitimate badge. + // the name we happened to see first, and lose a legitimate seal. let manager = makeManager() announce(manager, vouchee, as: "rav") announce(manager, vouchee, as: "ravi") @@ -117,9 +118,8 @@ struct SecureIdentityStateManagerNicknameBindingTests { manager.setVerified(fingerprint: voucher, verified: true) manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), "bound to the name it was vouched under, not the name seen first") - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "rav")) } @Test @@ -134,10 +134,67 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "medic") - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), "known gap, pinned here so it is explicit rather than a surprise") } + // MARK: - Not fooled by display decoration + + @Test + func theCheckIsUnaffectedByCollisionSuffixes() { + // `PeerDisplayNameResolver` renders two CONNECTED peers who both claim + // "medic" as "medic#a1b2" and "medic#c3d4" — which is exactly what + // happens during this attack. Comparing a rendered name would drop the + // real medic's seal at the worst possible moment, so the check reads + // announced names only. This pins that the resolver really does + // decorate, and that the binding ignores it. + let real = PeerDisplayNameResolver.resolve( + [(peerID: PeerID(str: "a1b2c3d4"), nickname: "medic", isConnected: true), + (peerID: PeerID(str: "c3d4e5f6"), nickname: "medic", isConnected: true)], + selfNickname: "me") + #expect(real[PeerID(str: "a1b2c3d4")] == "medic#a1b2", "the resolver does decorate") + + let manager = makeManager() + announce(manager, vouchee, as: "medic") + manager.setVerified(fingerprint: vouchee, verified: true) + announce(manager, vouchee, as: "medic") // still "medic" on the wire + + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), + "the impersonated peer keeps its seal while a namesake is connected") + } + + @Test + func aLocalPetnameKeepsTheSeal() { + // A petname outranks the claimed nickname everywhere it is displayed, + // so a rename cannot spoof anything and the seal stands. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + announce(manager, vouchee, as: "medic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) + + setPetname(manager, vouchee, "my neighbour") + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) + + setPetname(manager, vouchee, nil) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) + } + + @Test + func namesAreComparedInCanonicalForm() { + // Same rule as `normalizedNickname` everywhere else: a decomposed and a + // precomposed "café" are one name, not a rename. + let manager = makeManager() + announce(manager, vouchee, as: "cafe\u{0301}") // e + combining acute + manager.setVerified(fingerprint: vouchee, verified: true) + + announce(manager, vouchee, as: "caf\u{00E9}") // precomposed é + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) + + announce(manager, vouchee, as: "cafe") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) + } + // MARK: - Rebinding and clearing @Test @@ -147,13 +204,12 @@ struct SecureIdentityStateManagerNicknameBindingTests { manager.setVerified(fingerprint: voucher, verified: true) manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) announce(manager, vouchee, as: "medic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) // The user scanned this key themselves, under the name it shows now. manager.setVerified(fingerprint: vouchee, verified: true) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi"), - "the baseline moved to the name just checked in person") + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) } @Test @@ -162,71 +218,39 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "ravi") manager.setVerified(fingerprint: vouchee, verified: true) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) + announce(manager, vouchee, as: "medic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) + manager.setVerified(fingerprint: vouchee, verified: false) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), "with no trust left there is no baseline, so nothing to mismatch") // With a vouch still standing, the baseline has to survive: it is what - // that vouch's badge is bound to. + // that vouch's seal is bound to. + announce(manager, vouchee, as: "ravi") manager.setVerified(fingerprint: voucher, verified: true) manager.recordVouch(voucheeFingerprint: vouchee, voucherFingerprint: voucher, timestamp: Date()) manager.setVerified(fingerprint: vouchee, verified: true) manager.setVerified(fingerprint: vouchee, verified: false) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic"), - "the vouch's badge still needs the name it was bound to") - } - - // MARK: - Names as rendered in a message row - - @Test - func aDisambiguationSuffixIsNotMistakenForARename() { - // Message senders render as "ravi#a1b2" when nicknames collide; the - // announced nickname never carries the suffix. - let manager = makeManager() - announce(manager, vouchee, as: "ravi") - manager.setVerified(fingerprint: vouchee, verified: true) - - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi#a1b2")) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "@ravi#a1b2")) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi")) - } - - @Test - func aRenameIsStillCaughtThroughTheSuffix() { - let manager = makeManager() - announce(manager, vouchee, as: "ravi") - manager.setVerified(fingerprint: vouchee, verified: true) - - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "medic#a1b2")) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "medic")) - } - - @Test - func aNameEndingInSomethingSuffixLikeIsNotTruncated() { - // "#abcd" only counts as a suffix when those four characters are hex; - // a nickname that merely contains '#' must still compare whole. - let manager = makeManager() - announce(manager, vouchee, as: "ravi#zzzz") - manager.setVerified(fingerprint: vouchee, verified: true) - - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi#zzzz")) - #expect(manager.trustedNicknameMismatch(fingerprint: vouchee, displayedSender: "ravi")) + announce(manager, vouchee, as: "medic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee), + "the vouch's seal still needs the name it was bound to") } // MARK: - Failing open @Test - func aMissingBaselineNeverSuppressesABadge() { + func aMissingBaselineNeverSuppressesASeal() { // Verified before the peer ever announced a name — there is nothing to // bind to, and pinning "" would read as a mismatch against every later // announce. Peers trusted by builds before this shipped land here too, - // so an upgrade must not silently drop their badges. + // so an upgrade must not silently drop their seals. let manager = makeManager() manager.setVerified(fingerprint: vouchee, verified: true) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "medic")) - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "ravi")) + announce(manager, vouchee, as: "medic") + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) } @Test @@ -234,8 +258,9 @@ struct SecureIdentityStateManagerNicknameBindingTests { let manager = makeManager() announce(manager, vouchee, as: "ravi") manager.setVerified(fingerprint: vouchee, verified: true) + announce(manager, vouchee, as: "") - #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee, claimedNickname: "")) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) } // MARK: - Persistence compatibility diff --git a/bitchatTests/Services/UnifiedPeerServiceTests.swift b/bitchatTests/Services/UnifiedPeerServiceTests.swift index 7dfae43b..0176634a 100644 --- a/bitchatTests/Services/UnifiedPeerServiceTests.swift +++ b/bitchatTests/Services/UnifiedPeerServiceTests.swift @@ -291,7 +291,7 @@ private final class TestIdentityManager: SecureIdentityStateManagerProtocol { // No name binding is exercised here: these tests drive peer-list assembly, // not trust display. - func trustedNicknameMismatch(fingerprint: String, claimedNickname: String) -> Bool { false } + func trustedNicknameMismatch(fingerprint: String) -> Bool { false } // MARK: Vouching (unused by these tests) From a008850c9ea5abe2f58a9db8ab17911594596312 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Thu, 10 Sep 2026 13:05:30 +0800 Subject: [PATCH 3/8] Check a message row against its own name, and gate the vouched panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things from review. Two were bugs, one was a miss, one was an overstated claim of mine. **A row could get its seal back by renaming away and back.** Moving off the rendered name fixed three false positives and introduced this: a message row renders `message.sender`, frozen at receipt, while the gate asked about the peer's CURRENT name. So verify as "ravi", announce "medic", post to the public timeline, announce "ravi" again — the live name is bound once more and the archived row renders `<@medic ✓>`, a seal beside a name the key was never trusted under. Exactly what this PR exists to stop. The fix is not to check both names but to notice that a live row and a historical row ask different questions: live row (peer list, DM header, encryption status) is the name this peer is announcing NOW the one it was verified under? message row (public timeline, DM rows) is the name ON THIS ROW the one it was verified under? So `sealAppliesToRow(fingerprint:renderedSender:)` compares the row's own sender against the pinned baseline and never consults the live name. That kills the rename-back, and it also makes each row self-consistent in the other direction: a historical "ravi ✓" stays sealed while that key is currently renamed, because that row really was ravi. Requiring both names to match would have retroactively unsealed accurate history on every ordinary rename. Comparing a rendered name again needs the `#abcd` collision suffix stripped, which is what pushed me off it before — `splitSuffix()` also removes every "@", and nothing in `validateNickname` forbids one, so "ravi@hq" compared unequal to itself. Hence `withoutCollisionSuffix`, which strips only a trailing "#" plus four hex digits and touches nothing else. **The vouched panel in the fingerprint sheet was ungated** — `FingerprintView.swift:192`, the teal hollow seal with "vouched by N" and the voucher names, read `voucherCount` and never consulted the binding. That is the seal this PR is named for, in the sheet I had singled out as the place the distinction should be explained. `voucherCount` and `voucherNames` are now suppressed together: a voucher list beside a name nobody vouched for is the same claim as the seal. **Correcting my own claim.** I wrote that nothing branches on `.noiseVerified` beyond the glyph and that every behavioural site treats it and `.noiseSecured` alike. That was 3-for-4. `FingerprintView.swift:87` tints the status icon `== .noiseVerified ? .green : textColor`, and `accessibilityDescription` differs per case, so VoiceOver stops saying "verified" on a demoted DM header. Both are the right outcome, but "nothing moves" was wrong and the sheet now shows a plain lock above a bold green "verified" until the copy lands. **Collision counting now folds.** `PeerDisplayNameResolver` counted collisions on the displayed name, so the suffix that exists to make namesakes distinguishable stayed silent for a case variant or a fullwidth Ｍ. The count key is now `precomposedStringWithCompatibilityMapping.lowercased()` — one line, no new data — which closes case, fullwidth, mathematical, script and roman numeral forms, and subsumes the canonical folding Swift's String keys were giving for free. Cross-script homoglyphs still need a real table and are pinned as a known gap with a test to flip. The displayed name is never folded. Also: both seal sites took a `queue.sync` via `isVerified` and then a second for the binding, and in the formatter the check feeds the format cache key so the cache does not shield it. Both are one call now. The two derived queries are protocol requirements with default implementations, so the concrete manager answers in a single lock while conformances only supply the primitives. Tests: 6 new for `PeerDisplayNameResolver`, which had none, including the known-gap rows; 5 more in the binding suite for the per-row rule — the rename-back, a historical row keeping its seal, suffix and "@" handling, and the fail-open cases. With the row check reverted to the live name, 3 of them fail. Suites here: 187 pass / 1908 tests, no result differing from main; 34 still need an app bundle this machine cannot provide. --- bitchat/App/ConversationUIModel.swift | 10 ++- bitchat/App/VerificationModel.swift | 24 +++++- .../Identity/SecureIdentityStateManager.swift | 85 ++++++++++++++++--- bitchat/Utils/PeerDisplayNameResolver.swift | 25 +++++- bitchat/Utils/String+Nickname.swift | 15 ++++ bitchat/ViewModels/ChatMessageFormatter.swift | 8 +- bitchat/ViewModels/ChatViewModel.swift | 14 +++ bitchatTests/Mocks/MockIdentityManager.swift | 2 + ...tityStateManagerNicknameBindingTests.swift | 85 +++++++++++++++++++ .../Services/UnifiedPeerServiceTests.swift | 1 + .../Utils/PeerDisplayNameResolverTests.swift | 82 ++++++++++++++++++ 11 files changed, 327 insertions(+), 24 deletions(-) create mode 100644 bitchatTests/Utils/PeerDisplayNameResolverTests.swift diff --git a/bitchat/App/ConversationUIModel.swift b/bitchat/App/ConversationUIModel.swift index 508ca493..a201eba6 100644 --- a/bitchat/App/ConversationUIModel.swift +++ b/bitchat/App/ConversationUIModel.swift @@ -164,10 +164,12 @@ final class ConversationUIModel: ObservableObject { !isSentByCurrentUser(message), let peerID = message.senderPeerID else { return false } guard let fingerprint = chatViewModel.getFingerprint(for: peerID) else { return false } - guard chatViewModel.peerIdentityStore.isVerified(fingerprint) else { return false } - // A verified key that renamed itself onto a name the user trusts must - // not carry the seal beside the new one. - return !chatViewModel.trustedNicknameMismatch(fingerprint) + // The row renders `message.sender`, frozen at receipt, so the question + // is whether THAT name is the one this key was verified under — not + // 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) } func senderDisplayName(for peerID: PeerID, fallbackMessages: [BitchatMessage]) -> String? { diff --git a/bitchat/App/VerificationModel.swift b/bitchat/App/VerificationModel.swift index 4e1de457..992713c2 100644 --- a/bitchat/App/VerificationModel.swift +++ b/bitchat/App/VerificationModel.swift @@ -16,6 +16,15 @@ struct FingerprintPresentationState: Equatable { let voucherCount: Int /// Display names of the (verified) vouchers, where known. let voucherNames: [String] + /// The nickname this key was announcing when it was verified or first + /// vouched, if anything was bound. Non-nil and different from the name on + /// screen is what `nameChangedSinceVerification` reports. + let verifiedAsNickname: String? + /// The key is trusted, but it now announces a different name than when + /// that trust was established. The sheet is the one place this should be + /// explained rather than collapsed into a glyph, so it is surfaced here + /// instead of silently suppressing the badge. + let nameChangedSinceVerification: Bool /// Vouched for by ≥1 peer the user verified (and not explicitly verified). var isVouched: Bool { voucherCount > 0 } @@ -136,6 +145,13 @@ final class VerificationModel: ObservableObject { let isVerified = theirFingerprint.map { peerIdentityStore.isVerified($0) } ?? false let localPetname = theirFingerprint .flatMap { chatViewModel.identityManager.getSocialIdentity(for: $0)?.localPetname } + // A vouched seal is bound to the name it was earned under, exactly like + // the verified one. Missing baseline means nothing was bound, so this + // fails open like every other site. + let nameBound = theirFingerprint + .map { !chatViewModel.identityManager.trustedNicknameMismatch(fingerprint: $0) } ?? true + let verifiedAsNickname = theirFingerprint + .flatMap { chatViewModel.identityManager.trustedNickname(fingerprint: $0) } // Vouch state is recomputed on read: only vouchers still in the // verified set count, so removing a verification silently retires the @@ -161,8 +177,12 @@ final class VerificationModel: ObservableObject { myFingerprint: chatViewModel.getMyFingerprint(), isVerified: isVerified, localPetname: localPetname, - voucherCount: vouchers.count, - voucherNames: voucherNames + // Suppressed together: a voucher list beside a name nobody + // vouched for is the same claim as the seal itself. + voucherCount: nameBound ? vouchers.count : 0, + voucherNames: nameBound ? voucherNames : [], + verifiedAsNickname: verifiedAsNickname, + nameChangedSinceVerification: !nameBound ) } diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index 91b82624..622d9c58 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -130,9 +130,19 @@ protocol SecureIdentityStateManagerProtocol { func setVerified(fingerprint: String, verified: Bool) func isVerified(fingerprint: String) -> Bool func getVerifiedFingerprints() -> Set + /// The nickname `fingerprint` was announcing when trust in it was + /// established, or nil if nothing was bound. Shown to the user, so a sheet + /// can say *which* name was verified rather than only that one changed. + func trustedNickname(fingerprint: String) -> String? /// Whether this peer now announces a different nickname than the one its /// trust was earned under. func trustedNicknameMismatch(fingerprint: String) -> Bool + /// Verified AND still presenting the name it was verified under — what a + /// seal beside a LIVE name is actually asserting. One lock, not two. + func isVerifiedAndNameBound(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 // MARK: Vouching (transitive verification) @discardableResult @@ -153,6 +163,26 @@ protocol SecureIdentityStateManagerProtocol { func hasObservedPrivateMediaCapability(fingerprint: String) -> Bool } +extension SecureIdentityStateManagerProtocol { + // Default implementations so a conformance only has to provide the two + // primitives. `SecureIdentityStateManager` overrides both to answer in a + // single lock acquisition — these seal checks run per row, per render, and + // the format cache does not shield the one in ChatMessageFormatter because + // the answer is part of its cache key. + func isVerifiedAndNameBound(fingerprint: String) -> Bool { + isVerified(fingerprint: fingerprint) && !trustedNicknameMismatch(fingerprint: fingerprint) + } + + func sealAppliesToRow(fingerprint: String, renderedSender: String) -> 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.normalizedNickname == shown.normalizedNickname + } +} + /// Singleton manager for secure identity state persistence and retrieval. /// Provides thread-safe access to identity mappings with encryption at rest. /// All identity data is stored encrypted in the device Keychain for security. @@ -765,19 +795,54 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { /// before this existed have none, and dropping their seals on upgrade would /// train users to ignore the signal. func trustedNicknameMismatch(fingerprint: String) -> Bool { + queue.sync { liveNameMismatchLocked(fingerprint) } + } + + func trustedNickname(fingerprint: String) -> String? { + queue.sync { cache.trustedNicknames?[fingerprint] } + } + + func isVerifiedAndNameBound(fingerprint: String) -> Bool { queue.sync { - guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty - else { return false } - let social = cache.socialIdentities[fingerprint] - // A local petname outranks the claimed nickname everywhere it is - // displayed, so there is nothing to spoof and the seal stands. - if let petname = social?.localPetname, !petname.isEmpty { return false } - guard let claimed = social?.claimedNickname, !claimed.isEmpty else { return false } - // NFC, matching `normalizedNickname` everywhere else nicknames are - // compared: a decomposed and a precomposed "café" are one name. - return pinned.normalizedNickname != claimed.normalizedNickname + guard cache.verifiedFingerprints.contains(fingerprint) else { return false } + return !liveNameMismatchLocked(fingerprint) } } + + /// A message row shows the sender name frozen at receipt, so asking about + /// the peer's CURRENT name is the wrong question: rename away, post, rename + /// back, and the live name matches the baseline again while the archived row + /// still reads the name it was posted under. Each row is therefore checked + /// 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 { + queue.sync { + guard cache.verifiedFingerprints.contains(fingerprint) else { return false } + guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty + else { return true } // nothing bound: fail open + // 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.normalizedNickname == shown.normalizedNickname + } + } + + /// Requires `queue`. + private func liveNameMismatchLocked(_ fingerprint: String) -> Bool { + guard let pinned = cache.trustedNicknames?[fingerprint], !pinned.isEmpty + else { return false } + let social = cache.socialIdentities[fingerprint] + // A local petname outranks the claimed nickname everywhere a LIVE name + // is displayed, so there is nothing to spoof and the seal stands. + if let petname = social?.localPetname, !petname.isEmpty { return false } + guard let claimed = social?.claimedNickname, !claimed.isEmpty else { return false } + // NFC, matching `normalizedNickname` everywhere else nicknames are + // compared: a decomposed and a precomposed "café" are one name. + return pinned.normalizedNickname != claimed.normalizedNickname + } func getVerifiedFingerprints() -> Set { queue.sync { diff --git a/bitchat/Utils/PeerDisplayNameResolver.swift b/bitchat/Utils/PeerDisplayNameResolver.swift index a92953d3..cf6ec094 100644 --- a/bitchat/Utils/PeerDisplayNameResolver.swift +++ b/bitchat/Utils/PeerDisplayNameResolver.swift @@ -3,23 +3,40 @@ import BitFoundation /// Resolves a stable display name for peers, adding a short suffix when collisions exist. struct PeerDisplayNameResolver { + /// The form two nicknames are judged to collide in. Never displayed. + static func collisionKey(_ nickname: String) -> String { + nickname.precomposedStringWithCompatibilityMapping.lowercased() + } + /// Computes display names with a `#xxxx` suffix for connected peers when nickname collisions occur. /// - Parameters: /// - peers: Array of tuples (peerID, nickname, isConnected). /// - selfNickname: The local user's current nickname, included in collision counts to suffix remotes matching it. /// - Returns: Map of peerID -> displayName. static func resolve(_ peers: [(peerID: PeerID, nickname: String, isConnected: Bool)], selfNickname: String) -> [PeerID: String] { - // Count collisions among connected peers and include our own nickname + // Count collisions on a FOLDED key, not the displayed name. The point + // of the suffix is to make namesakes distinguishable, so anything that + // renders alike has to count as a collision. + // + // Compatibility mapping plus case folding covers more than it looks: + // fullwidth Ｍedic, mathematical 𝐌edic, script ℳedic and roman-numeral + // Ⅿedic all fold to "medic", and NFKC subsumes the canonical folding + // that Swift's String keys were already giving us for free. + // + // What it does NOT cover is cross-script homoglyphs — Cyrillic М and + // Greek Ο have their own compatibility forms — which needs a UTS #39 + // confusables table. Suffixing the displayed name is unchanged; only + // the collision key folds. var counts: [String: Int] = [:] for p in peers where p.isConnected { - counts[p.nickname, default: 0] += 1 + counts[Self.collisionKey(p.nickname), default: 0] += 1 } - counts[selfNickname, default: 0] += 1 + counts[Self.collisionKey(selfNickname), default: 0] += 1 var result: [PeerID: String] = [:] for p in peers { var name = p.nickname - if p.isConnected, (counts[p.nickname] ?? 0) > 1 { + if p.isConnected, (counts[Self.collisionKey(p.nickname)] ?? 0) > 1 { name += "#" + String(p.peerID.id.prefix(4)) } result[p.peerID] = name diff --git a/bitchat/Utils/String+Nickname.swift b/bitchat/Utils/String+Nickname.swift index 9a652667..2902613f 100644 --- a/bitchat/Utils/String+Nickname.swift +++ b/bitchat/Utils/String+Nickname.swift @@ -17,6 +17,21 @@ extension String { precomposedStringWithCanonicalMapping } + /// Strips ONLY a trailing `#abcd` collision suffix, leaving everything + /// else alone. + /// + /// Deliberately not `splitSuffix()`: that also removes every `@` in the + /// string, which is right for parsing a mention but wrong for comparing a + /// nickname — nothing in `validateNickname` forbids `@`, so `ravi@hq` + /// would compare unequal to itself. + var withoutCollisionSuffix: String { + guard count >= 5 else { return self } + let tail = suffix(5) + guard tail.first == "#", + tail.dropFirst().allSatisfy({ $0.isHexDigit }) else { return self } + return String(dropLast(5)) + } + /// 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 bb661a35..6b5f9899 100644 --- a/bitchat/ViewModels/ChatMessageFormatter.swift +++ b/bitchat/ViewModels/ChatMessageFormatter.swift @@ -448,10 +448,10 @@ private extension ChatMessageFormatter { let fingerprint = viewModel.getFingerprint(for: peerID) else { return false } - guard viewModel.peerIdentityStore.isVerified(fingerprint) else { return false } - // Bound to the name the key announces, for the same reason as the peer - // list and the DM seal: the badge attests to a key but is read as a name. - return !viewModel.trustedNicknameMismatch(fingerprint) + // 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) } func appendVerifiedSeal( diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index b2d9f48c..9e098ff9 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -1982,6 +1982,20 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, SynchronousMessage identityManager.trustedNicknameMismatch(fingerprint: fingerprint) } + /// The name this key was verified under, for a sheet that needs to say + /// which one it was. + @MainActor + func trustedNickname(_ fingerprint: String) -> String? { + identityManager.trustedNickname(fingerprint: fingerprint) + } + + /// 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) + } + // MARK: - BitchatDelegate Methods // MARK: - Command Handling diff --git a/bitchatTests/Mocks/MockIdentityManager.swift b/bitchatTests/Mocks/MockIdentityManager.swift index f0e1b656..2e1fc12d 100644 --- a/bitchatTests/Mocks/MockIdentityManager.swift +++ b/bitchatTests/Mocks/MockIdentityManager.swift @@ -102,6 +102,8 @@ final class MockIdentityManager: SecureIdentityStateManagerProtocol { var claimedNicknames: [String: String] = [:] + func trustedNickname(fingerprint: String) -> String? { trustedNicknames[fingerprint] } + func trustedNicknameMismatch(fingerprint: String) -> Bool { guard let pinned = trustedNicknames[fingerprint], !pinned.isEmpty, let claimed = claimedNicknames[fingerprint], !claimed.isEmpty else { return false } diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index 33945919..060a2693 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -195,6 +195,91 @@ struct SecureIdentityStateManagerNicknameBindingTests { #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) } + // MARK: - A seal beside a name frozen on a message row + + @Test + func renamingBackDoesNotRestoreTheSealOnARowPostedUnderTheOtherName() { + // The hole in checking only the CURRENT name: a message row renders + // `message.sender` frozen at receipt, so rename away, post, rename + // back, and the live name matches the baseline again while the archived + // row still reads the name it was posted under. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + + announce(manager, vouchee, as: "medic") // rename + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) + announce(manager, vouchee, as: "ravi") // ...and back + #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"), + "the row posted as medic must not be sealed") + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi"), + "a row posted as ravi still is — that row really was ravi") + } + + @Test + func aHistoricalRowKeepsItsSealWhileTheKeyIsCurrentlyRenamed() { + // The converse, so the rule is not simply "suppress harder": a row + // posted under the verified name stays truthful even once that key has + // moved on, otherwise ordinary renames retroactively unseal history. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + announce(manager, vouchee, as: "medic") + + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee), "live name is unbound") + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi"), + "but the archived ravi row is still accurate") + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic")) + } + + @Test + func aRowSenderCarryingACollisionSuffixIsStillMatched() { + // Message senders render as "ravi#a1b2" when nicknames collide. Only a + // trailing #abcd is stripped — `splitSuffix()` would also remove every + // "@", and nothing forbids one in a nickname. + let manager = makeManager() + 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")) + } + + @Test + func aRowIsNotSealedForAnUnverifiedKeyAndFailsOpenWithNoBaseline() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "ravi"), + "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")) + } + + @Test + func theCombinedLiveQueryAgreesWithItsParts() { + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + #expect(!manager.isVerifiedAndNameBound(fingerprint: vouchee), "not verified yet") + + manager.setVerified(fingerprint: vouchee, verified: true) + #expect(manager.isVerifiedAndNameBound(fingerprint: vouchee)) + #expect(manager.trustedNickname(fingerprint: vouchee) == "ravi") + + announce(manager, vouchee, as: "medic") + #expect(!manager.isVerifiedAndNameBound(fingerprint: vouchee)) + #expect(manager.trustedNickname(fingerprint: vouchee) == "ravi", + "the baseline is what the sheet needs to name") + } + // MARK: - Rebinding and clearing @Test diff --git a/bitchatTests/Services/UnifiedPeerServiceTests.swift b/bitchatTests/Services/UnifiedPeerServiceTests.swift index 0176634a..ec604c23 100644 --- a/bitchatTests/Services/UnifiedPeerServiceTests.swift +++ b/bitchatTests/Services/UnifiedPeerServiceTests.swift @@ -291,6 +291,7 @@ private final class TestIdentityManager: SecureIdentityStateManagerProtocol { // No name binding is exercised here: these tests drive peer-list assembly, // not trust display. + func trustedNickname(fingerprint: String) -> String? { nil } func trustedNicknameMismatch(fingerprint: String) -> Bool { false } // MARK: Vouching (unused by these tests) diff --git a/bitchatTests/Utils/PeerDisplayNameResolverTests.swift b/bitchatTests/Utils/PeerDisplayNameResolverTests.swift new file mode 100644 index 00000000..10a2ee48 --- /dev/null +++ b/bitchatTests/Utils/PeerDisplayNameResolverTests.swift @@ -0,0 +1,82 @@ +import Foundation +import BitFoundation +import Testing + +@testable import bitchat + +/// The `#abcd` suffix exists so two peers claiming one nickname are +/// distinguishable. It is therefore only as good as the notion of "one +/// nickname" it counts with — anything that renders alike has to collide, or +/// the guard stays silent exactly when two rows look the same. +@MainActor +struct PeerDisplayNameResolverTests { + private let real = PeerID(str: "aaaa1111") + private let other = PeerID(str: "bbbb2222") + + /// True when the resolver decided these two need telling apart. + private func suffixed(_ a: String, _ b: String, + connected: Bool = true, selfNickname: String = "me") -> Bool { + let out = PeerDisplayNameResolver.resolve( + [(peerID: real, nickname: a, isConnected: true), + (peerID: other, nickname: b, isConnected: connected)], + selfNickname: selfNickname) + return (out[real] ?? "").contains("#") || (out[other] ?? "").contains("#") + } + + @Test + func namesThatRenderAlikeCollide() { + #expect(suffixed("Medic", "Medic"), "exact duplicate") + #expect(suffixed("Medic", "medic"), "case alone must not make two peers look distinct") + #expect(suffixed("Medic", "\u{FF2D}edic"), "fullwidth Ｍ") + #expect(suffixed("Medic", "\u{1D40C}edic"), "mathematical bold 𝐌") + #expect(suffixed("Medic", "\u{2133}edic"), "script ℳ") + #expect(suffixed("Medic", "\u{216F}edic"), "roman numeral Ⅿ") + #expect(suffixed("Jos\u{00E9}", "Jose\u{0301}"), "canonically equivalent spellings") + } + + @Test + func unrelatedNamesDoNotCollide() { + #expect(!suffixed("Medic", "Zebra")) + #expect(!suffixed("Medic", "Medic2")) + #expect(!suffixed("Medic", "")) + } + + @Test + func crossScriptHomoglyphsAreStillMissed() { + // Cyrillic М and Greek Ο have their own compatibility forms, so + // folding cannot reach them — that needs a UTS #39 confusables table. + // Pinned as a KNOWN gap so it is explicit rather than a surprise, and + // so whoever adds the table has a test to flip. + #expect(!suffixed("Medic", "\u{041C}\u{0435}dic"), "Cyrillic М + е: known gap") + #expect(!suffixed("Organizer", "\u{039F}rganizer"), "Greek Ο: known gap") + } + + @Test + func onlyConnectedPeersAreSuffixed() { + // Unchanged by the folding: an offline namesake still gets no suffix, + // which is its own gap and not one this touches. + #expect(!suffixed("Medic", "medic", connected: false)) + } + + @Test + func ourOwnNicknameCountsTowardCollisions() { + // A remote peer claiming a folded variant of MY nickname must be + // suffixed, or it renders as me. + let out = PeerDisplayNameResolver.resolve( + [(peerID: other, nickname: "\u{FF2D}edic", isConnected: true)], + selfNickname: "Medic") + #expect((out[other] ?? "").contains("#")) + } + + @Test + func theDisplayedNameIsNeverFolded() { + // Only the collision KEY folds. What is shown stays as its owner typed + // it, suffix aside. + let out = PeerDisplayNameResolver.resolve( + [(peerID: real, nickname: "Medic", isConnected: true), + (peerID: other, nickname: "\u{FF2D}edic", isConnected: true)], + selfNickname: "me") + #expect(out[real]?.hasPrefix("Medic") == true) + #expect(out[other]?.hasPrefix("\u{FF2D}edic") == true, "the fullwidth Ｍ is preserved on screen") + } +} From ff39744580c24b163387b4c8748f8ab5af0556f5 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Thu, 10 Sep 2026 13:29:35 +0800 Subject: [PATCH 4/8] Self-audit: fold case in the binding, and fix the second collision counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in my own previous commit, found auditing it rather than waiting for review. Each has a test that fails without its fix. **Recasing your own nickname broke the binding.** The comparison was NFC only, so "Ravi" to "ravi" read as a rename and silently dropped the seal. Renames are meant to break the binding; recasing is not a rename. There is now an explicit `nicknameBindingKey` — NFC, case fold, NFC again, since case folding can itself emit decomposed sequences. It is deliberately NFC and NOT NFKC. A fullwidth `Ｍedic` merely *looks* like `Medic`, so it is a different name and must break the binding; folding look-alikes is `collisionKey`'s job, which answers the different question of whether two peers need telling apart. Both directions are now tested, because getting this backwards is how a vouch for one name comes to cover another. **`withoutCollisionSuffix` stripped more than this device generates.** `Character.isHexDigit` is true for fullwidth digits, so a nickname literally ending in "#ＡＢＣＤ" was truncated and could then match a baseline it is not. Now ASCII-only, matching how `splitSuffix()` recognises the suffix. **Collisions are counted in two places and I fixed one.** `BLEPeerSenderDisplayName.collisionResolvedName` compares nicknames exactly and feeds sender names on file transfers, so it kept the blind spot the peer-list resolver just lost. It now folds through the same `collisionKey`, so the two counters cannot drift. Suites here: 187 pass / 1912 tests, no result differing from main; 34 still need an app bundle. --- .../Identity/SecureIdentityStateManager.swift | 10 ++-- .../BLE/BLEPeerSenderDisplayName.swift | 11 ++++- bitchat/Utils/String+Nickname.swift | 26 +++++++++- ...tityStateManagerNicknameBindingTests.swift | 48 +++++++++++++++++++ .../Utils/PeerDisplayNameResolverTests.swift | 22 +++++++++ 5 files changed, 108 insertions(+), 9 deletions(-) diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index 622d9c58..2b7f69a4 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -179,7 +179,7 @@ extension SecureIdentityStateManagerProtocol { else { return true } let shown = renderedSender.withoutCollisionSuffix guard !shown.isEmpty else { return true } - return pinned.normalizedNickname == shown.normalizedNickname + return pinned.nicknameBindingKey == shown.nicknameBindingKey } } @@ -826,7 +826,7 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { // stand between the reader and a spoofed name on this row. let shown = renderedSender.withoutCollisionSuffix guard !shown.isEmpty else { return true } - return pinned.normalizedNickname == shown.normalizedNickname + return pinned.nicknameBindingKey == shown.nicknameBindingKey } } @@ -839,9 +839,9 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { // is displayed, so there is nothing to spoof and the seal stands. if let petname = social?.localPetname, !petname.isEmpty { return false } guard let claimed = social?.claimedNickname, !claimed.isEmpty else { return false } - // NFC, matching `normalizedNickname` everywhere else nicknames are - // compared: a decomposed and a precomposed "café" are one name. - return pinned.normalizedNickname != claimed.normalizedNickname + // See `nicknameBindingKey`: NFC plus case, so a decomposed and a + // precomposed "café" are one name and recasing is not a rename. + return pinned.nicknameBindingKey != claimed.nicknameBindingKey } func getVerifiedFingerprints() -> Set { diff --git a/bitchat/Services/BLE/BLEPeerSenderDisplayName.swift b/bitchat/Services/BLE/BLEPeerSenderDisplayName.swift index c17d8d7e..4f4868f4 100644 --- a/bitchat/Services/BLE/BLEPeerSenderDisplayName.swift +++ b/bitchat/Services/BLE/BLEPeerSenderDisplayName.swift @@ -50,9 +50,16 @@ enum BLEPeerSenderDisplayName { localNickname: String, peers: [PeerID: BLEPeerInfo] ) -> String { + // Folded, for the same reason as `PeerDisplayNameResolver`: the suffix + // exists to make namesakes distinguishable, so a case variant or a + // fullwidth Ｍ has to count as a collision. This is the second place + // collisions are counted — it feeds sender names on file transfers — + // and it had the same exact-match blind spot. + let target = PeerDisplayNameResolver.collisionKey(collisionNickname) let hasCollision = peers.values.contains { - $0.isConnected && $0.nickname == collisionNickname && $0.peerID != peerID - } || localNickname == collisionNickname + $0.isConnected && PeerDisplayNameResolver.collisionKey($0.nickname) == target + && $0.peerID != peerID + } || PeerDisplayNameResolver.collisionKey(localNickname) == target guard hasCollision else { return displayName } return displayName + "#" + String(peerID.id.prefix(4)) diff --git a/bitchat/Utils/String+Nickname.swift b/bitchat/Utils/String+Nickname.swift index 2902613f..e1fedf95 100644 --- a/bitchat/Utils/String+Nickname.swift +++ b/bitchat/Utils/String+Nickname.swift @@ -17,6 +17,22 @@ extension String { precomposedStringWithCanonicalMapping } + /// The form two nicknames are compared in to decide whether they are the + /// SAME NAME — used for the verified-name binding. + /// + /// NFC plus a locale-independent case fold, twice normalised because case + /// folding can itself emit decomposed sequences (Turkish İ lowercases to + /// i + U+0307). Recasing your own nickname is not a rename, so it must not + /// break the binding. + /// + /// Deliberately NFC and not NFKC: a fullwidth `Ｍedic` merely *looks* like + /// `Medic`, so it is a different name and must break the binding. Folding + /// look-alikes is `PeerDisplayNameResolver.collisionKey`'s job, which is + /// asking a different question — whether two peers need telling apart. + var nicknameBindingKey: String { + normalizedNickname.lowercased().normalizedNickname + } + /// Strips ONLY a trailing `#abcd` collision suffix, leaving everything /// else alone. /// @@ -27,8 +43,14 @@ extension String { var withoutCollisionSuffix: String { guard count >= 5 else { return self } let tail = suffix(5) - guard tail.first == "#", - tail.dropFirst().allSatisfy({ $0.isHexDigit }) else { return self } + // ASCII hex only, matching how `splitSuffix()` recognises the suffix + // this device generates. `Character.isHexDigit` would also accept + // fullwidth digits, so a nickname literally ending in "#ＡＢＣＤ" + // would be truncated and could then match a baseline it is not. + let isAsciiHex: (Character) -> Bool = { c in + ("0"..."9").contains(c) || ("a"..."f").contains(c) || ("A"..."F").contains(c) + } + guard tail.first == "#", tail.dropFirst().allSatisfy(isAsciiHex) else { return self } return String(dropLast(5)) } diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index 060a2693..adef9eb1 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -280,6 +280,54 @@ struct SecureIdentityStateManagerNicknameBindingTests { "the baseline is what the sheet needs to name") } + // MARK: - What counts as the same name + + @Test + func recasingYourOwnNicknameIsNotARename() { + // Found auditing my own revision: the binding compared NFC only, so + // changing "Ravi" to "ravi" broke it and silently dropped the seal. + let manager = makeManager() + announce(manager, vouchee, as: "Ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + + announce(manager, vouchee, as: "ravi") + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) + #expect(manager.isVerifiedAndNameBound(fingerprint: vouchee)) + #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "RAVI")) + } + + @Test + func aLookAlikeNameDoesBreakTheBinding() { + // The other half of that: the binding key is NFC, deliberately not + // NFKC. A fullwidth Ｍ merely LOOKS like M, so it is a different name + // and must break the binding — folding look-alikes is the collision + // resolver's job, which answers a different question. + let manager = makeManager() + announce(manager, vouchee, as: "Medic") + manager.setVerified(fingerprint: vouchee, verified: true) + + announce(manager, vouchee, as: "\u{FF2D}edic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "\u{FF2D}edic")) + } + + @Test + func onlyAnAsciiCollisionSuffixIsStripped() { + // `Character.isHexDigit` also accepts fullwidth digits, so a nickname + // literally ending in "#ＡＢＣＤ" was being truncated and could then + // match a baseline it is not. ASCII only, like `splitSuffix()`. + let manager = makeManager() + 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#\u{FF21}\u{FF22}\u{FF23}\u{FF24}"), + "a fullwidth tail is part of the name, not a suffix this device generates") + #expect(!manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "medic#zzzz"), + "non-hex is part of the name too") + } + // MARK: - Rebinding and clearing @Test diff --git a/bitchatTests/Utils/PeerDisplayNameResolverTests.swift b/bitchatTests/Utils/PeerDisplayNameResolverTests.swift index 10a2ee48..ad1c76b1 100644 --- a/bitchatTests/Utils/PeerDisplayNameResolverTests.swift +++ b/bitchatTests/Utils/PeerDisplayNameResolverTests.swift @@ -68,6 +68,28 @@ struct PeerDisplayNameResolverTests { #expect((out[other] ?? "").contains("#")) } + @Test + func theSenderDisplayNameResolverFoldsTheSameWay() { + // Collisions are counted in TWO places. This one feeds sender names on + // file transfers and had the same exact-match blind spot; folding only + // the peer-list resolver would have left it behind. + let mine = PeerID(str: "cccc3333") + func info(_ id: PeerID, _ nickname: String) -> BLEPeerInfo { + BLEPeerInfo(peerID: id, nickname: nickname, isConnected: true, + noisePublicKey: nil, signingPublicKey: nil, + isVerifiedNickname: true, lastSeen: Date()) + } + let peers: [PeerID: BLEPeerInfo] = [ + real: info(real, "Medic"), + other: info(other, "\u{FF2D}edic"), + ] + let name = BLEPeerSenderDisplayName.resolveKnownPeer( + peerID: other, localPeerID: mine, localNickname: "me", + peers: peers, allowConnectedUnverified: true) + #expect(name?.contains("#") == true, + "a fullwidth namesake must be suffixed here too") + } + @Test func theDisplayedNameIsNeverFolded() { // Only the collision KEY folds. What is shown stays as its owner typed From 729ca272b05b2bad932644816436b52ce2c7183b Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 14:03:28 +0800 Subject: [PATCH 5/8] Drop an API nothing calls, and correct a comment that was wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things found auditing the revision rather than the original, none of them a behaviour change. `isVerifiedAndNameBound` had no production caller. It was added at review suggestion to collapse the double `queue.sync` at `showsVerifiedSeal` and `isVerifiedSender` — but both of those now go through `sealAppliesToRow`, which already answers in one lock, and the two sites that still take two locks (`PeerListModel`, `VerificationModel`) need the two booleans separately to decide between the verified and vouched seals, so the conjunction cannot serve them. That leaves an unused requirement on a protocol three test doubles have to satisfy. Removed; its cases are folded into a test for what the baseline is actually kept for, which is letting a sheet name the old nickname. The comment on `demotedIfRenamed` still claimed nothing branches on `.noiseVerified` beyond the glyph. That was shown to be 3-for-4 in review and corrected in the thread, but not in the code, which is where it will be read. No *behaviour* branches on it; four things the user perceives do, and all four should move. `MockIdentityManager` compared nicknames with `!=`, so the double called a recase a rename while the real one does not. Which fold applies is the whole subject of this change, so a double that gets it backwards is a trap for whoever writes the next view test. Verified: 201 suites / 1975 tests, every difference from `main` an addition (8 -> 10, plus 7 and 23 new). The 17 XCTest-based suites now run too, under a shim over swift-testing that is not in this branch, which puts SecureIdentityStateManagerTests — the suite guarding the file this PR touches most — under test here for the first time. Mutation-checked both ways: stub the live mismatch and 11 binding cases fail; stub the demotion and the coordinator case fails. Co-Authored-By: Claude Opus 5 --- .../Identity/SecureIdentityStateManager.swift | 24 ++++--------------- .../ChatPeerIdentityCoordinator.swift | 13 +++++++--- bitchatTests/Mocks/MockIdentityManager.swift | 6 ++++- ...tityStateManagerNicknameBindingTests.swift | 11 ++++----- 4 files changed, 25 insertions(+), 29 deletions(-) diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index 2b7f69a4..fe6eb9c8 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -137,9 +137,6 @@ protocol SecureIdentityStateManagerProtocol { /// Whether this peer now announces a different nickname than the one its /// trust was earned under. func trustedNicknameMismatch(fingerprint: String) -> Bool - /// Verified AND still presenting the name it was verified under — what a - /// seal beside a LIVE name is actually asserting. One lock, not two. - func isVerifiedAndNameBound(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 @@ -164,15 +161,11 @@ protocol SecureIdentityStateManagerProtocol { } extension SecureIdentityStateManagerProtocol { - // Default implementations so a conformance only has to provide the two - // primitives. `SecureIdentityStateManager` overrides both to answer in a - // single lock acquisition — these seal checks run per row, per render, and - // the format cache does not shield the one in ChatMessageFormatter because - // the answer is part of its cache key. - func isVerifiedAndNameBound(fingerprint: String) -> Bool { - isVerified(fingerprint: fingerprint) && !trustedNicknameMismatch(fingerprint: fingerprint) - } - + // A default implementation so a conformance only has to provide the two + // primitives. `SecureIdentityStateManager` overrides it to answer in a + // 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 { guard isVerified(fingerprint: fingerprint) else { return false } guard let pinned = trustedNickname(fingerprint: fingerprint), !pinned.isEmpty @@ -802,13 +795,6 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { queue.sync { cache.trustedNicknames?[fingerprint] } } - func isVerifiedAndNameBound(fingerprint: String) -> Bool { - queue.sync { - guard cache.verifiedFingerprints.contains(fingerprint) else { return false } - return !liveNameMismatchLocked(fingerprint) - } - } - /// A message row shows the sender name frozen at receipt, so asking about /// the peer's CURRENT name is the wrong question: rename away, post, rename /// back, and the live name matches the baseline again while the archived row diff --git a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift index fbe8338e..0d6d92b8 100644 --- a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift +++ b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift @@ -477,9 +477,16 @@ final class ChatPeerIdentityCoordinator { /// invalidated by verification and session changes, never by a rename, so a /// demotion written into the cache would be both stale and sticky. The /// session really is Noise-secured; only the claim about *who* is weakened, - /// which is exactly the difference between the two cases. Nothing branches - /// on `.noiseVerified` beyond the glyph — every behavioural site treats it - /// and `.noiseSecured` alike. + /// which is exactly the difference between the two cases. + /// + /// No BEHAVIOUR branches on the distinction — `VerificationModel:33`, + /// `ContentSheetViews:659` and `:760` all treat `.noiseVerified` and + /// `.noiseSecured` alike. Four things the user perceives do, and all four + /// are meant to move: the icon (`checkmark.seal.fill` → `lock.fill`), the + /// `description` caption, the `accessibilityDescription` — so VoiceOver + /// stops saying "verified" on a demoted DM header — and the green tint on + /// the nickname at `FingerprintView:87`, which is the one site that does + /// NOT treat the two statuses identically. @MainActor private func demotedIfRenamed(_ status: EncryptionStatus, for peerID: PeerID) -> EncryptionStatus { guard status == .noiseVerified, diff --git a/bitchatTests/Mocks/MockIdentityManager.swift b/bitchatTests/Mocks/MockIdentityManager.swift index 2e1fc12d..8d00187b 100644 --- a/bitchatTests/Mocks/MockIdentityManager.swift +++ b/bitchatTests/Mocks/MockIdentityManager.swift @@ -107,7 +107,11 @@ final class MockIdentityManager: SecureIdentityStateManagerProtocol { func trustedNicknameMismatch(fingerprint: String) -> Bool { guard let pinned = trustedNicknames[fingerprint], !pinned.isEmpty, let claimed = claimedNicknames[fingerprint], !claimed.isEmpty else { return false } - return pinned != claimed + // Folded like the real one. A mock that compared raw strings would + // call a recase a rename, so a view test could pass here and fail in + // production — and which fold applies is the whole subject of this + // change, so it is the last thing a double should get wrong. + return pinned.nicknameBindingKey != claimed.nicknameBindingKey } func isVerified(fingerprint: String) -> Bool { diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index adef9eb1..f582041b 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -265,17 +265,17 @@ struct SecureIdentityStateManagerNicknameBindingTests { } @Test - func theCombinedLiveQueryAgreesWithItsParts() { + func theBaselineIsKeptSoASheetCanNameIt() { + // Suppressing the seal is the security fix; saying WHICH name was + // verified is what lets a reader recognise the peer instead of + // guessing. The baseline therefore survives the rename it detects. let manager = makeManager() announce(manager, vouchee, as: "ravi") - #expect(!manager.isVerifiedAndNameBound(fingerprint: vouchee), "not verified yet") - manager.setVerified(fingerprint: vouchee, verified: true) - #expect(manager.isVerifiedAndNameBound(fingerprint: vouchee)) #expect(manager.trustedNickname(fingerprint: vouchee) == "ravi") announce(manager, vouchee, as: "medic") - #expect(!manager.isVerifiedAndNameBound(fingerprint: vouchee)) + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee)) #expect(manager.trustedNickname(fingerprint: vouchee) == "ravi", "the baseline is what the sheet needs to name") } @@ -292,7 +292,6 @@ struct SecureIdentityStateManagerNicknameBindingTests { announce(manager, vouchee, as: "ravi") #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee)) - #expect(manager.isVerifiedAndNameBound(fingerprint: vouchee)) #expect(manager.sealAppliesToRow(fingerprint: vouchee, renderedSender: "RAVI")) } From ecdea9949ed0f5dbc2bb8f5752db72d46d6b5092 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 17:26:46 +0800 Subject: [PATCH 6/8] =?UTF-8?q?A=20row=20cannot=20tell=20a=20decoration=20?= =?UTF-8?q?from=20an=20announced=20name=20=E2=80=94=20so=20give=20it=20the?= =?UTF-8?q?=20peerID?= 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") } From 83eb89e8f90d951d241765ef4f2a8d8981aca74f Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Tue, 15 Sep 2026 11:32:30 +0800 Subject: [PATCH 7/8] The sheet stops endorsing the rename, word as well as glyph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @Chessing234 is right and my reasoning for the half-measure was wrong. I kept "✓ VERIFIED" in the fingerprint sheet on the grounds that the key genuinely is verified and only the glyph should demote. But that badge sits a few lines under `peerNickname`, so it is not a claim about a bare key — it is a claim about THIS NAME, and that claim is false. And this sheet is the screen someone opens *because* a seal vanished from a row, so the one surface they consult to resolve the doubt was the one endorsing the rename. The badge, the message and the verify button now all read `isVerified && !nameChangedSinceVerification`, which is what `nameChangedSinceVerification` was plumbed onto the presentation state for and then left unused. No new string, so no locale work is blocking it: the existing not-verified copy already names the current nickname and offers to verify it, and that offer IS the recovery — re-verifying re-pins the baseline to the name on screen. The stored verification is untouched underneath. One consequence worth stating rather than hiding: a renamed peer's sheet now offers "mark verified" instead of "remove verification", so unverify is one re-verification away rather than immediately to hand. I think that is the right trade — the alternative is a green checkmark next to a name nobody verified — but it is a real change to the affordance and I would rather it be argued with than found. 26 cases in the binding suite; 201 suites / 1977 tests, every difference from unmodified `main` an addition. Co-Authored-By: Claude Opus 5 --- bitchat/Views/FingerprintView.swift | 27 ++++++++++++++++--- ...tityStateManagerNicknameBindingTests.swift | 21 +++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/bitchat/Views/FingerprintView.swift b/bitchat/Views/FingerprintView.swift index bce7e89c..68b03d08 100644 --- a/bitchat/Views/FingerprintView.swift +++ b/bitchat/Views/FingerprintView.swift @@ -223,14 +223,33 @@ struct FingerprintView: View { // Verification status if fingerprintState.canToggleVerification { + // The seal is withheld when this key now presents a name it + // was not verified under — and the WORD goes with the glyph. + // + // An earlier revision kept "✓ VERIFIED" here on the grounds + // that the key genuinely is verified. Review put the + // objection better than I had: this badge sits a few lines + // under `peerNickname`, so it is not a claim about a bare + // key, it is a claim about THIS NAME — and that claim is + // false. Worse, this sheet is the screen someone opens + // *because* a seal vanished from a row, so the one surface + // they consult to resolve the doubt was the one endorsing + // the rename. + // + // No new string: the existing not-verified copy names the + // current nickname and offers to verify it, which is the + // recovery — re-verifying re-pins the baseline to the name + // on screen. The key's stored verified state is untouched. + let sealApplies = fingerprintState.isVerified + && !fingerprintState.nameChangedSinceVerification VStack(spacing: 12) { - Text(fingerprintState.isVerified ? Strings.verifiedBadge : Strings.notVerifiedBadge) + Text(sealApplies ? Strings.verifiedBadge : Strings.notVerifiedBadge) .bitchatFont(size: 14, weight: .bold) - .foregroundColor(fingerprintState.isVerified ? Color.green : Color.orange) + .foregroundColor(sealApplies ? Color.green : Color.orange) .frame(maxWidth: .infinity) Group { - if fingerprintState.isVerified { + if sealApplies { Text(Strings.verifiedMessage) } else { Text(Strings.verifyHint(fingerprintState.peerNickname)) @@ -243,7 +262,7 @@ struct FingerprintView: View { .fixedSize(horizontal: false, vertical: true) .frame(maxWidth: .infinity) - if !fingerprintState.isVerified { + if !sealApplies { Button(action: { verificationModel.verifyFingerprint(for: peerID) dismiss() diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index a844ea1a..5ac5da12 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -329,6 +329,27 @@ struct SecureIdentityStateManagerNicknameBindingTests { /// `PeerDisplayNameResolver` would give this peer and nothing else is. private var decorator: PeerID { PeerID(str: "a1b2c3d4") } + @Test + func theSheetStopsEndorsingAfterARename() { + // The fingerprint sheet drives its badge, its message and its verify + // button off `nameChangedSinceVerification`, so the word demotes with + // the glyph. Pinned here at the state level, which is what the view + // reads: the view itself needs an app bundle these tests cannot build. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + #expect(!manager.trustedNicknameMismatch(fingerprint: vouchee), + "nothing to explain while the name holds") + + announce(manager, vouchee, as: "medic") + #expect(manager.trustedNicknameMismatch(fingerprint: vouchee), + "the sheet is told the name changed, so its copy can demote") + #expect(manager.trustedNickname(fingerprint: vouchee) == "ravi", + "and the old name survives, so a future revision can name it") + #expect(manager.isVerified(fingerprint: vouchee), + "while the stored verification is untouched — re-verifying is the recovery") + } + // MARK: - What counts as the same name @Test From e2e929b60f5bded77640e7755498133093ee91d9 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Tue, 15 Sep 2026 18:43:22 +0800 Subject: [PATCH 8/8] Name a voucher by what you verified, not by what it announces now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The systematic pass over every trust-seal rendering site turned up one place the patch had not reached. The fingerprint sheet's voucher list is a trust attribution one level out — "people you verified vouch for this peer" — and it was built from `claimedNickname`, the voucher's CURRENT name. So Eve gets verified as "ravi", vouches for Mallory, renames to "medic", and Mallory's sheet reads "vouched by medic". The vouch is real and the key is trusted; the name beside it was never verified, which is exactly what the rest of this branch is about. Order is petname, then the pinned baseline, then the claimed nickname. The petname first because it is your own label and nothing on the network can influence it; the claimed nickname last as the same fail-open used everywhere else here, for vouchers recorded before baselines existed. The test covers the prerequisite — that the baseline outlives the rename, so the sheet has "ravi" to show — rather than the view itself, which needs a ChatViewModel this harness cannot build. Reverting the VerificationModel change alone would not fail it; said plainly rather than dressed up as coverage it does not have. Co-Authored-By: Claude Opus 5 --- bitchat/App/VerificationModel.swift | 19 +++++++++++++++++++ ...tityStateManagerNicknameBindingTests.swift | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/bitchat/App/VerificationModel.swift b/bitchat/App/VerificationModel.swift index 992713c2..d7ea4f63 100644 --- a/bitchat/App/VerificationModel.swift +++ b/bitchat/App/VerificationModel.swift @@ -162,11 +162,30 @@ final class VerificationModel: ObservableObject { } else { vouchers = [] } + // Each voucher is named by the name YOU VERIFIED IT UNDER, not by + // whatever it announces now. + // + // This list is a trust attribution — "people you verified vouch for + // this peer" — so it has the same problem as a seal, one level out. Use + // the live name and a voucher that renamed is attributed under its new + // one: Eve gets verified as "ravi", vouches for Mallory, renames to + // "medic", and Mallory's sheet reads "vouched by medic". The vouch is + // real and the key is trusted; the NAME beside it was never verified, + // which is the whole subject of this change. + // + // Order: a petname first, because it is your own label and nothing on + // the network can influence it; then the pinned baseline; then the + // claimed nickname, for vouchers recorded before baselines existed — + // the same fail-open as everywhere else here. let voucherNames = vouchers.compactMap { record -> String? in guard let social = chatViewModel.identityManager.getSocialIdentity(for: record.voucherFingerprint) else { return nil } if let petname = social.localPetname, !petname.isEmpty { return petname } + if let pinned = chatViewModel.identityManager.trustedNickname( + fingerprint: record.voucherFingerprint), !pinned.isEmpty { + return pinned + } return social.claimedNickname.isEmpty ? nil : social.claimedNickname } diff --git a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift index 5ac5da12..9a455438 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift @@ -350,6 +350,25 @@ struct SecureIdentityStateManagerNicknameBindingTests { "while the stored verification is untouched — re-verifying is the recovery") } + @Test + func aVoucherIsNamedByTheNameItWasVerifiedUnder() { + // The voucher list in the fingerprint sheet is a trust attribution, so + // it has the seal's problem one level out: name a voucher by what it + // announces NOW and a renamed voucher is credited under the new name. + // Eve verified as "ravi", vouches for Mallory, renames to "medic", and + // Mallory's sheet reads "vouched by medic". + // + // The baseline outlives the rename precisely so the sheet can say + // "ravi" — the person you actually checked. Pinned at the manager, + // which is where the view reads it from. + let manager = makeManager() + announce(manager, vouchee, as: "ravi") + manager.setVerified(fingerprint: vouchee, verified: true) + announce(manager, vouchee, as: "medic") + #expect(manager.trustedNickname(fingerprint: vouchee) == "ravi", + "a voucher that renamed is still named by what it was verified as") + } + // MARK: - What counts as the same name @Test