diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift index 02001d85..4d79112a 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift @@ -156,11 +156,18 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { /// Opaque rollback point for the account-scoped secret entries and their /// migration markers. The values stay in memory only for the duration of /// the editor save transaction. + /// + /// Each entry keeps the FULL read result, not just a value, so an entry we + /// could not read is recorded as exactly that rather than collapsing the + /// whole checkpoint. A checkpoint is a rollback *aid*; it must never become + /// a precondition for writing, or one unreadable Keychain item would make + /// an account permanently un-editable and un-deletable — see the header on + /// `makeSecretPersistenceCheckpoint(using:)`. public struct SecretPersistenceCheckpoint { - fileprivate let apiKey: String? - fileprivate let apiKeyMarker: String? - fileprivate let cookie: String? - fileprivate let cookieMarker: String? + fileprivate let apiKey: ProviderSecretReadResult + fileprivate let apiKeyMarker: ProviderSecretReadResult + fileprivate let cookie: ProviderSecretReadResult + fileprivate let cookieMarker: ProviderSecretReadResult } public func makeSecretPersistenceCheckpoint() @@ -314,19 +321,26 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: Self.migrationMarkerKey(accountID, "cookie"), accessGroup: group ) - guard - apiKey != .failure, - apiKeyMarker != .failure, - cookie != .failure, - cookieMarker != .failure - else { - return nil - } + // Deliberately NOT `guard ... != .failure else { return nil }`. + // + // That is what this used to do, and it was a trap: `readResult` maps + // EVERY OSStatus other than success/itemNotFound to `.failure`, so a + // single unreadable entry — a denied authorization prompt, an ACL + // mismatch after a re-sign or a MAS/Developer-ID channel switch, a + // missing entitlement, a non-UTF8 payload — made this return nil, and + // both `saveSecrets` and `deleteSecrets` guarded on it. The account + // could then be neither overwritten nor removed: fail-closed had become + // fail-forever, with no route back for the user. Reproduced against the + // real Keychain by planting a non-UTF8 item. + // + // An entry we cannot read has no known previous value, so there is + // nothing to preserve and nothing a caller could usefully refuse over. + // Record it as `.failure` and let `restoreSecrets` skip it. return SecretPersistenceCheckpoint( - apiKey: apiKey.value, - apiKeyMarker: apiKeyMarker.value, - cookie: cookie.value, - cookieMarker: cookieMarker.value + apiKey: apiKey, + apiKeyMarker: apiKeyMarker, + cookie: cookie, + cookieMarker: cookieMarker ) } @@ -336,7 +350,7 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { using store: any ProviderSecretStoring ) -> Bool { let group = Self.secretsAccessGroup - let entries: [(String, String?)] = [ + let entries: [(String, ProviderSecretReadResult)] = [ ( Self.accountKeychainKey(accountID, "apiKey"), checkpoint.apiKey @@ -355,9 +369,10 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { ), ] var restored = true - for (key, value) in entries { + for (key, captured) in entries { let entryRestored: Bool - if let value { + switch captured { + case let .value(value): entryRestored = store.save( key: key, @@ -368,7 +383,7 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: key, accessGroup: group ) == .value(value) - } else { + case .missing: entryRestored = store.delete( key: key, @@ -378,6 +393,14 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: key, accessGroup: group ) == .missing + case .failure: + // The entry was already unreadable when the checkpoint was + // taken, so there is no prior state to put back. Skipping is + // the only honest option: writing would invent a value and + // deleting would destroy one we never managed to see. It does + // NOT count against the rollback — we are no worse off here + // than before the attempt. + entryRestored = true } restored = entryRestored && restored } diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift index 76837358..3fe187e5 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift @@ -291,9 +291,22 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { ) } - func testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails() - throws - { + /// An unreadable entry must NOT collapse the checkpoint. + /// + /// Replaces `testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails`, + /// which pinned the opposite promise — "if any entry cannot be read, refuse + /// to write at all". That promise was the bug. `readResult` maps every + /// OSStatus other than success/itemNotFound to `.failure`, so one denied + /// authorization prompt, ACL mismatch after a re-sign, missing entitlement + /// or non-UTF8 payload made `makeSecretPersistenceCheckpoint` return nil — + /// and both `saveSecrets` and `deleteSecrets` guarded on it, leaving the + /// account neither editable nor removable with no route back for the user. + /// Reproduced against the real Keychain by planting a non-UTF8 item. + /// + /// The trade this encodes: an entry we could never read cannot be rolled + /// back either, because its previous value was never observed. Readable + /// entries are still rolled back exactly as before. + func testCheckpointSurvivesAnUnreadableEntry() throws { let store = InMemoryProviderSecretStore() let accountID = try XCTUnwrap( UUID( @@ -310,29 +323,78 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { XCTAssertTrue(original.saveSecrets(using: store)) let apiKey = accountKey(accountID, "apiKey") store.failingLoadKeys.insert(apiKey) + + XCTAssertNotNil( + original.makeSecretPersistenceCheckpoint(using: store), + "an unreadable entry must be recorded, not turned into a refusal" + ) + let edited = ProviderConfig( kind: .claude, accountID: accountID, apiKey: "new-api-key", manualCookieHeader: "new-cookie" ) - + // This double fails reads for that key permanently, so the write-back + // verification inside `persistSecret` can never succeed and the save + // still reports failure. What matters here is what happens to the OTHER + // entry while that is going on. XCTAssertFalse(edited.saveSecrets(using: store)) - store.failingLoadKeys.remove(apiKey) XCTAssertEqual( store.load( - key: apiKey, + key: accountKey(accountID, "cookie"), accessGroup: ProviderConfig.secretsAccessGroup ), - "old-api-key" + "old-cookie", + "the readable entry must still be rolled back" + ) + } + + /// The rollback path itself: a `.failure` entry is skipped rather than + /// invented or destroyed, and skipping it does not count as a failed + /// rollback — we end up no worse off than before the attempt. + func testRestoreSkipsUnreadableEntryAndStillReportsSuccess() throws { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ADADADAD-ADAD-4DAD-8DAD-ADADADADADAD" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "seed-api-key", + manualCookieHeader: "seed-cookie" + ) + XCTAssertTrue(config.saveSecrets(using: store)) + + store.failingLoadKeys.insert(accountKey(accountID, "apiKey")) + let checkpoint = try XCTUnwrap( + config.makeSecretPersistenceCheckpoint(using: store) + ) + store.failingLoadKeys.removeAll() + + XCTAssertTrue( + store.save( + key: accountKey(accountID, "cookie"), + value: "drifted-cookie", + accessGroup: ProviderConfig.secretsAccessGroup + ) + ) + + XCTAssertTrue( + config.restoreSecrets(from: checkpoint, using: store), + "skipping an unreadable entry must not be reported as a failed rollback" ) XCTAssertEqual( store.load( key: accountKey(accountID, "cookie"), accessGroup: ProviderConfig.secretsAccessGroup ), - "old-cookie" + "seed-cookie", + "the readable entry is restored" ) }