diff --git a/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift b/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift index 5df6bbc8..a5a7e8f7 100644 --- a/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift +++ b/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift @@ -55,7 +55,6 @@ enum ClaudeCredentialStore { userDefaultsOverride = nil keychainCache = LiveKeychainCredentialCache() lastCacheDeleteResult = nil - lastLegacyCleanupFailed = false unlinkLegacyOverride = nil tightenLegacyOverride = nil lock.withLock { memoryCache = nil } @@ -154,8 +153,6 @@ enum ClaudeCredentialStore { /// Last disconnect/cleanup result. Nil until the first delete attempt. nonisolated(unsafe) static var lastCacheDeleteResult: CacheDeleteResult? - /// Last legacy-unlink failure after a verified Keychain write (cleanup retry signal). - nonisolated(unsafe) static var lastLegacyCleanupFailed = false /// Test seam: force legacy unlink to throw so we can assert the 0600 repair. nonisolated(unsafe) static var unlinkLegacyOverride: ((URL) throws -> Void)? nonisolated(unsafe) static var tightenLegacyOverride: ((URL) throws -> Void)? @@ -185,7 +182,19 @@ enum ClaudeCredentialStore { if let cached = lock.withLock({ memoryCache }), cached.isFresh { return cached.record } - if let stored = try readOurCache() { + let fetched: CredentialRecord? + do { + fetched = try readOurCache() + } catch let err as KeychainCredentialCacheError { + // A locked/denied keychain means "can't look right now", not "the + // item is gone". Serve the last known token and leave the bootstrap + // flag alone so we don't silently disconnect the user. + if case .unavailable = err { + return lock.withLock { memoryCache }?.record + } + throw err + } + if let stored = fetched { cacheInMemory(stored) return stored } @@ -437,17 +446,37 @@ enum ClaudeCredentialStore { } } + /// Serializes migrate + unlink across processes so two menubar instances (or the + /// CLI) cannot race on the same legacy file. Only `SafeFile.Error` from acquiring + /// the lock is tolerated — `readOurCacheLocked` never lets one escape, because + /// `readLegacyFile` swallows its own read failures. private static func readOurCache() throws -> CredentialRecord? { + do { + return try SafeFile.withExclusiveLock(at: cacheFileURL().path + ".lock") { + try readOurCacheLocked() + } + } catch is SafeFile.Error { + return try readOurCacheLocked() + } + } + + private static func readOurCacheLocked() throws -> CredentialRecord? { if let data = try keychainCache.read(service: ourKeychainService, account: ourKeychainAccount), let record = decodePersisted(data) { + // The Keychain item can predate the legacy file by a long way — this + // service name has been in use since May 2026, so an upgrading install + // can hold a months-old item beside a file the old build wrote today. + // Adopt whichever expires later before unlinking anything. + if let fresher = legacyRecordIfNewer(than: record) { + try? writeOurCache(record: fresher) + return fresher + } // Rewrite historical Claude blobs once without refreshToken. - let sanitized = encodePersisted(record) if let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], object.keys.contains("refreshToken") { try? writeOurCache(record: record) } else { tryUnlinkLegacyAfterVerifiedKeychain() - _ = sanitized } return record } @@ -455,66 +484,63 @@ enum ClaudeCredentialStore { return try migrateLegacyFileIfPresent() } - /// Secure-read legacy JSON, upsert Keychain, verify read-back, then unlink. - private static func migrateLegacyFileIfPresent() throws -> CredentialRecord? { + /// Reads the legacy file only to compare recency. Returns it when it expires + /// strictly later than `record`; nil when absent, unreadable, or not newer. + private static func legacyRecordIfNewer(than record: CredentialRecord) -> CredentialRecord? { + guard let legacy = readLegacyFile() else { return nil } + guard let legacyExpiry = legacy.expiresAt else { return nil } + guard let currentExpiry = record.expiresAt else { return legacy } + return legacyExpiry > currentExpiry ? legacy : nil + } + + /// Secure-read + decode the legacy JSON, dropping any refreshToken it holds. + /// Returns nil on symlink / ownership / chmod / decode failure, leaving the + /// file in place. + private static func readLegacyFile() -> CredentialRecord? { let url = cacheFileURL() guard FileManager.default.fileExists(atPath: url.path) else { return nil } - - let data: Data - do { - data = try SafeFile.readAfterSecuringPermissions( - from: url.path, - maxBytes: maxCredentialBytes - ) - } catch { - // Symlink / ownership / chmod failures: leave the file alone. - return nil - } - + guard let data = try? SafeFile.readAfterSecuringPermissions( + from: url.path, + maxBytes: maxCredentialBytes + ) else { return nil } guard let decoded = try? JSONDecoder().decode(CredentialRecord.self, from: data) else { // Invalid data stays in place at 0600; do not delete. return nil } - let migrated = CredentialRecord( + return CredentialRecord( accessToken: decoded.accessToken, refreshToken: nil, expiresAt: decoded.expiresAt, rateLimitTier: decoded.rateLimitTier ) - - do { - try writeOurCache(record: migrated) - return migrated - } catch { - // Keychain write/read-back failure: leave repaired 0600 legacy file. - lastLegacyCleanupFailed = false - return migrated - } } + /// Secure-read legacy JSON, upsert Keychain, verify read-back, then unlink. + private static func migrateLegacyFileIfPresent() throws -> CredentialRecord? { + guard let migrated = readLegacyFile() else { return nil } + // Keychain write/read-back failure leaves the repaired 0600 legacy file + // in place; the next read retries the migration. + try? writeOurCache(record: migrated) + return migrated + } + + /// Unlink the redundant legacy JSON. Called on every successful cache read, + /// so a failure here is retried on the next read without a sticky flag. private static func tryUnlinkLegacyAfterVerifiedKeychain() { let url = cacheFileURL() - guard FileManager.default.fileExists(atPath: url.path) else { - lastLegacyCleanupFailed = false - return - } + guard FileManager.default.fileExists(atPath: url.path) else { return } do { if let unlinkLegacyOverride { try unlinkLegacyOverride(url) } else { try FileManager.default.removeItem(at: url) } - lastLegacyCleanupFailed = false } catch { - lastLegacyCleanupFailed = true - do { - if let tightenLegacyOverride { - try tightenLegacyOverride(url) - } else { - try SafeFile.tightenToOwnerReadWrite(at: url.path) - } - } catch { - // Still leftover, and 0600 is unproven. Retry signal stays set. + // Could not remove it — at least make sure it is not world-readable. + if let tightenLegacyOverride { + try? tightenLegacyOverride(url) + } else { + try? SafeFile.tightenToOwnerReadWrite(at: url.path) } } } diff --git a/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift b/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift index 00385b05..2d500cfa 100644 --- a/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift +++ b/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift @@ -42,7 +42,6 @@ enum CodexCredentialStore { userDefaultsOverride = nil keychainCache = LiveKeychainCredentialCache() lastCacheDeleteResult = nil - lastLegacyCleanupFailed = false unlinkLegacyOverride = nil tightenLegacyOverride = nil lock.withLock { memoryCache = nil } @@ -148,7 +147,6 @@ enum CodexCredentialStore { } nonisolated(unsafe) static var lastCacheDeleteResult: CacheDeleteResult? - nonisolated(unsafe) static var lastLegacyCleanupFailed = false nonisolated(unsafe) static var unlinkLegacyOverride: ((URL) throws -> Void)? nonisolated(unsafe) static var tightenLegacyOverride: ((URL) throws -> Void)? @@ -178,7 +176,18 @@ enum CodexCredentialStore { if let cached = lock.withLock({ memoryCache }), cached.isFresh { return cached.record } - if let stored = try readOurCache() { + let fetched: CredentialRecord? + do { + fetched = try readOurCache() + } catch let err as KeychainCredentialCacheError { + // Locked/denied keychain: serve the last known token rather than + // reporting the grant as missing. + if case .unavailable = err { + return lock.withLock { memoryCache }?.record + } + throw err + } + if let stored = fetched { cacheInMemory(stored) return stored } @@ -331,65 +340,83 @@ enum CodexCredentialStore { tryUnlinkLegacyAfterVerifiedKeychain() } + /// Serializes migrate + unlink across processes so two menubar instances (or the + /// CLI) cannot race on the same legacy file. Only `SafeFile.Error` from acquiring + /// the lock is tolerated — `readOurCacheLocked` never lets one escape, because + /// `readLegacyFile` swallows its own read failures. private static func readOurCache() throws -> CredentialRecord? { + do { + return try SafeFile.withExclusiveLock(at: cacheFileURL().path + ".lock") { + try readOurCacheLocked() + } + } catch is SafeFile.Error { + return try readOurCacheLocked() + } + } + + private static func readOurCacheLocked() throws -> CredentialRecord? { if let data = try keychainCache.read(service: ourKeychainService, account: ourKeychainAccount), let record = try? JSONDecoder().decode(CredentialRecord.self, from: data) { + // Only reached when auth.json is unreadable, but the Keychain item can + // still be older than the legacy file — and serving a spent rotating + // refresh token here ends in a terminal invalid_grant. Prefer the + // later `lastRefresh` before unlinking anything. + if let fresher = legacyRecordIfNewer(than: record) { + try? writeOurCache(record: fresher) + return fresher + } tryUnlinkLegacyAfterVerifiedKeychain() return record } return try migrateLegacyFileIfPresent() } - private static func migrateLegacyFileIfPresent() throws -> CredentialRecord? { - let url = cacheFileURL() - guard FileManager.default.fileExists(atPath: url.path) else { return nil } - - let data: Data - do { - data = try SafeFile.readAfterSecuringPermissions( - from: url.path, - maxBytes: maxCredentialBytes - ) - } catch { - return nil - } - - guard let decoded = try? JSONDecoder().decode(CredentialRecord.self, from: data) else { - return nil - } - - do { - try writeOurCache(record: decoded) - return decoded - } catch { - lastLegacyCleanupFailed = false - return decoded - } + /// Returns the legacy file's record when it refreshed strictly later than + /// `record`; nil when absent, unreadable, or not newer. + private static func legacyRecordIfNewer(than record: CredentialRecord) -> CredentialRecord? { + guard let legacy = readLegacyFile() else { return nil } + guard let legacyRefresh = legacy.lastRefresh else { return nil } + guard let currentRefresh = record.lastRefresh else { return legacy } + return legacyRefresh > currentRefresh ? legacy : nil } + /// Secure-read + decode the legacy JSON. Returns nil on symlink / ownership / + /// chmod / decode failure, leaving the file in place. + private static func readLegacyFile() -> CredentialRecord? { + let url = cacheFileURL() + guard FileManager.default.fileExists(atPath: url.path) else { return nil } + guard let data = try? SafeFile.readAfterSecuringPermissions( + from: url.path, + maxBytes: maxCredentialBytes + ) else { return nil } + return try? JSONDecoder().decode(CredentialRecord.self, from: data) + } + + private static func migrateLegacyFileIfPresent() throws -> CredentialRecord? { + guard let decoded = readLegacyFile() else { return nil } + // Keychain write/read-back failure leaves the repaired 0600 legacy file + // in place; the next read retries the migration. + try? writeOurCache(record: decoded) + return decoded + } + + /// Unlink the redundant legacy JSON. Called on every successful cache read, + /// so a failure here is retried on the next read without a sticky flag. private static func tryUnlinkLegacyAfterVerifiedKeychain() { let url = cacheFileURL() - guard FileManager.default.fileExists(atPath: url.path) else { - lastLegacyCleanupFailed = false - return - } + guard FileManager.default.fileExists(atPath: url.path) else { return } do { if let unlinkLegacyOverride { try unlinkLegacyOverride(url) } else { try FileManager.default.removeItem(at: url) } - lastLegacyCleanupFailed = false } catch { - lastLegacyCleanupFailed = true - do { - if let tightenLegacyOverride { - try tightenLegacyOverride(url) - } else { - try SafeFile.tightenToOwnerReadWrite(at: url.path) - } - } catch { - // Still leftover, and 0600 is unproven. Retry signal stays set. + // Could not remove it — at least make sure it is not world-readable. + if let tightenLegacyOverride { + try? tightenLegacyOverride(url) + } else { + try? SafeFile.tightenToOwnerReadWrite(at: url.path) } } } diff --git a/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift b/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift index b4b8ebb6..13385a94 100644 --- a/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift +++ b/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift @@ -204,7 +204,6 @@ struct CredentialKeychainContinuityTests { account: ClaudeCredentialStore.ourKeychainAccount ) #expect(keys?.contains("refreshToken") != true) - #expect(ClaudeCredentialStore.lastLegacyCleanupFailed == false) } } @@ -319,13 +318,18 @@ struct CredentialKeychainContinuityTests { } ClaudeCredentialStore.clearMemoryCacheForTesting() _ = try ClaudeCredentialStore.currentRecord() - #expect(ClaudeCredentialStore.lastLegacyCleanupFailed == true) #expect(FileManager.default.fileExists(atPath: ClaudeCredentialStore.cacheFileURL().path)) #expect(posixMode(at: ClaudeCredentialStore.cacheFileURL()) == 0o600) + + // No sticky flag: the next read retries the unlink on its own. + ClaudeCredentialStore.unlinkLegacyOverride = nil + ClaudeCredentialStore.clearMemoryCacheForTesting() + _ = try ClaudeCredentialStore.currentRecord() + #expect(!FileManager.default.fileExists(atPath: ClaudeCredentialStore.cacheFileURL().path)) } } - @Test("failed tighten after failed unlink keeps leftover and retry signal") + @Test("failed tighten after failed unlink keeps leftover in place") func failedTightenLeavesRetrySignal() throws { try withHarness { _ in let record = ClaudeCredentialStore.CredentialRecord( @@ -341,7 +345,6 @@ struct CredentialKeychainContinuityTests { ClaudeCredentialStore.tightenLegacyOverride = { _ in throw POSIXError(.EPERM) } ClaudeCredentialStore.clearMemoryCacheForTesting() _ = try ClaudeCredentialStore.currentRecord() - #expect(ClaudeCredentialStore.lastLegacyCleanupFailed == true) #expect(FileManager.default.fileExists(atPath: ClaudeCredentialStore.cacheFileURL().path)) } } @@ -381,6 +384,148 @@ struct CredentialKeychainContinuityTests { } } + // MARK: - Locked / denied Keychain + + @Test("unavailable Keychain read is a miss, not a disconnect") + func unavailableKeychainKeepsBootstrap() throws { + try withHarness { harness in + try ClaudeCredentialStore.writeOurCache(record: .init( + accessToken: accessSentinel, + refreshToken: refreshSentinel, + expiresAt: Date(timeIntervalSince1970: 1_900_000_000), + rateLimitTier: "default" + )) + ClaudeCredentialStore.isBootstrapCompleted = true + + let controllable = ControllableKeychainCredentialCache(inner: harness.fakeKeychain) + controllable.failRead = true + controllable.readStatus = errSecInteractionNotAllowed // -25308 + ClaudeCredentialStore.keychainCache = controllable + ClaudeCredentialStore.clearMemoryCacheForTesting() + + // Must not throw and must not clear bootstrap: a locked keychain is + // "can't look right now", not "the user disconnected". + #expect(try ClaudeCredentialStore.currentRecord() == nil) + #expect(ClaudeCredentialStore.isBootstrapCompleted == true) + } + } + + @Test("a genuine read failure still surfaces as an error") + func nonUnavailableReadStillThrows() throws { + try withHarness { harness in + ClaudeCredentialStore.isBootstrapCompleted = true + let controllable = ControllableKeychainCredentialCache(inner: harness.fakeKeychain) + controllable.failRead = true + controllable.readStatus = errSecDecode + ClaudeCredentialStore.keychainCache = controllable + ClaudeCredentialStore.clearMemoryCacheForTesting() + + #expect(throws: KeychainCredentialCacheError.self) { + _ = try ClaudeCredentialStore.currentRecord() + } + } + } + + @Test("Keychain errors render a readable message, not a struct dump") + func keychainErrorMessageIsReadable() { + let unavailable = KeychainCredentialCacheError.unavailable( + service: ClaudeCredentialStore.ourKeychainService, + status: errSecInteractionNotAllowed + ) + let text = unavailable.localizedDescription + #expect(text.contains("Keychain unavailable")) + // AppStore renders errors via localizedDescription; a struct dump would + // read "unavailable(service:" and leak the raw item name. + #expect(!text.contains("unavailable(service:")) + #expect(!text.contains(ClaudeCredentialStore.ourKeychainService)) + } + + // MARK: - Recency between Keychain item and legacy file + + @Test("newer legacy file beats an older Keychain item and is then unlinked") + func newerLegacyFileWins() throws { + try withHarness { harness in + // Keychain item from an old build: already expired. + try ClaudeCredentialStore.writeOurCache(record: .init( + accessToken: "cb-stale-keychain-token", + refreshToken: nil, + expiresAt: Date(timeIntervalSince1970: 1_700_000_000), + rateLimitTier: "default" + )) + // Legacy file written far later by the pre-migration build. + try writeLegacyClaude0644(record: .init( + accessToken: accessSentinel, + refreshToken: refreshSentinel, + expiresAt: Date(timeIntervalSince1970: 1_900_000_000), + rateLimitTier: "default" + )) + ClaudeCredentialStore.isBootstrapCompleted = true + ClaudeCredentialStore.clearMemoryCacheForTesting() + + let record = try #require(try ClaudeCredentialStore.currentRecord()) + #expect(record.accessToken == accessSentinel) + #expect(record.refreshToken == nil) + #expect(!FileManager.default.fileExists(atPath: ClaudeCredentialStore.cacheFileURL().path)) + let keys = harness.fakeKeychain.storedKeys( + service: ClaudeCredentialStore.ourKeychainService, + account: ClaudeCredentialStore.ourKeychainAccount + ) + #expect(keys?.contains("refreshToken") != true) + } + } + + @Test("older legacy file loses to a newer Keychain item and is unlinked") + func olderLegacyFileLoses() throws { + try withHarness { _ in + try ClaudeCredentialStore.writeOurCache(record: .init( + accessToken: accessSentinel, + refreshToken: nil, + expiresAt: Date(timeIntervalSince1970: 1_900_000_000), + rateLimitTier: "default" + )) + try writeLegacyClaude0644(record: .init( + accessToken: "cb-stale-file-token", + refreshToken: refreshSentinel, + expiresAt: Date(timeIntervalSince1970: 1_700_000_000), + rateLimitTier: "default" + )) + ClaudeCredentialStore.isBootstrapCompleted = true + ClaudeCredentialStore.clearMemoryCacheForTesting() + + let record = try #require(try ClaudeCredentialStore.currentRecord()) + #expect(record.accessToken == accessSentinel) + #expect(!FileManager.default.fileExists(atPath: ClaudeCredentialStore.cacheFileURL().path)) + } + } + + @Test("Codex prefers the legacy file with the later lastRefresh") + func codexNewerLegacyFileWins() throws { + try withHarness { _ in + try CodexCredentialStore.writeOurCache(record: .init( + accessToken: "cb-stale-codex-access", + refreshToken: "cb-stale-codex-refresh", + idToken: nil, + accountId: accountSentinel, + expiresAt: nil, + lastRefresh: Date(timeIntervalSince1970: 1_700_000_000) + )) + try writeLegacyCodex0644(record: .init( + accessToken: accessSentinel, + refreshToken: refreshSentinel, + idToken: idSentinel, + accountId: accountSentinel, + expiresAt: nil, + lastRefresh: Date(timeIntervalSince1970: 1_800_000_000) + )) + CodexCredentialStore.isBootstrapCompleted = true + CodexCredentialStore.clearMemoryCacheForTesting() + + let record = try #require(try CodexCredentialStore.currentRecord()) + #expect(record.refreshToken == refreshSentinel) + #expect(!FileManager.default.fileExists(atPath: CodexCredentialStore.cacheFileURL().path)) + } + } + @Test("corrupt Keychain plus valid legacy repairs the CodeBurn item") func corruptKeychainRepairedFromLegacy() throws { try withHarness { harness in