Fix silent Keychain failures and form-encoding in auth

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) }

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,17 +20,17 @@ 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? {
         guard let data = try? keychain.getData(Self.credentialsKey) else { return nil }
         return try? JSONDecoder().decode(ServerCredentials.self, from: data)
     }
 
-    func clearAll() {
-        try? keychain.removeAll()
+    func clearAll() throws {
+        try keychain.removeAll()
     }
 }

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: "&")
     }

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

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"
+        )
+    }
+}

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")
+    }
+}

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<State>
+
+    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
+        }
+    }
+}