Report keychain failures during AppKeychain and SharedKeychain access#25702
Open
crazytonyli wants to merge 1 commit into
Open
Report keychain failures during AppKeychain and SharedKeychain access#25702crazytonyli wants to merge 1 commit into
crazytonyli wants to merge 1 commit into
Conversation
Collaborator
Generated by 🚫 Danger |
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 32829 | |
| Version | PR #25702 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | c29ac79 | |
| Installation URL | 20kfs2sfc7ft8 |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 32829 | |
| Version | PR #25702 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | c29ac79 | |
| Installation URL | 2v949hp4t80sg |
Both keychain wrappers now route every read, write, and delete failure through a shared reporter. The expected not-found stays silent, other real failures are logged via swift-log together with the failing call site, and an entitlement mismatch crashes because it means the access group is unreachable for the entire build rather than a recoverable runtime condition. This adds a swift-log dependency to the WordPressShared target.
crazytonyli
force-pushed
the
task/keychain-error-handling
branch
from
f8900b9 to
c29ac79
Compare
jkmassel
reviewed
Jun 25, 2026
| return nsError.domain == sfhfKeychainErrorDomain | ||
| && nsError.code != Int(errSecItemNotFound) | ||
| guard nsError.domain == sfhfKeychainErrorDomain else { return nil } | ||
| return OSStatus(nsError.code) |
Contributor
There was a problem hiding this comment.
Keychain shouldn't return anything outside of Int32, but we can be extra careful:
Suggested change
| return OSStatus(nsError.code) | |
| return OSStatus(truncatingIfNeeded: nsError.code) |
jkmassel
approved these changes
Jun 25, 2026
jkmassel
left a comment
jkmassel
left a comment
Contributor
There was a problem hiding this comment.
This looks generally correct, though I wonder if we can collapse a bunch of the repeated do/catch statements?
| return code != errSecItemNotFound | ||
| } | ||
|
|
||
| private let keychainLogger = Logger(label: (Bundle.main.bundleIdentifier ?? "org.wordpress") + ".keychain") |
Contributor
There was a problem hiding this comment.
Suggested change
| private let keychainLogger = Logger(label: (Bundle.main.bundleIdentifier ?? "org.wordpress") + ".keychain") | |
| private let keychainLogger = Logger(label: (Bundle.main.bundleIdentifier!) + ".keychain") |
I'm pretty comfortable crashing if we don't have a bundle identifier set – something has gone very wrong in this case.
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
4 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.
Both keychain wrappers now route every read, write, and delete failure through a shared reporter. The expected not-found stays silent, other real failures are logged via swift-log together with the failing call site, and an entitlement mismatch crashes because it means the access group is unreachable for the entire build rather than a recoverable runtime condition. This adds a swift-log dependency to the WordPressShared target.