vrr/codeburn
2
0
Fork 0
mirror of https://github.com/AgentSeal/codeburn.git synced 2026-08-21 14:34:32 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix(menubar): keep the newer credential copy and drop the sticky retry flag

Browse source 
Three fixes in the store read path.

A locked keychain no longer reads as a disconnect. currentRecord() treated
any failure from readOurCache() as fatal, and a nil as "the item vanished",
which cleared isBootstrapCompleted. .unavailable now falls back to the last
known record and leaves the flag set.

Recency. A Keychain hit always won and the legacy file was then unlinked,
even when the file was newer. This service name has been in use since May
2026, so an upgrading install can hold a months-old item beside a file the
pre-migration build wrote today; the older token won and the newer copy was
deleted. Both stores now compare first (expiresAt for Claude, lastRefresh for
Codex) and adopt the later one before anything is removed. Codex matters most
here: serving a spent rotating refresh token ends in a terminal invalid_grant.

lastLegacyCleanupFailed is gone. It was set on every cleanup path and read
only by tests, never surfaced. The retry it was meant to signal already
happens, because the unlink is attempted on every successful read.

Also serializes migrate + unlink under the existing SafeFile.withExclusiveLock
so two menubar instances cannot race on the same legacy file, and drops a
leftover no-op local.
This commit is contained in:
iamtoruk 2026-08-19 11:43:32 -07:00
parent e213e192b4
commit 0fd8419bfd
3 changed files with 288 additions and 90 deletions
Download patch file Download diff file Expand all files Collapse all files

114
mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift
View file

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

111
mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift
View file

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

153
mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift
View file

@ -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