Skip to content

Cache keychain passwords to prevent duplicate prompts #5982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kodjima33 merged 1 commit into from
Mar 24, 2026
Merged

Cache keychain passwords to prevent duplicate prompts #5982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions desktop/Desktop/Sources/CalendarReaderService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,14 @@ sys.exit(0)
// MARK: - Keychain

private func getKeychainPassword(service: String) -> String? {
// Check shared cache first to avoid duplicate keychain prompts
if let cached = BrowserKeychainCache.shared.get(service) {
return cached.isEmpty ? nil : cached
}
if BrowserKeychainCache.shared.hasAttempted(service) {
return nil
}
Comment on lines +577 to +582
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Same dead 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.

Suggested change
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
}


let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: service,
Expand All @@ -584,11 +592,15 @@ sys.exit(0)
guard status == errSecSuccess, let data = result as? Data,
let password = String(data: data, encoding: .utf8)
else {
BrowserKeychainCache.shared.markFailed(service)
if status != errSecItemNotFound {
log("CalendarReaderService: Keychain lookup for '\(service)' failed with status \(status)")
}
return nil
}
if !password.isEmpty {
BrowserKeychainCache.shared.set(service, password: password)
}
return password.isEmpty ? nil : password
}
}
47 changes: 47 additions & 0 deletions desktop/Desktop/Sources/GmailReaderService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,41 @@ private struct BrowserConfig {
}
}

// MARK: - Shared Keychain Cache

/// Shared cache for browser keychain passwords so we only prompt once per session.
/// Used by both GmailReaderService and CalendarReaderService.
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] = ""
}
Comment on lines +93 to +97
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Permanent failure caching with no invalidation path

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.

}
Comment on lines +69 to +98
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing 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.

Suggested change
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()


// MARK: - GmailReaderService

actor GmailReaderService {
Expand Down Expand Up @@ -525,6 +560,14 @@ actor GmailReaderService {
// MARK: - Keychain

private func getKeychainPassword(service: String) -> String? {
// Check shared cache first to avoid duplicate keychain prompts
if let cached = BrowserKeychainCache.shared.get(service) {
return cached.isEmpty ? nil : cached
}
if BrowserKeychainCache.shared.hasAttempted(service) {
return nil
}
Comment on lines +564 to +569
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 hasAttempted check is unreachable dead code

The hasAttempted guard on lines 567-569 can never fire. Here's why:

  • get(_:) returns cache[service], which is nil only when the key is absent from the dictionary.
  • markFailed(_:) stores "" in the dictionary, so a failed lookup is represented as Optional("") — not nil.
  • Therefore if let cached = BrowserKeychainCache.shared.get(service) already handles the "previously failed" case: cached binds "", the empty check returns nil, and we exit early on line 565.
  • Code past that if let is only reached when get returns nil, which means the key is not in the cache at all. At that point hasAttempted is guaranteed to return false.

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.

Suggested change
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
}


let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: service,
Expand All @@ -536,11 +579,15 @@ actor GmailReaderService {
guard status == errSecSuccess, let data = result as? Data,
let password = String(data: data, encoding: .utf8)
else {
BrowserKeychainCache.shared.markFailed(service)
if status != errSecItemNotFound {
log("GmailReaderService: Keychain lookup for '\(service)' failed with status \(status)")
}
return nil
}
if !password.isEmpty {
BrowserKeychainCache.shared.set(service, password: password)
}
return password.isEmpty ? nil : password
}

Expand Down
Loading