Report keychain failures during AppKeychain and SharedKeychain access (#25702)

* Report keychain failures during AppKeychain and SharedKeychain access

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.

* Force-unwrap main bundle id

Co-authored-by: Jeremy Massel <1123407+jkmassel@users.noreply.github.com>

* NSError code to OSStatus

Co-authored-by: Jeremy Massel <1123407+jkmassel@users.noreply.github.com>

---------

Co-authored-by: Jeremy Massel <1123407+jkmassel@users.noreply.github.com>

Author: crazytonyli

Modules/Package.swift

@@ -230,6 +230,7 @@ let package = Package(
             name: "WordPressShared",
             dependencies: [
                 "BuildSettingsKit",
+                .product(name: "Logging", package: "swift-log"),
                 .product(name: "SwiftSoup", package: "SwiftSoup"),
                 .target(name: "SFHFKeychainUtils"),
                 .target(name: "WordPressSharedObjC")

Modules/Sources/WordPressShared/Keychain/AppKeychain.swift

@@ -46,27 +46,38 @@ public final class AppKeychain: KeychainAccessible {
                 accessGroup: privateGroup
             )
         } catch {
+            reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: privateGroup)
             // A real failure (for example errSecInteractionNotAllowed while the
             // device is locked) must surface: the fallback is long-lived now,
             // so masking it as not-found would be permanent. Fall back only on
             // a genuine not-found of the private read, and only when a shared
             // group exists.
             guard !isRealKeychainFailure(error), let sharedGroup else { throw error }
-            let value = try keychainUtils.getPasswordForUsername(
-                username,
-                andServiceName: serviceName,
-                accessGroup: sharedGroup
-            )
+            let value: String
+            do {
+                value = try keychainUtils.getPasswordForUsername(
+                    username,
+                    andServiceName: serviceName,
+                    accessGroup: sharedGroup
+                )
+            } catch {
+                reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: sharedGroup)
+                throw error
+            }
             // Read-repair: migrate the item into the private group so future
             // reads stop depending on the shared-group fallback. Best-effort,
             // the read already succeeded; the next read retries the repair.
-            try? keychainUtils.storeUsername(
-                username,
-                andPassword: value,
-                forServiceName: serviceName,
-                accessGroup: privateGroup,
-                updateExisting: true
-            )
+            do {
+                try keychainUtils.storeUsername(
+                    username,
+                    andPassword: value,
+                    forServiceName: serviceName,
+                    accessGroup: privateGroup,
+                    updateExisting: true
+                )
+            } catch {
+                reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: privateGroup)
+            }
             return value
         }
     }
@@ -99,13 +110,18 @@ public final class AppKeychain: KeychainAccessible {
             }
             return
         }
-        try keychainUtils.storeUsername(
-            username,
-            andPassword: newValue,
-            forServiceName: serviceName,
-            accessGroup: privateGroup,
-            updateExisting: true
-        )
+        do {
+            try keychainUtils.storeUsername(
+                username,
+                andPassword: newValue,
+                forServiceName: serviceName,
+                accessGroup: privateGroup,
+                updateExisting: true
+            )
+        } catch {
+            reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: privateGroup)
+            throw error
+        }
     }
 
     private func deleteIgnoringNotFound(_ username: String, serviceName: String, accessGroup: String) throws {
@@ -116,6 +132,7 @@ public final class AppKeychain: KeychainAccessible {
                 accessGroup: accessGroup
             )
         } catch {
+            reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: accessGroup)
             // Deleting a missing item is expected: the item usually exists
             // in only one of the two groups. Anything else (for example
             // errSecInteractionNotAllowed while the device is locked) must

Modules/Sources/WordPressShared/Keychain/KeychainErrorClassification.swift

@@ -1,19 +1,64 @@
 import Foundation
+import Logging
 import Security
 
 /// The error domain `SFHFKeychainUtils` uses for its own failures. Shared so
 /// the classifier and its tests cannot drift to different spellings.
 let sfhfKeychainErrorDomain = "SFHFKeychainUtilsErrorDomain"
 
-/// Classifies errors thrown by `SFHFKeychainUtils`.
+/// The underlying Keychain `OSStatus` for an error produced by
+/// `SFHFKeychainUtils`, or `nil` when the error is not from that domain.
 ///
