From e70b41c997e3ba907c0117177fd5edeb4636caf8 Mon Sep 17 00:00:00 2001 From: YE Date: Mon, 17 Aug 2026 19:29:13 +0900 Subject: [PATCH] fix: an unreadable credential must not brick the account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readResult` maps every OSStatus other than success/itemNotFound to `.failure`, and `makeSecretPersistenceCheckpoint` returned nil if ANY of its four reads came back that way. Both `saveSecrets` and `deleteSecrets` guarded on that checkpoint, so a single unreadable entry left the account neither editable nor removable — fail-closed had become fail-forever, with no route back for the user short of Keychain Access.app. Reproduced against the real login Keychain by planting a non-UTF8 payload: readResult -> .failure makeSecretPersistence… -> nil saveSecrets -> false deleteSecretsForAccount…-> false This is a regression rather than a pre-existing gap: before the save transaction landed, neither function read anything first, so overwriting an unreadable item succeeded and so did deleting it. The checkpoint now keeps the full `ProviderSecretReadResult` per entry instead of collapsing to `String?`, so "unreadable" is recorded rather than fatal, and `restoreSecrets` skips those entries. Skipping is the only honest option: writing would invent a value we never saw and deleting would destroy one. It does not count against the rollback, because we end up no worse off than before the attempt. Readable entries are still rolled back exactly as before. Same repro after the change: checkpoint is produced, save succeeds and the replacement value is what is stored, and an account holding a corrupt credential can be deleted. `makeSecretPersistenceCheckpoint` keeps its optional return type on purpose, so every existing `guard let` call site compiles unchanged and simply stops tripping. The blast radius is one type and one function. `testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails` pinned the promise that caused this ("if any entry cannot be read, refuse to write at all") and is replaced by two tests that pin the corrected contract. Both were negative- controlled: with the guard restored they fail, with it removed they pass. 2,717 tests, 4 skipped, 0 failures. Co-Authored-By: Claude Opus 5 --- .../Sources/CLIPulseCore/ProviderConfig.swift | 63 ++++++++++----- ...roviderAccountKeychainMigrationTests.swift | 78 +++++++++++++++++-- 2 files changed, 113 insertions(+), 28 deletions(-) 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" ) }