fix(menubar): keep Keychain failure paths from leaving plaintext

A valid Keychain item plus a leftover JSON used to skip chmod, so a
failed unlink could leave 0644 secrets on disk. Failed Disconnect also
cleared bootstrap and hid the retry. Repair leftover files to 0600,
keep bootstrap when Keychain delete fails, revalidate the opened fd,
and loop the secure read.
This commit is contained in:
Aditya Vikram Singh 2026-08-19 20:02:44 +05:30
parent cdaa5b7ed3
commit 252ea92d3b
5 changed files with 89 additions and 14 deletions

View file

@ -1069,6 +1069,13 @@ final class AppStore {
// resumes after this point detects the disconnect and discards its
// result instead of re-populating the cleared state.
claudeRefreshGen &+= 1
if let result = ClaudeCredentialStore.lastCacheDeleteResult, !result.keychainDeletedOrAbsent {
// Keychain item still present keep Connect/Disconnect on the
// connected path so the user can retry delete.
subscriptionError = "Could not fully remove the local Claude credential cache. Disconnect again to retry."
NotificationCenter.default.post(name: .codeBurnSubscriptionDisconnected, object: nil)
return
}
subscription = nil
if let result = ClaudeCredentialStore.lastCacheDeleteResult, !result.isSuccess {
subscriptionError = "Could not fully remove the local Claude credential cache."
@ -1137,6 +1144,11 @@ final class AppStore {
func disconnectCodex() {
CodexSubscriptionService.disconnect()
codexRefreshGen &+= 1
if let result = CodexCredentialStore.lastCacheDeleteResult, !result.keychainDeletedOrAbsent {
codexError = "Could not fully remove the local Codex credential cache. Disconnect again to retry."
NotificationCenter.default.post(name: .codeBurnSubscriptionDisconnected, object: nil)
return
}
codexUsage = nil
if let result = CodexCredentialStore.lastCacheDeleteResult, !result.isSuccess {
codexError = "Could not fully remove the local Codex credential cache."

View file

@ -56,6 +56,7 @@ enum ClaudeCredentialStore {
keychainCache = LiveKeychainCredentialCache()
lastCacheDeleteResult = nil
lastLegacyCleanupFailed = false
unlinkLegacyOverride = nil
lock.withLock { memoryCache = nil }
}
@ -134,7 +135,11 @@ enum ClaudeCredentialStore {
lock.withLock { memoryCache = nil }
let result = deleteOurCache()
lastCacheDeleteResult = result
isBootstrapCompleted = false
// A failed Keychain delete must not pretend the provider is disconnected.
// Clearing the flag hides Disconnect and orphans the remaining item.
if result.keychainDeletedOrAbsent {
isBootstrapCompleted = false
}
return result
}
@ -150,6 +155,8 @@ enum ClaudeCredentialStore {
/// 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)?
// MARK: - Public API
@ -490,11 +497,16 @@ enum ClaudeCredentialStore {
return
}
do {
try FileManager.default.removeItem(at: url)
if let unlinkLegacyOverride {
try unlinkLegacyOverride(url)
} else {
try FileManager.default.removeItem(at: url)
}
lastLegacyCleanupFailed = false
} catch {
// Retain valid Keychain item + 0600 file; surface for later retry.
lastLegacyCleanupFailed = true
// Verified Keychain item exists leftover plaintext must not stay 0644.
try? FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: url.path)
}
}

View file

@ -43,6 +43,7 @@ enum CodexCredentialStore {
keychainCache = LiveKeychainCredentialCache()
lastCacheDeleteResult = nil
lastLegacyCleanupFailed = false
unlinkLegacyOverride = nil
lock.withLock { memoryCache = nil }
}
@ -133,7 +134,9 @@ enum CodexCredentialStore {
lock.withLock { memoryCache = nil }
let result = deleteOurCache()
lastCacheDeleteResult = result
isBootstrapCompleted = false
if result.keychainDeletedOrAbsent {
isBootstrapCompleted = false
}
return result
}
@ -145,6 +148,7 @@ enum CodexCredentialStore {
nonisolated(unsafe) static var lastCacheDeleteResult: CacheDeleteResult?
nonisolated(unsafe) static var lastLegacyCleanupFailed = false
nonisolated(unsafe) static var unlinkLegacyOverride: ((URL) throws -> Void)?
// MARK: - Public API
@ -368,10 +372,15 @@ enum CodexCredentialStore {
return
}
do {
try FileManager.default.removeItem(at: url)
if let unlinkLegacyOverride {
try unlinkLegacyOverride(url)
} else {
try FileManager.default.removeItem(at: url)
}
lastLegacyCleanupFailed = false
} catch {
lastLegacyCleanupFailed = true
try? FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: url.path)
}
}

View file

@ -136,6 +136,17 @@ enum SafeFile {
}
defer { Darwin.close(fd) }
var opened = stat()
guard fstat(fd, &opened) == 0 else {
throw Error.readFailed(path, errno)
}
guard (opened.st_mode & S_IFMT) == S_IFREG else {
throw SecureReadError.notRegularFile(path)
}
guard opened.st_uid == expectedOwner else {
throw SecureReadError.wrongOwner(path)
}
if fchmod(fd, 0o600) != 0 {
throw SecureReadError.chmodFailed(path, errno)
}
@ -153,16 +164,22 @@ enum SafeFile {
throw Error.sizeLimitExceeded(path, size)
}
var data = Data(count: max(size, 0))
let readBytes: Int = data.withUnsafeMutableBytes { buffer -> Int in
guard let base = buffer.baseAddress else { return 0 }
return Darwin.read(fd, base, buffer.count)
var data = Data()
data.reserveCapacity(max(size, 0))
var chunk = [UInt8](repeating: 0, count: 4096)
while data.count < maxBytes {
let n = chunk.withUnsafeMutableBytes { buffer -> Int in
guard let base = buffer.baseAddress else { return 0 }
return Darwin.read(fd, base, buffer.count)
}
guard n >= 0 else {
throw Error.readFailed(path, errno)
}
if n == 0 { break }
data.append(contentsOf: chunk.prefix(n))
}
guard readBytes >= 0 else {
throw Error.readFailed(path, errno)
}
if readBytes < data.count {
data = data.prefix(readBytes)
if data.count > maxBytes {
throw Error.sizeLimitExceeded(path, data.count)
}
return data
}

View file

@ -289,6 +289,7 @@ struct CredentialKeychainContinuityTests {
expiresAt: nil,
rateLimitTier: nil
))
ClaudeCredentialStore.isBootstrapCompleted = true
let controllable = ControllableKeychainCredentialCache(inner: harness.fakeKeychain)
controllable.failDelete = true
ClaudeCredentialStore.keychainCache = controllable
@ -297,6 +298,30 @@ struct CredentialKeychainContinuityTests {
#expect(!failed.isSuccess)
#expect(failed.keychainDeletedOrAbsent == false)
#expect(ClaudeCredentialStore.lastCacheDeleteResult?.isSuccess == false)
#expect(ClaudeCredentialStore.isBootstrapCompleted == true)
}
}
@Test("failed unlink after verified Keychain repairs leftover JSON to 0600")
func failedUnlinkRepairsLegacyMode() throws {
try withHarness { _ in
let record = ClaudeCredentialStore.CredentialRecord(
accessToken: accessSentinel,
refreshToken: refreshSentinel,
expiresAt: Date(timeIntervalSince1970: 1_900_000_000),
rateLimitTier: "default"
)
try ClaudeCredentialStore.writeOurCache(record: record)
try writeLegacyClaude0644(record: record)
ClaudeCredentialStore.isBootstrapCompleted = true
ClaudeCredentialStore.unlinkLegacyOverride = { _ in
throw POSIXError(.EPERM)
}
ClaudeCredentialStore.clearMemoryCacheForTesting()
_ = try ClaudeCredentialStore.currentRecord()
#expect(ClaudeCredentialStore.lastLegacyCleanupFailed == true)
#expect(FileManager.default.fileExists(atPath: ClaudeCredentialStore.cacheFileURL().path))
#expect(posixMode(at: ClaudeCredentialStore.cacheFileURL()) == 0o600)
}
}