mirror of
https://github.com/permissionlesstech/bitchat.git
synced 2026-07-24 21:45:20 +00:00
Fix panic-wipe window, frame byte bound, and delivery-side test waits
Three confirmed defects from adversarial review of the per-relay inbound pipeline (PR #1352): - MEDIUM-2: a detached gift-wrap decrypt spawned just before a panic wipe strongly captures the pre-wipe Nostr private key and could deliver plaintext into ChatViewModel after the wipe. Add a per-pipeline monotonic wipe generation (NostrInboundPipeline.wipeGeneration), bumped from panicClearAllData via invalidateInFlightDecrypts(); spawn sites capture it and every main-actor hop drops the task's result on mismatch. The account-mailbox path (processNostrMessage) had the same pre-existing hazard and gets the identical guard, capturing the generation atomically with the identity fetch. - MEDIUM-1: the per-relay stream cap bounds FRAMES (256) but not BYTES; with the URLSession default of 1 MiB per WebSocket frame a hostile relay could pile up ~256 MiB. Set URLSessionWebSocketTask .maximumMessageSize to TransportConfig.nostrInboundMaxFrameBytes (512 KiB — an order of magnitude above any legitimate Nostr event or gift wrap we produce or expect), halving the worst case to 128 MiB, and correct the "cannot exhaust memory" comments to state the actual cap × maxFrameBytes bound. - LOW-5: three NostrRelayManagerTests gated on messagesReceived (first main hop) then immediately asserted delivery-side state that only lands after off-main verification plus a second main hop. Wait on the delivery-side state (receivedIDs / duplicate-drop counts) directly, keeping all assertions. Also: brief comment documenting pendingGiftWrapIDs growth (LOW-6) and a regression test that a panic wipe issued after spawn drops the decrypted result while leaving the pipeline usable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -48,7 +48,14 @@ private struct URLSessionAdapter: NostrRelaySessionProtocol {
|
||||
let base: URLSession
|
||||
|
||||
func webSocketTask(with url: URL) -> NostrRelayConnectionProtocol {
|
||||
URLSessionWebSocketTaskAdapter(base: base.webSocketTask(with: url))
|
||||
let task = base.webSocketTask(with: url)
|
||||
// Byte bound per inbound frame; without it the per-relay buffer cap
|
||||
// (nostrInboundPerRelayBufferCap) bounds FRAMES but not BYTES, and a
|
||||
// hostile relay could pile up cap × 1 MiB (URLSession default) per
|
||||
// connection. See TransportConfig.nostrInboundMaxFrameBytes for the
|
||||
// sizing rationale. Oversized frames fail the receive with an error.
|
||||
task.maximumMessageSize = TransportConfig.nostrInboundMaxFrameBytes
|
||||
return URLSessionWebSocketTaskAdapter(base: task)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -106,7 +113,10 @@ private extension NostrRelayManagerDependencies {
|
||||
@MainActor
|
||||
final class NostrRelayManager: ObservableObject {
|
||||
static let shared = NostrRelayManager()
|
||||
// Track gift-wraps (kind 1059) we initiated so we can log OK acks at info
|
||||
// Track gift-wraps (kind 1059) we initiated so we can log OK acks at info.
|
||||
// Entries are removed only on OK acks (or panic wipe); relays that never
|
||||
// ack leave entries behind for the process lifetime. Observability-only
|
||||
// state, bounded in practice by outbound DM volume.
|
||||
private(set) static var pendingGiftWrapIDs = Set<String>()
|
||||
static func registerPendingGiftWrap(id: String) {
|
||||
pendingGiftWrapIDs.insert(id)
|
||||
@@ -1427,7 +1437,9 @@ private final class InboundFrameRouter: @unchecked Sendable {
|
||||
/// Start a relay's stream + consumer if one does not already exist.
|
||||
/// Returns true when a new pipeline was created. The bounded
|
||||
/// `.bufferingNewest` policy makes a single relay shed its OWN oldest
|
||||
/// frames under a flood, never other relays' frames and never memory.
|
||||
/// frames under a flood, never other relays' frames. Buffered memory per
|
||||
/// relay is bounded (not eliminated) at the frame cap times the per-frame
|
||||
/// byte cap (`maximumMessageSize`) — see TransportConfig.
|
||||
func startPipeline(
|
||||
for relayUrl: String,
|
||||
makeConsumer: (AsyncStream<InboundFrame>) -> Task<Void, Never>
|
||||
|
||||
@@ -58,11 +58,23 @@ enum TransportConfig {
|
||||
// Bounded per-relay inbound frame buffer. Each relay connection owns its
|
||||
// own serial verify pipeline; if a relay floods faster than its Schnorr
|
||||
// verification drains, the oldest buffered frames for THAT relay are
|
||||
// dropped (bufferingNewest) so one relay can neither exhaust memory nor
|
||||
// stall other relays. Nostr inbound is already best-effort (relays are
|
||||
// redundant and events replay), so dropping a flooding relay's backlog is
|
||||
// safe.
|
||||
// dropped (bufferingNewest) so one relay cannot stall other relays.
|
||||
// Nostr inbound is already best-effort (relays are redundant and events
|
||||
// replay), so dropping a flooding relay's backlog is safe. Together with
|
||||
// nostrInboundMaxFrameBytes this caps buffered inbound bytes at
|
||||
// cap × maxFrameBytes (128 MiB) per hostile relay — bounded, not zero.
|
||||
static let nostrInboundPerRelayBufferCap: Int = 256
|
||||
// Hard per-frame byte bound, applied as URLSessionWebSocketTask
|
||||
// .maximumMessageSize (oversized frames fail the receive instead of
|
||||
// buffering). BitChat's legitimate Nostr traffic is small: geohash chat /
|
||||
// presence events (kind 20000/20001), kind-1 notes, and NIP-17
|
||||
// gift-wrapped DMs carrying text payloads or receipts are all a few KiB,
|
||||
// and most public relays reject events beyond ~64–256 KiB anyway. 512 KiB
|
||||
// leaves an order-of-magnitude margin over anything we produce or expect
|
||||
// while halving the URLSession default (1 MiB), so a hostile relay's
|
||||
// worst-case buffered pile-up per connection is
|
||||
// nostrInboundPerRelayBufferCap × 512 KiB = 128 MiB instead of 256 MiB.
|
||||
static let nostrInboundMaxFrameBytes: Int = 512 * 1024
|
||||
|
||||
// Conversation store diagnostics (field observability)
|
||||
// Sample interval for the periodic store-audit "OK" heartbeat line
|
||||
|
||||
@@ -1177,6 +1177,11 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele
|
||||
// Geohash DM handlers can capture pre-wipe Nostr identities, so a plain
|
||||
// disconnect is not enough here.
|
||||
NostrRelayManager.shared.resetForPanicWipe()
|
||||
// Clearing relay handlers stops NEW events, but a detached gift-wrap
|
||||
// decrypt spawned just before the wipe still holds a pre-wipe key and
|
||||
// ciphertext; bump the pipeline's wipe generation so its result is
|
||||
// dropped at the main-actor delivery hop instead of landing here.
|
||||
nostrCoordinator.inbound.invalidateInFlightDecrypts()
|
||||
nostrRelayManager = nil
|
||||
|
||||
// Clear Nostr identity associations
|
||||
|
||||
@@ -91,6 +91,22 @@ final class NostrInboundPipeline {
|
||||
private let presence: GeoPresenceTracker
|
||||
private var geoEventLogCount = 0
|
||||
|
||||
/// Monotonic panic-wipe generation for this pipeline. A panic wipe clears
|
||||
/// relay handlers so no NEW events flow, but a detached decrypt task
|
||||
/// spawned just BEFORE the wipe — which strongly captures a pre-wipe Nostr
|
||||
/// private key and ciphertext — survives it. Spawn sites capture this
|
||||
/// value; the task compares it at its main-actor hops and drops its result
|
||||
/// (no delivery; the captured identity and plaintext die with the task)
|
||||
/// if `invalidateInFlightDecrypts()` bumped it in between.
|
||||
@MainActor private(set) var wipeGeneration: UInt64 = 0
|
||||
|
||||
/// Called from `ChatViewModel.panicClearAllData()` so plaintext decrypted
|
||||
/// with pre-wipe keys can never land in post-wipe state.
|
||||
@MainActor
|
||||
func invalidateInFlightDecrypts() {
|
||||
wipeGeneration &+= 1
|
||||
}
|
||||
|
||||
init(context: any NostrInboundPipelineContext, presence: GeoPresenceTracker) {
|
||||
self.context = context
|
||||
self.presence = presence
|
||||
@@ -269,8 +285,13 @@ final class NostrInboundPipeline {
|
||||
// once, off the main actor) by NostrRelayManager.
|
||||
guard !context.hasProcessedNostrEvent(giftWrap.id) else { return }
|
||||
|
||||
// Capture the wipe generation at spawn, alongside the per-geohash
|
||||
// identity (private key) the detached task strongly captures. A panic
|
||||
// wipe between spawn and delivery bumps the generation, and the task
|
||||
// drops its result instead of delivering plaintext post-wipe.
|
||||
let wipeGeneration = self.wipeGeneration
|
||||
Task.detached(priority: .userInitiated) { [weak self] in
|
||||
await self?.processGeohashGiftWrap(giftWrap, id: id, verbose: false)
|
||||
await self?.processGeohashGiftWrap(giftWrap, id: id, verbose: false, wipeGeneration: wipeGeneration)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -282,8 +303,10 @@ final class NostrInboundPipeline {
|
||||
return
|
||||
}
|
||||
|
||||
// Spawn-time wipe-generation capture; see subscribeGiftWrap.
|
||||
let wipeGeneration = self.wipeGeneration
|
||||
Task.detached(priority: .userInitiated) { [weak self] in
|
||||
await self?.processGeohashGiftWrap(giftWrap, id: id, verbose: true)
|
||||
await self?.processGeohashGiftWrap(giftWrap, id: id, verbose: true, wipeGeneration: wipeGeneration)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -291,11 +314,24 @@ final class NostrInboundPipeline {
|
||||
/// layers) runs off the main actor; results hop back for state updates.
|
||||
/// `verbose` keeps `handleGiftWrap`'s decrypt logging without adding it
|
||||
/// to the sampling path.
|
||||
private func processGeohashGiftWrap(_ giftWrap: NostrEvent, id: NostrIdentity, verbose: Bool) async {
|
||||
///
|
||||
/// `wipeGeneration` is this pipeline's generation captured at spawn (the
|
||||
/// moment the pre-wipe `id` was captured); a mismatch at either main-actor
|
||||
/// hop means a panic wipe happened in between, so the task bails without
|
||||
/// decrypting (first hop) or without delivering the plaintext (second
|
||||
/// hop) — the captured identity and any decrypted material are simply
|
||||
/// dropped with the task.
|
||||
private func processGeohashGiftWrap(
|
||||
_ giftWrap: NostrEvent,
|
||||
id: NostrIdentity,
|
||||
verbose: Bool,
|
||||
wipeGeneration: UInt64
|
||||
) async {
|
||||
guard let context else { return }
|
||||
// Authoritative check-and-record, atomic on the main actor so two
|
||||
// concurrent detached tasks can't both process the same event.
|
||||
let alreadyProcessed: Bool = await MainActor.run {
|
||||
guard self.wipeGeneration == wipeGeneration else { return true }
|
||||
if context.hasProcessedNostrEvent(giftWrap.id) { return true }
|
||||
context.recordProcessedNostrEvent(giftWrap.id)
|
||||
return false
|
||||
@@ -320,6 +356,10 @@ final class NostrInboundPipeline {
|
||||
}
|
||||
|
||||
await MainActor.run {
|
||||
// A panic wipe during the off-main decrypt must not let the
|
||||
// pre-wipe plaintext reach post-wipe state; drop it here, atomic
|
||||
// with the wipe on the main actor.
|
||||
guard self.wipeGeneration == wipeGeneration else { return }
|
||||
guard let packet = Self.decodeEmbeddedBitChatPacket(from: content),
|
||||
packet.type == MessageType.noiseEncrypted.rawValue,
|
||||
let payload = NoisePayload.decode(packet.payload)
|
||||
@@ -376,8 +416,13 @@ final class NostrInboundPipeline {
|
||||
return false
|
||||
}
|
||||
if alreadyProcessed { return }
|
||||
let currentIdentity: NostrIdentity? = await MainActor.run {
|
||||
context.currentNostrIdentity()
|
||||
// Fetch the identity and the wipe generation in ONE main-actor hop:
|
||||
// the generation then vouches for exactly this identity. A wipe after
|
||||
// this point bumps the generation and the delivery hop below drops
|
||||
// the decrypted result (same guard as processGeohashGiftWrap; this
|
||||
// account-mailbox path had the identical hazard).
|
||||
let (currentIdentity, wipeGeneration): (NostrIdentity?, UInt64) = await MainActor.run {
|
||||
(context.currentNostrIdentity(), self.wipeGeneration)
|
||||
}
|
||||
guard let currentIdentity else { return }
|
||||
|
||||
@@ -409,6 +454,9 @@ final class NostrInboundPipeline {
|
||||
let payload = NoisePayload.decode(packet.payload) {
|
||||
let messageTimestamp = Date(timeIntervalSince1970: TimeInterval(rumorTimestamp))
|
||||
await MainActor.run {
|
||||
// Drop pre-wipe plaintext if a panic wipe landed
|
||||
// during the off-main decrypt (see above).
|
||||
guard self.wipeGeneration == wipeGeneration else { return }
|
||||
context.registerNostrKeyMapping(senderPubkey, for: targetPeerID)
|
||||
|
||||
switch payload.type {
|
||||
|
||||
@@ -405,6 +405,45 @@ struct ChatNostrCoordinatorContextTests {
|
||||
#expect(context.handledPrivateMessages.count == 1)
|
||||
}
|
||||
|
||||
@Test @MainActor
|
||||
func handleGiftWrap_panicWipeAfterSpawnDropsDecryptedResult() async throws {
|
||||
let context = MockChatNostrContext()
|
||||
let coordinator = ChatNostrCoordinator(context: context)
|
||||
|
||||
let recipient = try NostrIdentity.generate()
|
||||
let sender = try NostrIdentity.generate()
|
||||
let embedded = try #require(NostrEmbeddedBitChat.encodePMForNostrNoRecipient(
|
||||
content: "pre-wipe secret",
|
||||
messageID: "gm-wipe-1",
|
||||
senderPeerID: PeerID(str: "aabbccddeeff0011")
|
||||
))
|
||||
let giftWrap = try NostrProtocol.createPrivateMessage(
|
||||
content: embedded,
|
||||
recipientPubkey: recipient.publicKeyHex,
|
||||
senderIdentity: sender
|
||||
)
|
||||
|
||||
// Spawn the detached decrypt (it strongly captures the pre-wipe
|
||||
// identity), then panic-wipe in the SAME main-actor turn — guaranteed
|
||||
// to land before the task's first main-actor hop.
|
||||
coordinator.inbound.handleGiftWrap(giftWrap, id: recipient)
|
||||
coordinator.inbound.invalidateInFlightDecrypts()
|
||||
|
||||
// Give the detached task ample time to have delivered if the wipe
|
||||
// guard were broken.
|
||||
try? await Task.sleep(nanoseconds: 200_000_000)
|
||||
await drainMainQueue()
|
||||
|
||||
#expect(context.handledPrivateMessages.isEmpty)
|
||||
#expect(context.recordedNostrEventIDs.isEmpty)
|
||||
|
||||
// The pipeline itself stays usable: a gift wrap spawned AFTER the
|
||||
// wipe (new generation) still decrypts and delivers.
|
||||
coordinator.inbound.handleGiftWrap(giftWrap, id: recipient)
|
||||
let delivered = await TestHelpers.waitUntil({ context.handledPrivateMessages.count == 1 })
|
||||
#expect(delivered)
|
||||
}
|
||||
|
||||
// NOTE: Inbound Schnorr signature verification (and the forged-copy
|
||||
// dedup-poisoning invariant) is enforced once, off the main actor, at the
|
||||
// relay boundary — see NostrRelayManagerTests
|
||||
|
||||
@@ -639,11 +639,15 @@ final class NostrRelayManagerTests: XCTestCase {
|
||||
try context.sessionFactory.latestConnection(for: firstRelayURL)?.emitEventMessage(subscriptionID: "events", event: event)
|
||||
try context.sessionFactory.latestConnection(for: secondRelayURL)?.emitEventMessage(subscriptionID: "events", event: event)
|
||||
|
||||
let countedOnBothRelays = await waitUntil {
|
||||
// Wait on the DELIVERY-side state: handler dispatch and the duplicate
|
||||
// drop both land on the second main hop, after off-main verification.
|
||||
let settled = await waitUntil {
|
||||
context.manager.relays.first(where: { $0.url == firstRelayURL })?.messagesReceived == 1 &&
|
||||
context.manager.relays.first(where: { $0.url == secondRelayURL })?.messagesReceived == 1
|
||||
context.manager.relays.first(where: { $0.url == secondRelayURL })?.messagesReceived == 1 &&
|
||||
receivedIDs == [event.id] &&
|
||||
context.manager.debugDuplicateInboundEventDropCount == 1
|
||||
}
|
||||
XCTAssertTrue(countedOnBothRelays)
|
||||
XCTAssertTrue(settled)
|
||||
XCTAssertEqual(receivedIDs, [event.id])
|
||||
XCTAssertEqual(context.manager.debugDuplicateInboundEventDropCount, 1)
|
||||
XCTAssertEqual(context.manager.debugDuplicateInboundEventDropCount(forSubscriptionID: "events"), 1)
|
||||
@@ -676,12 +680,17 @@ final class NostrRelayManagerTests: XCTestCase {
|
||||
)
|
||||
}
|
||||
|
||||
let countedOnEveryRelay = await waitUntil {
|
||||
// Wait on the DELIVERY-side state: the winner's handler dispatch and
|
||||
// the losers' duplicate drops both land on the second main hop, after
|
||||
// off-main verification — messagesReceived (first hop) settles sooner.
|
||||
let settled = await waitUntil {
|
||||
relayURLs.allSatisfy { relayURL in
|
||||
context.manager.relays.first(where: { $0.url == relayURL })?.messagesReceived == 1
|
||||
}
|
||||
} &&
|
||||
receivedIDs == [event.id] &&
|
||||
context.manager.debugDuplicateInboundEventDropCount == relayURLs.count - 1
|
||||
}
|
||||
XCTAssertTrue(countedOnEveryRelay)
|
||||
XCTAssertTrue(settled)
|
||||
XCTAssertEqual(receivedIDs, [event.id])
|
||||
XCTAssertEqual(context.manager.debugDuplicateInboundEventDropCount, relayURLs.count - 1)
|
||||
XCTAssertEqual(
|
||||
@@ -714,11 +723,15 @@ final class NostrRelayManagerTests: XCTestCase {
|
||||
try context.sessionFactory.latestConnection(for: firstRelayURL)?.emitEventMessage(subscriptionID: "events", event: invalidEvent)
|
||||
try context.sessionFactory.latestConnection(for: secondRelayURL)?.emitEventMessage(subscriptionID: "events", event: event)
|
||||
|
||||
let countedOnBothRelays = await waitUntil {
|
||||
// Wait on the DELIVERY-side state (second main hop, after off-main
|
||||
// verification), not just messagesReceived (first main hop) — the
|
||||
// handler only fires after verify + a second hop.
|
||||
let genuineDelivered = await waitUntil {
|
||||
context.manager.relays.first(where: { $0.url == firstRelayURL })?.messagesReceived == 1 &&
|
||||
context.manager.relays.first(where: { $0.url == secondRelayURL })?.messagesReceived == 1
|
||||
context.manager.relays.first(where: { $0.url == secondRelayURL })?.messagesReceived == 1 &&
|
||||
receivedIDs == [event.id]
|
||||
}
|
||||
XCTAssertTrue(countedOnBothRelays)
|
||||
XCTAssertTrue(genuineDelivered)
|
||||
XCTAssertEqual(receivedIDs, [event.id])
|
||||
XCTAssertEqual(context.manager.debugDuplicateInboundEventDropCount, 0)
|
||||
XCTAssertEqual(context.manager.debugDuplicateInboundEventDropCount(forSubscriptionID: "events"), 0)
|
||||
@@ -758,11 +771,15 @@ final class NostrRelayManagerTests: XCTestCase {
|
||||
try context.sessionFactory.latestConnection(for: firstRelayURL)?.emitEventMessage(subscriptionID: "gift-wraps", event: tampered)
|
||||
try context.sessionFactory.latestConnection(for: secondRelayURL)?.emitEventMessage(subscriptionID: "gift-wraps", event: giftWrap)
|
||||
|
||||
let countedOnBothRelays = await waitUntil {
|
||||
// Wait on the DELIVERY-side state (second main hop, after off-main
|
||||
// verification), not just messagesReceived (first main hop) — the
|
||||
// handler only fires after verify + a second hop.
|
||||
let genuineDelivered = await waitUntil {
|
||||
context.manager.relays.first(where: { $0.url == firstRelayURL })?.messagesReceived == 1 &&
|
||||
context.manager.relays.first(where: { $0.url == secondRelayURL })?.messagesReceived == 1
|
||||
context.manager.relays.first(where: { $0.url == secondRelayURL })?.messagesReceived == 1 &&
|
||||
receivedIDs == [giftWrap.id]
|
||||
}
|
||||
XCTAssertTrue(countedOnBothRelays)
|
||||
XCTAssertTrue(genuineDelivered)
|
||||
XCTAssertEqual(receivedIDs, [giftWrap.id])
|
||||
XCTAssertEqual(context.manager.debugDuplicateInboundEventDropCount, 0)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user