From d499c3e4158a296a07032c78e6a9d6fb8d80fa71 Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Wed, 8 Jul 2026 17:20:00 +0200 Subject: [PATCH] Isolate tests from the developer's real login keychain (#1413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Isolate tests from the developer's real login keychain Every test run prompted for the login keychain password (repeatedly, since the xctest runner's code signature changes each build, so "Always Allow" can never stick) and silently deleted the developer's real Nostr identity via panic-mode tests. Two holes let tests reach the real keychain: - NostrIdentityBridge() defaulted to the real KeychainManager. Tests inject mocks, but app-side constructions with no injection point (LocationNotesManager's static bridge, GeohashPresenceService, BoardManager, AppRuntime) read the real chat.bitchat.nostr item when exercised under test. - clearAllAssociations() used raw SecItem* calls that bypassed the injected keychain entirely, so panicClearAllData tests wiped the real Nostr identity items on every run. Fix: centralize FavoritesPersistenceService's test-guarded in-memory default as KeychainManager.makeDefault() and use it for all default keychain parameters, and add deleteAll(service:) to KeychainManagerProtocol so clearAllAssociations() goes through the injected keychain like every other operation. Co-Authored-By: Claude Fable 5 * Share one in-memory test keychain per process (Codex P2) A fresh store per makeDefault() call diverges from production, where separate default-constructed bridges share chat.bitchat.nostr — BoardManager's publish and NIP-09 delete paths would derive different geohash identities under test. PreviewKeychainManager gains a lock since the shared instance is reached from arbitrary threads. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: jack Co-authored-by: Claude Fable 5 --- bitchat/App/AppRuntime.swift | 2 +- bitchat/Nostr/NostrIdentityBridge.swift | 29 ++--------- .../FavoritesPersistenceService.swift | 18 +------ bitchat/Services/KeychainManager.swift | 52 +++++++++++++++++++ .../PreviewKeychainManager.swift | 41 ++++++++++++--- bitchatTests/Mocks/MockKeychain.swift | 8 +++ .../SecureIdentityStateManagerTests.swift | 4 ++ .../KeychainManagerProtocol.swift | 2 + .../BitFoundationTests/MockKeychain.swift | 8 +++ 9 files changed, 115 insertions(+), 49 deletions(-) diff --git a/bitchat/App/AppRuntime.swift b/bitchat/App/AppRuntime.swift index 81f2995b..abd1e49f 100644 --- a/bitchat/App/AppRuntime.swift +++ b/bitchat/App/AppRuntime.swift @@ -40,7 +40,7 @@ final class AppRuntime: ObservableObject { #endif init( - keychain: KeychainManagerProtocol = KeychainManager(), + keychain: KeychainManagerProtocol = KeychainManager.makeDefault(), idBridge: NostrIdentityBridge = NostrIdentityBridge() ) { self.idBridge = idBridge diff --git a/bitchat/Nostr/NostrIdentityBridge.swift b/bitchat/Nostr/NostrIdentityBridge.swift index ab0c89db..c91ffc42 100644 --- a/bitchat/Nostr/NostrIdentityBridge.swift +++ b/bitchat/Nostr/NostrIdentityBridge.swift @@ -15,7 +15,7 @@ final class NostrIdentityBridge { private let keychain: KeychainManagerProtocol - init(keychain: KeychainManagerProtocol = KeychainManager()) { + init(keychain: KeychainManagerProtocol = KeychainManager.makeDefault()) { self.keychain = keychain } @@ -49,29 +49,10 @@ final class NostrIdentityBridge { /// Clear all Nostr identity associations and current identity func clearAllAssociations() { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: keychainService, - kSecMatchLimit as String: kSecMatchLimitAll, - kSecReturnAttributes as String: true - ] - - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) - if status == errSecSuccess, let items = result as? [[String: Any]] { - for item in items { - var deleteQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: keychainService - ] - if let account = item[kSecAttrAccount as String] as? String { - deleteQuery[kSecAttrAccount as String] = account - } - SecItemDelete(deleteQuery as CFDictionary) - } - } else if status == errSecItemNotFound { - // nothing persisted; no action needed - } + // Must go through the injected keychain, not raw SecItem calls: + // under test that keychain is in-memory, and a direct delete here + // would wipe the developer's real Nostr identity on every test run. + keychain.deleteAll(service: keychainService) deviceSeedCache = nil // Also drop the in-memory derived per-geohash identities. These hold the diff --git a/bitchat/Services/FavoritesPersistenceService.swift b/bitchat/Services/FavoritesPersistenceService.swift index aa045841..84abe5d3 100644 --- a/bitchat/Services/FavoritesPersistenceService.swift +++ b/bitchat/Services/FavoritesPersistenceService.swift @@ -34,23 +34,7 @@ final class FavoritesPersistenceService: ObservableObject { static let shared = FavoritesPersistenceService() - /// Default keychain for the `shared` singleton. Under test this is an - /// in-memory keychain so touching `shared` never blocks on securityd - /// (`SecItemCopyMatching` can hang in test environments) and never reads - /// or writes the developer's real keychain. Production behavior is - /// unchanged. Tests that need their own instance keep injecting a mock - /// via `init(keychain:)`. - private nonisolated static func makeDefaultKeychain() -> KeychainManagerProtocol { - // PreviewKeychainManager lives in _PreviewHelpers, a development - // asset excluded from archive builds — release code must not - // reference it. Tests always run Debug, so the guard is lossless. - #if DEBUG - if TestEnvironment.isRunningTests { return PreviewKeychainManager() } - #endif - return KeychainManager() - } - - init(keychain: KeychainManagerProtocol = FavoritesPersistenceService.makeDefaultKeychain()) { + init(keychain: KeychainManagerProtocol = KeychainManager.makeDefault()) { self.keychain = keychain loadFavorites() diff --git a/bitchat/Services/KeychainManager.swift b/bitchat/Services/KeychainManager.swift index 39035547..cbd61f18 100644 --- a/bitchat/Services/KeychainManager.swift +++ b/bitchat/Services/KeychainManager.swift @@ -12,6 +12,32 @@ import Foundation import Security 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 + /// xctest runner's code signature changes every build, so any read of a + /// real login-keychain item triggers a macOS password prompt that + /// "Always Allow" can never satisfy — and tests must never read or + /// mutate the developer's real keychain (`SecItemCopyMatching` can also + /// hang in test environments). Production behavior is unchanged. + static func makeDefault() -> KeychainManagerProtocol { + // PreviewKeychainManager lives in _PreviewHelpers, a development + // asset excluded from archive builds — release code must not + // reference it. Tests always run Debug, so the guard is lossless. + #if DEBUG + if TestEnvironment.isRunningTests { return sharedTestKeychain } + #endif + return KeychainManager() + } + + #if DEBUG + /// One store per process, mirroring the real keychain: separate + /// default-constructed components (e.g. two NostrIdentityBridge + /// instances in BoardManager's publish and delete paths) must see each + /// other's writes, or they would derive different Nostr identities + /// under test. + private static let sharedTestKeychain = PreviewKeychainManager() + #endif + // Use consistent service name for all keychain items private let service = BitchatApp.bundleID private let appGroup = "group.\(BitchatApp.bundleID)" @@ -540,4 +566,30 @@ final class KeychainManager: KeychainManagerProtocol { SecItemDelete(query as CFDictionary) } + + /// Delete every item stored under a custom service + func deleteAll(service customService: String) { + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: customService, + kSecMatchLimit as String: kSecMatchLimitAll, + kSecReturnAttributes as String: true + ] + + var result: AnyObject? + let status = SecItemCopyMatching(query as CFDictionary, &result) + guard status == errSecSuccess, let items = result as? [[String: Any]] else { + return + } + for item in items { + var deleteQuery: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: customService + ] + if let account = item[kSecAttrAccount as String] as? String { + deleteQuery[kSecAttrAccount as String] = account + } + SecItemDelete(deleteQuery as CFDictionary) + } + } } diff --git a/bitchat/_PreviewHelpers/PreviewKeychainManager.swift b/bitchat/_PreviewHelpers/PreviewKeychainManager.swift index 49e63d3f..32827f48 100644 --- a/bitchat/_PreviewHelpers/PreviewKeychainManager.swift +++ b/bitchat/_PreviewHelpers/PreviewKeychainManager.swift @@ -10,25 +10,37 @@ import BitFoundation import Foundation final class PreviewKeychainManager: KeychainManagerProtocol { + // Locked: KeychainManager.makeDefault() hands one shared instance to + // every default-constructed component under test, which access it from + // arbitrary threads. + private let lock = NSLock() private var storage: [String: Data] = [:] private var serviceStorage: [String: [String: Data]] = [:] init() {} func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool { + lock.lock() + defer { lock.unlock() } storage[key] = keyData return true } func getIdentityKey(forKey key: String) -> Data? { - storage[key] + lock.lock() + defer { lock.unlock() } + return storage[key] } func deleteIdentityKey(forKey key: String) -> Bool { + lock.lock() + defer { lock.unlock() } storage.removeValue(forKey: key) return true } func deleteAllKeychainData() -> Bool { + lock.lock() + defer { lock.unlock() } storage.removeAll() serviceStorage.removeAll() return true @@ -39,11 +51,15 @@ final class PreviewKeychainManager: KeychainManagerProtocol { func secureClear(_ string: inout String) {} func verifyIdentityKeyExists() -> Bool { - storage["identity_noiseStaticKey"] != nil + lock.lock() + defer { lock.unlock() } + return storage["identity_noiseStaticKey"] != nil } // BCH-01-009: New methods with proper error classification func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult { + lock.lock() + defer { lock.unlock() } if let data = storage[key] { return .success(data) } @@ -51,6 +67,8 @@ final class PreviewKeychainManager: KeychainManagerProtocol { } func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult { + lock.lock() + defer { lock.unlock() } storage[key] = keyData return .success } @@ -58,17 +76,26 @@ final class PreviewKeychainManager: KeychainManagerProtocol { // MARK: - Generic Data Storage (consolidated from KeychainHelper) func save(key: String, data: Data, service: String, accessible: CFString?) { - if serviceStorage[service] == nil { - serviceStorage[service] = [:] - } - serviceStorage[service]?[key] = data + lock.lock() + defer { lock.unlock() } + serviceStorage[service, default: [:]][key] = data } func load(key: String, service: String) -> Data? { - serviceStorage[service]?[key] + lock.lock() + defer { lock.unlock() } + return serviceStorage[service]?[key] } func delete(key: String, service: String) { + lock.lock() + defer { lock.unlock() } serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + lock.lock() + defer { lock.unlock() } + serviceStorage.removeValue(forKey: service) + } } diff --git a/bitchatTests/Mocks/MockKeychain.swift b/bitchatTests/Mocks/MockKeychain.swift index 2c8d7c32..cb974bae 100644 --- a/bitchatTests/Mocks/MockKeychain.swift +++ b/bitchatTests/Mocks/MockKeychain.swift @@ -85,6 +85,10 @@ final class MockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } /// Typealias for backwards compatibility with tests using MockKeychainHelper @@ -198,4 +202,8 @@ final class TrackingMockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } diff --git a/bitchatTests/Services/SecureIdentityStateManagerTests.swift b/bitchatTests/Services/SecureIdentityStateManagerTests.swift index 27e57fdf..a280d02e 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerTests.swift @@ -496,4 +496,8 @@ private final class FailingCacheSaveKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } diff --git a/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift b/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift index ea78288e..5a7ec448 100644 --- a/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift +++ b/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift @@ -34,6 +34,8 @@ public protocol KeychainManagerProtocol { func load(key: String, service: String) -> Data? /// Delete data from a custom service func delete(key: String, service: String) + /// Delete every item stored under a custom service + func deleteAll(service: String) } // MARK: - Keychain Error Types diff --git a/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift b/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift index 5d4071ec..870911ce 100644 --- a/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift +++ b/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift @@ -85,6 +85,10 @@ final class MockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } /// Typealias for backwards compatibility with tests using MockKeychainHelper @@ -198,4 +202,8 @@ final class TrackingMockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } }