Cache keychain passwords to prevent duplicate prompts#5982
Conversation
Add BrowserKeychainCache shared between Gmail and Calendar services. Keychain is only accessed once per browser per session — second service reuses the cached password. No more double prompts after restart. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR introduces a shared Key observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant GRS as GmailReaderService
participant CRS as CalendarReaderService
participant Cache as BrowserKeychainCache
participant KC as macOS Keychain
Note over GRS,KC: App startup — Gmail loads first
GRS->>Cache: get("Chrome Safe Storage")
Cache-->>GRS: nil (cold cache)
GRS->>KC: SecItemCopyMatching(...)
KC-->>GRS: password (user prompted once)
GRS->>Cache: set("Chrome Safe Storage", password)
Note over GRS,KC: Calendar loads shortly after
CRS->>Cache: get("Chrome Safe Storage")
Cache-->>CRS: password (cache hit — no prompt)
CRS-->>CRS: return cached password
Note over GRS,KC: Failure path
GRS->>Cache: get("Arc Safe Storage")
Cache-->>GRS: nil (cold cache)
GRS->>KC: SecItemCopyMatching(...)
KC-->>GRS: errSecItemNotFound
GRS->>Cache: markFailed("Arc Safe Storage")
CRS->>Cache: get("Arc Safe Storage")
Cache-->>CRS: "" (returns nil — no second prompt)
Reviews (1): Last reviewed commit: "fix(desktop): cache keychain passwords t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let cached = BrowserKeychainCache.shared.get(service) { | ||
| return cached.isEmpty ? nil : cached | ||
| } | ||
| if BrowserKeychainCache.shared.hasAttempted(service) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
hasAttempted check is unreachable dead code
The hasAttempted guard on lines 567-569 can never fire. Here's why:
get(_:)returnscache[service], which isnilonly when the key is absent from the dictionary.markFailed(_:)stores""in the dictionary, so a failed lookup is represented asOptional("")— notnil.- Therefore
if let cached = BrowserKeychainCache.shared.get(service)already handles the "previously failed" case:cachedbinds"", the empty check returnsnil, and we exit early on line 565. - Code past that
if letis only reached whengetreturnsnil, which means the key is not in the cache at all. At that pointhasAttemptedis guaranteed to returnfalse.
The result is that the block if BrowserKeychainCache.shared.hasAttempted(service) { return nil } is dead code and creates a false impression that two separate paths exist for "never tried" vs "tried and failed".
Fix: remove the redundant check, or change get to return nil for failures and use a separate Bool-typed flag so the distinction is explicit.
| if let cached = BrowserKeychainCache.shared.get(service) { | |
| return cached.isEmpty ? nil : cached | |
| } | |
| if BrowserKeychainCache.shared.hasAttempted(service) { | |
| return nil | |
| } | |
| // Check shared cache first to avoid duplicate keychain prompts | |
| if let cached = BrowserKeychainCache.shared.get(service) { | |
| return cached.isEmpty ? nil : cached | |
| } | |
| final class BrowserKeychainCache: @unchecked Sendable { | ||
| static let shared = BrowserKeychainCache() | ||
| private var cache: [String: String] = [:] | ||
| private let lock = NSLock() | ||
|
|
||
| func get(_ service: String) -> String? { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return cache[service] | ||
| } | ||
|
|
||
| func set(_ service: String, password: String) { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| cache[service] = password | ||
| } | ||
|
|
||
| /// Returns true if we already tried and failed for this service | ||
| func hasAttempted(_ service: String) -> Bool { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return cache.keys.contains(service) | ||
| } | ||
|
|
||
| func markFailed(_ service: String) { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| cache[service] = "" | ||
| } | ||
| } |
There was a problem hiding this comment.
private init() on singleton
BrowserKeychainCache is designed as a singleton (static let shared), but without a private init(), any external code can create additional instances via BrowserKeychainCache(), bypassing the shared cache entirely and defeating the duplicate-prompt prevention.
| final class BrowserKeychainCache: @unchecked Sendable { | |
| static let shared = BrowserKeychainCache() | |
| private var cache: [String: String] = [:] | |
| private let lock = NSLock() | |
| func get(_ service: String) -> String? { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return cache[service] | |
| } | |
| func set(_ service: String, password: String) { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| cache[service] = password | |
| } | |
| /// Returns true if we already tried and failed for this service | |
| func hasAttempted(_ service: String) -> Bool { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return cache.keys.contains(service) | |
| } | |
| func markFailed(_ service: String) { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| cache[service] = "" | |
| } | |
| } | |
| final class BrowserKeychainCache: @unchecked Sendable { | |
| static let shared = BrowserKeychainCache() | |
| private init() {} | |
| private var cache: [String: String] = [:] | |
| private let lock = NSLock() |
| if let cached = BrowserKeychainCache.shared.get(service) { | ||
| return cached.isEmpty ? nil : cached | ||
| } | ||
| if BrowserKeychainCache.shared.hasAttempted(service) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
hasAttempted check as in GmailReaderService
The same unreachable logic described in GmailReaderService.swift is duplicated here. Since get already handles the "" sentinel, hasAttempted can never return true at this point, and the block is dead code.
| if let cached = BrowserKeychainCache.shared.get(service) { | |
| return cached.isEmpty ? nil : cached | |
| } | |
| if BrowserKeychainCache.shared.hasAttempted(service) { | |
| return nil | |
| } | |
| // Check shared cache first to avoid duplicate keychain prompts | |
| if let cached = BrowserKeychainCache.shared.get(service) { | |
| return cached.isEmpty ? nil : cached | |
| } | |
| func markFailed(_ service: String) { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| cache[service] = "" | ||
| } |
There was a problem hiding this comment.
markFailed is called for every failure branch, including errSecItemNotFound (item simply doesn't exist in the keychain yet). Once cached as a failure, there is no way to invalidate the entry for the rest of the app's lifetime.
This means: if a user logs into a browser after the app has already attempted (and failed) a keychain lookup, the credential will never be picked up without a full app restart. While the PR description intentionally scopes this to "one session," it may be surprising in practice — e.g., during a first-run onboarding flow where the browser is opened after the desktop app.
Consider calling markFailed only for hard errors (not errSecItemNotFound), so a "not found yet" case can be retried without restarting the app.
…5982) ## Summary Add shared `BrowserKeychainCache` between GmailReaderService and CalendarReaderService. Keychain is only accessed once per browser per session — the second service reuses the cached password. No more double keychain prompts after restart. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 participant
Summary
Add shared
BrowserKeychainCachebetween GmailReaderService and CalendarReaderService. Keychain is only accessed once per browser per session — the second service reuses the cached password. No more double keychain prompts after restart.🤖 Generated with Claude Code