nik/bitchat
1
0
Fork 0
mirror of https://github.com/permissionlesstech/bitchat.git synced 2026-09-19 05:00:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

A row cannot tell a decoration from an announced name — so give it the peerID

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Shubham Bhandari 2026-09-14 17:26:46 +08:00
parent 729ca272b0
commit ecdea9949e
6 changed files with 112 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
bitchat/App/ConversationUIModel.swift
View File

@ -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? {

14
bitchat/Identity/SecureIdentityStateManager.swift
View File

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

34
bitchat/Utils/String+Nickname.swift
View File

@ -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: "")

3
bitchat/ViewModels/ChatMessageFormatter.swift
View File

@ -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(

6
bitchat/ViewModels/ChatViewModel.swift
View File

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

80
bitchatTests/Services/SecureIdentityStateManagerNicknameBindingTests.swift
View File

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