From 31957336d3964ea17840c7e037879a5d45a1420b Mon Sep 17 00:00:00 2001 From: Adam Borbas Date: Thu, 16 Jul 2026 18:45:29 +0200 Subject: [PATCH] Fix silent Keychain failures and form-encoding in auth Three code-audit fixes in the auth layer, sharing one KeychainStore seam: - KeychainStore: introduce `KeychainStoring` protocol; make `saveCredentials`/`clearAll` throw instead of swallowing errors with `try?`. - AuthManager: `didAuthenticate` now logs a persist failure instead of pretending success; `clearKeychainIfReinstalled` sets the `hasLaunchedBefore` flag only after a confirmed wipe (a failed wipe is logged and retried next launch). The one-time-logout-on-upgrade trade-off is documented in-code (confidence low due to squashed history). - TokenRefresher: log a failed refreshed-credentials persist. - TokenClient.formEncode: use an explicit RFC 3986 unreserved CharacterSet so `+ & = ,` are escaped in a form body (`.urlQueryAllowed` left them raw). Tests (Swift Testing): MockKeychainStore stub; AuthManager persist/wipe failure paths; formEncode escaping. Co-Authored-By: Claude Opus 4.8 --- Hemera/Auth/AuthManager.swift | 50 ++++++++--- Hemera/Auth/KeychainStore.swift | 18 ++-- Hemera/Auth/TokenClient.swift | 15 +++- Hemera/Auth/TokenRefresher.swift | 12 ++- HemeraTests/Auth/AuthManagerTests.swift | 101 ++++++++++++++++++++++ HemeraTests/Auth/TokenClientTests.swift | 36 ++++++++ HemeraTests/Mocks/MockKeychainStore.swift | 58 +++++++++++++ 7 files changed, 264 insertions(+), 26 deletions(-) create mode 100644 HemeraTests/Auth/AuthManagerTests.swift create mode 100644 HemeraTests/Auth/TokenClientTests.swift create mode 100644 HemeraTests/Mocks/MockKeychainStore.swift diff --git a/Hemera/Auth/AuthManager.swift b/Hemera/Auth/AuthManager.swift index 298dee4..947cf77 100644 --- a/Hemera/Auth/AuthManager.swift +++ b/Hemera/Auth/AuthManager.swift @@ -28,11 +28,11 @@ final class AuthManager: AuthManaging { private(set) var state: AuthState = .unauthenticated private(set) var credentials: ServerCredentials? - private let keychainStore: KeychainStore + private let keychainStore: any KeychainStoring private let tokenRefresher: TokenRefresher private var onChangeHandlers: [(AuthState, AuthChangeReason) -> Void] = [] - init(keychainStore: KeychainStore, userDefaults: UserDefaults = .standard) { + init(keychainStore: any KeychainStoring, userDefaults: UserDefaults = .standard) { self.keychainStore = keychainStore self.tokenRefresher = TokenRefresher(keychainStore: keychainStore) @@ -45,7 +45,7 @@ final class AuthManager: AuthManaging { } convenience init() { - self.init(keychainStore: .shared) + self.init(keychainStore: KeychainStore.shared) } func addOnChangeHandler(_ handler: @escaping (AuthState, AuthChangeReason) -> Void) { @@ -70,7 +70,11 @@ final class AuthManager: AuthManaging { func didAuthenticate(with creds: ServerCredentials) { Log.info("Authenticated with \(creds.serverURL.host() ?? "unknown")") - keychainStore.saveCredentials(creds) + do { + try keychainStore.saveCredentials(creds) + } catch { + Log.error("Failed to persist credentials — session will not survive relaunch", cause: error) + } credentials = creds state = .authenticated onChangeHandlers.forEach { $0(.authenticated, .userInitiated) } @@ -79,21 +83,35 @@ final class AuthManager: AuthManaging { func logout() { performLogout(reason: .userInitiated) } + + // Internal (not private) so tests can reference the same key instead of duplicating the literal. + static let hasLaunchedKey = "com.hemera.hasLaunchedBefore" } // MARK: - Private Methods private extension AuthManager { - private static let hasLaunchedKey = "com.hemera.hasLaunchedBefore" - - /// Clears stale keychain credentials left over from a previous install. - /// iOS preserves keychain items across app uninstall/reinstall, but - /// UserDefaults are wiped. If the flag is missing, this is a fresh install. - static func clearKeychainIfReinstalled(keychainStore: KeychainStore, userDefaults: UserDefaults) { - if !userDefaults.bool(forKey: hasLaunchedKey) { - keychainStore.clearAll() - userDefaults.set(true, forKey: hasLaunchedKey) + /** + Clears stale keychain credentials left over from a previous install. + iOS preserves keychain items across app uninstall/reinstall, but + UserDefaults are wiped. If the flag is missing, this is a fresh install. + + Trade-off (accepted knowingly): freshness is inferred only from the missing + flag, which is equally absent for a legitimate upgrade from a build that + predated this flag — so such an upgrade triggers a one-time logout. We accept + that for the safer security posture (never inherit foreign credentials). + Gating the wipe on install-identity is not reliably possible from the Keychain + alone without risking a reinstall inheriting a previous user's credentials. + */ + static func clearKeychainIfReinstalled(keychainStore: any KeychainStoring, userDefaults: UserDefaults) { + guard !userDefaults.bool(forKey: hasLaunchedKey) else { return } + do { + try keychainStore.clearAll() + userDefaults.set(true, forKey: hasLaunchedKey) // only mark done if the wipe actually happened + } catch { + Log.error("First-run keychain wipe failed — will retry next launch", cause: error) + // Flag intentionally NOT set, so the wipe is retried on the next launch. } } @@ -108,7 +126,11 @@ private extension AuthManager { } } } - keychainStore.clearAll() + do { + try keychainStore.clearAll() + } catch { + Log.warning("Failed to clear credentials on logout", cause: error) + } credentials = nil state = .unauthenticated onChangeHandlers.forEach { $0(.unauthenticated, reason) } diff --git a/Hemera/Auth/KeychainStore.swift b/Hemera/Auth/KeychainStore.swift index acc949e..91d2cfd 100644 --- a/Hemera/Auth/KeychainStore.swift +++ b/Hemera/Auth/KeychainStore.swift @@ -1,7 +1,13 @@ import Foundation import KeychainAccess -final class KeychainStore: @unchecked Sendable { +protocol KeychainStoring: Sendable { + func saveCredentials(_ credentials: ServerCredentials) throws + func loadCredentials() -> ServerCredentials? + func clearAll() throws +} + +final class KeychainStore: KeychainStoring, @unchecked Sendable { static let shared = KeychainStore() @@ -14,9 +20,9 @@ final class KeychainStore: @unchecked Sendable { private static let credentialsKey = "server_credentials" - func saveCredentials(_ credentials: ServerCredentials) { - guard let data = try? JSONEncoder().encode(credentials) else { return } - try? keychain.set(data, key: Self.credentialsKey) + func saveCredentials(_ credentials: ServerCredentials) throws { + let data = try JSONEncoder().encode(credentials) + try keychain.set(data, key: Self.credentialsKey) } func loadCredentials() -> ServerCredentials? { @@ -24,7 +30,7 @@ final class KeychainStore: @unchecked Sendable { return try? JSONDecoder().decode(ServerCredentials.self, from: data) } - func clearAll() { - try? keychain.removeAll() + func clearAll() throws { + try keychain.removeAll() } } diff --git a/Hemera/Auth/TokenClient.swift b/Hemera/Auth/TokenClient.swift index 874e072..1fbfefc 100644 --- a/Hemera/Auth/TokenClient.swift +++ b/Hemera/Auth/TokenClient.swift @@ -80,10 +80,19 @@ enum TokenClient { return try JSONDecoder().decode(TokenResponse.self, from: data) } - private static func formEncode(_ params: [String: String]) -> String { + /** + RFC 3986 unreserved set — safe for x-www-form-urlencoded values. + Excludes sub-delims (+ & = , ; …) which are structural in a form body; + `.urlQueryAllowed` would leave them unescaped (notably `+` → space). + */ + private static let formAllowedCharacters = CharacterSet(charactersIn: + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~") + + // Internal (not private) so `formEncode` can be unit-tested directly via @testable import. + static func formEncode(_ params: [String: String]) -> String { params.map { key, value in - let encodedKey = key.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? key - let encodedValue = value.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? value + let encodedKey = key.addingPercentEncoding(withAllowedCharacters: formAllowedCharacters) ?? key + let encodedValue = value.addingPercentEncoding(withAllowedCharacters: formAllowedCharacters) ?? value return "\(encodedKey)=\(encodedValue)" }.joined(separator: "&") } diff --git a/Hemera/Auth/TokenRefresher.swift b/Hemera/Auth/TokenRefresher.swift index 1fc8764..4e158f3 100644 --- a/Hemera/Auth/TokenRefresher.swift +++ b/Hemera/Auth/TokenRefresher.swift @@ -3,10 +3,10 @@ import HemeraLog actor TokenRefresher { - private let keychainStore: KeychainStore + private let keychainStore: any KeychainStoring private var refreshTask: Task<(String, ServerCredentials), Error>? - init(keychainStore: KeychainStore) { + init(keychainStore: any KeychainStoring) { self.keychainStore = keychainStore } @@ -39,7 +39,13 @@ actor TokenRefresher { let credentialsToSave = updated let store = keychainStore - await MainActor.run { store.saveCredentials(credentialsToSave) } + await MainActor.run { + do { + try store.saveCredentials(credentialsToSave) + } catch { + Log.error("Failed to persist refreshed credentials", cause: error) + } + } return (updated.accessToken, updated) } refreshTask = task diff --git a/HemeraTests/Auth/AuthManagerTests.swift b/HemeraTests/Auth/AuthManagerTests.swift new file mode 100644 index 0000000..d8f25cb --- /dev/null +++ b/HemeraTests/Auth/AuthManagerTests.swift @@ -0,0 +1,101 @@ +import Foundation +import Testing +@testable import Hemera + +@MainActor +@Suite(.serialized) +struct AuthManagerTests { + + private static let suiteName = "com.hemera.tests.authmanager" + private let userDefaults: UserDefaults + + init() { + // Dedicated suite, wiped before each test — isolated from `.standard` and self-cleaning + // (a fixed name means no per-run accumulation of orphaned preference files). `.serialized` + // keeps the shared suite race-free across this struct's tests. + userDefaults = UserDefaults(suiteName: Self.suiteName)! + userDefaults.removePersistentDomain(forName: Self.suiteName) + } + + // MARK: - didAuthenticate + + @Test + func didAuthenticate_persistsCredentialsAndAuthenticates() { + let store = MockKeychainStore() + let manager = AuthManager(keychainStore: store, userDefaults: userDefaults) + let creds = makeCredentials() + + manager.didAuthenticate(with: creds) + + #expect(store.saveCredentialsCallCount == 1) + #expect(store.savedCredentials?.accessToken == creds.accessToken) + #expect(manager.state == .authenticated) + #expect(manager.credentials?.accessToken == creds.accessToken) + } + + @Test + func didAuthenticate_whenPersistFails_stillAttemptsSaveAndDoesNotCrash() { + let store = MockKeychainStore(saveError: MockKeychainError()) + let manager = AuthManager(keychainStore: store, userDefaults: userDefaults) + let creds = makeCredentials() + + manager.didAuthenticate(with: creds) + + // The throwing save seam was actually invoked (no silent `try?` swallow). + #expect(store.saveCredentialsCallCount == 1) + // Nothing was durably persisted despite the reported success. + #expect(store.savedCredentials == nil) + // Conservative behavior retained: the session is still usable this launch. + #expect(manager.state == .authenticated) + } + + // MARK: - clearKeychainIfReinstalled (exercised via init) + + @Test + func init_firstLaunch_clearsKeychainAndSetsFlag() { + let store = MockKeychainStore(storedCredentials: makeCredentials()) + + _ = AuthManager(keychainStore: store, userDefaults: userDefaults) + + #expect(store.clearAllCallCount == 1) + #expect(userDefaults.bool(forKey: AuthManager.hasLaunchedKey) == true) + } + + @Test + func init_firstLaunch_whenClearFails_doesNotSetFlag() { + let store = MockKeychainStore(storedCredentials: makeCredentials(), clearError: MockKeychainError()) + + _ = AuthManager(keychainStore: store, userDefaults: userDefaults) + + #expect(store.clearAllCallCount == 1) + // Flag intentionally NOT set so the wipe is retried on the next launch. + #expect(userDefaults.bool(forKey: AuthManager.hasLaunchedKey) == false) + } + + @Test + func init_subsequentLaunch_doesNotClearKeychain() { + let store = MockKeychainStore(storedCredentials: makeCredentials()) + userDefaults.set(true, forKey: AuthManager.hasLaunchedKey) + + let manager = AuthManager(keychainStore: store, userDefaults: userDefaults) + + #expect(store.clearAllCallCount == 0) + #expect(manager.state == .authenticated) + } +} + +// MARK: - Helpers + +private extension AuthManagerTests { + + func makeCredentials() -> ServerCredentials { + ServerCredentials( + serverURL: URL(string: "https://home.example.com:8123")!, + externalURL: nil, + accessToken: "access-token", + refreshToken: "refresh-token", + tokenExpiresAt: Date().addingTimeInterval(3600), + clientId: "https://home.example.com:8123" + ) + } +} diff --git a/HemeraTests/Auth/TokenClientTests.swift b/HemeraTests/Auth/TokenClientTests.swift new file mode 100644 index 0000000..8d8b26f --- /dev/null +++ b/HemeraTests/Auth/TokenClientTests.swift @@ -0,0 +1,36 @@ +import Foundation +import Testing +@testable import Hemera + +struct TokenClientTests { + + // MARK: - formEncode + + @Test + func formEncode_escapesStructuralCharactersInValue() { + let encoded = TokenClient.formEncode(["a": "x+y=z&w,v"]) + + #expect(encoded == "a=x%2By%3Dz%26w%2Cv") + } + + @Test + func formEncode_escapesStructuralCharactersInKey() { + let encoded = TokenClient.formEncode(["a+b": "c"]) + + #expect(encoded == "a%2Bb=c") + } + + @Test + func formEncode_hexTokenRoundTripsUnchanged() { + let encoded = TokenClient.formEncode(["refresh_token": "abcdef0123456789"]) + + #expect(encoded == "refresh_token=abcdef0123456789") + } + + @Test + func formEncode_clientIdOriginURL_escapesSchemeSeparators() { + let encoded = TokenClient.formEncode(["client_id": "https://home.example.com:8123"]) + + #expect(encoded == "client_id=https%3A%2F%2Fhome.example.com%3A8123") + } +} diff --git a/HemeraTests/Mocks/MockKeychainStore.swift b/HemeraTests/Mocks/MockKeychainStore.swift new file mode 100644 index 0000000..14c4dc2 --- /dev/null +++ b/HemeraTests/Mocks/MockKeychainStore.swift @@ -0,0 +1,58 @@ +import Foundation +import Synchronization +@testable import Hemera + +/// Error surfaced by `MockKeychainStore` when configured to simulate a Keychain failure. +struct MockKeychainError: Error {} + +/** + Hand-written spy/stub for `KeychainStoring`. Tracks call counts and can be + configured to throw from `saveCredentials`/`clearAll` to exercise failure paths. + State is guarded by a `Mutex` so the type is genuinely `Sendable`. + */ +final class MockKeychainStore: KeychainStoring { + + private struct State { + var saveCredentialsCallCount = 0 + var clearAllCallCount = 0 + var savedCredentials: ServerCredentials? + var storedCredentials: ServerCredentials? + var saveError: Error? + var clearError: Error? + } + + private let state: Mutex + + init(storedCredentials: ServerCredentials? = nil, saveError: Error? = nil, clearError: Error? = nil) { + var initial = State() + initial.storedCredentials = storedCredentials + initial.saveError = saveError + initial.clearError = clearError + self.state = Mutex(initial) + } + + var saveCredentialsCallCount: Int { state.withLock { $0.saveCredentialsCallCount } } + var clearAllCallCount: Int { state.withLock { $0.clearAllCallCount } } + var savedCredentials: ServerCredentials? { state.withLock { $0.savedCredentials } } + + func saveCredentials(_ credentials: ServerCredentials) throws { + try state.withLock { + $0.saveCredentialsCallCount += 1 + if let error = $0.saveError { throw error } + $0.savedCredentials = credentials + $0.storedCredentials = credentials + } + } + + func loadCredentials() -> ServerCredentials? { + state.withLock { $0.storedCredentials } + } + + func clearAll() throws { + try state.withLock { + $0.clearAllCallCount += 1 + if let error = $0.clearError { throw error } + $0.storedCredentials = nil + } + } +}