diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index d66dc8bf0..6867a0637 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -62,7 +62,15 @@ jobs: echo "⏱️ Starting integration tests at $(date)" run_tests() { - set -o pipefail && xcodebuild test \ + # RNMigrationCleanupTests deletes the real ~/Documents/mmkv and ~/Documents/ldk, so it + # skips itself unless this is set. This lane runs on a simulator it erases between + # attempts; the unit lane does not, and must not run it. + # + # The TEST_RUNNER_ prefix is required: xcodebuild does not pass the invoking shell's + # environment to the simulator-hosted runner, only variables named TEST_RUNNER_, + # which it forwards with the prefix stripped. Without it the guard never sees the + # variable, every test in the suite skips, and the lane still reports green. + set -o pipefail && TEST_RUNNER_BITKIT_DESTRUCTIVE_TESTS=1 xcodebuild test \ -scheme Bitkit \ -destination 'platform=iOS Simulator,name=iPhone 16,OS=latest' \ -enableCodeCoverage NO \ @@ -72,6 +80,7 @@ jobs: -only-testing:BitkitTests/PaymentFlowTests \ -only-testing:BitkitTests/BlocktankRefundAddressLiveIntegrationTests \ -only-testing:BitkitTests/AddressTypeIntegrationTests \ + -only-testing:BitkitTests/RNMigrationCleanupTests \ | xcbeautify --report junit } diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 65f297c72..e3b85ff8a 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -70,12 +70,12 @@ jobs: -destination 'platform=iOS Simulator,name=iPhone 16,OS=latest' \ -enableCodeCoverage NO \ -parallel-testing-enabled NO \ - -skip-testing:BitkitTests/TxBumpingTests \ -skip-testing:BitkitTests/UtxoSelectionTests \ -skip-testing:BitkitTests/BlocktankTests \ -skip-testing:BitkitTests/PaymentFlowTests \ -skip-testing:BitkitTests/BlocktankRefundAddressLiveIntegrationTests \ -skip-testing:BitkitTests/AddressTypeIntegrationTests \ + -skip-testing:BitkitTests/RNMigrationCleanupTests \ | xcbeautify --report junit echo "✅ Unit tests completed at $(date)" diff --git a/Bitkit/Utilities/InstallationMarker.swift b/Bitkit/Utilities/InstallationMarker.swift index 92ee51ccf..9f9662bae 100644 --- a/Bitkit/Utilities/InstallationMarker.swift +++ b/Bitkit/Utilities/InstallationMarker.swift @@ -16,7 +16,12 @@ enum InstallationMarker { /// App sandbox Documents directory (NOT app group) - gets deleted on uninstall private static var sandboxDocumentsUrl: URL { - FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0] + let documents = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0] + // Tests delete the marker, and a missing marker makes the next real launch treat the keychain + // as orphaned and wipe it (`AppScene.handleOrphanedKeychain`). Give them their own directory so + // that only ever happens to a marker a test created. `Env.appStorageUrl` namespaces the app + // group the same way; this file is deliberately outside it, so it needs its own redirect. + return Env.isUnitTest ? documents.appendingPathComponent("unit-tests") : documents } static var markerPath: URL { @@ -32,6 +37,7 @@ enum InstallationMarker { /// Should be called after handling any orphaned keychain detection static func create() throws { let data = UUID().uuidString.data(using: .utf8)! + try FileManager.default.createDirectory(at: sandboxDocumentsUrl, withIntermediateDirectories: true) try data.write(to: markerPath) Logger.info("Installation marker created", context: "InstallationMarker") } diff --git a/Bitkit/Utilities/Keychain.swift b/Bitkit/Utilities/Keychain.swift index 584e60e17..9097577ea 100644 --- a/Bitkit/Utilities/Keychain.swift +++ b/Bitkit/Utilities/Keychain.swift @@ -32,6 +32,19 @@ enum KeychainEntryType { } class Keychain { + private static let unitTestAccountPrefix = "unit-tests." + + /// Under test, entries live under a prefixed account name so a suite cannot read, overwrite or + /// delete the wallet belonging to whoever is running it. `BitkitTests` is hosted in the app and + /// `Env.network` resolves to regtest for both, so tests and a Debug build otherwise share one + /// keychain access group. This mirrors what `Env.appStorageUrl` already does for file storage, + /// and only ever narrows what a process can see — the access group stays pinned either way. + /// Internal rather than private so tests that build their own `SecItem` queries can address the + /// same entry `save`/`load` use. + class func account(for key: KeychainEntryType) -> String { + Env.isUnitTest ? unitTestAccountPrefix + key.storageKey : key.storageKey + } + class func save(key: KeychainEntryType, data: Data) throws { Logger.debug("Saving \(key.storageKey)", context: "Keychain") @@ -39,7 +52,7 @@ class Keychain { kSecClass as String: kSecClassGenericPassword as String, kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly as String, kSecAttrSynchronizable as String: false, - kSecAttrAccount as String: key.storageKey, + kSecAttrAccount as String: account(for: key), kSecValueData as String: data, kSecAttrAccessGroup as String: Env.keychainGroup, ] as [String: Any] @@ -87,7 +100,7 @@ class Keychain { let query = [ kSecClass as String: kSecClassGenericPassword as String, - kSecAttrAccount as String: key.storageKey, + kSecAttrAccount as String: account(for: key), kSecAttrAccessGroup as String: Env.keychainGroup, ] as [String: Any] @@ -130,7 +143,7 @@ class Keychain { class func delete(key: KeychainEntryType) throws { let query = [ kSecClass as String: kSecClassGenericPassword as String, - kSecAttrAccount as String: key.storageKey, + kSecAttrAccount as String: account(for: key), kSecAttrAccessGroup as String: Env.keychainGroup, ] as [String: Any] @@ -158,7 +171,7 @@ class Keychain { if existingData != nil { let searchQuery: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, - kSecAttrAccount as String: key.storageKey, + kSecAttrAccount as String: account(for: key), kSecAttrAccessGroup as String: Env.keychainGroup, ] let updateAttributes: [String: Any] = [ @@ -188,7 +201,7 @@ class Keychain { class func load(key: KeychainEntryType) throws -> Data? { let query = [ kSecClass as String: kSecClassGenericPassword, - kSecAttrAccount as String: key.storageKey, + kSecAttrAccount as String: account(for: key), kSecReturnData as String: kCFBooleanTrue!, kSecMatchLimit as String: kSecMatchLimitOne, kSecAttrAccessGroup as String: Env.keychainGroup, @@ -250,6 +263,12 @@ class Keychain { class func wipeEntireKeychain() throws { let keys = getAllKeyChainStorageKeys() for key in keys { + // A test run must only ever clear its own namespaced entries. Deleting an un-prefixed + // account here is what destroys the wallet on the simulator the suite runs against. + if Env.isUnitTest, !key.hasPrefix(unitTestAccountPrefix) { + continue + } + let query = [ kSecClass as String: kSecClassGenericPassword as String, kSecAttrAccount as String: key, diff --git a/BitkitTests/AppStateIsolation.swift b/BitkitTests/AppStateIsolation.swift index 2033f643b..754d8a4a3 100644 --- a/BitkitTests/AppStateIsolation.swift +++ b/BitkitTests/AppStateIsolation.swift @@ -39,6 +39,23 @@ extension XCTestCase { } } + /// Skips the test unless `BITKIT_DESTRUCTIVE_TESTS=1` is set. For the handful of suites that + /// deliberately operate on real, un-namespaceable state — the React-Native migration source under + /// `~/Documents`, for instance — and so can only run on a simulator that may be erased afterwards. + /// `integration-tests.yml` sets it; a plain `xcodebuild test` does not. + /// + /// To run one of these locally, set it as `TEST_RUNNER_BITKIT_DESTRUCTIVE_TESTS=1`. xcodebuild + /// forwards only variables named `TEST_RUNNER_` into the simulator-hosted runner, stripping + /// the prefix; the unprefixed name never arrives, and the tests skip while the run reports green. + func skipUnlessDestructiveTestsEnabled(file: StaticString = #filePath, line: UInt = #line) throws { + try XCTSkipUnless( + ProcessInfo.processInfo.environment["BITKIT_DESTRUCTIVE_TESTS"] == "1", + "Destroys real on-disk state; set BITKIT_DESTRUCTIVE_TESTS=1 to run.", + file: file, + line: line + ) + } + /// Fails the test if it leaves any of `keys` in `UserDefaults.standard` changed. The regression /// guard for #733: a suite that should be writing to an isolated suite goes red here instead of /// silently corrupting the wallet on the simulator. diff --git a/BitkitTests/ChannelMigrationPersistenceTests.swift b/BitkitTests/ChannelMigrationPersistenceTests.swift index 8b262d44b..06b026ca1 100644 --- a/BitkitTests/ChannelMigrationPersistenceTests.swift +++ b/BitkitTests/ChannelMigrationPersistenceTests.swift @@ -10,14 +10,13 @@ final class ChannelMigrationPersistenceTests: XCTestCase { override func setUp() { super.setUp() + // `pendingChannelMigration` is backed by UserDefaults.standard, which is the host app's own + // preferences. Nilling it unguarded discards a real pending migration — an RN channel manager + // and its monitors — so snapshot it first and put it back afterwards. + snapshotAppDefaults("rnPendingChannelMigration") migrations.pendingChannelMigration = nil } - override func tearDown() { - migrations.pendingChannelMigration = nil - super.tearDown() - } - func testPendingMigrationIsRetainedWhenSetupFails() async { let migration = makeMigration(seed: 1) migrations.pendingChannelMigration = migration diff --git a/BitkitTests/InstallationMarkerTests.swift b/BitkitTests/InstallationMarkerTests.swift index 248b8a208..08de3f955 100644 --- a/BitkitTests/InstallationMarkerTests.swift +++ b/BitkitTests/InstallationMarkerTests.swift @@ -66,6 +66,30 @@ final class InstallationMarkerTests: XCTestCase { XCTAssertFalse(InstallationMarker.markerPath.path.contains("group.bitkit")) } + func testMarkerPathIsIsolatedUnderTest() { + XCTAssertTrue(InstallationMarker.markerPath.pathComponents.contains("unit-tests")) + } + + /// `delete()` under test must not reach the real marker: a missing marker makes the next launch + /// treat the keychain as orphaned and wipe it. Plant a stand-in only when no real marker exists and + /// remove only that stand-in — on a simulator with a real install, deleting it is the bug itself. + func testDeleteLeavesTheRealMarkerAlone() throws { + let documents = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0] + let realMarker = documents.appendingPathComponent(".bitkit_installed") + if !FileManager.default.fileExists(atPath: realMarker.path) { + try Data("probe".utf8).write(to: realMarker) + addTeardownBlock { try? FileManager.default.removeItem(at: realMarker) } + } + + try InstallationMarker.create() + try InstallationMarker.delete() + + XCTAssertTrue( + FileManager.default.fileExists(atPath: realMarker.path), + "InstallationMarker.delete() removed the real marker" + ) + } + func testCreateMarkerIsIdempotent() throws { // Create the marker try InstallationMarker.create() diff --git a/BitkitTests/KeychainTests.swift b/BitkitTests/KeychainTests.swift index c34101c4e..f296a119b 100644 --- a/BitkitTests/KeychainTests.swift +++ b/BitkitTests/KeychainTests.swift @@ -21,7 +21,7 @@ final class KeychainTests: XCTestCase { // Query the item with attributes returned let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, - kSecAttrAccount as String: KeychainEntryType.bip39Mnemonic(index: 0).storageKey, + kSecAttrAccount as String: Keychain.account(for: .bip39Mnemonic(index: 0)), kSecAttrAccessGroup as String: Env.keychainGroup, kSecReturnAttributes as String: true, kSecReturnData as String: false, @@ -67,7 +67,7 @@ final class KeychainTests: XCTestCase { let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, - kSecAttrAccount as String: KeychainEntryType.securityPin.storageKey, + kSecAttrAccount as String: Keychain.account(for: .securityPin), kSecAttrAccessGroup as String: Env.keychainGroup, kSecReturnAttributes as String: true, ] @@ -136,4 +136,28 @@ final class KeychainTests: XCTestCase { XCTAssertFalse((try? Keychain.exists(key: .bip39Passphrase(index: i))) ?? false) } } + + /// The wipe must leave accounts outside the unit-test namespace alone, or running this suite deletes + /// the wallet on the simulator it runs against. The probe uses a unique account name rather than a + /// real key like `bip39_mnemonic_0`: on a simulator with a wallet that name is already taken, and + /// cleaning it up would delete the real seed. + func testWipeLeavesAccountsOutsideTheTestNamespace() throws { + let probe: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrAccount as String: "wipe-guard-probe-\(UUID().uuidString)", + kSecAttrAccessGroup as String: Env.keychainGroup, + ] + var item = probe + item[kSecValueData as String] = Data("probe".utf8) + XCTAssertEqual(SecItemAdd(item as CFDictionary, nil), errSecSuccess, "Failed to plant the probe account") + addTeardownBlock { SecItemDelete(probe as CFDictionary) } + + try Keychain.wipeEntireKeychain() + + XCTAssertEqual( + SecItemCopyMatching(probe as CFDictionary, nil), + errSecSuccess, + "wipeEntireKeychain deleted an account outside the unit-test namespace" + ) + } } diff --git a/BitkitTests/RNMigrationCleanupTests.swift b/BitkitTests/RNMigrationCleanupTests.swift index 4f5a3217b..ddb2e7b1e 100644 --- a/BitkitTests/RNMigrationCleanupTests.swift +++ b/BitkitTests/RNMigrationCleanupTests.swift @@ -25,17 +25,29 @@ final class RNMigrationCleanupTests: XCTestCase { sandboxDocuments.appendingPathComponent("ldk") } - override func setUp() { - super.setUp() + /// `tearDown` runs even when `setUpWithError` skips, so it must not clean up a run that never + /// started — that is the destructive part. + private var didRunDestructiveSetUp = false + + override func setUpWithError() throws { + try super.setUpWithError() + // `cleanupRNTestData` deletes the real ~/Documents/mmkv and ~/Documents/ldk, and + // `cleanupRNKeychain` deletes RN keychain items by service with no access-group filter. On a + // device mid-migration from React-Native Bitkit that is the migration source, so unlike the + // native keychain this cannot be namespaced away — it has to be opted into. + try skipUnlessDestructiveTestsEnabled() + didRunDestructiveSetUp = true // Clean up any existing test data cleanupRNTestData() try? Keychain.wipeEntireKeychain() } override func tearDown() { - // Clean up after tests - cleanupRNTestData() - try? Keychain.wipeEntireKeychain() + if didRunDestructiveSetUp { + // Clean up after tests + cleanupRNTestData() + try? Keychain.wipeEntireKeychain() + } super.tearDown() }