From 252ea92d3b8a8fc3057fe835e74ff72d4f445fd3 Mon Sep 17 00:00:00 2001 From: Aditya Vikram Singh <247195684+avs-io@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:02:44 +0530 Subject: [PATCH] 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. --- mac/Sources/CodeBurnMenubar/AppStore.swift | 12 +++++++ .../Data/ClaudeCredentialStore.swift | 18 ++++++++-- .../Data/CodexCredentialStore.swift | 13 +++++-- .../CodeBurnMenubar/Security/SafeFile.swift | 35 ++++++++++++++----- .../CredentialKeychainContinuityTests.swift | 25 +++++++++++++ 5 files changed, 89 insertions(+), 14 deletions(-) diff --git a/mac/Sources/CodeBurnMenubar/AppStore.swift b/mac/Sources/CodeBurnMenubar/AppStore.swift index 33ac8d12..98bd91f8 100644 --- a/mac/Sources/CodeBurnMenubar/AppStore.swift +++ b/mac/Sources/CodeBurnMenubar/AppStore.swift @@ -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." diff --git a/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift b/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift index d9e691e6..438dc397 100644 --- a/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift +++ b/mac/Sources/CodeBurnMenubar/Data/ClaudeCredentialStore.swift @@ -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) } } diff --git a/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift b/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift index 96bf71fd..8ab257e3 100644 --- a/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift +++ b/mac/Sources/CodeBurnMenubar/Data/CodexCredentialStore.swift @@ -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) } } diff --git a/mac/Sources/CodeBurnMenubar/Security/SafeFile.swift b/mac/Sources/CodeBurnMenubar/Security/SafeFile.swift index 080682d0..6a300a7f 100644 --- a/mac/Sources/CodeBurnMenubar/Security/SafeFile.swift +++ b/mac/Sources/CodeBurnMenubar/Security/SafeFile.swift @@ -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 } diff --git a/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift b/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift index 00ffc36f..1b75a9e1 100644 --- a/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift +++ b/mac/Tests/CodeBurnMenubarTests/CredentialKeychainContinuityTests.swift @@ -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) } }