-/// `SFHFKeychainUtils` populates an `NSError` in its own domain (with the raw
-/// `OSStatus` as the code) only for real failures. A not-found surfaces either
-/// as a nil result that Swift bridges to a generic non-SFHF error (reads), or
-/// as an SFHF error whose code is `errSecItemNotFound` (deletes). So a real
-/// failure is exactly an SFHF-domain error whose code is not `errSecItemNotFound`.
-public func isRealKeychainFailure(_ error: Error) -> Bool {
+/// `SFHFKeychainUtils` reports real failures as an `NSError` in its own domain,
+/// carrying the raw Keychain `OSStatus` as the code. A not-found read instead
+/// bridges to a generic error in another domain. Centralizing the domain check
+/// and the `Int` -> `OSStatus` conversion here lets callers compare directly
+/// against the `errSec*` constants.
+func keychainErrorCode(error: Error) -> OSStatus? {
     let nsError = error as NSError
-    return nsError.domain == sfhfKeychainErrorDomain
-        && nsError.code != Int(errSecItemNotFound)
+    guard nsError.domain == sfhfKeychainErrorDomain else { return nil }
+    return OSStatus(truncatingIfNeeded: nsError.code)
+}
+
+/// Whether an error from `SFHFKeychainUtils` is a real failure rather than the
+/// expected not-found. A not-found surfaces either as a non-SFHF error (reads)
+/// or as an SFHF error whose code is `errSecItemNotFound` (deletes).
+public func isRealKeychainFailure(_ error: Error) -> Bool {
+    guard let code = keychainErrorCode(error: error) else { return false }
+    return code != errSecItemNotFound
+}
+
+private let keychainLogger = Logger(label: (Bundle.main.bundleIdentifier!) + ".keychain")
+
+/// Logs a real keychain failure. An entitlement mismatch is fatal: it means
+/// the access group is unreachable for the entire build (a provisioning or
+/// build-settings defect, not a runtime condition), so there is nothing to
+/// recover to and we crash to surface it immediately. Does nothing for the
+/// expected not-found.
+///
+/// For every other failure this only emits telemetry and does not change
+/// control flow: callers still throw, fall back, or swallow as before. The
+/// source location identifies which call site (read, write, or delete) failed.
+func reportKeychainFailureIfNeeded(
+    _ error: Error,
+    serviceName: String,
+    accessGroup: String,
+    file: StaticString = #fileID,
+    line: UInt = #line
+) {
+    guard let status = keychainErrorCode(error: error), status != errSecItemNotFound else {
+        return
+    }
+
+    if status == errSecMissingEntitlement {
+        fatalError(
+            "Keychain entitlement mismatch (service: \(serviceName), group: \(accessGroup))",
+            file: file,
+            line: line
+        )
+    }
+
+    keychainLogger.error(
+        "Keychain failure (service: \(serviceName), group: \(accessGroup), status: \(status)) at \(file):\(line)"
+    )
 }

Modules/Sources/WordPressShared/Keychain/SharedKeychain.swift

@@ -28,22 +28,32 @@ public final class SharedKeychain: KeychainAccessible {
     }
 
     public func getPassword(for username: String, serviceName: String) throws -> String {
-        try keychainUtils.getPasswordForUsername(
-            username,
-            andServiceName: serviceName,
-            accessGroup: group
-        )
+        do {
+            return try keychainUtils.getPasswordForUsername(
+                username,
+                andServiceName: serviceName,
+                accessGroup: group
+            )
+        } catch {
+            reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: group)
+            throw error
+        }
     }
 
     public func setPassword(for username: String, to newValue: String?, serviceName: String) throws {
         if let newValue {
-            try keychainUtils.storeUsername(
-                username,
-                andPassword: newValue,
-                forServiceName: serviceName,
-                accessGroup: group,
-                updateExisting: true
-            )
+            do {
+                try keychainUtils.storeUsername(
+                    username,
+                    andPassword: newValue,
+                    forServiceName: serviceName,
+                    accessGroup: group,
+                    updateExisting: true
+                )
+            } catch {
+                reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: group)
+                throw error
+            }
         } else {
             do {
                 try keychainUtils.deleteItem(
@@ -52,6 +62,7 @@ public final class SharedKeychain: KeychainAccessible {
                     accessGroup: group
                 )
             } catch {
+                reportKeychainFailureIfNeeded(error, serviceName: serviceName, accessGroup: group)
                 // Deleting an already-absent item is success: the migration
                 // cleanup path removes a token that may legitimately be gone.
                 // Real failures must surface, same as AppKeychain.

Modules/Tests/WordPressSharedTests/KeychainErrorClassificationTests.swift

@@ -19,4 +19,19 @@ struct KeychainErrorClassificationTests {
         enum ReadError: Error { case notFound }
         #expect(!isRealKeychainFailure(ReadError.notFound))
     }
+
+    @Test func entitlementMismatchIsRealFailure() {
+        let error = NSError(domain: sfhfKeychainErrorDomain, code: Int(errSecMissingEntitlement))
+        #expect(isRealKeychainFailure(error))
+    }
+
+    @Test func keychainErrorCodeReturnsStatusForSFHFError() {
+        let error = NSError(domain: sfhfKeychainErrorDomain, code: Int(errSecMissingEntitlement))
+        #expect(keychainErrorCode(error: error) == errSecMissingEntitlement)
+    }
+
+    @Test func keychainErrorCodeIsNilForForeignDomain() {
+        let error = NSError(domain: NSCocoaErrorDomain, code: Int(errSecMissingEntitlement))
+        #expect(keychainErrorCode(error: error) == nil)
+    }
 }