[fix] SDKS-5172 Defensive recovery for EncryptorError after iCloud device migration#167
Merged
george-bafaloukas-forgerock merged 2 commits intoJun 24, 2026
Merged
Conversation
…vice migration After an iCloud backup / Quick Start device migration, the OIDC token ciphertext migrates to the new device but the Secure Enclave private key does not, causing every OidcClient.token() call to fail permanently with EncryptorError.failedToDecrypt. Phases: - Phase 1: Inner do/catch in token() catches EncryptorError, deletes corrupted token, falls through to re-auth - Phase 2: Keychain.save() split queries — delete by primary key only, add with kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly - Phase 3: Tests — ThrowingMockStorage, testTokenRecoveryOnDecryptionFailure, testSavedItemHasDeviceOnlyAccessibility, testUpgradePathDoesNotThrowDuplicateItem - Phase 4: revoke() hardened — try? storage.get() replaced with explicit do/catch to actually clear corrupted token on EncryptorError Refs: SDKS-5172 CHANGELOG: entry added for SDKS-5172.
george-bafaloukas-forgerock
requested review from
rodrigoareis,
spetrov,
vahancouver and
witrisna
vahancouver
reviewed
Jun 17, 2026
vahancouver
left a comment
vahancouver
left a comment
Contributor
There was a problem hiding this comment.
The fix addresses the reported issue. However there might be other edge cases.
Please see my inline comments.
Plus we we could add tests for those cases
- No test for delete() throwing during recovery (ThrowingMock has no
throwOnDelete flag, so the infinite loop is untestable) - No test for non-EncryptorError from storage.get()
- No test for revoke() with a transient storage error
- No test verifying the old token survives a failed save() (which would catch
the data loss regression)
…en deletion Addresses Vahan's review on PR #167: - Keychain.save() now encrypts BEFORE deleting the existing item, so a failed encrypt (e.g. SE key transiently unavailable) never leaves the slot empty. - token() recovery surfaces a failure if delete() cannot clear the corrupted token, instead of falling through to re-auth and looping forever. - token() recovery now also handles DecodingError (corrupt/incompatible payload), not just EncryptorError; transient errors still propagate without deletion. - revoke() only deletes on EncryptorError/DecodingError; transient read errors (e.g. errSecInteractionNotAllowed while locked) leave a valid token intact. Tests: - testExistingTokenSurvivesFailedSave (data-loss regression guard) - testTokenRecoveryFailsWhenDeleteFails (infinite-loop guard) - testTokenDoesNotRecoverOnTransientStorageError - testRevokeLeavesTokenIntactOnTransientStorageError - ThrowingMock gains throwOnDelete + configurable getError Refs: SDKS-5172
Contributor
Author
|
Thanks for the thorough review @vahancouver — all four points were valid (two were regressions this PR introduced). Addressed in 73798ee:
Added all four tests you suggested:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
JIRA "[TRIAGE] OIDC authentication where the app returns: "Authorization error: The operation couldn't be completed. (PingStorage.EncryptorError error 1)""
Description
After an iCloud backup / Quick Start device migration, the OIDC token ciphertext migrates to the new device but the Secure Enclave private key does not. This causes every call to
OidcClient.token()to fail permanently withEncryptorError.failedToDecrypt. The fix has two complementary parts: (1) catch the decryption failure at thetoken()call site, delete the unreadable ciphertext, and fall through to re-authentication; (2) setkSecAttrAccessibleAfterFirstUnlockThisDeviceOnlyonSecItemAddso that newly written tokens are device-bound and will not migrate in the first place. Both changes ship forward-only.Phases completed:
OidcClient.token(): adds innerdo/catch let error as EncryptorError— deletes corrupted ciphertext, falls through to re-auth; addsimport PingStorageKeychain.save()split into separatedeleteQuery(primary key only) andaddQuerywithkSecAttrAccessibleAfterFirstUnlockThisDeviceOnlyto prevent iCloud backup migration on new savesThrowingMockStorage,testTokenRecoveryOnDecryptionFailure,testSavedItemHasDeviceOnlyAccessibility,testUpgradePathDoesNotThrowDuplicateItem(mandatory upgrade-path test verifying noerrSecDuplicateItemon first save after upgrade)revoke()hardened:try?onconfig.storage.get()replaced with explicitdo/catchso the documented workaround actually clears the corrupted keychain entryNote:
endSession(signOff:)has a similar swallow pattern (known deferred — no impact on the authentication lockout scenario). The ordering testtestTokenRecoveryDeletesStorageBeforeReAuthwas not implemented (production code order is clearly correct; follow-up if needed).Checklist: