-
Notifications
You must be signed in to change notification settings - Fork 251
fix: Reduce keychain permission prompts with ACL helper and setup wizard #279
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.
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
base: main
Are you sure you want to change the base?
fix: Reduce keychain permission prompts with ACL helper and setup wizard #279
Conversation
When storing tokens/cookies in keychain, now creates items with CodexBar in the trusted applications list. This prevents repeated keychain permission prompts after the initial user approval. Changes: - Add KeychainACLHelper.swift with SecAccess/SecACL utilities - Update all token stores to use trusted access on first write: - ZaiTokenStore - CookieHeaderStore - SyntheticTokenStore - CopilotTokenStore - KimiTokenStore - KimiK2TokenStore - MiniMaxAPITokenStore - MiniMaxCookieStore Falls back to standard keychain add if ACL creation fails. Fixes keychain prompt fatigue reported in tweet: https://x.com/steipete/status/2017306253416923154
When Chrome Safe Storage requires keychain interaction, shows a "Fix Now" button in Settings > Advanced that: - Detects if access is already granted (shows green checkmark) - Opens Keychain Access and searches for "Chrome Safe Storage" - Provides step-by-step instructions for adding CodexBar to ACL This complements the ACL helper by guiding users through the one-time manual setup required for Chrome-owned keychain items. Changes: - Add KeychainSetupHelper.swift with access detection and Keychain opener - Update PreferencesAdvancedPane with Browser Cookie Access section - Add KeychainSetupInstructionsView sheet with guided setup flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98c6cb52ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Give it a moment to open, then use AppleScript to search | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { | ||
| self.searchInKeychainAccess(for: "Chrome Safe Storage") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use correct Safe Storage item in setup flow
The setup wizard always searches for the literal "Chrome Safe Storage" and the instructions hard‑code that name, but checkChromeSafeStorageAccess iterates Browser.safeStorageLabels (which includes non‑Chrome Chromium variants). If the detected item is from Edge/Brave/Arc/etc., this flow opens Keychain Access and searches for a non‑existent item, so users can’t add CodexBar to the ACL and will keep getting prompts when using those browsers. Consider carrying through the actual label/service that triggered .needsSetup and using it in the UI + AppleScript search.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses keychain permission prompt fatigue by implementing a two-pronged solution for managing keychain access in CodexBar.
Changes:
- Introduces
KeychainACLHelperto create keychain items with CodexBar pre-authorized in the ACL, preventing future permission prompts for CodexBar-owned tokens - Adds
KeychainSetupHelperand UI wizard in Settings > Advanced to guide users through manual Chrome Safe Storage setup - Updates 8 token/cookie stores to use the ACL helper when adding new keychain items, with graceful fallback to standard keychain operations
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| KeychainACLHelper.swift | New helper that creates keychain items with CodexBar in the trusted apps ACL using SecTrustedApplicationCreateFromPath and SecAccessCreate |
| KeychainSetupHelper.swift | New helper that detects Chrome Safe Storage access status and opens Keychain Access app with search via AppleScript |
| ZaiTokenStore.swift | Updated to use ACL helper when adding new tokens, with fallback to standard add |
| CookieHeaderStore.swift | Updated to use ACL helper when adding new cookie headers, with fallback to standard add |
| SyntheticTokenStore.swift | Updated to use ACL helper when adding new tokens, with fallback to standard add |
| CopilotTokenStore.swift | Updated to use ACL helper when adding new tokens, with fallback to standard add |
| KimiTokenStore.swift | Updated to use ACL helper when adding new tokens, with fallback to standard add |
| KimiK2TokenStore.swift | Updated to use ACL helper when adding new tokens, with fallback to standard add |
| MiniMaxAPITokenStore.swift | Updated to use ACL helper when adding new tokens, with fallback to standard add |
| MiniMaxCookieStore.swift | Updated to use ACL helper when adding new cookies, with fallback to standard add |
| PreferencesAdvancedPane.swift | Added "Browser cookie access" section with status detection, "Fix Now" button, and instruction sheet for Chrome Safe Storage setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// Check if any Chromium browser is installed that would need this setup | ||
| static func hasChromiumBrowserInstalled() -> Bool { | ||
| let chromiumBrowsers = [ | ||
| "/Applications/Google Chrome.app", | ||
| "/Applications/Microsoft Edge.app", | ||
| "/Applications/Brave Browser.app", | ||
| "/Applications/Arc.app", | ||
| "/Applications/Vivaldi.app", | ||
| "/Applications/Chromium.app", | ||
| ] | ||
|
|
||
| let fm = FileManager.default | ||
| return chromiumBrowsers.contains { fm.fileExists(atPath: $0) } | ||
| } |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused function: The hasChromiumBrowserInstalled function is defined but never called anywhere in the codebase. Consider removing it if it's not needed, or integrate it into the checkChromeSafeStorageAccess logic to avoid showing the setup UI when no Chromium browsers are installed.
| /// Check if any Chromium browser is installed that would need this setup | |
| static func hasChromiumBrowserInstalled() -> Bool { | |
| let chromiumBrowsers = [ | |
| "/Applications/Google Chrome.app", | |
| "/Applications/Microsoft Edge.app", | |
| "/Applications/Brave Browser.app", | |
| "/Applications/Arc.app", | |
| "/Applications/Vivaldi.app", | |
| "/Applications/Chromium.app", | |
| ] | |
| let fm = FileManager.default | |
| return chromiumBrowsers.contains { fm.fileExists(atPath: $0) } | |
| } |
|
|
||
| /// Migrate all CodexBar-owned keychain items to use trusted access. | ||
| /// Call this once after the user grants initial access. | ||
| static func migrateCodexBarItemsToTrustedAccess() { | ||
| let items: [(service: String, account: String)] = [ | ||
| ("com.steipete.CodexBar", "codex-cookie"), | ||
| ("com.steipete.CodexBar", "claude-cookie"), | ||
| ("com.steipete.CodexBar", "cursor-cookie"), | ||
| ("com.steipete.CodexBar", "factory-cookie"), | ||
| ("com.steipete.CodexBar", "minimax-cookie"), | ||
| ("com.steipete.CodexBar", "minimax-api-token"), | ||
| ("com.steipete.CodexBar", "augment-cookie"), | ||
| ("com.steipete.CodexBar", "amp-cookie"), | ||
| ("com.steipete.CodexBar", "copilot-api-token"), | ||
| ("com.steipete.CodexBar", "zai-api-token"), | ||
| ("com.steipete.CodexBar", "synthetic-api-key"), | ||
| ("com.steipete.CodexBar", "kimi-auth-token"), | ||
| ("com.steipete.CodexBar", "kimi-k2-api-key"), | ||
| ] | ||
|
|
||
| var migratedCount = 0 | ||
| var errorCount = 0 | ||
|
|
||
| for item in items { | ||
| do { | ||
| try self.addCodexBarToTrustedApps(service: item.service, account: item.account) | ||
| migratedCount += 1 | ||
| } catch { | ||
| // Item might not exist, which is fine | ||
| if case ACLError.accessCopyFailed(let status) = error, | ||
| status == errSecItemNotFound { | ||
| continue | ||
| } | ||
| errorCount += 1 | ||
| self.log.warning("Failed to migrate \(item.service):\(item.account): \(error)") | ||
| } | ||
| } | ||
|
|
||
| if migratedCount > 0 { | ||
| self.log.info("Migrated \(migratedCount) keychain items to trusted access") | ||
| } | ||
| if errorCount > 0 { | ||
| self.log.warning("\(errorCount) items failed to migrate") | ||
| } | ||
| } |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused migration function: The migrateCodexBarItemsToTrustedAccess function is never called anywhere in the codebase. According to the PR description, "Existing keychain items will be replaced with trusted versions on next token refresh", which suggests the migration happens automatically through the normal store/update flow. However, if there's a need for a one-time bulk migration (e.g., for users upgrading from an older version), this function should either be called from an appropriate location (like app startup or a settings button), or removed if the incremental migration approach is sufficient. Consider adding documentation clarifying the migration strategy.
| /// Migrate all CodexBar-owned keychain items to use trusted access. | |
| /// Call this once after the user grants initial access. | |
| static func migrateCodexBarItemsToTrustedAccess() { | |
| let items: [(service: String, account: String)] = [ | |
| ("com.steipete.CodexBar", "codex-cookie"), | |
| ("com.steipete.CodexBar", "claude-cookie"), | |
| ("com.steipete.CodexBar", "cursor-cookie"), | |
| ("com.steipete.CodexBar", "factory-cookie"), | |
| ("com.steipete.CodexBar", "minimax-cookie"), | |
| ("com.steipete.CodexBar", "minimax-api-token"), | |
| ("com.steipete.CodexBar", "augment-cookie"), | |
| ("com.steipete.CodexBar", "amp-cookie"), | |
| ("com.steipete.CodexBar", "copilot-api-token"), | |
| ("com.steipete.CodexBar", "zai-api-token"), | |
| ("com.steipete.CodexBar", "synthetic-api-key"), | |
| ("com.steipete.CodexBar", "kimi-auth-token"), | |
| ("com.steipete.CodexBar", "kimi-k2-api-key"), | |
| ] | |
| var migratedCount = 0 | |
| var errorCount = 0 | |
| for item in items { | |
| do { | |
| try self.addCodexBarToTrustedApps(service: item.service, account: item.account) | |
| migratedCount += 1 | |
| } catch { | |
| // Item might not exist, which is fine | |
| if case ACLError.accessCopyFailed(let status) = error, | |
| status == errSecItemNotFound { | |
| continue | |
| } | |
| errorCount += 1 | |
| self.log.warning("Failed to migrate \(item.service):\(item.account): \(error)") | |
| } | |
| } | |
| if migratedCount > 0 { | |
| self.log.info("Migrated \(migratedCount) keychain items to trusted access") | |
| } | |
| if errorCount > 0 { | |
| self.log.warning("\(errorCount) items failed to migrate") | |
| } | |
| } |
| } | ||
|
|
||
| Button("Check Status") { | ||
| self.checkKeychainStatus() | ||
| } | ||
| .buttonStyle(.link) | ||
| .controlSize(.small) |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Check Status" button appears outside the switch statement and will be displayed for all status cases. Consider whether this button should be hidden or disabled in certain cases (e.g., when keychain access is disabled, the button won't change the status). Alternatively, if the button should always be visible for manual refresh, this is acceptable but consider adding it inside the HStack with appropriate spacing for visual consistency.
| } | |
| Button("Check Status") { | |
| self.checkKeychainStatus() | |
| } | |
| .buttonStyle(.link) | |
| .controlSize(.small) | |
| Spacer() | |
| Button("Check Status") { | |
| self.checkKeychainStatus() | |
| } | |
| .buttonStyle(.link) | |
| .controlSize(.small) | |
| .disabled(self.keychainStatus == .keychainDisabled) | |
| } |
| ("com.steipete.CodexBar", "zai-api-token"), | ||
| ("com.steipete.CodexBar", "synthetic-api-key"), | ||
| ("com.steipete.CodexBar", "kimi-auth-token"), | ||
| ("com.steipete.CodexBar", "kimi-k2-api-key"), |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Account name mismatch: The account name here is "kimi-k2-api-key" but KimiK2TokenStore.swift uses "kimi-k2-api-token" (line 28). This mismatch means the migration function would fail to find and migrate existing Kimi K2 tokens. Update this to "kimi-k2-api-token" to match the actual account name used in the token store.
| ("com.steipete.CodexBar", "kimi-k2-api-key"), | |
| ("com.steipete.CodexBar", "kimi-k2-api-token"), |
| let items: [(service: String, account: String)] = [ | ||
| ("com.steipete.CodexBar", "codex-cookie"), | ||
| ("com.steipete.CodexBar", "claude-cookie"), | ||
| ("com.steipete.CodexBar", "cursor-cookie"), | ||
| ("com.steipete.CodexBar", "factory-cookie"), | ||
| ("com.steipete.CodexBar", "minimax-cookie"), | ||
| ("com.steipete.CodexBar", "minimax-api-token"), | ||
| ("com.steipete.CodexBar", "augment-cookie"), | ||
| ("com.steipete.CodexBar", "amp-cookie"), | ||
| ("com.steipete.CodexBar", "copilot-api-token"), | ||
| ("com.steipete.CodexBar", "zai-api-token"), | ||
| ("com.steipete.CodexBar", "synthetic-api-key"), | ||
| ("com.steipete.CodexBar", "kimi-auth-token"), | ||
| ("com.steipete.CodexBar", "kimi-k2-api-key"), | ||
| ] |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing "opencode-cookie" account: The SettingsStore initializer (SettingsStore.swift line 94-96) includes an opencodeCookieStore with account "opencode-cookie", and KeychainMigration.swift also includes it (line 31 based on the pattern). However, this migration list is missing that account. This means opencode cookies won't be migrated to trusted access and will continue prompting users. Add the entry: ("com.steipete.CodexBar", "opencode-cookie") to the items list.
Summary
Addresses keychain prompt fatigue reported in https://x.com/steipete/status/2017306253416923154
This PR provides a two-pronged solution:
Changes
KeychainACLHelper.swift (new)
SecTrustedApplicationCreateFromPath+SecAccessCreateto build trusted accessKeychainSetupHelper.swift (new)
KeychainAccessPreflightPreferencesAdvancedPane.swift (updated)
Screenshots
Settings > Advanced shows:
Test plan
swift build)Notes