From 7ba946b73466ab421c7c97a47b0037826348163e Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 13:23:54 -0700 Subject: [PATCH] Fix the same claim-before-liveness bug in the authentication retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retrySecurePrivateMessagesAfterAuthentication` carries the bug this branch just fixed three lines away in the merged flush. Its guard opens with `retriedMessageIDs.insert(...).inserted`, and a comma-guard short-circuits left to right — so the insert lands even when a later clause rejects the candidate. A synchronous ack from an earlier send in the same loop is peer-scoped, so it can clear one copy while the twin under the other alias stays live and eligible; the dead candidate claims the ID first and the live twin is skipped, silently missing a retry that was due. Pre-existing on main, not introduced here: the guard is from #1462 and the two-alias call it needs is already in `ChatVerificationCoordinator`. I fixed it anyway because shipping the fix for one function while its twin sits broken next door is worse than the scope it adds. Happy to split it out. Reordering to check-then-claim is enough — the insert now runs only once everything else about the candidate is confirmed. Mutation-proven: restoring the original clause order fails the new test and nothing else. Also, from the same review: `OutboxCandidate` and the four-key sort comparator were duplicated verbatim between the two functions and are now shared; `flushOutbox(forAliases:)` no longer reuses one set for two different jobs; and `skippingSecurelyTransmitted` loses its default, since the wrong value there means a double-send and every caller should have to say what it means. 1957 app tests and 122 package tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 89 ++++++++++--------- .../Services/MessageRouterTests.swift | 53 ++++++++++- 2 files changed, 94 insertions(+), 48 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index e3ed1f54..257cdd84 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -587,13 +587,37 @@ final class MessageRouter { /// undecryptable remotely. Normal pre-handshake sends are intentionally /// absent from `secureTransmissions` because BLE already queues /// and drains them when authentication completes. + /// One retained message paired with the alias whose queue holds it, plus + /// the tie-breakers that make a merge across aliases deterministic. + private typealias OutboxCandidate = ( + peerID: PeerID, + message: QueuedMessage, + aliasOrder: Int, + queueOrder: Int + ) + + /// Merge candidates drawn from several alias queues into one chronological + /// stream, so the order the aliases happen to arrive in cannot send newer + /// mail ahead of older mail for the same conversation. + private static func chronologically( + _ candidates: [OutboxCandidate] + ) -> [OutboxCandidate] { + candidates.sorted { lhs, rhs in + if lhs.message.timestamp != rhs.message.timestamp { + return lhs.message.timestamp < rhs.message.timestamp + } + if lhs.aliasOrder != rhs.aliasOrder { + return lhs.aliasOrder < rhs.aliasOrder + } + if lhs.queueOrder != rhs.queueOrder { + return lhs.queueOrder < rhs.queueOrder + } + return lhs.message.messageID < rhs.message.messageID + } + } + func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) { - typealias Candidate = ( - peerID: PeerID, - message: QueuedMessage, - aliasOrder: Int, - queueOrder: Int - ) + typealias Candidate = OutboxCandidate var visitedPeerIDs = Set() var retriedMessageIDs = Set() @@ -625,28 +649,22 @@ final class MessageRouter { // ephemeral and stable outbox keys. Merge both queues into one // chronological stream so callback alias order cannot send newer mail // ahead of older mail. - candidates.sort { lhs, rhs in - if lhs.message.timestamp != rhs.message.timestamp { - return lhs.message.timestamp < rhs.message.timestamp - } - if lhs.aliasOrder != rhs.aliasOrder { - return lhs.aliasOrder < rhs.aliasOrder - } - if lhs.queueOrder != rhs.queueOrder { - return lhs.queueOrder < rhs.queueOrder - } - return lhs.message.messageID < rhs.message.messageID - } + candidates = Self.chronologically(candidates) for candidate in candidates { let peerID = candidate.peerID let message = candidate.message let key = PeerMessageKey(peerID: peerID, messageID: message.messageID) - guard retriedMessageIDs.insert(message.messageID).inserted, - secureTransmissions.contains(key), + // Claim the ID last. A synchronous ack from an earlier send in + // this loop is peer-scoped, so it can clear this copy while the + // twin under the other alias stays live and eligible — and a + // claim made before these checks would let the dead candidate + // suppress that twin, silently skipping a retry that was due. + guard secureTransmissions.contains(key), queuedMessage(message.messageID, for: peerID) != nil, let transport = connectedTransport(for: peerID), - transport.canDeliverSecurely(to: peerID) else { + transport.canDeliverSecurely(to: peerID), + retriedMessageIDs.insert(message.messageID).inserted else { continue } @@ -725,16 +743,10 @@ final class MessageRouter { /// of it — re-sending every message the retry just put on the air and /// burning a second attempt against the cap. Skipping that set makes the /// two passes disjoint by construction rather than by comment. - func flushOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool = false) { - typealias Candidate = ( - peerID: PeerID, - message: QueuedMessage, - aliasOrder: Int, - queueOrder: Int - ) + func flushOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + typealias Candidate = OutboxCandidate - var visitedPeerIDs = Set() - for peerID in peerIDAliases { visitedPeerIDs.insert(peerID) } + let aliasSet = Set(peerIDAliases) // The retry that precedes a skipping flush covers a message ID once, // under whichever alias holds the securely-transmitted copy. So the @@ -745,12 +757,12 @@ final class MessageRouter { // precisely the double-send this flag exists to prevent. var retriedMessageIDs = Set() if skippingSecurelyTransmitted { - for key in secureTransmissions where visitedPeerIDs.contains(key.peerID) { + for key in secureTransmissions where aliasSet.contains(key.peerID) { retriedMessageIDs.insert(key.messageID) } } - visitedPeerIDs.removeAll() + var visitedPeerIDs = Set() var candidates: [Candidate] = [] for (aliasOrder, peerID) in peerIDAliases.enumerated() { @@ -769,18 +781,7 @@ final class MessageRouter { guard !candidates.isEmpty else { return } - candidates.sort { lhs, rhs in - if lhs.message.timestamp != rhs.message.timestamp { - return lhs.message.timestamp < rhs.message.timestamp - } - if lhs.aliasOrder != rhs.aliasOrder { - return lhs.aliasOrder < rhs.aliasOrder - } - if lhs.queueOrder != rhs.queueOrder { - return lhs.queueOrder < rhs.queueOrder - } - return lhs.message.messageID < rhs.message.messageID - } + candidates = Self.chronologically(candidates) SecureLogger.debug( "Flushing merged outbox for \(peerIDAliases.count) alias(es) count=\(candidates.count)", diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index 722d1287..497040eb 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -167,6 +167,51 @@ struct MessageRouterTests { #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) } + /// The retry claims each message ID once across the alias set. It must not + /// claim one for a candidate that turns out to be dead: a synchronous ack + /// from an earlier send in the same loop is peer-scoped, so it can clear + /// one copy while the twin under the other alias stays live and eligible. + /// A claim made before that check would silently skip a retry that was due. + @Test @MainActor + func authenticationRetry_aDeadCandidateDoesNotSuppressTheLiveTwin() async { + let shortPeerID = PeerID(str: "0000000000000028") + let stablePeerID = PeerID(hexData: Data(repeating: 0x28, count: 32)) + let transport = MockTransport() + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + let clock = MutableTestClock() + let router = MessageRouter(transports: [transport], now: { clock.now }) + + // Oldest, so it is retried first and its ack lands mid-loop. + router.sendPrivate("First", to: shortPeerID, recipientNickname: "Peer", messageID: "r-first") + clock.now = clock.now.addingTimeInterval(1) + router.sendPrivate("Twin", to: shortPeerID, recipientNickname: "Peer", messageID: "r-twin") + clock.now = clock.now.addingTimeInterval(1) + router.sendPrivate("Twin", to: stablePeerID, recipientNickname: "Peer", messageID: "r-twin") + + // Flushing marks all three as securely transmitted, which is what puts + // them in the retry's candidate set. + router.flushOutbox(for: shortPeerID) + router.flushOutbox(for: stablePeerID) + transport.resetRecordings() + + // Retrying the first message synchronously acks the short-ID copy of + // the twin, scoped to that alias alone. + transport.onSendPrivateMessage = { messageID in + guard messageID == "r-first" else { return } + router.markDelivered("r-twin", for: [shortPeerID]) + } + + router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) + transport.onSendPrivateMessage = nil + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["r-first", "r-twin"], + "a copy acked mid-retry claimed the message ID and suppressed the live twin" + ) + #expect(transport.sentPrivateMessages.map(\.peerID) == [shortPeerID, stablePeerID]) + } + /// Reconnect drains both the ephemeral and the stable outbox key. Draining /// them as two sequential `flushOutbox(for:)` calls would put the newer /// short-ID mail on the air first; the merged flush must order by @@ -189,7 +234,7 @@ struct MessageRouterTests { transport.connectedPeers = [shortPeerID, stablePeerID] transport.securePeers = [shortPeerID, stablePeerID] - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) #expect(transport.sentPrivateMessages.map(\.messageID) == ["flush-old", "flush-new"]) #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) @@ -211,7 +256,7 @@ struct MessageRouterTests { transport.connectedPeers = [shortPeerID, stablePeerID] transport.securePeers = [shortPeerID, stablePeerID] - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) #expect(transport.sentPrivateMessages.map(\.messageID) == ["dup-1"]) @@ -220,7 +265,7 @@ struct MessageRouterTests { // holds the ID, so the copy the flush passed over goes with it. router.markDelivered("dup-1", for: [shortPeerID, stablePeerID]) transport.resetRecordings() - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) #expect( transport.sentPrivateMessages.isEmpty, "the copy the merged flush skipped was re-sent after the ack" @@ -336,7 +381,7 @@ struct MessageRouterTests { router.markDelivered("gone-1", for: [shortPeerID]) } - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) transport.onSendPrivateMessage = nil #expect(