diff --git a/bitchat/Services/KeychainManager.swift b/bitchat/Services/KeychainManager.swift index 4ddfd111..abc86b10 100644 --- a/bitchat/Services/KeychainManager.swift +++ b/bitchat/Services/KeychainManager.swift @@ -18,6 +18,47 @@ enum KeychainInstallLifecycleAction: Equatable { case retryLater } +/// Process-local fail-closed gate for an unresolved install lifecycle. +/// +/// A blocked caller may perform one synchronous reconciliation attempt. +/// Concurrent callers fail closed instead of reading while that cleanup is +/// in flight. Once reconciliation succeeds, access remains open. +final class KeychainInstallAccessGate: @unchecked Sendable { + private let lock = NSLock() + private var blocked = false + private var reconciliationInProgress = false + + func block() { + lock.lock() + blocked = true + lock.unlock() + } + + func allowsAccess(reconcile: () -> Bool) -> Bool { + lock.lock() + if !blocked { + lock.unlock() + return true + } + guard !reconciliationInProgress else { + lock.unlock() + return false + } + reconciliationInProgress = true + lock.unlock() + + let completed = reconcile() + + lock.lock() + if completed { + blocked = false + } + reconciliationInProgress = false + lock.unlock() + return completed + } +} + final class KeychainManager: KeychainManagerProtocol { /// Default keychain for components that construct their own rather than /// having one injected. Under test this is an in-memory keychain: the @@ -48,6 +89,24 @@ final class KeychainManager: KeychainManagerProtocol { // Use consistent service name for all keychain items private let service = BitchatApp.bundleID private let appGroup = "group.\(BitchatApp.bundleID)" + #if os(iOS) + private let installAccessGate = KeychainInstallAccessGate() + #endif + /// Every generic-password service owned by this app, including names used + /// by older releases. Keep custom services here so one-time security + /// migrations and panic deletion cannot silently miss them. + private static let additionalApplicationOwnedServices = [ + "chat.bitchat.nostr", + "chat.bitchat.favorites", + "chat.bitchat.outbox", + "com.bitchat.passwords", + "com.bitchat.deviceidentity", + "com.bitchat.noise.identity", + "chat.bitchat.passwords", + "bitchat.keychain", + "bitchat", + "com.bitchat" + ] // AfterFirstUnlock, not WhenUnlocked: the mesh keeps running with the // device locked (identity-cache saves failed with -25308 throughout // locked-phone testing), and a wake-on-proximity relaunch via BLE state @@ -58,15 +117,27 @@ final class KeychainManager: KeychainManagerProtocol { init() { #if os(iOS) - reconcileInstallLifecycle() - migrateAccessibilityIfNeeded() + if reconcileInstallLifecycle() { + migrateAccessibilityIfNeeded() + } else { + installAccessGate.block() + } #endif } static func installLifecycleAction( containerKnowsMarker: Bool, + cleanupPending: Bool = false, markerRead: KeychainReadResult ) -> KeychainInstallLifecycleAction { + // Once a reinstall cleanup has started, its container-local latch + // must win even if the keychain marker was deleted before a later + // keychain operation failed. Otherwise the next launch could mistake + // a partial cleanup for a fresh bootstrap and preserve stale secrets. + if cleanupPending { + return .clearStaleKeys + } + switch markerRead { case .success: return containerKnowsMarker ? .markerPresent : .clearStaleKeys @@ -77,42 +148,158 @@ final class KeychainManager: KeychainManagerProtocol { } } + static func applicationOwnedKeychainServices(primaryService: String) -> [String] { + var seen = Set() + return ([primaryService] + additionalApplicationOwnedServices).filter { + seen.insert($0).inserted + } + } + + /// Runs every service update even after one failure. Successful updates + /// are idempotent, while returning false keeps the one-time flag unset so + /// a later unlocked launch retries the incomplete migration. + static func migrateAccessibilityForApplicationOwnedServices( + primaryService: String, + updateService: (String) -> OSStatus + ) -> Bool { + var completed = true + for serviceName in applicationOwnedKeychainServices( + primaryService: primaryService + ) { + let status = updateService(serviceName) + if status != errSecSuccess && status != errSecItemNotFound { + completed = false + } + } + return completed + } + + /// Deletes every declared service even after one failure. An empty scope + /// is already clean, while any other status leaves the cleanup + /// incomplete so its durable retry marker remains set. + static func deleteApplicationOwnedKeychainServices( + primaryService: String, + deleteService: (String) -> OSStatus + ) -> Bool { + var completed = true + for serviceName in applicationOwnedKeychainServices( + primaryService: primaryService + ) { + let status = deleteService(serviceName) + if status != errSecSuccess && status != errSecItemNotFound { + completed = false + } + } + return completed + } + + /// The app currently has an application-group entitlement, not a + /// keychain-access-group entitlement. Keep the historical group cleanup + /// probe as best effort without making its expected -34018 response block + /// panic recovery forever. + static func completedApplicationGroupDelete(status: OSStatus) -> Bool { + status == errSecSuccess + || status == errSecItemNotFound + || status == -34018 + } + #if os(iOS) private static let installMarkerAccount = "install_lifecycle_marker" private static let installMarkerDefaultsKey = "keychain.installLifecycleMarker.present" + private static let installCleanupPendingDefaultsKey = + "keychain.installLifecycleCleanup.pending" /// Keychain items can survive app removal while the app container and its /// UserDefaults do not. The first version carrying this marker bootstraps /// without deleting existing users' identities. On a later reinstall, a /// surviving keychain marker plus a missing defaults marker proves the app /// container was replaced, so stale secrets are removed before use. - private func reconcileInstallLifecycle() { + @discardableResult + private func reconcileInstallLifecycle() -> Bool { let defaults = UserDefaults.standard let containerKnowsMarker = defaults.bool(forKey: Self.installMarkerDefaultsKey) + let cleanupPending = defaults.bool( + forKey: Self.installCleanupPendingDefaultsKey + ) let markerRead = retrieveDataWithResult(forKey: Self.installMarkerAccount) switch Self.installLifecycleAction( containerKnowsMarker: containerKnowsMarker, + cleanupPending: cleanupPending, markerRead: markerRead ) { case .markerPresent: defaults.set(true, forKey: Self.installMarkerDefaultsKey) + return true case .bootstrapMarker: if case .success = saveDataWithResult(Data([1]), forKey: Self.installMarkerAccount) { defaults.set(true, forKey: Self.installMarkerDefaultsKey) } + // A missing marker is the intentional bootstrap path for both a + // fresh install and the first marker-carrying upgrade. Preserve + // existing users' identities even if marker creation must retry + // on a later construction. + return true case .clearStaleKeys: - _ = deleteAllKeychainData() + // Establish a container-local retry latch before deleting the + // surviving keychain marker. If the process exits or any keychain + // operation fails, the next launch retries even when that marker + // can no longer be read. + defaults.set(true, forKey: Self.installCleanupPendingDefaultsKey) + guard defaults.synchronize(), + defaults.bool(forKey: Self.installCleanupPendingDefaultsKey) + else { + SecureLogger.error( + "Could not persist reinstall keychain-cleanup intent", + category: .security + ) + return false + } + + guard deleteAllKeychainData() else { + SecureLogger.error( + "Reinstall keychain cleanup incomplete; retry remains pending", + category: .security + ) + return false + } + defaults.set(true, forKey: Self.installMarkerDefaultsKey) + defaults.removeObject( + forKey: Self.installCleanupPendingDefaultsKey + ) + guard defaults.synchronize(), + defaults.bool(forKey: Self.installMarkerDefaultsKey), + !defaults.bool( + forKey: Self.installCleanupPendingDefaultsKey + ) + else { + // Preserve the fail-closed state in memory and make one more + // best-effort persistence attempt before startup continues. + defaults.set( + true, + forKey: Self.installCleanupPendingDefaultsKey + ) + _ = defaults.synchronize() + SecureLogger.error( + "Could not commit reinstall keychain-cleanup state; retry remains pending", + category: .security + ) + return false + } + return true case .retryLater: // Do not guess that a temporarily unreadable marker is absent. - // Leaving the defaults flag unchanged lets a later construction - // retry once protected keychain data is available. - break + // An established container may keep using ordinary protected-data + // semantics: reads fail while locked and recover after unlock. A + // container that has not committed the marker must stay blocked + // until the marker becomes readable and this state machine can + // distinguish bootstrap from reinstall. + return containerKnowsMarker } } @@ -124,30 +311,59 @@ final class KeychainManager: KeychainManagerProtocol { let flag = "keychain.accessibility.afterFirstUnlockThisDeviceOnly.migrated" guard !UserDefaults.standard.bool(forKey: flag) else { return } - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service - ] let update: [String: Any] = [ kSecAttrAccessible as String: Self.itemAccessibility ] - let status = SecItemUpdate(query as CFDictionary, update as CFDictionary) - switch status { - case errSecSuccess, errSecItemNotFound: - // Nothing to migrate on a fresh install; both are terminal. + let completed = Self.migrateAccessibilityForApplicationOwnedServices( + primaryService: service + ) { serviceName in + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: serviceName + ] + return SecItemUpdate( + query as CFDictionary, + update as CFDictionary + ) + } + if completed { + // Missing services on a fresh install are terminal, but the flag is + // set only after every application-owned service was considered. UserDefaults.standard.set(true, forKey: flag) - SecureLogger.info("Keychain accessibility migrated to AfterFirstUnlockThisDeviceOnly (status \(status))", category: .keychain) - default: + SecureLogger.info( + "Keychain accessibility migrated to AfterFirstUnlockThisDeviceOnly", + category: .keychain + ) + } else { // Likely errSecInteractionNotAllowed (relaunched while locked) — // leave the flag unset so the next launch retries. - SecureLogger.warning("Keychain accessibility migration deferred (status \(status))", category: .keychain) + SecureLogger.warning( + "Keychain accessibility migration deferred for at least one application-owned service", + category: .keychain + ) } } #endif + private func installAccessAllowed() -> Bool { + #if os(iOS) + return installAccessGate.allowsAccess { [self] in + guard reconcileInstallLifecycle() else { return false } + migrateAccessibilityIfNeeded() + return true + } + #else + return true + #endif + } + // MARK: - Identity Keys func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool { + guard installAccessAllowed() else { + SecureLogger.logKeyOperation(.save, keyType: key, success: false) + return false + } let fullKey = "identity_\(key)" let result = saveData(keyData, forKey: fullKey) SecureLogger.logKeyOperation(.save, keyType: key, success: result) @@ -155,11 +371,16 @@ final class KeychainManager: KeychainManagerProtocol { } func getIdentityKey(forKey key: String) -> Data? { + guard installAccessAllowed() else { return nil } let fullKey = "identity_\(key)" return retrieveData(forKey: fullKey) } func deleteIdentityKey(forKey key: String) -> Bool { + guard installAccessAllowed() else { + SecureLogger.logKeyOperation(.delete, keyType: key, success: false) + return false + } let result = delete(forKey: "identity_\(key)") SecureLogger.logKeyOperation(.delete, keyType: key, success: result) return result @@ -170,12 +391,14 @@ final class KeychainManager: KeychainManagerProtocol { /// Get identity key with detailed result for proper error handling /// Distinguishes between missing keys (expected) and critical failures func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult { + guard installAccessAllowed() else { return .accessDenied } let fullKey = "identity_\(key)" return retrieveDataWithResult(forKey: fullKey) } /// Save identity key with detailed result and retry logic for transient errors func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult { + guard installAccessAllowed() else { return .accessDenied } let fullKey = "identity_\(key)" return saveDataWithResult(keyData, forKey: fullKey) } @@ -445,123 +668,165 @@ final class KeychainManager: KeychainManagerProtocol { // Delete ALL keychain data for panic mode func deleteAllKeychainData() -> Bool { SecureLogger.warning("Panic mode - deleting all keychain data", category: .security) - - var totalDeleted = 0 - - // Search without service restriction to catch all items + + let ownedServices = Set( + Self.applicationOwnedKeychainServices( + primaryService: service + ) + ) + var enumerationCompleted = true let searchQuery: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecMatchLimit as String: kSecMatchLimitAll, kSecReturnAttributes as String: true ] - var result: AnyObject? - let searchStatus = SecItemCopyMatching(searchQuery as CFDictionary, &result) - - if searchStatus == errSecSuccess, let items = result as? [[String: Any]] { + let searchStatus = SecItemCopyMatching( + searchQuery as CFDictionary, + &result + ) + switch searchStatus { + case errSecSuccess: + guard let items = result as? [[String: Any]] else { + enumerationCompleted = false + SecureLogger.error( + "Unable to decode application-owned keychain inventory", + category: .security + ) + break + } + + // Preserve the access-group sweep for custom services that are + // not yet in the declared legacy-service list. for item in items { - var shouldDelete = false - let account = item[kSecAttrAccount as String] as? String ?? "" - let service = item[kSecAttrService as String] as? String ?? "" - let accessGroup = item[kSecAttrAccessGroup as String] as? String - - // More precise deletion criteria: - // 1. Check for our specific app group - // 2. OR check for our exact service name - // 3. OR check for known legacy service names - if accessGroup == appGroup { - shouldDelete = true - } else if service == self.service { - shouldDelete = true - } else if [ - "com.bitchat.passwords", - "com.bitchat.deviceidentity", - "com.bitchat.noise.identity", - "chat.bitchat.passwords", - "bitchat.keychain", - "bitchat", - "com.bitchat" - ].contains(service) { - shouldDelete = true + let account = + item[kSecAttrAccount as String] as? String ?? "" + let itemService = + item[kSecAttrService as String] as? String ?? "" + let accessGroup = + item[kSecAttrAccessGroup as String] as? String + guard accessGroup == appGroup + || ownedServices.contains(itemService) + else { + continue } - - if shouldDelete { - // Build delete query with all available attributes for precise deletion - var deleteQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword - ] - - if !account.isEmpty { - deleteQuery[kSecAttrAccount as String] = account - } - if !service.isEmpty { - deleteQuery[kSecAttrService as String] = service - } - - // Add access group if present - if let accessGroup = item[kSecAttrAccessGroup as String] as? String, - !accessGroup.isEmpty && accessGroup != "test" { - deleteQuery[kSecAttrAccessGroup as String] = accessGroup - } - - let deleteStatus = SecItemDelete(deleteQuery as CFDictionary) - if deleteStatus == errSecSuccess { - totalDeleted += 1 - SecureLogger.info("Deleted keychain item: \(account) from \(service)", category: .keychain) - } + + var deleteQuery: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword + ] + if !account.isEmpty { + deleteQuery[kSecAttrAccount as String] = account + } + if !itemService.isEmpty { + deleteQuery[kSecAttrService as String] = itemService + } + if let accessGroup, + !accessGroup.isEmpty, + accessGroup != "test" { + deleteQuery[kSecAttrAccessGroup as String] = accessGroup + } + + let status = SecItemDelete(deleteQuery as CFDictionary) + if status != errSecSuccess && status != errSecItemNotFound { + enumerationCompleted = false + SecureLogger.error( + NSError(domain: "Keychain", code: Int(status)), + context: "Unable to delete enumerated application-owned keychain item", + category: .keychain + ) } } + + case errSecItemNotFound: + break + + default: + enumerationCompleted = false + SecureLogger.error( + NSError(domain: "Keychain", code: Int(searchStatus)), + context: "Unable to enumerate application-owned keychain items", + category: .keychain + ) } - - // Also try to delete by known service names and app group - // This catches any items that might have been missed above - let knownServices = [ - self.service, // Current service name - "com.bitchat.passwords", - "com.bitchat.deviceidentity", - "com.bitchat.noise.identity", - "chat.bitchat.passwords", - "chat.bitchat.nostr", - "bitchat.keychain", - "bitchat", - "com.bitchat" - ] - - for serviceName in knownServices { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: serviceName - ] - - let status = SecItemDelete(query as CFDictionary) - if status == errSecSuccess { - totalDeleted += 1 + + // Bulk deletion by every application-owned service is authoritative + // and idempotent. It also verifies that every known service scope is + // empty even when the inventory pass found no items. + let servicesCompleted = + Self.deleteApplicationOwnedKeychainServices( + primaryService: service + ) { serviceName in + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: serviceName + ] + let status = SecItemDelete(query as CFDictionary) + if status != errSecSuccess && status != errSecItemNotFound { + SecureLogger.error( + NSError(domain: "Keychain", code: Int(status)), + context: "Unable to delete application-owned keychain service \(serviceName)", + category: .keychain + ) + } + return status } - } - - // Also delete by app group to ensure complete cleanup + + // Historical builds attempted this application-group identifier as a + // keychain access group. It is not currently entitled, so -34018 + // means the scope is inapplicable rather than partially deleted. let groupQuery: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrAccessGroup as String: appGroup ] - let groupStatus = SecItemDelete(groupQuery as CFDictionary) - if groupStatus == errSecSuccess { - totalDeleted += 1 + let groupCompleted = Self.completedApplicationGroupDelete( + status: groupStatus + ) + if !groupCompleted { + SecureLogger.error( + NSError(domain: "Keychain", code: Int(groupStatus)), + context: "Unable to delete historical application-group keychain items", + category: .keychain + ) } - - SecureLogger.warning("Panic mode cleanup completed. Total items deleted: \(totalDeleted)", category: .keychain) + var markerCompleted = true #if os(iOS) // The non-secret marker is intentionally recreated after a panic so a // later uninstall/reinstall can still be distinguished from an in-place - // upgrade. It never restores any wiped identity or relationship data. - if case .success = saveDataWithResult(Data([1]), forKey: Self.installMarkerAccount) { - UserDefaults.standard.set(true, forKey: Self.installMarkerDefaultsKey) + // upgrade. Do not commit the container-side marker here: reinstall + // reconciliation may still need to retry an incomplete cleanup. + if case .success = saveDataWithResult( + Data([1]), + forKey: Self.installMarkerAccount + ) { + markerCompleted = true + } else { + markerCompleted = false + SecureLogger.error( + "Unable to restore install-lifecycle keychain marker", + category: .security + ) } #endif - - return totalDeleted > 0 + + let completed = + enumerationCompleted + && servicesCompleted + && groupCompleted + && markerCompleted + if completed { + SecureLogger.warning( + "Panic mode keychain cleanup completed", + category: .keychain + ) + } else { + SecureLogger.error( + "Panic mode keychain cleanup incomplete", + category: .security + ) + } + return completed } // MARK: - Security Utilities @@ -587,6 +852,7 @@ final class KeychainManager: KeychainManagerProtocol { // MARK: - Debug func verifyIdentityKeyExists() -> Bool { + guard installAccessAllowed() else { return false } let key = "identity_noiseStaticKey" return retrieveData(forKey: key) != nil } @@ -595,6 +861,7 @@ final class KeychainManager: KeychainManagerProtocol { /// Save data with a custom service name func save(key: String, data: Data, service customService: String, accessible: CFString?) { + guard installAccessAllowed() else { return } let primaryKeyQuery: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: customService, @@ -641,6 +908,7 @@ final class KeychainManager: KeychainManagerProtocol { /// Load custom-service data without collapsing `itemNotFound` and /// protected-data/keychain failures into the same nil result. func loadWithResult(key: String, service customService: String) -> KeychainReadResult { + guard installAccessAllowed() else { return .accessDenied } let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: customService, @@ -655,6 +923,7 @@ final class KeychainManager: KeychainManagerProtocol { /// Delete data from a custom service func delete(key: String, service customService: String) { + guard installAccessAllowed() else { return } let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: customService, @@ -666,6 +935,7 @@ final class KeychainManager: KeychainManagerProtocol { /// Delete every item stored under a custom service func deleteAll(service customService: String) { + guard installAccessAllowed() else { return } let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: customService, diff --git a/bitchat/ViewModels/ChatMediaTransferCoordinator.swift b/bitchat/ViewModels/ChatMediaTransferCoordinator.swift index 0a54fdaa..4aec8033 100644 --- a/bitchat/ViewModels/ChatMediaTransferCoordinator.swift +++ b/bitchat/ViewModels/ChatMediaTransferCoordinator.swift @@ -72,16 +72,98 @@ extension ChatViewModel: ChatMediaTransferContext { } } +/// Synchronous boundary between detached image writers and panic deletion. +/// +/// Invalidation closes admission before waiting for writers that already +/// entered. Those writers never need the main actor while inside the boundary, +/// so a synchronous panic transaction can safely join them and then delete +/// every output before reporting completion. +private final class ImagePreparationBarrier: @unchecked Sendable { + private let condition = NSCondition() + private var generation: UInt64 = 0 + private var activeOperations = 0 + + var currentGeneration: UInt64 { + condition.lock() + defer { condition.unlock() } + return generation + } + + func isCurrent(_ candidate: UInt64) -> Bool { + condition.lock() + defer { condition.unlock() } + return generation == candidate + } + + func performIfCurrent( + generation candidate: UInt64, + operation: () throws -> T + ) rethrows -> T? { + condition.lock() + guard generation == candidate else { + condition.unlock() + return nil + } + activeOperations += 1 + condition.unlock() + + defer { + condition.lock() + activeOperations -= 1 + if activeOperations == 0 { + condition.broadcast() + } + condition.unlock() + } + return try operation() + } + + func invalidateAndWait() { + condition.lock() + generation &+= 1 + while activeOperations > 0 { + condition.wait() + } + condition.unlock() + } +} + +/// Runs synchronous media encoding and file I/O on a dispatch worker. +/// +/// These operations can legitimately block. Keeping them off Swift's +/// cooperative executor preserves forward progress when several transfers or +/// test doubles wait at the same time. +private func runBlockingMediaPreparation( + _ operation: @escaping @Sendable () throws -> T +) async throws -> T { + try await withCheckedThrowingContinuation { continuation in + DispatchQueue.global(qos: .userInitiated).async { + do { + continuation.resume(returning: try operation()) + } catch { + continuation.resume(throwing: error) + } + } + } +} + @MainActor final class ChatMediaTransferCoordinator { private unowned let context: any ChatMediaTransferContext + private let prepareImagePacket: @Sendable (URL) throws -> ChatPreparedImage + private let imagePreparationBarrier = ImagePreparationBarrier() private(set) var transferIdToMessageIDs: [String: [String]] = [:] private(set) var messageIDToTransferId: [String: String] = [:] - private var preparationGeneration: UInt64 = 0 - init(context: any ChatMediaTransferContext) { + init( + context: any ChatMediaTransferContext, + prepareImagePacket: @escaping @Sendable (URL) throws -> ChatPreparedImage = { + try ChatMediaPreparation.prepareImagePacket(from: $0) + } + ) { self.context = context + self.prepareImagePacket = prepareImagePacket } func sendVoiceNote(at url: URL) { @@ -99,14 +181,19 @@ final class ChatMediaTransferCoordinator { ) let messageID = message.id let transferId = makeTransferID(messageID: messageID) - let generation = preparationGeneration + let generation = imagePreparationBarrier.currentGeneration Task.detached(priority: .userInitiated) { [weak self] in do { - let packet = try ChatMediaPreparation.prepareVoiceNotePacket(at: url) + let packet = try await runBlockingMediaPreparation { + try ChatMediaPreparation.prepareVoiceNotePacket(at: url) + } await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { return } + guard let self, + self.imagePreparationBarrier.isCurrent(generation) else { + return + } self.registerTransfer(transferId: transferId, messageID: messageID) if let peerID = targetPeer { self.context.sendFilePrivate(packet, to: peerID, transferId: transferId) @@ -118,13 +205,19 @@ final class ChatMediaTransferCoordinator { SecureLogger.warning("Voice note exceeds size limit (\(size) bytes)", category: .session) try? FileManager.default.removeItem(at: url) await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { return } + guard let self, + self.imagePreparationBarrier.isCurrent(generation) else { + return + } self.handleMediaSendFailure(messageID: messageID, reason: String(localized: "content.delivery.reason.voice_too_large", comment: "Failure reason shown when a voice note exceeds the size limit")) } } catch { SecureLogger.error("Voice note send failed: \(error)", category: .session) await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { return } + guard let self, + self.imagePreparationBarrier.isCurrent(generation) else { + return + } self.handleMediaSendFailure(messageID: messageID, reason: String(localized: "content.delivery.reason.voice_send_failed", comment: "Failure reason shown when a voice note could not be sent")) } } @@ -134,12 +227,24 @@ final class ChatMediaTransferCoordinator { #if os(iOS) func processThenSendImage(_ image: UIImage?) { guard let image else { return } - let generation = preparationGeneration - Task.detached { [weak self] in + let generation = imagePreparationBarrier.currentGeneration + let barrier = imagePreparationBarrier + Task.detached(priority: .userInitiated) { [weak self, barrier] in do { - let processedURL = try ImageUtils.processImage(image) - await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { + let processedURL = try await runBlockingMediaPreparation { + try barrier.performIfCurrent( + generation: generation, + operation: { + try ImageUtils.processImage(image) + } + ) + } + guard let processedURL else { + return + } + await MainActor.run { [weak self, barrier] in + guard let self, + barrier.isCurrent(generation) else { try? FileManager.default.removeItem(at: processedURL) return } @@ -153,12 +258,24 @@ final class ChatMediaTransferCoordinator { #elseif os(macOS) func processThenSendImage(from url: URL?) { guard let url else { return } - let generation = preparationGeneration - Task.detached { [weak self] in + let generation = imagePreparationBarrier.currentGeneration + let barrier = imagePreparationBarrier + Task.detached(priority: .userInitiated) { [weak self, barrier] in do { - let processedURL = try ImageUtils.processImage(at: url) - await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { + let processedURL = try await runBlockingMediaPreparation { + try barrier.performIfCurrent( + generation: generation, + operation: { + try ImageUtils.processImage(at: url) + } + ) + } + guard let processedURL else { + return + } + await MainActor.run { [weak self, barrier] in + guard let self, + barrier.isCurrent(generation) else { try? FileManager.default.removeItem(at: processedURL) return } @@ -180,7 +297,7 @@ final class ChatMediaTransferCoordinator { } let targetPeer = context.selectedPrivateChatPeer - let generation = preparationGeneration + let generation = imagePreparationBarrier.currentGeneration do { try ImageUtils.validateImageSource(at: sourceURL) @@ -190,12 +307,25 @@ final class ChatMediaTransferCoordinator { return } - Task.detached(priority: .userInitiated) { [weak self] in + let prepareImagePacket = self.prepareImagePacket + let barrier = imagePreparationBarrier + Task.detached(priority: .userInitiated) { [weak self, barrier] in do { - let prepared = try ChatMediaPreparation.prepareImagePacket(from: sourceURL) + let prepared = try await runBlockingMediaPreparation { + try barrier.performIfCurrent( + generation: generation, + operation: { + try prepareImagePacket(sourceURL) + } + ) + } + guard let prepared else { + return + } - await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { + await MainActor.run { [weak self, barrier] in + guard let self, + barrier.isCurrent(generation) else { try? FileManager.default.removeItem(at: prepared.outputURL) return } @@ -214,14 +344,20 @@ final class ChatMediaTransferCoordinator { } } catch ChatMediaPreparationError.imageTooLarge(let size) { SecureLogger.warning("Processed image exceeds size limit (\(size) bytes)", category: .session) - await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { return } + await MainActor.run { [weak self, barrier] in + guard let self, + barrier.isCurrent(generation) else { + return + } self.context.addSystemMessage("Image is too large to send.") } } catch { SecureLogger.error("Image send preparation failed: \(error)", category: .session) - await MainActor.run { [weak self] in - guard let self, self.preparationGeneration == generation else { return } + await MainActor.run { [weak self, barrier] in + guard let self, + barrier.isCurrent(generation) else { + return + } self.context.addSystemMessage("Failed to prepare image for sending.") } } @@ -357,10 +493,11 @@ final class ChatMediaTransferCoordinator { } /// Invalidates detached preparation work and cancels every transfer that - /// reached the transport. Stale tasks check the generation before they - /// can recreate a message or send after a panic wipe. + /// reached the transport. Closing image-preparation admission and joining + /// active synchronous writers ensures the following panic media deletion + /// is the last filesystem mutation before the transaction can complete. func resetForPanic() { - preparationGeneration &+= 1 + imagePreparationBarrier.invalidateAndWait() let transferIDs = Set(transferIdToMessageIDs.keys) transferIdToMessageIDs.removeAll(keepingCapacity: false) messageIDToTransferId.removeAll(keepingCapacity: false) diff --git a/bitchat/_PreviewHelpers/PreviewKeychainManager.swift b/bitchat/_PreviewHelpers/PreviewKeychainManager.swift index 32827f48..17bc9ead 100644 --- a/bitchat/_PreviewHelpers/PreviewKeychainManager.swift +++ b/bitchat/_PreviewHelpers/PreviewKeychainManager.swift @@ -14,11 +14,25 @@ final class PreviewKeychainManager: KeychainManagerProtocol { // every default-constructed component under test, which access it from // arbitrary threads. private let lock = NSLock() + private let installAccessGate: KeychainInstallAccessGate + private let reconcileInstallAccess: () -> Bool private var storage: [String: Data] = [:] private var serviceStorage: [String: [String: Data]] = [:] - init() {} + + init( + installAccessGate: KeychainInstallAccessGate = KeychainInstallAccessGate(), + reconcileInstallAccess: @escaping () -> Bool = { true } + ) { + self.installAccessGate = installAccessGate + self.reconcileInstallAccess = reconcileInstallAccess + } + + private func installAccessAllowed() -> Bool { + installAccessGate.allowsAccess(reconcile: reconcileInstallAccess) + } func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool { + guard installAccessAllowed() else { return false } lock.lock() defer { lock.unlock() } storage[key] = keyData @@ -26,12 +40,14 @@ final class PreviewKeychainManager: KeychainManagerProtocol { } func getIdentityKey(forKey key: String) -> Data? { + guard installAccessAllowed() else { return nil } lock.lock() defer { lock.unlock() } return storage[key] } func deleteIdentityKey(forKey key: String) -> Bool { + guard installAccessAllowed() else { return false } lock.lock() defer { lock.unlock() } storage.removeValue(forKey: key) @@ -51,6 +67,7 @@ final class PreviewKeychainManager: KeychainManagerProtocol { func secureClear(_ string: inout String) {} func verifyIdentityKeyExists() -> Bool { + guard installAccessAllowed() else { return false } lock.lock() defer { lock.unlock() } return storage["identity_noiseStaticKey"] != nil @@ -58,6 +75,7 @@ final class PreviewKeychainManager: KeychainManagerProtocol { // BCH-01-009: New methods with proper error classification func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult { + guard installAccessAllowed() else { return .accessDenied } lock.lock() defer { lock.unlock() } if let data = storage[key] { @@ -67,6 +85,7 @@ final class PreviewKeychainManager: KeychainManagerProtocol { } func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult { + guard installAccessAllowed() else { return .accessDenied } lock.lock() defer { lock.unlock() } storage[key] = keyData @@ -76,24 +95,38 @@ final class PreviewKeychainManager: KeychainManagerProtocol { // MARK: - Generic Data Storage (consolidated from KeychainHelper) func save(key: String, data: Data, service: String, accessible: CFString?) { + guard installAccessAllowed() else { return } lock.lock() defer { lock.unlock() } serviceStorage[service, default: [:]][key] = data } func load(key: String, service: String) -> Data? { + guard case .success(let data) = loadWithResult(key: key, service: service) else { + return nil + } + return data + } + + func loadWithResult(key: String, service: String) -> KeychainReadResult { + guard installAccessAllowed() else { return .accessDenied } lock.lock() defer { lock.unlock() } - return serviceStorage[service]?[key] + guard let data = serviceStorage[service]?[key] else { + return .itemNotFound + } + return .success(data) } func delete(key: String, service: String) { + guard installAccessAllowed() else { return } lock.lock() defer { lock.unlock() } serviceStorage[service]?.removeValue(forKey: key) } func deleteAll(service: String) { + guard installAccessAllowed() else { return } lock.lock() defer { lock.unlock() } serviceStorage.removeValue(forKey: service) diff --git a/bitchatTests/ChatMediaTransferCoordinatorContextTests.swift b/bitchatTests/ChatMediaTransferCoordinatorContextTests.swift index bbecbce7..52371d06 100644 --- a/bitchatTests/ChatMediaTransferCoordinatorContextTests.swift +++ b/bitchatTests/ChatMediaTransferCoordinatorContextTests.swift @@ -16,6 +16,11 @@ import Testing import Foundation import BitFoundation +#if os(iOS) +import UIKit +#else +import AppKit +#endif @testable import bitchat // MARK: - Mock Context @@ -203,6 +208,99 @@ struct ChatMediaTransferCoordinatorContextTests { #expect(coordinator.messageIDToTransferId.isEmpty) } + @Test @MainActor + func resetForPanic_waitsForActiveImageWriterBeforeReturning() async throws { + let context = MockChatMediaTransferContext() + let sourceURL = try makeCoordinatorTestImageURL() + let outputURL = FileManager.default.temporaryDirectory + .appendingPathComponent("panic-prepared-\(UUID().uuidString).jpg") + let preparer = PausedImagePreparer(outputURL: outputURL) + let coordinator = ChatMediaTransferCoordinator( + context: context, + prepareImagePacket: { sourceURL in + try preparer.prepare(sourceURL) + } + ) + defer { + preparer.release() + try? FileManager.default.removeItem(at: sourceURL) + try? FileManager.default.removeItem(at: outputURL) + } + + coordinator.sendImage(from: sourceURL) + #expect(await TestHelpers.waitUntil( + { preparer.hasStarted }, + timeout: TestConstants.longTimeout + )) + + DispatchQueue.global(qos: .userInitiated).asyncAfter( + deadline: .now() + .milliseconds(100) + ) { + preparer.release() + } + coordinator.resetForPanic() + + // The synchronous reset boundary cannot return while a pre-panic + // writer can still create output. The real panic path deletes media + // immediately after this method returns. + #expect(preparer.hasFinished) + + try? FileManager.default.removeItem(at: outputURL) + #expect(await TestHelpers.waitUntil( + { !FileManager.default.fileExists(atPath: outputURL.path) }, + timeout: TestConstants.longTimeout + )) + await Task.yield() + #expect(context.privateFileSends.isEmpty) + #expect(context.broadcastFileSends.isEmpty) + #expect(context.systemMessages.isEmpty) + } + + @Test @MainActor + func imagePreparation_doesNotRetainCoordinatorOrDeallocatedContext() async throws { + let sourceURL = try makeCoordinatorTestImageURL() + let outputURL = FileManager.default.temporaryDirectory + .appendingPathComponent("released-context-\(UUID().uuidString).jpg") + let preparer = PausedImagePreparer(outputURL: outputURL) + var context: MockChatMediaTransferContext? = MockChatMediaTransferContext() + var coordinator: ChatMediaTransferCoordinator? = ChatMediaTransferCoordinator( + context: context!, + prepareImagePacket: { sourceURL in + try preparer.prepare(sourceURL) + } + ) + weak var weakContext: MockChatMediaTransferContext? + weak var weakCoordinator: ChatMediaTransferCoordinator? + weakContext = context + weakCoordinator = coordinator + defer { + preparer.release() + try? FileManager.default.removeItem(at: sourceURL) + try? FileManager.default.removeItem(at: outputURL) + } + + coordinator?.sendImage(from: sourceURL) + #expect(await TestHelpers.waitUntil( + { preparer.hasStarted }, + timeout: TestConstants.longTimeout + )) + + coordinator = nil + context = nil + #expect(weakCoordinator == nil) + #expect(weakContext == nil) + + preparer.release() + #expect(await TestHelpers.waitUntil( + { preparer.hasFinished }, + timeout: TestConstants.longTimeout + )) + #expect(await TestHelpers.waitUntil( + { !FileManager.default.fileExists(atPath: outputURL.path) }, + timeout: TestConstants.longTimeout + )) + } + @Test @MainActor func sendVoiceNote_blockedContextRemovesFileAndExplains() async throws { let context = MockChatMediaTransferContext() @@ -222,3 +320,91 @@ struct ChatMediaTransferCoordinatorContextTests { #expect(coordinator.transferIdToMessageIDs.isEmpty) } } + +private final class PausedImagePreparer: @unchecked Sendable { + private let condition = NSCondition() + private let outputURL: URL + private var started = false + private var released = false + private var finished = false + + init(outputURL: URL) { + self.outputURL = outputURL + } + + var hasStarted: Bool { + condition.lock() + defer { condition.unlock() } + return started + } + + var hasFinished: Bool { + condition.lock() + defer { condition.unlock() } + return finished + } + + func prepare(_ _: URL) throws -> ChatPreparedImage { + condition.lock() + started = true + condition.broadcast() + while !released { + condition.wait() + } + condition.unlock() + + let data = Data("prepared image".utf8) + try data.write(to: outputURL, options: .atomic) + let packet = BitchatFilePacket( + fileName: outputURL.lastPathComponent, + fileSize: UInt64(data.count), + mimeType: "image/jpeg", + content: data + ) + + condition.lock() + finished = true + condition.broadcast() + condition.unlock() + return ChatPreparedImage(outputURL: outputURL, packet: packet) + } + + func release() { + condition.lock() + released = true + condition.broadcast() + condition.unlock() + } +} + +private func makeCoordinatorTestImageURL() throws -> URL { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("coordinator-image-\(UUID().uuidString).png") + #if os(iOS) + let image = UIGraphicsImageRenderer(size: CGSize(width: 16, height: 16)) + .image { context in + UIColor.systemBlue.setFill() + context.fill(CGRect(x: 0, y: 0, width: 16, height: 16)) + } + guard let data = image.pngData() else { + throw CoordinatorImageTestError.encodingFailed + } + #else + let image = NSImage(size: NSSize(width: 16, height: 16)) + image.lockFocus() + NSColor.systemBlue.setFill() + NSRect(x: 0, y: 0, width: 16, height: 16).fill() + image.unlockFocus() + guard let tiff = image.tiffRepresentation, + let bitmap = NSBitmapImageRep(data: tiff), + let data = bitmap.representation(using: .png, properties: [:]) else { + throw CoordinatorImageTestError.encodingFailed + } + #endif + try data.write(to: url, options: .atomic) + return url +} + +private enum CoordinatorImageTestError: Error { + case encodingFailed +} diff --git a/bitchatTests/PreviewKeychainManagerTests.swift b/bitchatTests/PreviewKeychainManagerTests.swift index 1041b12c..fffc7e25 100644 --- a/bitchatTests/PreviewKeychainManagerTests.swift +++ b/bitchatTests/PreviewKeychainManagerTests.swift @@ -1,4 +1,5 @@ import Foundation +import Security import Testing import BitFoundation @testable import bitchat @@ -24,6 +25,77 @@ struct PreviewKeychainManagerTests { containerKnowsMarker: false, markerRead: .deviceLocked ) == .retryLater) + #expect(KeychainManager.installLifecycleAction( + containerKnowsMarker: false, + cleanupPending: true, + markerRead: .itemNotFound + ) == .clearStaleKeys) + } + + @Test("Accessibility migration covers custom services and retries after any incomplete update") + func accessibilityMigrationCoversEveryApplicationOwnedService() { + let primaryService = "chat.bitchat.test-primary" + var visitedServices: [String] = [] + + let completed = KeychainManager + .migrateAccessibilityForApplicationOwnedServices( + primaryService: primaryService + ) { service in + visitedServices.append(service) + return service == "chat.bitchat.favorites" + ? errSecInteractionNotAllowed + : errSecItemNotFound + } + + #expect(!completed) + #expect(visitedServices.first == primaryService) + #expect(Set(visitedServices).isSuperset(of: [ + "chat.bitchat.nostr", + "chat.bitchat.favorites", + "chat.bitchat.outbox" + ])) + #expect(Set(visitedServices).count == visitedServices.count) + + let retryCompleted = KeychainManager + .migrateAccessibilityForApplicationOwnedServices( + primaryService: primaryService + ) { _ in errSecSuccess } + #expect(retryCompleted) + } + + @Test("Keychain cleanup is complete only when every owned scope is clean") + func keychainCleanupRequiresEveryApplicationOwnedService() { + let primaryService = "chat.bitchat.test-primary" + var visitedServices: [String] = [] + + let partialCleanup = KeychainManager + .deleteApplicationOwnedKeychainServices( + primaryService: primaryService + ) { service in + visitedServices.append(service) + return service == "chat.bitchat.outbox" + ? errSecInteractionNotAllowed + : errSecSuccess + } + + #expect(!partialCleanup) + #expect(visitedServices.first == primaryService) + #expect(Set(visitedServices).isSuperset(of: [ + "chat.bitchat.nostr", + "chat.bitchat.favorites", + "chat.bitchat.outbox" + ])) + #expect(Set(visitedServices).count == visitedServices.count) + + let emptyCleanup = KeychainManager + .deleteApplicationOwnedKeychainServices( + primaryService: primaryService + ) { _ in errSecItemNotFound } + #expect(emptyCleanup) + #expect(KeychainManager.completedApplicationGroupDelete(status: -34018)) + #expect(!KeychainManager.completedApplicationGroupDelete( + status: errSecInteractionNotAllowed + )) } @Test("Preview keychain manager stores identity and service-scoped data in memory") @@ -72,4 +144,132 @@ struct PreviewKeychainManagerTests { Issue.record("Expected preview keychain to be empty after deleteAllKeychainData") } } + + @Test("Failed reinstall cleanup blocks stale data until a successful retry") + func failedReinstallCleanupBlocksEveryNamespaceUntilSuccessfulRetry() { + let gate = KeychainInstallAccessGate() + var cleanupCanComplete = false + var reconciliationAttempts = 0 + var manager: PreviewKeychainManager! + manager = PreviewKeychainManager( + installAccessGate: gate + ) { + reconciliationAttempts += 1 + guard cleanupCanComplete else { return false } + return manager.deleteAllKeychainData() + } + + let staleIdentity = Data([1, 2, 3]) + let staleFavorite = Data([4, 5, 6]) + let staleOutbox = Data([7, 8, 9]) + let staleCustom = Data([10, 11, 12]) + #expect(manager.saveIdentityKey( + staleIdentity, + forKey: "noiseStaticKey" + )) + #expect(manager.saveIdentityKey( + staleIdentity, + forKey: "identity_noiseStaticKey" + )) + #expect(manager.verifyIdentityKeyExists()) + manager.save( + key: "favorite", + data: staleFavorite, + service: "chat.bitchat.favorites", + accessible: nil + ) + manager.save( + key: "outbox", + data: staleOutbox, + service: "chat.bitchat.outbox", + accessible: nil + ) + manager.save( + key: "custom", + data: staleCustom, + service: "chat.bitchat.future-custom", + accessible: nil + ) + + gate.block() + + #expect(manager.getIdentityKey(forKey: "noiseStaticKey") == nil) + #expect(!manager.verifyIdentityKeyExists()) + if case .accessDenied = manager.getIdentityKeyWithResult( + forKey: "noiseStaticKey" + ) { + } else { + Issue.record("Expected blocked identity read to fail closed") + } + + for (key, service) in [ + ("favorite", "chat.bitchat.favorites"), + ("outbox", "chat.bitchat.outbox"), + ("custom", "chat.bitchat.future-custom") + ] { + #expect(manager.load(key: key, service: service) == nil) + if case .accessDenied = manager.loadWithResult( + key: key, + service: service + ) { + } else { + Issue.record( + "Expected blocked \(service) read to fail closed" + ) + } + } + + #expect(!manager.saveIdentityKey( + Data([13]), + forKey: "replacement" + )) + if case .accessDenied = manager.saveIdentityKeyWithResult( + Data([14]), + forKey: "replacement" + ) { + } else { + Issue.record("Expected blocked identity save to fail closed") + } + + let failedAttempts = reconciliationAttempts + #expect(failedAttempts > 0) + cleanupCanComplete = true + + // The first access retries cleanup synchronously. It must not return + // any surviving value from before the reinstall. + #expect(manager.getIdentityKey(forKey: "noiseStaticKey") == nil) + #expect(reconciliationAttempts == failedAttempts + 1) + #expect(manager.load( + key: "favorite", + service: "chat.bitchat.favorites" + ) == nil) + #expect(manager.load( + key: "outbox", + service: "chat.bitchat.outbox" + ) == nil) + #expect(manager.load( + key: "custom", + service: "chat.bitchat.future-custom" + ) == nil) + + let replacementIdentity = Data([21, 22, 23]) + let replacementCustom = Data([24, 25, 26]) + #expect(manager.saveIdentityKey( + replacementIdentity, + forKey: "noiseStaticKey" + )) + #expect(manager.getIdentityKey( + forKey: "noiseStaticKey" + ) == replacementIdentity) + manager.save( + key: "custom", + data: replacementCustom, + service: "chat.bitchat.future-custom", + accessible: nil + ) + #expect(manager.load( + key: "custom", + service: "chat.bitchat.future-custom" + ) == replacementCustom) + } }