From 5c4d0211ea776660f61f82b3c6315c00190b5e4f Mon Sep 17 00:00:00 2001 From: Joao Dordio Date: Thu, 21 May 2026 16:52:12 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=E2=9C=A8=20move=20keychain=20migration=20o?= =?UTF-8?q?ff=20main=20thread?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 6 + swift-sdk/Core/Constants.swift | 1 + swift-sdk/Internal/InternalIterableAPI.swift | 22 ++- swift-sdk/Internal/IterableUserDefaults.swift | 11 +- .../Utilities/Keychain/IterableKeychain.swift | 170 +++++++++--------- .../Internal/Utilities/LocalStorage.swift | 10 +- .../Utilities/LocalStorageProtocol.swift | 8 +- tests/common/MockLocalStorage.swift | 4 +- tests/unit-tests/AuthTests.swift | 86 +++++++++ tests/unit-tests/KeychainMigrationTests.swift | 69 +++++++ 10 files changed, 291 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e4e46027..68908b51c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Fixed +- [SDK-478] Moved keychain migration (`migrateKeychainToIsolatedStorage`) off the main thread and gated it as a one-shot via a UserDefaults flag. Prior to this change the migration ran on the caller's thread on every `start()` invocation; for cross-platform bridges (React Native and Flutter), `start()` is called from `DispatchQueue.main.async`, so the migration's keychain syscalls blocked the main thread on every cold start. The migration now dispatches to `DispatchQueue.global(qos: .userInitiated)` and runs only when a UserDefaults flag indicates it has not yet completed for this install. Reads of `email`, `userId`, `userIdUnknownUser`, and `authToken` are coordinated against the in-flight migration via a concurrent dispatch queue with a barrier write, so callers observe consistent state regardless of timing. See [SDK-294] for context on the original isolated-keychain change. +- Added empty-key guard and tighter `SecItemCopyMatching` status handling on `KeychainWrapper.data(forKey:)` / `set(_:forKey:)` / `removeValue(forKey:)` to prevent edge-case crashes on app launch. +- Added duration logging to `IterableKeychain.migrateFromLegacy()` so future regressions of this shape surface in dev logs immediately. +- Added non-success logging to `KeychainWrapper.set(_:forKey:)` and `update(_:forKey:)` so silent `SecItemAdd`/`SecItemUpdate` failures (e.g. entitlement issues) become observable. + ## [6.7.1] ### Fixed - Fixed the `enableUnknownUserActivation` default value to `false` diff --git a/swift-sdk/Core/Constants.swift b/swift-sdk/Core/Constants.swift index f2dd4423c..751f7938f 100644 --- a/swift-sdk/Core/Constants.swift +++ b/swift-sdk/Core/Constants.swift @@ -96,6 +96,7 @@ enum Const { static let visitorConsentTimestamp = "itbl_visitor_consent_timestamp" static let isNotificationsEnabled = "itbl_isNotificationsEnabled" static let hasStoredNotificationSetting = "itbl_hasStoredNotificationSetting" + static let keychainMigrationCompleted = "itbl_keychain_migration_completed" static let attributionInfoExpiration = 24 } diff --git a/swift-sdk/Internal/InternalIterableAPI.swift b/swift-sdk/Internal/InternalIterableAPI.swift index 1c0a1ef9b..566fd2e8d 100644 --- a/swift-sdk/Internal/InternalIterableAPI.swift +++ b/swift-sdk/Internal/InternalIterableAPI.swift @@ -1101,10 +1101,24 @@ final class InternalIterableAPI: NSObject, PushTrackerProtocol, AuthProvider { } private func updateSDKVersion() { - // Always attempt keychain migration to handle uninstall/reinstall scenario - // where UserDefaults are cleared but keychain persists - localStorage.migrateKeychainToIsolatedStorage() - + // Keychain migration is one-shot per SDK upgrade. We gate on a + // UserDefaults flag so subsequent cold starts skip the SecItem + // syscall cost entirely, and dispatch the actual work to a + // background queue so the calling thread (which for cross-platform + // bridges is the main thread, via DispatchQueue.main.async) is not + // blocked for the duration of the migration. See SDK-478. + // + // Reads of email / userId / authToken / userIdUnknownUser are + // race-safe against the in-flight migration thanks to the barrier + // coordinator inside IterableKeychain. + if !localStorage.keychainMigrationCompleted { + DispatchQueue.global(qos: .userInitiated).async { [localStorage] in + var localStorage = localStorage + localStorage.migrateKeychainToIsolatedStorage() + localStorage.keychainMigrationCompleted = true + } + } + if let lastVersion = localStorage.sdkVersion, lastVersion != IterableAPI.sdkVersion { performUpgrade(lastVersion: lastVersion, newVersion: IterableAPI.sdkVersion) } else { diff --git a/swift-sdk/Internal/IterableUserDefaults.swift b/swift-sdk/Internal/IterableUserDefaults.swift index c3fbe9ad9..c79ddf1ea 100644 --- a/swift-sdk/Internal/IterableUserDefaults.swift +++ b/swift-sdk/Internal/IterableUserDefaults.swift @@ -177,7 +177,15 @@ class IterableUserDefaults { save(bool: newValue, withKey: .hasStoredNotificationSetting) } } - + + var keychainMigrationCompleted: Bool { + get { + bool(withKey: .keychainMigrationCompleted) + } set { + save(bool: newValue, withKey: .keychainMigrationCompleted) + } + } + func getAttributionInfo(currentDate: Date) -> IterableAttributionInfo? { (try? codable(withKey: .attributionInfo, currentDate: currentDate)) ?? nil } @@ -355,6 +363,7 @@ class IterableUserDefaults { static let isNotificationsEnabled = UserDefaultsKey(value: Const.UserDefault.isNotificationsEnabled) static let hasStoredNotificationSetting = UserDefaultsKey(value: Const.UserDefault.hasStoredNotificationSetting) + static let keychainMigrationCompleted = UserDefaultsKey(value: Const.UserDefault.keychainMigrationCompleted) } private struct Envelope: Codable { let payload: Data diff --git a/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift b/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift index 981cc5028..ba22768ab 100644 --- a/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift +++ b/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift @@ -14,131 +14,125 @@ class IterableKeychain { /// Migrates keychain data from legacy storage to isolated storage /// This is needed when upgrading from older SDK versions that used a shared service name /// - Returns: true if migration was performed, false otherwise + /// + /// Runs as a barrier on `coordinatorQueue` so it blocks until in-flight + /// reads finish, then blocks subsequent reads / writes until the barrier + /// returns. After it returns, reads run fully in parallel. Uses raw + /// `wrapper`/`legacyWrapper` access internally to avoid re-entering the + /// queue (the public properties also go through the queue and would + /// deadlock if used from inside the barrier). @discardableResult func migrateFromLegacy() -> Bool { guard let legacyWrapper = legacyWrapper else { return false } - var migrated = false + return coordinatorQueue.sync(flags: .barrier) { + var migrated = false + let migrationStartedAt = Date() - // Migrate email if isolated keychain is empty and legacy has data - if email == nil, let legacyEmail = getString(forKey: Const.Keychain.Key.email, from: legacyWrapper) { - email = legacyEmail - legacyWrapper.removeValue(forKey: Const.Keychain.Key.email) - ITBInfo("UPDATED: migrated email from legacy keychain to isolated keychain") - migrated = true - } + if rawString(forKey: Const.Keychain.Key.email, from: wrapper) == nil, + let legacyEmail = rawString(forKey: Const.Keychain.Key.email, from: legacyWrapper) { + rawWrite(string: legacyEmail, forKey: Const.Keychain.Key.email, to: wrapper) + legacyWrapper.removeValue(forKey: Const.Keychain.Key.email) + ITBInfo("UPDATED: migrated email from legacy keychain to isolated keychain") + migrated = true + } - // Migrate userId if isolated keychain is empty and legacy has data - if userId == nil, let legacyUserId = getString(forKey: Const.Keychain.Key.userId, from: legacyWrapper) { - userId = legacyUserId - legacyWrapper.removeValue(forKey: Const.Keychain.Key.userId) - ITBInfo("UPDATED: migrated userId from legacy keychain to isolated keychain") - migrated = true - } + if rawString(forKey: Const.Keychain.Key.userId, from: wrapper) == nil, + let legacyUserId = rawString(forKey: Const.Keychain.Key.userId, from: legacyWrapper) { + rawWrite(string: legacyUserId, forKey: Const.Keychain.Key.userId, to: wrapper) + legacyWrapper.removeValue(forKey: Const.Keychain.Key.userId) + ITBInfo("UPDATED: migrated userId from legacy keychain to isolated keychain") + migrated = true + } - // Migrate userIdUnknownUser if isolated keychain is empty and legacy has data - if userIdUnknownUser == nil, let legacyUserIdUnknownUser = getString(forKey: Const.Keychain.Key.userIdUnknownUser, from: legacyWrapper) { - userIdUnknownUser = legacyUserIdUnknownUser - legacyWrapper.removeValue(forKey: Const.Keychain.Key.userIdUnknownUser) - ITBInfo("UPDATED: migrated userIdUnknownUser from legacy keychain to isolated keychain") - migrated = true - } + if rawString(forKey: Const.Keychain.Key.userIdUnknownUser, from: wrapper) == nil, + let legacyUserIdUnknownUser = rawString(forKey: Const.Keychain.Key.userIdUnknownUser, from: legacyWrapper) { + rawWrite(string: legacyUserIdUnknownUser, forKey: Const.Keychain.Key.userIdUnknownUser, to: wrapper) + legacyWrapper.removeValue(forKey: Const.Keychain.Key.userIdUnknownUser) + ITBInfo("UPDATED: migrated userIdUnknownUser from legacy keychain to isolated keychain") + migrated = true + } - // Migrate authToken if isolated keychain is empty and legacy has data - if authToken == nil, let legacyAuthToken = getString(forKey: Const.Keychain.Key.authToken, from: legacyWrapper) { - authToken = legacyAuthToken - legacyWrapper.removeValue(forKey: Const.Keychain.Key.authToken) - ITBInfo("UPDATED: migrated authToken from legacy keychain to isolated keychain") - migrated = true - } + if rawString(forKey: Const.Keychain.Key.authToken, from: wrapper) == nil, + let legacyAuthToken = rawString(forKey: Const.Keychain.Key.authToken, from: legacyWrapper) { + rawWrite(string: legacyAuthToken, forKey: Const.Keychain.Key.authToken, to: wrapper) + legacyWrapper.removeValue(forKey: Const.Keychain.Key.authToken) + ITBInfo("UPDATED: migrated authToken from legacy keychain to isolated keychain") + migrated = true + } - return migrated - } + let elapsedMs = Int(Date().timeIntervalSince(migrationStartedAt) * 1000) + ITBInfo("keychain migrateFromLegacy completed in \(elapsedMs)ms migrated=\(migrated)") - /// Helper to get string from a specific wrapper - private func getString(forKey key: String, from wrapper: KeychainWrapper) -> String? { - let data = wrapper.data(forKey: key) - return data.flatMap { String(data: $0, encoding: .utf8) } + return migrated + } } - + var email: String? { - get { - let data = wrapper.data(forKey: Const.Keychain.Key.email) - - return data.flatMap { String(data: $0, encoding: .utf8) } - } - + get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.email, from: wrapper) } } set { - guard let token = newValue, - let data = token.data(using: .utf8) else { - wrapper.removeValue(forKey: Const.Keychain.Key.email) - return + coordinatorQueue.sync(flags: .barrier) { + rawWrite(string: newValue, forKey: Const.Keychain.Key.email, to: wrapper) } - - wrapper.set(data, forKey: Const.Keychain.Key.email) } } - + var userId: String? { - get { - let data = wrapper.data(forKey: Const.Keychain.Key.userId) - - return data.flatMap { String(data: $0, encoding: .utf8) } - } - + get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.userId, from: wrapper) } } set { - guard let token = newValue, - let data = token.data(using: .utf8) else { - wrapper.removeValue(forKey: Const.Keychain.Key.userId) - return + coordinatorQueue.sync(flags: .barrier) { + rawWrite(string: newValue, forKey: Const.Keychain.Key.userId, to: wrapper) } - - wrapper.set(data, forKey: Const.Keychain.Key.userId) } } - + var userIdUnknownUser: String? { - get { - let data = wrapper.data(forKey: Const.Keychain.Key.userIdUnknownUser) - - return data.flatMap { String(data: $0, encoding: .utf8) } - } - + get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.userIdUnknownUser, from: wrapper) } } set { - guard let token = newValue, - let data = token.data(using: .utf8) else { - wrapper.removeValue(forKey: Const.Keychain.Key.userIdUnknownUser) - return + coordinatorQueue.sync(flags: .barrier) { + rawWrite(string: newValue, forKey: Const.Keychain.Key.userIdUnknownUser, to: wrapper) } - - wrapper.set(data, forKey: Const.Keychain.Key.userIdUnknownUser) } } - + var authToken: String? { - get { - let data = wrapper.data(forKey: Const.Keychain.Key.authToken) - - return data.flatMap { String(data: $0, encoding: .utf8) } - } - + get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.authToken, from: wrapper) } } set { - guard let token = newValue, - let data = token.data(using: .utf8) else { - wrapper.removeValue(forKey: Const.Keychain.Key.authToken) - return + coordinatorQueue.sync(flags: .barrier) { + rawWrite(string: newValue, forKey: Const.Keychain.Key.authToken, to: wrapper) } - - wrapper.set(data, forKey: Const.Keychain.Key.authToken) } } - + // MARK: - PRIVATE/INTERNAL private let wrapper: KeychainWrapper private let legacyWrapper: KeychainWrapper? + + /// Concurrent queue used to serialize the migration (which runs as a + /// barrier) against the property reads/writes. Reads run fully in + /// parallel after migration completes. See SDK-478. + private let coordinatorQueue = DispatchQueue(label: "com.iterable.IterableKeychain.coordinator", + attributes: .concurrent) + + /// Raw read - bypasses the coordinator queue. Only safe to call from + /// inside a `coordinatorQueue.sync { }` block. + private func rawString(forKey key: String, from wrapper: KeychainWrapper) -> String? { + let data = wrapper.data(forKey: key) + return data.flatMap { String(data: $0, encoding: .utf8) } + } + + /// Raw write - bypasses the coordinator queue. Only safe to call from + /// inside a `coordinatorQueue.sync(flags: .barrier) { }` block. + private func rawWrite(string: String?, forKey key: String, to wrapper: KeychainWrapper) { + guard let value = string, let data = value.data(using: .utf8) else { + wrapper.removeValue(forKey: key) + return + } + wrapper.set(data, forKey: key) + } private func encodeJsonPayload(_ json: [AnyHashable: Any]?) -> Data? { guard let json = json, JSONSerialization.isValidJSONObject(json) else { diff --git a/swift-sdk/Internal/Utilities/LocalStorage.swift b/swift-sdk/Internal/Utilities/LocalStorage.swift index 83e64946e..53c4888a6 100644 --- a/swift-sdk/Internal/Utilities/LocalStorage.swift +++ b/swift-sdk/Internal/Utilities/LocalStorage.swift @@ -154,7 +154,15 @@ struct LocalStorage: LocalStorageProtocol { iterableUserDefaults.hasStoredNotificationSetting = newValue } } - + + var keychainMigrationCompleted: Bool { + get { + iterableUserDefaults.keychainMigrationCompleted + } set { + iterableUserDefaults.keychainMigrationCompleted = newValue + } + } + func getAttributionInfo(currentDate: Date) -> IterableAttributionInfo? { iterableUserDefaults.getAttributionInfo(currentDate: currentDate) } diff --git a/swift-sdk/Internal/Utilities/LocalStorageProtocol.swift b/swift-sdk/Internal/Utilities/LocalStorageProtocol.swift index 7a479c945..d7a3c1099 100644 --- a/swift-sdk/Internal/Utilities/LocalStorageProtocol.swift +++ b/swift-sdk/Internal/Utilities/LocalStorageProtocol.swift @@ -38,7 +38,13 @@ protocol LocalStorageProtocol { var isNotificationsEnabled: Bool { get set } var hasStoredNotificationSetting: Bool { get set } - + + /// Tracks whether `migrateKeychainToIsolatedStorage()` has run to + /// completion at least once on this install. Gates the migration so it + /// is a true one-shot - subsequent `start()` calls do not pay the + /// SecItem syscall cost. See SDK-478. + var keychainMigrationCompleted: Bool { get set } + func getAttributionInfo(currentDate: Date) -> IterableAttributionInfo? func save(attributionInfo: IterableAttributionInfo?, withExpiration expiration: Date?) diff --git a/tests/common/MockLocalStorage.swift b/tests/common/MockLocalStorage.swift index 4b48fb25b..41977df69 100644 --- a/tests/common/MockLocalStorage.swift +++ b/tests/common/MockLocalStorage.swift @@ -41,7 +41,9 @@ class MockLocalStorage: LocalStorageProtocol { var isNotificationsEnabled: Bool = false var hasStoredNotificationSetting: Bool = false - + + var keychainMigrationCompleted: Bool = false + func getAttributionInfo(currentDate: Date) -> IterableAttributionInfo? { guard !MockLocalStorage.isExpired(expiration: attributionInfoExpiration, currentDate: currentDate) else { return nil diff --git a/tests/unit-tests/AuthTests.swift b/tests/unit-tests/AuthTests.swift index ac82a1950..8bc48810e 100644 --- a/tests/unit-tests/AuthTests.swift +++ b/tests/unit-tests/AuthTests.swift @@ -1251,6 +1251,92 @@ class AuthTests: XCTestCase { _ = authDelegate } + /// SDK-478 gap-fill: complements `testLogout_resolvesPendingFulfills_viaExhaustion` by + /// exercising `logoutUser()` while *multiple* callers are queued in + /// `pendingExhaustionCallbacks` via the timer-piggyback path + /// (`scheduleAuthTokenRefreshTimer` with `isTimerScheduled=true`). Pre-#1058 every + /// piggybacked caller would orphan; the fix routes all of them through + /// `invokePendingExhaustionCallbacks()` on logout. + func testLogout_drainsAllPiggybackedExhaustionCallbacks() { + let firstExhausted = expectation(description: "first piggybacked caller exhaustion fires on logout") + let secondExhausted = expectation(description: "second piggybacked caller exhaustion fires on logout") + let thirdExhausted = expectation(description: "third piggybacked caller exhaustion fires on logout") + + class HangingAuthDelegate: IterableAuthDelegate { + func onAuthTokenRequested(completion: @escaping AuthTokenRetrievalHandler) { /* never completes */ } + func onAuthFailure(_ authFailure: AuthFailure) {} + } + + let localStorage = MockLocalStorage() + localStorage.email = AuthTests.email + + let authManager = AuthManager(delegate: HangingAuthDelegate(), + authRetryPolicy: RetryPolicy(maxRetry: 10, retryInterval: 0, retryBackoff: .linear), + expirationRefreshPeriod: 60, + localStorage: localStorage, + dateProvider: MockDateProvider()) + + // Three back-to-back schedules: the first arms the timer, the next two piggyback + // via `isTimerScheduled` and end up in pendingExhaustionCallbacks. + authManager.scheduleAuthTokenRefreshTimer(interval: 5.0, + isScheduledRefresh: false, + successCallback: { _ in XCTFail("success should not fire after logout") }, + onRetryExhausted: { firstExhausted.fulfill() }) + authManager.scheduleAuthTokenRefreshTimer(interval: 5.0, + isScheduledRefresh: false, + successCallback: { _ in XCTFail("success should not fire after logout") }, + onRetryExhausted: { secondExhausted.fulfill() }) + authManager.scheduleAuthTokenRefreshTimer(interval: 5.0, + isScheduledRefresh: false, + successCallback: { _ in XCTFail("success should not fire after logout") }, + onRetryExhausted: { thirdExhausted.fulfill() }) + + DispatchQueue.main.asyncAfter(deadline: .now() + 0.05) { + authManager.logoutUser() + } + + wait(for: [firstExhausted, secondExhausted, thirdExhausted], timeout: testExpectationTimeout) + } + + /// SDK-478 gap-fill: covers the third early-return inside `AuthManager.requestNewAuthToken` + /// (the `hasFailedAuth(_:)` branch). Pre-#1058 this branch dropped both the success and + /// the (non-existent) exhaustion callback silently — the upstream `Pending` would hang + /// forever once a prior auth attempt had failed. The fix drains via + /// `invokePendingExhaustionCallbacks()`. We assert that both callers' `onRetryExhausted` + /// fire when the second call hits the early return. + func testRequestNewAuthToken_drainsExhaustion_whenHasFailedPriorAuthEarlyReturn() { + let firstExhausted = expectation(description: "first caller exhaustion fires") + let secondExhausted = expectation(description: "second caller exhaustion fires after hasFailedAuth early return") + + let authDelegate = createAuthDelegate({ nil }) + let localStorage = MockLocalStorage() + localStorage.email = AuthTests.email + + // High retryInterval keeps the failure-path Timer from racing the second call; + // high maxRetry keeps shouldPauseRetry from short-circuiting first. + let authManager = AuthManager(delegate: authDelegate, + authRetryPolicy: RetryPolicy(maxRetry: 10, retryInterval: 60, retryBackoff: .linear), + expirationRefreshPeriod: 60, + localStorage: localStorage, + dateProvider: MockDateProvider()) + + // Call 1: drives self.hasFailedPriorAuth → true via the delegate-returns-nil path, + // queues onRetryExhausted into pendingExhaustionCallbacks via scheduleAuthTokenRefreshTimer. + authManager.requestNewAuthToken(hasFailedPriorAuth: true, + onSuccess: { _ in XCTFail("success should not fire when delegate returns nil") }, + onRetryExhausted: { firstExhausted.fulfill() }, + shouldIgnoreRetryPolicy: true) + + // Call 2: hits the hasFailedAuth(_:) early return; the fix path appends our exhaustion + // callback then drains, so both calls' exhaustion callbacks fire. + authManager.requestNewAuthToken(hasFailedPriorAuth: true, + onSuccess: { _ in XCTFail("success should not fire after hasFailedAuth early return") }, + onRetryExhausted: { secondExhausted.fulfill() }, + shouldIgnoreRetryPolicy: true) + + wait(for: [firstExhausted, secondExhausted], timeout: testExpectationTimeout) + } + // MARK: - Private class DefaultAuthDelegate: IterableAuthDelegate { diff --git a/tests/unit-tests/KeychainMigrationTests.swift b/tests/unit-tests/KeychainMigrationTests.swift index 31ccab328..fb382c075 100644 --- a/tests/unit-tests/KeychainMigrationTests.swift +++ b/tests/unit-tests/KeychainMigrationTests.swift @@ -225,4 +225,73 @@ class KeychainMigrationTests: XCTestCase { // Email should still be correct XCTAssertEqual(keychain.email, testEmail) } + + // MARK: - SDK-478 thread / one-shot / race tests + + /// SDK-478: cross-platform iOS bridges invoke `IterableAPI.initialize` + /// from `DispatchQueue.main.async`. The migration must run off the main + /// thread so it does not block JS / Dart callers. updateSDKVersion() + /// dispatches it to `DispatchQueue.global(qos: .userInitiated)`; this + /// test calls the migration directly from a background queue and asserts + /// it executes there, not on main. + func testMigrationRunsOffMainThread() throws { + let legacyWrapper = KeychainWrapper(serviceName: legacyServiceName) + let isolatedWrapper = KeychainWrapper(serviceName: isolatedServiceName) + XCTAssertTrue(legacyWrapper.set("user@example.com".data(using: .utf8)!, + forKey: Const.Keychain.Key.email)) + + let keychain = IterableKeychain(wrapper: isolatedWrapper, legacyWrapper: legacyWrapper) + let didRunOffMain = expectation(description: "migration body executed off main thread") + + DispatchQueue.global(qos: .userInitiated).async { + // The migration coordinator queue is concurrent and the barrier + // sync runs on whichever queue invoked it (here, the global + // userInitiated queue) - never on main. + keychain.migrateFromLegacy() + if !Thread.isMainThread { + didRunOffMain.fulfill() + } + } + + wait(for: [didRunOffMain], timeout: 5) + } + + /// SDK-478: the migration must remain race-safe against concurrent reads + /// that arrive while it is in flight. Concurrent reads through the + /// coordinator queue should observe the migrated value, never a + /// half-migrated state where the isolated keychain is empty and the + /// legacy one has been wiped. + func testReadDuringMigrationReturnsConsistentData() throws { + let legacyWrapper = KeychainWrapper(serviceName: legacyServiceName) + let isolatedWrapper = KeychainWrapper(serviceName: isolatedServiceName) + let migratedEmail = "race@example.com" + XCTAssertTrue(legacyWrapper.set(migratedEmail.data(using: .utf8)!, + forKey: Const.Keychain.Key.email)) + + let keychain = IterableKeychain(wrapper: isolatedWrapper, legacyWrapper: legacyWrapper) + let readsCompleted = expectation(description: "concurrent reads finished") + readsCompleted.expectedFulfillmentCount = 10 + + // Kick off the migration on a background queue. + DispatchQueue.global(qos: .userInitiated).async { + keychain.migrateFromLegacy() + } + + // Fire concurrent reads. Each one routes through the same + // coordinator queue. They must all observe either nil (read landed + // before migration started) or the migrated email - never an + // inconsistent intermediate state. + for _ in 0..<10 { + DispatchQueue.global(qos: .utility).async { + let observed = keychain.email + XCTAssert(observed == nil || observed == migratedEmail, + "unexpected intermediate keychain state: \(String(describing: observed))") + readsCompleted.fulfill() + } + } + + wait(for: [readsCompleted], timeout: 5) + // Once the dust settles, the migrated value must be visible. + XCTAssertEqual(keychain.email, migratedEmail) + } } From 6215042c3800961d151f1c0e20bdd4ca9cb511ec Mon Sep 17 00:00:00 2001 From: Joao Dordio Date: Thu, 21 May 2026 16:52:40 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=90=9B=20KeychainWrapper=20crash=20fi?= =?UTF-8?q?x?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Utilities/Keychain/KeychainWrapper.swift | 50 +++++++++++++------ tests/unit-tests/KeychainWrapperTests.swift | 22 ++++++++ 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift b/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift index b003147ec..4464d2ba8 100644 --- a/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift +++ b/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift @@ -30,47 +30,65 @@ class KeychainWrapper { @discardableResult func set(_ value: Data, forKey key: String) -> Bool { + guard !key.isEmpty else { return false } + var keychainQueryDictionary: [String: Any] = setupKeychainQueryDictionary(forKey: key) - + keychainQueryDictionary[SecValueData] = value - + // Assign default protection - Protect the keychain entry so it's only valid when the device is unlocked keychainQueryDictionary[SecAttrAccessible] = SecAttrAccessibleWhenUnlocked - + let status: OSStatus = SecItemAdd(keychainQueryDictionary as CFDictionary, nil) - + if status == errSecSuccess { return true } else if status == errSecDuplicateItem { return update(value, forKey: key) } else { + // Surface silent failure paths (e.g. errSecMissingEntitlement in + // misconfigured Expo apps) so they become observable in dev logs. + // See SDK-478. + ITBError("keychain SecItemAdd failed for key=\(key) status=\(status)") return false } } - + func data(forKey key: String) -> Data? { + guard !key.isEmpty else { return nil } + var keychainQueryDictionary = setupKeychainQueryDictionary(forKey: key) - + // Limit search results to one keychainQueryDictionary[SecMatchLimit] = SecMatchLimitOne - + // Specify we want Data/CFData returned keychainQueryDictionary[SecReturnData] = CFBooleanTrue - + // Search var result: AnyObject? let status = SecItemCopyMatching(keychainQueryDictionary as CFDictionary, &result) - + + // Treat anything other than noErr / errSecItemNotFound as a benign + // miss rather than passing weird statuses through; this prevents + // downstream crashes when callers force-unwrap the returned data. + // See SDK-478 / b521d533. + guard status == noErr || status == errSecItemNotFound else { + return nil + } + return status == noErr ? result as? Data : nil } - + @discardableResult func removeValue(forKey key: String) -> Bool { + guard !key.isEmpty else { return false } + let keychainQueryDictionary: [String: Any] = setupKeychainQueryDictionary(forKey: key) - + // Delete let status: OSStatus = SecItemDelete(keychainQueryDictionary as CFDictionary) - + return status == errSecSuccess } @@ -109,10 +127,14 @@ class KeychainWrapper { private func update(_ value: Data, forKey key: String) -> Bool { let keychainQueryDictionary: [String: Any] = setupKeychainQueryDictionary(forKey: key) let updateDictionary = [SecValueData: value] - + // Update let status: OSStatus = SecItemUpdate(keychainQueryDictionary as CFDictionary, updateDictionary as CFDictionary) - + + if status != errSecSuccess { + ITBError("keychain SecItemUpdate failed for key=\(key) status=\(status)") + } + return status == errSecSuccess } diff --git a/tests/unit-tests/KeychainWrapperTests.swift b/tests/unit-tests/KeychainWrapperTests.swift index 69df26657..b5312452d 100644 --- a/tests/unit-tests/KeychainWrapperTests.swift +++ b/tests/unit-tests/KeychainWrapperTests.swift @@ -178,4 +178,26 @@ class KeychainWrapperTests: XCTestCase { isolatedWrapper1.removeAll() isolatedWrapper2.removeAll() } + + // MARK: - SDK-478 / b521d533 defensive guards + + /// SDK-478: empty keys can reach the wrapper via misconfigured callers. + /// Each accessor must return its safe-default rather than passing the + /// empty string into `SecItem*` (which can return non-standard statuses + /// that downstream code does not handle). + func testEmptyKeyReturnsNilImmediately() throws { + let wrapper = KeychainWrapper(serviceName: "test-keychain") + XCTAssertNil(wrapper.data(forKey: "")) + } + + func testEmptyKeySetReturnsFalse() throws { + let wrapper = KeychainWrapper(serviceName: "test-keychain") + let value = "anything".data(using: .utf8)! + XCTAssertFalse(wrapper.set(value, forKey: "")) + } + + func testEmptyKeyRemoveReturnsFalse() throws { + let wrapper = KeychainWrapper(serviceName: "test-keychain") + XCTAssertFalse(wrapper.removeValue(forKey: "")) + } } From 29fa5ed373985d1216eeeb60c75002d92faa0564 Mon Sep 17 00:00:00 2001 From: Joao Dordio Date: Thu, 21 May 2026 17:26:53 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=A7=AA=20Added=20more=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/unit-tests/KeychainMigrationTests.swift | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/unit-tests/KeychainMigrationTests.swift b/tests/unit-tests/KeychainMigrationTests.swift index fb382c075..9eece1e9d 100644 --- a/tests/unit-tests/KeychainMigrationTests.swift +++ b/tests/unit-tests/KeychainMigrationTests.swift @@ -294,4 +294,49 @@ class KeychainMigrationTests: XCTestCase { // Once the dust settles, the migrated value must be visible. XCTAssertEqual(keychain.email, migratedEmail) } + + /// SDK-478: exercise the four public property setters through the + /// coordinator queue's barrier write. Existing migration tests write + /// through the underlying wrappers directly and only read via the + /// public getters, so the setter paths went uncovered. + func testPublicSettersWriteThroughCoordinatorQueue() throws { + let isolatedWrapper = KeychainWrapper(serviceName: isolatedServiceName) + let keychain = IterableKeychain(wrapper: isolatedWrapper, legacyWrapper: nil) + + // Set non-nil values. Each setter takes the barrier sync path and + // calls rawWrite, which writes to the underlying KeychainWrapper. + keychain.email = "set-via-property@example.com" + keychain.userId = "set-via-property-user" + keychain.userIdUnknownUser = "set-via-property-uua" + keychain.authToken = "set-via-property-token" + + XCTAssertEqual(keychain.email, "set-via-property@example.com") + XCTAssertEqual(keychain.userId, "set-via-property-user") + XCTAssertEqual(keychain.userIdUnknownUser, "set-via-property-uua") + XCTAssertEqual(keychain.authToken, "set-via-property-token") + } + + /// SDK-478: setting a property to nil routes rawWrite into the + /// `wrapper.removeValue` branch. Covers the nil-removal path of the + /// setter without requiring the migration flow. + func testPublicSetterNilRemovesValue() throws { + let isolatedWrapper = KeychainWrapper(serviceName: isolatedServiceName) + let keychain = IterableKeychain(wrapper: isolatedWrapper, legacyWrapper: nil) + + // Seed via the setter, then clear via the setter. + keychain.email = "to-be-cleared@example.com" + keychain.userId = "to-be-cleared" + keychain.userIdUnknownUser = "to-be-cleared-uua" + keychain.authToken = "to-be-cleared-token" + + keychain.email = nil + keychain.userId = nil + keychain.userIdUnknownUser = nil + keychain.authToken = nil + + XCTAssertNil(keychain.email) + XCTAssertNil(keychain.userId) + XCTAssertNil(keychain.userIdUnknownUser) + XCTAssertNil(keychain.authToken) + } } From c35b9271dfda4772d872b5545901c6caa3081521 Mon Sep 17 00:00:00 2001 From: Joao Dordio Date: Mon, 25 May 2026 14:13:44 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=94=A7=20Added=20migrationAttempted?= =?UTF-8?q?=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 2 +- swift-sdk/Internal/InternalIterableAPI.swift | 24 +++++++------------ .../Utilities/Keychain/IterableKeychain.swift | 9 +++++++ .../Utilities/Keychain/KeychainWrapper.swift | 2 +- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68908b51c..1f0bc9a56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ### Fixed -- [SDK-478] Moved keychain migration (`migrateKeychainToIsolatedStorage`) off the main thread and gated it as a one-shot via a UserDefaults flag. Prior to this change the migration ran on the caller's thread on every `start()` invocation; for cross-platform bridges (React Native and Flutter), `start()` is called from `DispatchQueue.main.async`, so the migration's keychain syscalls blocked the main thread on every cold start. The migration now dispatches to `DispatchQueue.global(qos: .userInitiated)` and runs only when a UserDefaults flag indicates it has not yet completed for this install. Reads of `email`, `userId`, `userIdUnknownUser`, and `authToken` are coordinated against the in-flight migration via a concurrent dispatch queue with a barrier write, so callers observe consistent state regardless of timing. See [SDK-294] for context on the original isolated-keychain change. +- [SDK-478] Gated the keychain migration (`migrateKeychainToIsolatedStorage`) as a one-shot via a UserDefaults flag. Prior to this change the migration ran on the caller's thread on every `start()` invocation; for cross-platform bridges (React Native and Flutter), `start()` is called from `DispatchQueue.main.async`, so the migration's `SecItem` syscalls blocked the main thread on every cold start. The migration now runs only when the flag indicates it has not yet completed for this install, so every subsequent cold start (the hot path) skips the syscall cost entirely. Reads of `email`, `userId`, `userIdUnknownUser`, and `authToken` are coordinated against the migration via a concurrent dispatch queue with a barrier write, so callers always observe a consistent state. See [SDK-294] for context on the original isolated-keychain change. - Added empty-key guard and tighter `SecItemCopyMatching` status handling on `KeychainWrapper.data(forKey:)` / `set(_:forKey:)` / `removeValue(forKey:)` to prevent edge-case crashes on app launch. - Added duration logging to `IterableKeychain.migrateFromLegacy()` so future regressions of this shape surface in dev logs immediately. - Added non-success logging to `KeychainWrapper.set(_:forKey:)` and `update(_:forKey:)` so silent `SecItemAdd`/`SecItemUpdate` failures (e.g. entitlement issues) become observable. diff --git a/swift-sdk/Internal/InternalIterableAPI.swift b/swift-sdk/Internal/InternalIterableAPI.swift index 566fd2e8d..bba003a23 100644 --- a/swift-sdk/Internal/InternalIterableAPI.swift +++ b/swift-sdk/Internal/InternalIterableAPI.swift @@ -1101,22 +1101,16 @@ final class InternalIterableAPI: NSObject, PushTrackerProtocol, AuthProvider { } private func updateSDKVersion() { - // Keychain migration is one-shot per SDK upgrade. We gate on a - // UserDefaults flag so subsequent cold starts skip the SecItem - // syscall cost entirely, and dispatch the actual work to a - // background queue so the calling thread (which for cross-platform - // bridges is the main thread, via DispatchQueue.main.async) is not - // blocked for the duration of the migration. See SDK-478. - // - // Reads of email / userId / authToken / userIdUnknownUser are - // race-safe against the in-flight migration thanks to the barrier - // coordinator inside IterableKeychain. + // Keychain migration is one-shot per install. We gate on a + // UserDefaults flag so every subsequent cold start (the hot path) + // skips the SecItem syscall cost entirely. On the rare first launch + // after an upgrade from a pre-isolated-keychain build we run the + // migration synchronously on the calling thread, because + // retrieveIdentifierData() runs immediately after this and must + // observe the migrated identity. See SDK-478. if !localStorage.keychainMigrationCompleted { - DispatchQueue.global(qos: .userInitiated).async { [localStorage] in - var localStorage = localStorage - localStorage.migrateKeychainToIsolatedStorage() - localStorage.keychainMigrationCompleted = true - } + localStorage.migrateKeychainToIsolatedStorage() + localStorage.keychainMigrationCompleted = true } if let lastVersion = localStorage.sdkVersion, lastVersion != IterableAPI.sdkVersion { diff --git a/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift b/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift index ba22768ab..da286ca37 100644 --- a/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift +++ b/swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift @@ -28,6 +28,9 @@ class IterableKeychain { } return coordinatorQueue.sync(flags: .barrier) { + if migrationAttempted { return false } + migrationAttempted = true + var migrated = false let migrationStartedAt = Date() @@ -111,6 +114,12 @@ class IterableKeychain { private let wrapper: KeychainWrapper private let legacyWrapper: KeychainWrapper? + /// Guards against re-running the migration body within a single + /// process lifetime if `migrateFromLegacy()` is invoked more than once + /// (e.g. a second `start()` call before the UserDefaults flag is + /// observed). Read/write only inside the `coordinatorQueue` barrier. + private var migrationAttempted = false + /// Concurrent queue used to serialize the migration (which runs as a /// barrier) against the property reads/writes. Reads run fully in /// parallel after migration completes. See SDK-478. diff --git a/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift b/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift index 4464d2ba8..62ba0f8f4 100644 --- a/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift +++ b/swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift @@ -77,7 +77,7 @@ class KeychainWrapper { return nil } - return status == noErr ? result as? Data : nil + return result as? Data } @discardableResult