From cfa875459d02bf61f64658bfd36baeeb60fadea2 Mon Sep 17 00:00:00 2001 From: jack Date: Tue, 11 Aug 2026 10:03:01 +0200 Subject: [PATCH 1/3] Review fix: record withheld receipts in both tracking sets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1: the manager-path withheld claim landed only in PrivateChatManager.sentReadReceipts, while the lifecycle read pass dedups against ChatViewModel's persisted set — enabling receipts before the next lifecycle pass could send a receipt for a message read while the setting was off. The withheld branch now records into the owner's persisted set too (markReceiptHandled, wired in the bootstrapper); test strengthened to require both sets. Co-Authored-By: Claude Fable 5 --- bitchat/Services/PrivateChatManager.swift | 12 ++++++++++-- bitchat/ViewModels/ChatViewModelBootstrapper.swift | 3 +++ bitchatTests/ChatViewModelTests.swift | 9 ++++++--- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/bitchat/Services/PrivateChatManager.swift b/bitchat/Services/PrivateChatManager.swift index 3141ec0b..a243d570 100644 --- a/bitchat/Services/PrivateChatManager.swift +++ b/bitchat/Services/PrivateChatManager.swift @@ -252,15 +252,23 @@ final class PrivateChatManager: ObservableObject { /// suites through the shared UserDefaults-backed setting. var sendsReadReceipts: () -> Bool = { ReadReceiptSettings.sendReadReceipts } + /// Records a withheld receipt in the owner's persisted set too: the + /// lifecycle read pass dedups against ChatViewModel.sentReadReceipts, + /// not this manager's set, so claiming only locally would let a receipt + /// for a message read while the setting was OFF fire after re-enabling. + var markReceiptHandled: ((String) -> Void)? + private func sendReadReceipt(for message: BitchatMessage) { guard !sentReadReceipts.contains(message.id), let senderPeerID = message.senderPeerID else { return } - // Withheld receipts are still claimed below as sent: re-enabling the - // setting must never fire a retroactive burst disclosing past reads. + // Withheld receipts are still claimed as sent — in BOTH tracking + // sets: re-enabling the setting must never fire a retroactive burst + // disclosing past reads, from this manager or the lifecycle pass. guard sendsReadReceipts() else { sentReadReceipts.insert(message.id) + markReceiptHandled?(message.id) return } diff --git a/bitchat/ViewModels/ChatViewModelBootstrapper.swift b/bitchat/ViewModels/ChatViewModelBootstrapper.swift index 3ccb7d12..ca769b05 100644 --- a/bitchat/ViewModels/ChatViewModelBootstrapper.swift +++ b/bitchat/ViewModels/ChatViewModelBootstrapper.swift @@ -90,6 +90,9 @@ private extension ChatViewModelBootstrapper { viewModel.privateChatManager.conversationStore = viewModel.conversations viewModel.privateChatManager.messageRouter = viewModel.messageRouter viewModel.privateChatManager.unifiedPeerService = viewModel.unifiedPeerService + viewModel.privateChatManager.markReceiptHandled = { [weak viewModel] messageID in + viewModel?.markReadReceiptSent(messageID) + } viewModel.unifiedPeerService.messageRouter = viewModel.messageRouter // Surface silent outbox drops (attempt cap, TTL expiry, overflow // eviction) as a visible failure. The store's no-downgrade rule does diff --git a/bitchatTests/ChatViewModelTests.swift b/bitchatTests/ChatViewModelTests.swift index 1bd66497..b4e9109e 100644 --- a/bitchatTests/ChatViewModelTests.swift +++ b/bitchatTests/ChatViewModelTests.swift @@ -462,10 +462,13 @@ struct ChatViewModelServiceLifecycleTests { #expect(!sentReadReceipt) // ...while the chat is still marked read locally and the receipt is - // recorded as handled, so re-enabling the setting never fires a - // retroactive burst disclosing past reading activity. + // recorded as handled in BOTH tracking sets (the lifecycle pass + // dedups against the owner's persisted set, the manager against its + // own), so re-enabling the setting never fires a retroactive burst + // disclosing past reading activity from either path. #expect(!viewModel.unreadPrivateMessages.contains(peerID)) - #expect(viewModel.sentReadReceipts.contains("read-2") || viewModel.privateChatManager.sentReadReceipts.contains("read-2")) + #expect(viewModel.sentReadReceipts.contains("read-2")) + #expect(viewModel.privateChatManager.sentReadReceipts.contains("read-2")) } @Test @MainActor From 6dd14961c6c26f8490c7fad5741c07e70b1924f9 Mon Sep 17 00:00:00 2001 From: jack Date: Tue, 11 Aug 2026 10:06:45 +0200 Subject: [PATCH 2/3] Review fix: opt live-voice tests in via injectable provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1: the fixtures assumed the old default-ON preference — on a clean CI process the coordinator guard drops their frames. Instead of mutating the shared UserDefaults per test (which races parallel suites), the live-voice reads are now injectable (ChatLiveVoiceCoordinator.liveVoiceEnabled), the fixtures opt in per instance, and the toggle-off test drives the provider directly. Co-Authored-By: Claude Fable 5 --- .../ViewModels/ChatLiveVoiceCoordinator.swift | 9 ++++++-- .../ChatViewModel+PrivateChat.swift | 2 +- .../ChatLiveVoiceCoordinatorTests.swift | 22 +++++++++++++++---- bitchatTests/ChatViewModelTests.swift | 3 +++ 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/bitchat/ViewModels/ChatLiveVoiceCoordinator.swift b/bitchat/ViewModels/ChatLiveVoiceCoordinator.swift index 78fa83e1..acc4a298 100644 --- a/bitchat/ViewModels/ChatLiveVoiceCoordinator.swift +++ b/bitchat/ViewModels/ChatLiveVoiceCoordinator.swift @@ -79,6 +79,11 @@ enum VoiceBurstScope: Hashable { /// bubble so nobody sees a duplicate. @MainActor final class ChatLiveVoiceCoordinator { + /// Injectable so tests exercise the live path without racing other + /// suites through the shared UserDefaults-backed preference (which now + /// defaults OFF). + var liveVoiceEnabled: () -> Bool = { PTTSettings.liveVoiceEnabled } + /// Burst IDs are sender-chosen, so they only identify a burst *within* /// an authenticated (peer, scope) pair: keying assemblies by the full /// triple stops an attacker who observed a public burst ID from racing @@ -187,7 +192,7 @@ final class ChatLiveVoiceCoordinator { // Live voice off means classic-notes-only in both directions: no live // bubble, no partial file, no early notification — the finalized // voice note still arrives through the normal pipeline. - guard PTTSettings.liveVoiceEnabled else { + guard liveVoiceEnabled() else { SecureLogger.debug("PTT: dropping inbound voice frame — live voice is toggled off", category: .session) return } @@ -403,7 +408,7 @@ final class ChatLiveVoiceCoordinator { case .directMessage: context.selectedPrivateChatPeer == peerID case .publicMesh: context.isViewingPublicMeshTimeline } - if PTTSettings.liveVoiceEnabled, PTTSettings.isAppActive, isViewing { + if liveVoiceEnabled(), PTTSettings.isAppActive, isViewing { assembly.player = PTTBurstPlayer() } diff --git a/bitchat/ViewModels/Extensions/ChatViewModel+PrivateChat.swift b/bitchat/ViewModels/Extensions/ChatViewModel+PrivateChat.swift index 9dad8030..968da1a9 100644 --- a/bitchat/ViewModels/Extensions/ChatViewModel+PrivateChat.swift +++ b/bitchat/ViewModels/Extensions/ChatViewModel+PrivateChat.swift @@ -69,7 +69,7 @@ extension ChatViewModel { @MainActor private func liveVoiceTarget() -> LiveVoiceTarget? { - guard PTTSettings.liveVoiceEnabled else { return nil } + guard liveVoiceCoordinator.liveVoiceEnabled() else { return nil } if let selectedPeer = selectedPrivateChatPeer { guard !selectedPeer.isGeoDM, !selectedPeer.isGeoChat, !selectedPeer.isGroup else { return nil } diff --git a/bitchatTests/ChatLiveVoiceCoordinatorTests.swift b/bitchatTests/ChatLiveVoiceCoordinatorTests.swift index 2e5ca317..67b509c7 100644 --- a/bitchatTests/ChatLiveVoiceCoordinatorTests.swift +++ b/bitchatTests/ChatLiveVoiceCoordinatorTests.swift @@ -127,6 +127,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func burstCreatesBubbleAndPersistsFramesInOrder() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0xA1) defer { fallbackFileURL(burstID: burstID, peerID: peer).map { try? FileManager.default.removeItem(at: $0) } } @@ -168,6 +169,7 @@ struct ChatLiveVoiceCoordinatorTests { let context = MockChatLiveVoiceContext() context.selectedPrivateChatPeer = peer let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0xB2) let hex = burstID.hexEncodedString() let fileName = "voice_\(hex).m4a" @@ -223,6 +225,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func absorbIgnoresUnrelatedVoiceNotes() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } // A classic voice note (date-stamped name) and a live-capture name // must both pass through untouched. @@ -247,6 +250,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func canceledBurstRemovesBubbleAndFile() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0xC3) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 1, kind: .frames([Data(repeating: 9, count: 40)]))), to: coordinator, from: peer) @@ -262,6 +266,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func emptyBurstLeavesNoBubble() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0xD4) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 0, kind: .start(codec: .aacLC16kMono))), to: coordinator, from: peer) @@ -275,6 +280,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func ignoresBlockedPeersAndUnknownControlPackets() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } context.blockedPeers = [peer] send(try #require(VoiceBurstPacket(burstID: makeBurstID(0xE5), seq: 0, kind: .start(codec: .aacLC16kMono))), to: coordinator, from: peer) @@ -290,6 +296,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func concurrentAssemblyCapDropsExtraBursts() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } var cleanup: [Data] = [] defer { @@ -309,12 +316,10 @@ struct ChatLiveVoiceCoordinatorTests { } @Test func liveVoiceToggleOffDropsInboundFrames() throws { - let previous = PTTSettings.liveVoiceEnabled - PTTSettings.liveVoiceEnabled = false - defer { PTTSettings.liveVoiceEnabled = previous } - let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } + coordinator.liveVoiceEnabled = { false } let burstID = makeBurstID(0xE8) // Off means classic-notes-only: no live bubble, no partial file. @@ -328,6 +333,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func publicBurstCreatesMeshBubbleAndTracksTalker() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x71) defer { incomingFileURL(burstID: burstID, peerID: peer, scope: .publicMesh).map { try? FileManager.default.removeItem(at: $0) } @@ -369,6 +375,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func absorbEnforcesScopeBinding() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x72) defer { fallbackFileURL(burstID: burstID, peerID: peer).map { try? FileManager.default.removeItem(at: $0) } } @@ -400,6 +407,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func collidingBurstIDFromAnotherPeerCannotHijackAssembly() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x73) let attacker = PeerID(str: "ddddeeeeffff0002") defer { @@ -435,6 +443,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func sameBurstIDCoexistsAcrossScopes() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x74) defer { fallbackFileURL(burstID: burstID, peerID: peer).map { try? FileManager.default.removeItem(at: $0) } @@ -492,6 +501,7 @@ struct ChatLiveVoiceCoordinatorTests { @Test func finalizedNoteBindsToItsAuthenticatedSender() throws { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, sweepsOnInit: false) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x75) let hex = burstID.hexEncodedString() let attacker = PeerID(str: "ddddeeeeffff0002") @@ -550,6 +560,7 @@ struct ChatLiveVoiceCoordinatorTests { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, fileStore: store) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x76) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 0, kind: .start(codec: .aacLC16kMono))), to: coordinator, from: peer) @@ -571,6 +582,7 @@ struct ChatLiveVoiceCoordinatorTests { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, fileStore: store) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x77) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 0, kind: .start(codec: .aacLC16kMono))), to: coordinator, from: peer) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 1, kind: .frames([Data(repeating: 8, count: 60)]))), to: coordinator, from: peer) @@ -618,6 +630,7 @@ struct ChatLiveVoiceCoordinatorTests { // sender out of range): the capture is the row's only audio. let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, fileStore: store) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x79) let frame = Data(repeating: 0x0B, count: 60) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 1, kind: .frames([frame]))), to: coordinator, from: peer) @@ -642,6 +655,7 @@ struct ChatLiveVoiceCoordinatorTests { let context = MockChatLiveVoiceContext() let coordinator = ChatLiveVoiceCoordinator(context: context, fileStore: store) + coordinator.liveVoiceEnabled = { true } let burstID = makeBurstID(0x7A) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 1, kind: .frames([Data(repeating: 0x0C, count: 60)]))), to: coordinator, from: peer) send(try #require(VoiceBurstPacket(burstID: burstID, seq: 2, kind: .end(totalDataPackets: 1, durationMs: 64))), to: coordinator, from: peer) diff --git a/bitchatTests/ChatViewModelTests.swift b/bitchatTests/ChatViewModelTests.swift index b4e9109e..b7684aa3 100644 --- a/bitchatTests/ChatViewModelTests.swift +++ b/bitchatTests/ChatViewModelTests.swift @@ -1591,6 +1591,9 @@ struct ChatViewModelPrivateMediaDeletionTests { kind: .canceled )) let coordinator = viewModel.liveVoiceCoordinator + // The live-voice preference defaults OFF now; this fixture exercises + // the opted-in live path. + coordinator.liveVoiceEnabled = { true } defer { coordinator.handleVoiceFramePayload( from: peerID, From c58ad9af1090463fdb4bb5ee9b6d154b7d16fc7e Mon Sep 17 00:00:00 2001 From: jack Date: Tue, 11 Aug 2026 10:09:57 +0200 Subject: [PATCH 3/3] Review fixes: unread flag + favorites-aware naming for the warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Codex P1: a background identity change appended the warning without marking the chat unread — a security event nobody was looking at stayed invisible until the conversation was manually opened. It now sets the unread flag unless the chat is open. - Codex P2: offline key rotation is the common case here, and resolveNickname falls back to an anon prefix precisely then (no mesh nickname, no social identity for the unverified new fingerprint). The persisted favorite relationship's nickname is preferred. Both pinned by tests. Co-Authored-By: Claude Fable 5 --- .../ChatPeerIdentityCoordinator.swift | 19 ++++++++++- ...tPeerIdentityCoordinatorContextTests.swift | 33 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift index 2f6f7270..b67fae5f 100644 --- a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift +++ b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift @@ -24,6 +24,9 @@ protocol ChatPeerIdentityContext: AnyObject { var unreadPrivateMessages: Set { get } /// Clears the peer's unread flag (single-writer store intent). func markPrivateChatRead(_ peerID: PeerID) + /// Sets the peer's unread flag (shared requirement with the inbound DM + /// paths; witness on `ChatViewModel`). + func markPrivateChatUnread(_ peerID: PeerID) /// Moves all messages from `oldPeerID`'s chat into `newPeerID`'s chat /// (dedup by ID, order preserved, unread carried, old chat removed). func migratePrivateChat(from oldPeerID: PeerID, to newPeerID: PeerID) @@ -621,12 +624,20 @@ extension ChatPeerIdentityCoordinator { // inherit the thread's earned trust under the same nickname. Only // warn when there is a conversation to protect. if wasSelected || !context.privateMessages(for: newPeerID).isEmpty { + // Offline key rotation is the common case here (a favorite came + // back with new keys), and resolveNickname has no mesh nickname + // and no social identity for a fingerprint nobody verified yet — + // the persisted favorite relationship still knows who this is. + let favoriteNickname = newPeerID.noiseKey + .flatMap { context.favoriteRelationship(forNoiseKey: $0)?.peerNickname } + .flatMap { $0.isEmpty ? nil : $0 } + let displayName = favoriteNickname ?? resolveNickname(for: newPeerID) let notice = BitchatMessage( sender: "system", content: String( format: String(localized: "system.identity.key_changed", defaultValue: "%@'s identity key changed — this can mean a new device or a reset. earlier verification no longer applies; verify them again before trusting this chat.", comment: "Private-chat system warning after a peer's Noise identity key changed; placeholder is the peer's name"), locale: .current, - resolveNickname(for: newPeerID) + displayName ), timestamp: Date(), isRelay: false, @@ -636,6 +647,12 @@ extension ChatPeerIdentityCoordinator { senderPeerID: context.myPeerID ) context.appendPrivateMessage(notice, to: newPeerID) + // A security event nobody is looking at must not stay silent: + // surface it through the unread indicator unless the chat is + // open right now. + if !wasSelected { + context.markPrivateChatUnread(newPeerID) + } context.notifyUIChanged() } } diff --git a/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift b/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift index d14f766a..0def7e2c 100644 --- a/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift +++ b/bitchatTests/ChatPeerIdentityCoordinatorContextTests.swift @@ -46,6 +46,10 @@ private final class MockChatPeerIdentityContext: ChatPeerIdentityContext { unreadPrivateMessages.remove(peerID) } + func markPrivateChatUnread(_ peerID: PeerID) { + unreadPrivateMessages.insert(peerID) + } + @discardableResult func appendPrivateMessage(_ message: BitchatMessage, to peerID: PeerID) -> Bool { var chat = privateChats[peerID] ?? [] @@ -390,6 +394,35 @@ struct ChatPeerIdentityCoordinatorContextTests { #expect(notice?.content.contains("identity key changed") == true) #expect(notice?.content.contains("alice") == true) #expect(context.privateChats[oldPeerID] == nil) + // A background security event must surface via the unread indicator. + #expect(context.unreadPrivateMessages.contains(newPeerID)) + } + + @Test @MainActor + func migrateNoiseKeyUpdate_namesOfflineFavoritesFromTheRelationship() async { + let context = MockChatPeerIdentityContext() + let coordinator = ChatPeerIdentityCoordinator(context: context) + let oldPeerID = PeerID(str: "5555555555555555") + let newKey = Data(repeating: 0xCD, count: 32) + let newPeerID = PeerID(hexData: newKey) + // Offline key rotation: no mesh nickname, no social identity for the + // unverified new fingerprint — only the favorite relationship knows + // who this is. + context.favoriteRelationshipsByNoiseKey[newKey] = FavoritesPersistenceService.FavoriteRelationship( + peerNoisePublicKey: newKey, + peerNostrPublicKey: nil, + peerNickname: "carol", + isFavorite: true, + theyFavoritedUs: true, + favoritedAt: Date(timeIntervalSince1970: 0), + lastUpdated: Date(timeIntervalSince1970: 0) + ) + context.privateChats[oldPeerID] = [makePrivateMessage(id: "m2", timestamp: Date(timeIntervalSince1970: 1))] + + coordinator.migrateNoiseKeyUpdate(oldPeerID: oldPeerID, newPeerID: newPeerID) + + let notice = (context.privateChats[newPeerID] ?? []).last + #expect(notice?.content.contains("carol") == true) } @Test @MainActor