From cc6e2bdad4881808b766a1370e7b74a5ccb448c4 Mon Sep 17 00:00:00 2001 From: jack Date: Sun, 26 Jul 2026 13:44:10 +0200 Subject: [PATCH] Actually delete legacy incoming media on explicit delete and /clear Explicit deletion of LEGACY (non-stable-ID) incoming media left the decrypted payload on disk indefinitely, waiting for 100MB oldest-first quota cleanup. Restore real deletion safely with the machinery this PR built: BLEIncomingFileStore.removeLegacyIncomingFile unlinks under the payload-coordination lock only when the path is not pending delivery, not held by a deletion reservation, and not owned by a stable receipt or journal entry; otherwise the file keeps the leave-for-quota fallback. The view model routes per-bubble deletes and /clear through the gated unlink once no bubble in any conversation references the basename. Co-Authored-By: Claude Opus 4.8 --- .../Services/BLE/BLEIncomingFileStore.swift | 77 ++++++++++ bitchat/Services/BLE/BLEService.swift | 8 + bitchat/Services/Transport.swift | 8 + .../ChatMediaTransferCoordinator.swift | 53 +++++-- bitchat/ViewModels/ChatViewModel.swift | 6 +- bitchatTests/ChatViewModelTests.swift | 144 ++++++++++++------ bitchatTests/Mocks/MockTransport.swift | 12 ++ .../BLEFileTransferHandlerTests.swift | 97 ++++++++++++ 8 files changed, 350 insertions(+), 55 deletions(-) diff --git a/bitchat/Services/BLE/BLEIncomingFileStore.swift b/bitchat/Services/BLE/BLEIncomingFileStore.swift index c3536675..5fea056b 100644 --- a/bitchat/Services/BLE/BLEIncomingFileStore.swift +++ b/bitchat/Services/BLE/BLEIncomingFileStore.swift @@ -392,6 +392,83 @@ struct BLEIncomingFileStore: @unchecked Sendable { ) } + /// Explicit deletion of a LEGACY (non-stable-ID) incoming payload. + /// + /// Legacy media has no durable receipt, so the only safe unlink is one + /// that can prove no other owner may hold the basename: the path must + /// not be pending delivery, must not belong to an in-flight deletion + /// reservation, and must not be owned by a stable receipt or journal + /// entry. When any of those hold — or receipt state cannot be read — + /// the file stays for bounded quota cleanup (the fail-safe fallback). + /// Returns true only when the payload was verifiably unlinked. + @discardableResult + func removeLegacyIncomingFile(relativePath: String) -> Bool { + payloadCoordination.lock.lock() + defer { payloadCoordination.lock.unlock() } + + guard let payload = incomingPayloadURL( + relativePath: relativePath + ) else { + return false + } + let standardizedPath = payload.standardizedFileURL.path + let reservedByDeletion = payloadCoordination.deletionReservations + .values.contains { $0.contains(standardizedPath) } + guard !reservedByDeletion, + !payloadCoordination.pendingDeliveryPaths.contains( + standardizedPath + ), + let receiptOwnedPaths = + privateMediaReceipts.reservedPayloadPaths(), + !receiptOwnedPaths.contains(standardizedPath) else { + return false + } + guard fileManager.fileExists(atPath: payload.path), + (try? payload.resourceValues( + forKeys: [.isRegularFileKey] + ).isRegularFile) == true else { + return false + } + do { + try fileManager.removeItem(at: payload) + return !fileManager.fileExists(atPath: payload.path) + } catch { + SecureLogger.warning( + "⚠️ Failed to remove explicitly deleted legacy media: \(error)", + category: .session + ) + return false + } + } + + /// Resolves a `files/`-relative path iff it lands directly inside one of + /// the incoming media directories. Anything else is not a deletable + /// incoming payload. + private func incomingPayloadURL(relativePath: String) -> URL? { + guard !relativePath.isEmpty, + let base = try? filesDirectory().standardizedFileURL else { + return nil + } + let candidate = base + .appendingPathComponent(relativePath, isDirectory: false) + .standardizedFileURL + let parentPath = candidate.deletingLastPathComponent().path + let incomingDirectories = [ + "voicenotes/incoming", + "images/incoming", + "files/incoming" + ] + guard incomingDirectories.contains(where: { relativeDirectory in + base.appendingPathComponent( + relativeDirectory, + isDirectory: true + ).standardizedFileURL.path == parentPath + }) else { + return nil + } + return candidate + } + /// Releases the short window between disk save and synchronous /// conversation insertion. Before this callback, a deletion transaction /// may not infer ownership from a stale bubble that names the same path. diff --git a/bitchat/Services/BLE/BLEService.swift b/bitchat/Services/BLE/BLEService.swift index e36ea784..b04cce5a 100644 --- a/bitchat/Services/BLE/BLEService.swift +++ b/bitchat/Services/BLE/BLEService.swift @@ -4511,6 +4511,14 @@ extension BLEService: PrivateMediaDeletionPersisting { } } } + + @MainActor + func removeLegacyPrivateMediaPayload(relativePath: String) { + let fileStore = incomingFileStore + messageQueue.async(flags: .barrier) { + fileStore.removeLegacyIncomingFile(relativePath: relativePath) + } + } } // MARK: - Private Helpers diff --git a/bitchat/Services/Transport.swift b/bitchat/Services/Transport.swift index 6515d38a..ad4d3cec 100644 --- a/bitchat/Services/Transport.swift +++ b/bitchat/Services/Transport.swift @@ -108,6 +108,14 @@ protocol PrivateMediaDeletionPersisting: AnyObject { protectedPayloadRelativePaths: Set, completion: @escaping @MainActor (Bool) -> Void ) + + /// Gated unlink for a LEGACY (non-stable-ID) incoming payload whose + /// bubble was explicitly removed. Implementations delete the file only + /// when its path is not pending delivery and not reserved by any receipt + /// or in-flight deletion transaction; otherwise the file stays for + /// bounded quota cleanup. + @MainActor + func removeLegacyPrivateMediaPayload(relativePath: String) } protocol TransportEventDelegate: AnyObject { diff --git a/bitchat/ViewModels/ChatMediaTransferCoordinator.swift b/bitchat/ViewModels/ChatMediaTransferCoordinator.swift index 670158db..671b7683 100644 --- a/bitchat/ViewModels/ChatMediaTransferCoordinator.swift +++ b/bitchat/ViewModels/ChatMediaTransferCoordinator.swift @@ -226,18 +226,51 @@ extension ChatViewModel: ChatMediaTransferContext { let message = conversations.conversationsByID.values.lazy .flatMap(\.messages) .first { $0.id == messageID } - if let message { - if !isIncomingPrivateMessage(message) { - mediaTransferCoordinator.cleanupOutgoingLocalFile( - forMessage: message - ) - } - // Legacy/raw incoming media has no durable ID-to-file ownership. - // Its basename may already belong to a pending newer arrival, so - // leave the payload for bounded quota cleanup rather than unlink - // an ambiguous path. + if let message, !isIncomingPrivateMessage(message) { + mediaTransferCoordinator.cleanupOutgoingLocalFile( + forMessage: message + ) } removeMessage(withID: messageID, cleanupFile: false) + if let message { + cleanupLegacyIncomingMediaPayloads(for: [message]) + } + } + + /// Explicitly deleted LEGACY (non-stable-ID) incoming media has no + /// durable ID-to-file ownership, so the actual unlink is delegated to + /// the transport's gated cleanup: a basename that is pending delivery or + /// reserved by a receipt/deletion transaction stays on disk for bounded + /// quota cleanup instead. Must run after the bubbles were removed; a + /// surviving reference in any conversation keeps the payload. + func cleanupLegacyIncomingMediaPayloads(for messages: [BitchatMessage]) { + guard let cleanup = + meshService as? any PrivateMediaDeletionPersisting else { + return + } + let legacyPaths = Set(messages.compactMap { message -> String? in + guard !PrivateMediaMessageIdentity.isStableID(message.id), + isIncomingPrivateMessage(message) else { + return nil + } + return incomingMediaRelativePath(for: message) + }) + guard !legacyPaths.isEmpty else { return } + let survivingPaths = Set( + conversations.conversationsByID.values.lazy + .flatMap(\.messages) + .compactMap { message -> String? in + guard self.isIncomingPrivateMessage(message) else { + return nil + } + return self.incomingMediaRelativePath(for: message) + } + ) + for relativePath in legacyPaths.subtracting(survivingPaths).sorted() { + cleanup.removeLegacyPrivateMediaPayload( + relativePath: relativePath + ) + } } func removeOutgoingMediaMessage(withID messageID: String) { diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index 4ec2de2c..ee94b34b 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -908,8 +908,9 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, SynchronousMessage // Stable payload cleanup belongs entirely to the durable receiver // journal. Legacy/raw incoming payloads have no durable identity, - // so their basenames may already belong to a pending new arrival; - // leave those files for bounded quota cleanup. + // so once their bubbles are gone the transport's gated cleanup + // decides per basename: unlink when unreferenced, or leave any + // pending/reserved path for bounded quota cleanup. let finalPlan = currentRemovalPlan() for (conversationID, messageIDs) in finalPlan { @@ -917,6 +918,7 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, SynchronousMessage messageIDs.contains($0.id) } } + cleanupLegacyIncomingMediaPayloads(for: capturedIncomingMedia) completion() } diff --git a/bitchatTests/ChatViewModelTests.swift b/bitchatTests/ChatViewModelTests.swift index f384c963..e640c5bc 100644 --- a/bitchatTests/ChatViewModelTests.swift +++ b/bitchatTests/ChatViewModelTests.swift @@ -1278,47 +1278,6 @@ struct ChatViewModelPrivateMediaDeletionTests { ) } - @Test @MainActor - func legacyIncomingDeleteLeavesAmbiguousPayloadForQuota() throws { - let (viewModel, transport) = makeTestableViewModel() - let peerID = PeerID(str: String(repeating: "3", count: 64)) - let incoming = try FileManager.default.url( - for: .applicationSupportDirectory, - in: .userDomainMask, - appropriateFor: nil, - create: true - ) - .appendingPathComponent( - "files/images/incoming", - isDirectory: true - ) - try FileManager.default.createDirectory( - at: incoming, - withIntermediateDirectories: true - ) - let payload = incoming.appendingPathComponent( - "legacy-delete-\(UUID().uuidString).jpg" - ) - try Data([0x01]).write(to: payload) - defer { try? FileManager.default.removeItem(at: payload) } - let legacyID = UUID().uuidString - viewModel.seedPrivateChat([ - privateMediaMessage( - id: legacyID, - sender: "Old client", - senderPeerID: peerID, - recipient: viewModel.nickname, - filename: payload.lastPathComponent - ) - ], for: peerID) - - viewModel.deleteMediaMessage(messageID: legacyID) - - #expect(transport.deletedPrivateMediaMessageIDBatches.isEmpty) - #expect((viewModel.privateChats[peerID] ?? []).isEmpty) - #expect(FileManager.default.fileExists(atPath: payload.path)) - } - @Test @MainActor func clearPrivateChatTombstonesIncomingAndCancelsOutgoingBeforeClear() { let (viewModel, transport) = makeTestableViewModel() @@ -1874,7 +1833,7 @@ struct ChatViewModelPrivateMediaDeletionTests { } @Test @MainActor - func clearPrivateChatLeavesAmbiguousLegacyFileForQuotaCleanup() + func clearPrivateChatUnlinksLegacyFileOnlyAfterLastReference() throws { let (viewModel, transport) = makeTestableViewModel() let firstPeerID = PeerID(str: String(repeating: "9", count: 64)) @@ -1907,14 +1866,113 @@ struct ChatViewModelPrivateMediaDeletionTests { viewModel.clearPrivateChat(firstPeerID) + // A surviving mirror in another conversation keeps the payload. #expect(transport.deletedPrivateMediaMessageIDBatches.isEmpty) + #expect(transport.removedLegacyPrivateMediaPaths.isEmpty) #expect(FileManager.default.fileExists(atPath: fileURL.path)) #expect(viewModel.privateChats[aliasPeerID]?.map(\.id) == [message.id]) viewModel.clearPrivateChat(aliasPeerID) - #expect(FileManager.default.fileExists(atPath: fileURL.path)) + // Clearing the last reference routes the legacy payload through the + // transport's gated unlink, which deletes it (nothing pending or + // reserved names this basename). #expect((viewModel.privateChats[aliasPeerID] ?? []).isEmpty) + #expect( + transport.removedLegacyPrivateMediaPaths + == ["images/incoming/\(fileURL.lastPathComponent)"] + ) + #expect(!FileManager.default.fileExists(atPath: fileURL.path)) + } + + @Test @MainActor + func deleteLegacyIncomingMediaUnlinksUnreferencedPayload() throws { + let (viewModel, transport) = makeTestableViewModel() + let peerID = PeerID(str: String(repeating: "b", count: 64)) + let incomingDirectory = try FileManager.default.url( + for: .applicationSupportDirectory, + in: .userDomainMask, + appropriateFor: nil, + create: true + ) + .appendingPathComponent("files/images/incoming", isDirectory: true) + try FileManager.default.createDirectory( + at: incomingDirectory, + withIntermediateDirectories: true + ) + let fileURL = incomingDirectory.appendingPathComponent( + "legacy-delete-\(UUID().uuidString).jpg" + ) + try Data("legacy-image".utf8).write(to: fileURL, options: .atomic) + defer { try? FileManager.default.removeItem(at: fileURL) } + let message = privateMediaMessage( + id: UUID().uuidString, + sender: "Old client", + senderPeerID: peerID, + recipient: viewModel.nickname, + filename: fileURL.lastPathComponent + ) + viewModel.seedPrivateChat([message], for: peerID) + + viewModel.deleteMediaMessage(messageID: message.id) + + // Legacy incoming media has no stable receipt, so no journal batch — + // but the explicit delete must still remove the decrypted payload + // through the gated unlink. + #expect(transport.deletedPrivateMediaMessageIDBatches.isEmpty) + #expect((viewModel.privateChats[peerID] ?? []).isEmpty) + #expect( + transport.removedLegacyPrivateMediaPaths + == ["images/incoming/\(fileURL.lastPathComponent)"] + ) + #expect(!FileManager.default.fileExists(atPath: fileURL.path)) + } + + @Test @MainActor + func deleteLegacyIncomingMediaKeepsPayloadReferencedByAnotherBubble() + throws { + let (viewModel, transport) = makeTestableViewModel() + let peerID = PeerID(str: String(repeating: "c", count: 64)) + let incomingDirectory = try FileManager.default.url( + for: .applicationSupportDirectory, + in: .userDomainMask, + appropriateFor: nil, + create: true + ) + .appendingPathComponent("files/images/incoming", isDirectory: true) + try FileManager.default.createDirectory( + at: incomingDirectory, + withIntermediateDirectories: true + ) + let fileURL = incomingDirectory.appendingPathComponent( + "legacy-shared-\(UUID().uuidString).jpg" + ) + try Data("legacy-image".utf8).write(to: fileURL, options: .atomic) + defer { try? FileManager.default.removeItem(at: fileURL) } + let deleted = privateMediaMessage( + id: UUID().uuidString, + sender: "Old client", + senderPeerID: peerID, + recipient: viewModel.nickname, + filename: fileURL.lastPathComponent + ) + // A different bubble (different ID) references the same basename. + let survivor = privateMediaMessage( + id: UUID().uuidString, + sender: "Old client", + senderPeerID: peerID, + recipient: viewModel.nickname, + filename: fileURL.lastPathComponent + ) + viewModel.seedPrivateChat([deleted, survivor], for: peerID) + + viewModel.deleteMediaMessage(messageID: deleted.id) + + #expect( + viewModel.privateChats[peerID]?.map(\.id) == [survivor.id] + ) + #expect(transport.removedLegacyPrivateMediaPaths.isEmpty) + #expect(FileManager.default.fileExists(atPath: fileURL.path)) } @Test @MainActor diff --git a/bitchatTests/Mocks/MockTransport.swift b/bitchatTests/Mocks/MockTransport.swift index 2efe9f15..b98bee5f 100644 --- a/bitchatTests/Mocks/MockTransport.swift +++ b/bitchatTests/Mocks/MockTransport.swift @@ -255,6 +255,18 @@ final class MockTransport: Transport, PrivateMediaDeletionPersisting { completion(result ?? persistDeletedPrivateMediaResult) } + /// Real store instance so view-model tests exercise the gated legacy + /// unlink end to end (same Application Support tree the tests write to). + let legacyIncomingFileStore = BLEIncomingFileStore() + private(set) var removedLegacyPrivateMediaPaths: [String] = [] + + func removeLegacyPrivateMediaPayload(relativePath: String) { + removedLegacyPrivateMediaPaths.append(relativePath) + legacyIncomingFileStore.removeLegacyIncomingFile( + relativePath: relativePath + ) + } + func sendVerifyChallenge(to peerID: PeerID, noiseKeyHex: String, nonceA: Data) { sentVerifyChallenges.append((peerID, noiseKeyHex, nonceA)) } diff --git a/bitchatTests/Services/BLEFileTransferHandlerTests.swift b/bitchatTests/Services/BLEFileTransferHandlerTests.swift index a4f40193..ea3b3554 100644 --- a/bitchatTests/Services/BLEFileTransferHandlerTests.swift +++ b/bitchatTests/Services/BLEFileTransferHandlerTests.swift @@ -1040,6 +1040,103 @@ struct BLEFileTransferHandlerTests { #expect(!FileManager.default.fileExists(atPath: evictable.path)) } + @Test + func legacyIncomingDeleteUnlinksUnreferencedPayload() throws { + let base = FileManager.default.temporaryDirectory + .appendingPathComponent( + "legacy-unlink-\(UUID().uuidString)", + isDirectory: true + ) + defer { try? FileManager.default.removeItem(at: base) } + let store = BLEIncomingFileStore(baseDirectory: base) + let incoming = try store.incomingDirectory( + subdirectory: "images/incoming" + ) + let payload = incoming.appendingPathComponent("legacy.jpg") + try Data([0xFF, 0xD8, 0xFF, 0xD9]).write(to: payload) + + #expect(store.removeLegacyIncomingFile( + relativePath: "images/incoming/legacy.jpg" + )) + #expect(!FileManager.default.fileExists(atPath: payload.path)) + + // Only paths directly inside an incoming media directory qualify. + let outgoing = base.appendingPathComponent( + "files/images/outgoing", + isDirectory: true + ) + try FileManager.default.createDirectory( + at: outgoing, + withIntermediateDirectories: true + ) + let victim = outgoing.appendingPathComponent("victim.jpg") + try Data([0x01]).write(to: victim) + #expect(!store.removeLegacyIncomingFile( + relativePath: "images/outgoing/victim.jpg" + )) + #expect(!store.removeLegacyIncomingFile( + relativePath: "images/incoming/../outgoing/victim.jpg" + )) + #expect(FileManager.default.fileExists(atPath: victim.path)) + } + + @Test + func legacyIncomingDeleteLeavesPendingDeliveryPayload() throws { + let base = FileManager.default.temporaryDirectory + .appendingPathComponent( + "legacy-pending-\(UUID().uuidString)", + isDirectory: true + ) + defer { try? FileManager.default.removeItem(at: base) } + let store = BLEIncomingFileStore(baseDirectory: base) + + // save() marks the stored path pending until delivery finishes; a + // legacy delete naming the same basename must not unlink it. + let stored = try #require(store.save( + data: Data([0xFF, 0xD8, 0xFF, 0xD9]), + preferredName: "pending.jpg", + subdirectory: "images/incoming", + fallbackExtension: "jpg", + defaultPrefix: "img" + )) + let relativePath = "images/incoming/\(stored.lastPathComponent)" + + #expect(!store.removeLegacyIncomingFile(relativePath: relativePath)) + #expect(FileManager.default.fileExists(atPath: stored.path)) + + // Once the delivery window closes the same unlink is allowed. + store.finishIncomingFileDelivery(at: stored) + #expect(store.removeLegacyIncomingFile(relativePath: relativePath)) + #expect(!FileManager.default.fileExists(atPath: stored.path)) + } + + @Test + func legacyIncomingDeleteLeavesReceiptProtectedPayload() throws { + let base = FileManager.default.temporaryDirectory + .appendingPathComponent( + "legacy-protected-\(UUID().uuidString)", + isDirectory: true + ) + defer { try? FileManager.default.removeItem(at: base) } + let store = BLEIncomingFileStore(baseDirectory: base) + let incoming = try store.incomingDirectory( + subdirectory: "images/incoming" + ) + let payload = incoming.appendingPathComponent("owned.jpg") + try Data([0xFF, 0xD8, 0xFF, 0xD9]).write(to: payload) + + // The basename belongs to a stable receipt (another record). The + // legacy delete must leave it for that owner. + #expect(store.commitPrivateMediaFile( + messageID: "media-00112233445566778899aabbccddeeff", + storedURL: payload + )) + #expect(!store.removeLegacyIncomingFile( + relativePath: "images/incoming/owned.jpg" + )) + #expect(FileManager.default.fileExists(atPath: payload.path)) + } + @Test func panicWipeDeletesEveryManagedMediaFileAndRecreatesEmptyDirectories() throws { let base = FileManager.default.temporaryDirectory