From fd2dffdda7800510199d14c670256799a25b4e96 Mon Sep 17 00:00:00 2001 From: jack Date: Mon, 6 Jul 2026 21:12:18 +0200 Subject: [PATCH] Fix CI media-wipe race, per-link ping rate limiting, and /ping output routing Three fixes for PR #1377 review: 1. CI flake (sendImage_privateChatProcessesAndTransfersImage): the panicClearAllData / clearCurrentPublicTimeline detached utility-priority tasks delete the real ~/Library/Application Support/files tree, which the test process shares. The wipe fires at a nondeterministic time and raced the sendImage test's JPEG in files/images/outgoing (write then re-read), so prepareImagePacket threw and the test timed out. Both wipes are now skipped under tests (existing TestEnvironment.isRunningTests pattern); this also stops test runs from deleting the developer's real media. 2. Codex P1: ping packets are unsigned, so keying the pong rate limiter on packet.senderID let one connected peer rotate forged sender IDs to bypass the 5-per-10s budget. The limiter now keys on the ingress link (the directly connected peer that delivered the packet); the pong still goes to the claimed sender. Regression test proves rotating senders over one link exhaust one budget (fails 10 vs 5 pongs on the old code). 3. Codex P2: /ping output arrived up to 10s later and was routed from selectedPrivateChatPeer at callback time, misrouting the result after a chat switch. The origin conversation is now captured when the command is issued (CommandOutputDestination) and deferred output is routed there: a DM result lands in the origin chat's history even if deselected, and a mesh-timeline result pins to #mesh instead of the active channel. Co-Authored-By: Claude Fable 5 --- bitchat/Services/BLE/BLEService.swift | 32 +++++++--- bitchat/Services/CommandProcessor.swift | 27 ++++++-- bitchat/Services/TransportConfig.swift | 2 +- .../ChatPublicConversationCoordinator.swift | 5 ++ bitchat/ViewModels/ChatViewModel.swift | 40 ++++++++++-- bitchatTests/BLEServiceCoreTests.swift | 63 +++++++++++++++++++ bitchatTests/CommandProcessorTests.swift | 11 +++- .../Services/MeshDiagnosticsTests.swift | 49 ++++++++++++++- 8 files changed, 208 insertions(+), 21 deletions(-) diff --git a/bitchat/Services/BLE/BLEService.swift b/bitchat/Services/BLE/BLEService.swift index c3e39764..f21bc871 100644 --- a/bitchat/Services/BLE/BLEService.swift +++ b/bitchat/Services/BLE/BLEService.swift @@ -69,9 +69,11 @@ final class BLEService: NSObject { private let meshTopology = MeshTopologyTracker() // Mesh diagnostics: outstanding /ping probes keyed by nonce, plus the - // inbound per-peer ping budget so a directed unencrypted probe cannot be - // turned into an amplification primitive. Both are owned by - // collectionsQueue barriers like the other mutable collections. + // inbound ping budget — keyed by the ingress link (the directly connected + // peer that delivered the packet), since the unsigned claimed sender is + // spoofable — so a directed unencrypted probe cannot be turned into an + // amplification primitive. Both are owned by collectionsQueue barriers + // like the other mutable collections. private struct PendingMeshPing { let peerID: PeerID let sentAt: Date @@ -80,7 +82,7 @@ final class BLEService: NSObject { } private var pendingMeshPings: [Data: PendingMeshPing] = [:] private var meshPingResponseLimiter = SyncResponseRateLimiter( - maxResponses: TransportConfig.meshPingInboundMaxPerPeer, + maxResponses: TransportConfig.meshPingInboundMaxPerLink, window: TransportConfig.meshPingInboundWindowSeconds ) @@ -2474,18 +2476,25 @@ extension BLEService { /// Answers a ping addressed to us with a pong echoing its nonce; pings /// addressed elsewhere are left to the generic directed-relay path. - private func handleMeshPing(_ packet: BitchatPacket, from peerID: PeerID) { + /// + /// `linkPeerID` is the directly connected peer that delivered the packet + /// (the ingress link), NOT the packet's claimed sender: pings are + /// unsigned, so `packet.senderID` is attacker-controlled, and keying the + /// response budget on it would let one connected peer rotate forged + /// sender IDs to emit unbounded pongs. The budget is per physical link; + /// the pong still goes to the claimed sender (that's the protocol). + private func handleMeshPing(_ packet: BitchatPacket, fromLink linkPeerID: PeerID) { guard packet.recipientID == myPeerIDData else { return } guard let ping = MeshPingPayload.decode(packet.payload) else { - SecureLogger.debug("⚠️ Malformed ping from \(peerID.id.prefix(8))…", category: .session) + SecureLogger.debug("⚠️ Malformed ping via \(linkPeerID.id.prefix(8))…", category: .session) return } let allowed = collectionsQueue.sync(flags: .barrier) { - meshPingResponseLimiter.shouldRespond(to: peerID, now: Date()) + meshPingResponseLimiter.shouldRespond(to: linkPeerID, now: Date()) } guard allowed else { - if logRateLimiter.shouldLog(key: "ping-limit:\(peerID.id)") { - SecureLogger.warning("🚫 Rate-limiting pings from \(peerID.id.prefix(8))…", category: .security) + if logRateLimiter.shouldLog(key: "ping-limit:\(linkPeerID.id)") { + SecureLogger.warning("🚫 Rate-limiting pings via link \(linkPeerID.id.prefix(8))…", category: .security) } return } @@ -3357,7 +3366,10 @@ extension BLEService { handleCourierEnvelope(packet, from: peerID) case .ping: - handleMeshPing(packet, from: senderID) + // Rate limiting must key on the ingress link (`peerID`), not the + // packet-claimed sender: pings are unsigned, so `senderID` is + // attacker-controlled and rotating it would reset the budget. + handleMeshPing(packet, fromLink: peerID) case .pong: handleMeshPong(packet, from: senderID) diff --git a/bitchat/Services/CommandProcessor.swift b/bitchat/Services/CommandProcessor.swift index db9b07dd..9c007b41 100644 --- a/bitchat/Services/CommandProcessor.swift +++ b/bitchat/Services/CommandProcessor.swift @@ -22,6 +22,17 @@ struct CommandGeoParticipant { let displayName: String } +/// The conversation a command was typed into, captured when the command is +/// issued so deferred output (e.g. an async /ping result, which can arrive +/// many seconds later) lands there even if the user switches chats first. +enum CommandOutputDestination: Equatable { + /// The #mesh public timeline. Commands that defer output (/ping) are + /// mesh-only, so a non-DM origin is always the mesh timeline. + case meshTimeline + /// The private chat that was open when the command was typed. + case privateChat(PeerID) +} + /// Protocol defining what CommandProcessor needs from its context. /// This breaks the circular dependency between CommandProcessor and ChatViewModel. @MainActor @@ -49,9 +60,13 @@ protocol CommandContextProvider: AnyObject { // MARK: - System Messages func addLocalPrivateSystemMessage(_ content: String, to peerID: PeerID) func addPublicSystemMessage(_ content: String) + /// The conversation the user is typing into right now. Commands that + /// finish asynchronously capture this BEFORE starting async work, so a + /// chat switch cannot misroute their deferred output. + func currentCommandDestination() -> CommandOutputDestination /// Routes deferred command output (e.g. an async /ping result) into the - /// conversation where the user typed the command. - func addCommandOutput(_ content: String) + /// conversation captured when the command was issued. + func addCommandOutput(_ content: String, to destination: CommandOutputDestination) // MARK: - Favorites /// Toggles the favorite via the unified peer flow, which persists by the @@ -373,16 +388,20 @@ final class CommandProcessor { let nickname = target.nickname let currentProvider = contextProvider + // Capture the origin conversation now: the pong can arrive up to + // meshPingTimeoutSeconds later, and reading the selected chat at + // callback time would misroute the result after a chat switch. + let destination = contextProvider?.currentCommandDestination() ?? .meshTimeline meshService?.sendMeshPing(to: target.peerID) { [weak currentProvider] result in let provider = currentProvider guard let result else { - provider?.addCommandOutput("no reply from \(nickname)") + provider?.addCommandOutput("no reply from \(nickname)", to: destination) return } let hopText: String = result.hops.map { hops in hops == 1 ? " · direct (1 hop)" : " · \(hops) hops" } ?? "" - provider?.addCommandOutput("pong from \(nickname): \(result.rttMs) ms\(hopText)") + provider?.addCommandOutput("pong from \(nickname): \(result.rttMs) ms\(hopText)", to: destination) } return .success(message: "pinging \(nickname)…") } diff --git a/bitchat/Services/TransportConfig.swift b/bitchat/Services/TransportConfig.swift index 04cc0c21..da95c34e 100644 --- a/bitchat/Services/TransportConfig.swift +++ b/bitchat/Services/TransportConfig.swift @@ -18,7 +18,7 @@ enum TransportConfig { // Mesh diagnostics (/ping) static let meshPingTimeoutSeconds: TimeInterval = 10 // Give up on a probe after this window - static let meshPingInboundMaxPerPeer: Int = 5 // Inbound ping budget per peer... + static let meshPingInboundMaxPerLink: Int = 5 // Inbound ping budget per ingress link (claimed sender is spoofable)... static let meshPingInboundWindowSeconds: TimeInterval = 10 // ...per sliding window (anti-amplification) // UI / Storage Caps diff --git a/bitchat/ViewModels/ChatPublicConversationCoordinator.swift b/bitchat/ViewModels/ChatPublicConversationCoordinator.swift index d6777a71..e279c454 100644 --- a/bitchat/ViewModels/ChatPublicConversationCoordinator.swift +++ b/bitchat/ViewModels/ChatPublicConversationCoordinator.swift @@ -291,6 +291,11 @@ final class ChatPublicConversationCoordinator: PublicMessagePipelineDelegate { context.clearPublicConversation(ConversationID(channelID: context.activeChannel)) Task.detached(priority: .utility) { + // Skipped under tests: the test process shares the user's real + // ~/Library/Application Support/files tree, and this detached + // wipe fires at a nondeterministic time — racing tests that + // write media there (see the same guard in panicClearAllData). + guard !TestEnvironment.isRunningTests else { return } do { let base = try FileManager.default.url( for: .applicationSupportDirectory, diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index bea1e619..c09d5a6d 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -1273,6 +1273,13 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele // Delete ALL media files (incoming and outgoing) in background Task.detached(priority: .utility) { + // Skipped under tests: the test process shares the user's real + // ~/Library/Application Support/files tree, and this detached + // utility-priority wipe fires at a nondeterministic time — + // deleting media that concurrently running tests (e.g. the + // sendImage flow) just wrote there, and the developer's real + // app data with it. + guard !TestEnvironment.isRunningTests else { return } do { let base = try FileManager.default.url(for: .applicationSupportDirectory, in: .userDomainMask, appropriateFor: nil, create: true) let filesDir = base.appendingPathComponent("files", isDirectory: true) @@ -1514,11 +1521,9 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele /// Command output belongs in the conversation where the user typed the /// command; the public timeline is invisible while a DM is open. The DM /// selection is read *after* processing so commands that switch chats - /// (`/msg`) print into the conversation they just opened. Internal (not - /// private) because CommandProcessor routes deferred output (async /ping - /// results) through it via CommandContextProvider. + /// (`/msg`) print into the conversation they just opened. @MainActor - func addCommandOutput(_ content: String) { + private func addCommandOutput(_ content: String) { if let peerID = selectedPrivateChatPeer { addLocalPrivateSystemMessage(content, to: peerID) } else { @@ -1526,6 +1531,33 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele } } + /// Origin conversation for deferred command output, captured when the + /// command is issued (before any async work starts). + @MainActor + func currentCommandDestination() -> CommandOutputDestination { + if let peerID = selectedPrivateChatPeer { + return .privateChat(peerID) + } + // Deferring commands (/ping) are rejected in geohash channels, so a + // non-DM origin is always the #mesh timeline. + return .meshTimeline + } + + /// Routes deferred command output (async /ping results) into the + /// conversation captured at issue time, immune to chat switches in the + /// meantime. A DM result lands in the origin chat's history even if that + /// chat is no longer selected (or was cleared — it then reappears as the + /// first message when the chat is reopened). + @MainActor + func addCommandOutput(_ content: String, to destination: CommandOutputDestination) { + switch destination { + case .privateChat(let peerID): + addLocalPrivateSystemMessage(content, to: peerID) + case .meshTimeline: + publicConversationCoordinator.addMeshOnlySystemMessage(content) + } + } + // MARK: - Message Reception @MainActor diff --git a/bitchatTests/BLEServiceCoreTests.swift b/bitchatTests/BLEServiceCoreTests.swift index fe7624ec..6a9cdfc1 100644 --- a/bitchatTests/BLEServiceCoreTests.swift +++ b/bitchatTests/BLEServiceCoreTests.swift @@ -216,6 +216,69 @@ struct BLEServiceCoreTests { cachedServiceUUIDs: [BLEService.serviceUUID, otherService] )) } + + /// Pings are unsigned, so their claimed sender is attacker-controlled. + /// The pong budget must be keyed on the ingress link (the directly + /// connected peer that delivered the packet): rotating forged sender IDs + /// over one link exhausts one budget instead of resetting it, so a single + /// malicious link cannot turn /ping into an amplification primitive. + @Test + func meshPingResponseBudget_isPerIngressLinkNotClaimedSender() async throws { + let ble = makeService() + let outbound = OutboundPacketTap() + ble._test_onOutboundPacket = outbound.record + + let link = PeerID(str: "1122334455667788") + let budget = TransportConfig.meshPingInboundMaxPerLink + let myRecipientData = try #require(Data(hexString: ble.myPeerID.id)) + + for i in 0..<(budget * 2) { + // A fresh forged sender for every ping, all arriving on one link. + let forgedSender = PeerID(str: String(format: "%016x", 0xA0_0000 + i)) + var nonce = Data(repeating: 0, count: MeshPingPayload.nonceLength) + nonce[0] = UInt8(i) + let payload = try #require(MeshPingPayload(nonce: nonce, originTTL: 7)) + let packet = BitchatPacket( + type: MessageType.ping.rawValue, + senderID: Data(hexString: forgedSender.id) ?? Data(), + recipientID: myRecipientData, + timestamp: UInt64(Date().timeIntervalSince1970 * 1000), + payload: payload.encode(), + signature: nil, + ttl: 7 + ) + ble._test_handlePacket(packet, fromPeerID: link, preseedPeer: false) + } + + let reachedBudget = await TestHelpers.waitUntil( + { outbound.count(ofType: .pong) >= budget }, + timeout: TestConstants.defaultTimeout + ) + #expect(reachedBudget) + // Give any over-budget pong a chance to surface, then confirm the + // rotated sender IDs never bought a sixth response. + let exceededBudget = await TestHelpers.waitUntil( + { outbound.count(ofType: .pong) > budget }, + timeout: TestConstants.shortTimeout + ) + #expect(!exceededBudget) + #expect(outbound.count(ofType: .pong) == budget) + } +} + +/// Thread-safe capture of packets leaving the service under test. +private final class OutboundPacketTap { + private let lock = NSLock() + private var packets: [BitchatPacket] = [] + + func record(_ packet: BitchatPacket) { + lock.lock(); packets.append(packet); lock.unlock() + } + + func count(ofType type: MessageType) -> Int { + lock.lock(); defer { lock.unlock() } + return packets.filter { $0.type == type.rawValue }.count + } } private func makeService() -> BLEService { diff --git a/bitchatTests/CommandProcessorTests.swift b/bitchatTests/CommandProcessorTests.swift index f5eab5f4..62899d3a 100644 --- a/bitchatTests/CommandProcessorTests.swift +++ b/bitchatTests/CommandProcessorTests.swift @@ -436,6 +436,7 @@ private final class MockCommandContextProvider: CommandContextProvider { private(set) var localPrivateSystemMessages: [(content: String, peerID: PeerID)] = [] private(set) var publicSystemMessages: [String] = [] private(set) var commandOutputs: [String] = [] + private(set) var commandOutputDestinations: [CommandOutputDestination] = [] private(set) var toggledFavorites: [PeerID] = [] private(set) var favoriteNotifications: [(peerID: PeerID, isFavorite: Bool)] = [] @@ -486,8 +487,16 @@ private final class MockCommandContextProvider: CommandContextProvider { publicSystemMessages.append(content) } - func addCommandOutput(_ content: String) { + func currentCommandDestination() -> CommandOutputDestination { + if let peerID = selectedPrivateChatPeer { + return .privateChat(peerID) + } + return .meshTimeline + } + + func addCommandOutput(_ content: String, to destination: CommandOutputDestination) { commandOutputs.append(content) + commandOutputDestinations.append(destination) } func toggleFavorite(peerID: PeerID) { diff --git a/bitchatTests/Services/MeshDiagnosticsTests.swift b/bitchatTests/Services/MeshDiagnosticsTests.swift index 142474f4..af22d393 100644 --- a/bitchatTests/Services/MeshDiagnosticsTests.swift +++ b/bitchatTests/Services/MeshDiagnosticsTests.swift @@ -129,6 +129,44 @@ struct MeshDiagnosticsTests { #expect(context.commandOutputs == ["no reply from alice"]) } + @MainActor + @Test func pingOutputRoutesToConversationWhereCommandWasIssued() async { + let context = DiagnosticsMockContext() + let alice = PeerID(str: "abcd1234abcd1234") + let bob = PeerID(str: "b0b0b0b0b0b0b0b0") + context.nicknameToPeerID["alice"] = alice + let transport = MockTransport() + transport.meshPingResult = MeshPingResult(rttMs: 42, hops: 2) + let processor = makeProcessor(context: context, transport: transport) + + // Issue the ping from bob's DM, then switch chats before the async + // result lands. The output must follow the origin conversation, not + // whatever is selected at callback time. + context.selectedPrivateChatPeer = bob + _ = processor.process("/ping @alice") + context.selectedPrivateChatPeer = nil + + await waitForCommandOutput(context) + #expect(context.commandOutputDestinations == [.privateChat(bob)]) + } + + @MainActor + @Test func pingIssuedFromPublicTimelineRoutesToMeshTimeline() async { + let context = DiagnosticsMockContext() + let alice = PeerID(str: "abcd1234abcd1234") + context.nicknameToPeerID["alice"] = alice + let transport = MockTransport() + transport.meshPingResult = MeshPingResult(rttMs: 7, hops: 1) + let processor = makeProcessor(context: context, transport: transport) + + _ = processor.process("/ping @alice") + // Opening a DM afterwards must not swallow the public-timeline result. + context.selectedPrivateChatPeer = alice + + await waitForCommandOutput(context) + #expect(context.commandOutputDestinations == [.meshTimeline]) + } + // MARK: - /trace @MainActor @@ -225,6 +263,7 @@ private final class DiagnosticsMockContext: CommandContextProvider { var nicknameToPeerID: [String: PeerID] = [:] private(set) var commandOutputs: [String] = [] + private(set) var commandOutputDestinations: [CommandOutputDestination] = [] func getPeerIDForNickname(_ nickname: String) -> PeerID? { nicknameToPeerID[nickname] @@ -241,7 +280,15 @@ private final class DiagnosticsMockContext: CommandContextProvider { func addPublicSystemMessage(_ content: String) {} func toggleFavorite(peerID: PeerID) {} - func addCommandOutput(_ content: String) { + func currentCommandDestination() -> CommandOutputDestination { + if let peerID = selectedPrivateChatPeer { + return .privateChat(peerID) + } + return .meshTimeline + } + + func addCommandOutput(_ content: String, to destination: CommandOutputDestination) { commandOutputs.append(content) + commandOutputDestinations.append(destination) } }