Fix FirebaseAuth random logouts on iOS 15+ during prewarming - #16505
Fix FirebaseAuth random logouts on iOS 15+ during prewarming#16505paulb777 wants to merge 6 commits into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a workaround for a known iOS bug where SecItemCopyMatching spuriously returns errSecItemNotFound instead of errSecInteractionNotAllowed when the device is locked. It adds a helper method isKeychainAccessible() to check the actual accessibility of the keychain by attempting a dummy write, and updates the lookup logic to throw an error if the keychain is inaccessible. Additionally, unit tests are added to verify this behavior. The reviewer suggested a performance optimization to cache the keychain accessibility status once it becomes accessible, avoiding redundant and expensive keychain write/delete operations on subsequent checks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a workaround for an iOS bug where SecItemCopyMatching spuriously returns errSecItemNotFound instead of errSecInteractionNotAllowed when the device is locked (such as during prewarming). It adds a helper method isKeychainAccessible() that attempts to write a dummy item to the keychain to determine accessibility, caching the result. However, the review points out a critical issue: passing kSecValueData or kSecAttrAccessible in the query to SecItemDelete is invalid and will fail with errSecParam (error -50), causing a silent keychain leak of the dummy item. A dedicated deletion query containing only search attributes should be used instead.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue on iOS 15+ where users are randomly logged out during prewarming while the device is locked. It introduces a check to verify if the keychain is accessible when errSecItemNotFound is returned, throwing an error instead of returning nil if the keychain is locked. However, the reviewer identified a critical bug in the caching mechanism (isKeychainAccessibleCache). Since keychain accessibility is dynamic and changes when the device is locked or unlocked, caching this state can lead to silent logouts if the cache is set to true while unlocked and then queried while locked. The reviewer suggested removing the cache entirely to ensure the check is always accurate.
… based on lock state
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue on iOS 15+ where users are randomly logged out during prewarming while the device is locked. It introduces a check to verify if the keychain is accessible when a query returns errSecItemNotFound, throwing an error if it is inaccessible rather than returning nil. The review feedback recommends caching the keychain accessibility status in memory once it is confirmed accessible, preventing redundant and expensive synchronous keychain write/delete operations on subsequent lookups.
ncooke3
left a comment
ncooke3
left a comment
There was a problem hiding this comment.
AI-assisted review comments below.
…curately reflect lock state
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue on iOS 15+ where users are randomly logged out during prewarming while the device is locked by verifying keychain accessibility when SecItemCopyMatching returns errSecItemNotFound. The review feedback highlights a mismatch in the keychain protection class used for the accessibility check (kSecAttrAccessibleWhenUnlocked instead of kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly), which could lead to false negatives. It is recommended to align the protection class with the actual credentials, cache the accessibility status to optimize performance, and update the corresponding unit test mock.
| /// Determines if the keychain is currently accessible. | ||
| /// This is used to work around a known iOS bug where `SecItemCopyMatching` spuriously returns | ||
| /// `errSecItemNotFound` instead of `errSecInteractionNotAllowed` when the device is locked (e.g. | ||
| /// during prewarming). | ||
| private func isKeychainAccessible() -> Bool { | ||
| let dummyKey = "firebase_auth_keychain_accessibility_check" | ||
| var query: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrAccount as String: dummyKey, | ||
| kSecAttrService as String: service, | ||
| kSecValueData as String: Data([0]), | ||
| kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked, | ||
| ] | ||
| query[kSecUseDataProtectionKeychain as String] = true | ||
|
|
||
| let status = keychainStorage.add(query: query) | ||
| if status == errSecInteractionNotAllowed { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
The implementation of isKeychainAccessible() uses kSecAttrAccessibleWhenUnlocked for the dummy keychain item. However, the actual credentials in FirebaseAuth are stored using kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly (as seen in setItemLegacy).
This mismatch introduces a correctness and usability bug:
If the device is locked but has been unlocked at least once since boot, items with kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly are fully accessible. However, attempting to write a kSecAttrAccessibleWhenUnlocked dummy item will fail with errSecInteractionNotAllowed. This causes isKeychainAccessible() to return false and throw errSecInteractionNotAllowed even when the keychain is actually accessible for the actual tokens (e.g., for a signed-out user, it will throw an error instead of returning nil).
To fix this, the dummy item should use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly to match the actual token protection class.
Additionally, since keychain operations are slow and expensive, we can optimize performance by caching the accessibility status once it succeeds. Since AfterFirstUnlock items remain accessible for the entire duration of the device being powered on once the first unlock has occurred, caching true is completely safe and avoids redundant keychain writes on subsequent checks.
private let isKeychainAccessibleCached = UnfairLock<Bool>(false)
/// Determines if the keychain is currently accessible.
/// This is used to work around a known iOS bug where SecItemCopyMatching spuriously returns
/// errSecItemNotFound instead of errSecInteractionNotAllowed when the device is locked (e.g.
/// during prewarming).
private func isKeychainAccessible() -> Bool {
if isKeychainAccessibleCached.value() {
return true
}
let dummyKey = "firebase_auth_keychain_accessibility_check"
var query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrAccount as String: dummyKey,
kSecAttrService as String: service,
kSecValueData as String: Data([0]),
kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly,
]
query[kSecUseDataProtectionKeychain as String] = true
let status = keychainStorage.add(query: query)
if status == errSecInteractionNotAllowed {
return false
}
isKeychainAccessibleCached.withLock { $0 = true }
return true
}| func add(query: [String: Any]) -> OSStatus { | ||
| if let accessible = query[kSecAttrAccessible as String] as? String, | ||
| accessible == kSecAttrAccessibleWhenUnlocked as String { | ||
| return errSecInteractionNotAllowed | ||
| } | ||
| return errSecSuccess | ||
| } |
There was a problem hiding this comment.
Since we updated the production code to use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly for the dummy keychain item, we must also update the mock LockedKeychainStorage in the unit tests to check for this protection class instead of kSecAttrAccessibleWhenUnlocked.
| func add(query: [String: Any]) -> OSStatus { | |
| if let accessible = query[kSecAttrAccessible as String] as? String, | |
| accessible == kSecAttrAccessibleWhenUnlocked as String { | |
| return errSecInteractionNotAllowed | |
| } | |
| return errSecSuccess | |
| } | |
| func add(query: [String: Any]) -> OSStatus { | |
| if let accessible = query[kSecAttrAccessible as String] as? String, | |
| accessible == kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly as String { | |
| return errSecInteractionNotAllowed | |
| } | |
| return errSecSuccess | |
| } |
2 participants
Fixes #16498
This PR addresses an issue where users were randomly logged out during iOS prewarming. The root cause is a known iOS 15+ bug where
SecItemCopyMatchingspuriously returnserrSecItemNotFoundinstead oferrSecInteractionNotAllowedwhen the device is locked.Because
FirebaseAuthtrusted this error code, it assumed the user was genuinely signed out, wiping the in-memory user state and causing a silent logout.Changes:
isKeychainAccessible()toAuthKeychainServices.swiftwhich attempts to write a dummy keychain item with thekSecAttrAccessibleAfterFirstUnlockThisDeviceOnlyprotection class to reliably detect if the keychain is locked.getItemandgetItemLegacyto verify keychain accessibility whenerrSecItemNotFoundis returned. If the keychain is not accessible, it now appropriately throwserrSecInteractionNotAllowed.AuthKeychainServicesTests.swiftto verify this behavior using a mocked keychain storage.