Repository navigation
Store LLM provider API keys in the OS keychain - #96
Conversation
Keys for the cloud/local AI providers (chat + coding agent, one per saved provider) now live in the OS keychain (Electron safeStorage) via the existing secure-store, instead of plaintext in the Zustand store, localStorage, and ai-settings-cache.json. - secure-store: add "llm" ToolKind + setLlmApiKey/deleteLlmApiKey/ resolveLlmApiKey/llmSecretRef helpers (+ unit tests) - secrets IPC/preload accept the "llm" toolType (identity guard updated) - renderer stores the raw key via secrets.set and keeps only a secret://llm:<id>/apiKey reference; the API-key field never shows the key - main-process IPC resolvers (chat, ai-handlers, flow, pi-agent) resolve the ref to the real key at request time only; downstream header sites unchanged - config-cache scrubs any non-ref apiKey so no plaintext key is written to disk - new ai:fetchModels main IPC so model listing no longer fetches with the key in the renderer (also keeps it off the CSP path) - providers-only connection UI: drop the standalone Base URL/API Key/Model rows from AI + Agent settings (the provider form owns them) - one-time hydrate migration relocates existing plaintext keys into the keychain and scrubs the old locations
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAI provider API keys are now stored in the OS keychain as secret references. Existing plaintext keys migrate during hydration, IPC handlers resolve references at request time, provider settings manage masked keys, and model-list requests use a main-process handler. ChangesLLM keychain security
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProviderManager
participant OSKeychain
participant ElectronMain
participant AIProvider
User->>ProviderManager: Enter or replace API key
ProviderManager->>OSKeychain: Store provider key
OSKeychain-->>ProviderManager: Return secret reference
ProviderManager->>ElectronMain: Submit provider configuration
ElectronMain->>OSKeychain: Resolve secret reference at request time
ElectronMain->>AIProvider: Send authenticated AI request
AIProvider-->>ElectronMain: Return response
ElectronMain-->>ProviderManager: Return request result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The saved provider defines the shared connection (name/baseUrl/key); the model is now a per-surface choice on aiConfig/agentConfig, so AI Chat and the coding agent can run different models on the same provider. - SavedProvider.model is the DEFAULT model (seeds a surface on first select) - reconcileConfig re-syncs only the connection (baseUrl/apiKey), keeping each surface's chosen model; re-selecting the current provider keeps the model too - ProviderManager shows a Model picker under the switcher that writes to the surface config (setAIConfig/setAgentConfig), not the shared provider - switcher subtitle shows the surface's active model; form field relabeled "Default model"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/lib/config-cache.ts (1)
91-131: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExplicit apiKey clears are silently dropped by the
refOnlyfallback — stale/legacy keys can persist indefinitely.
refOnly("")returnsundefined, so bothaiConfig.apiKey(line 104) andagentConfig.apiKey(line 126) fall back tocurrent.*.apiKeywhenever the caller sends a non-ref value — including a legitimate explicit""clear. SinceAIConfig/AgentConfig.apiKeyare required non-optional fields andhydrateFromElectronalways ships the full config throughsaveAiSettings/saveAgentSettings,""is a routine value (keyless provider, or a migration failure that intentionally scrubs the key). The result: switching to a keyless provider, or a failed keychain migration that meant to clear a key, leaves the old cached ref (possibly a stale provider's key, or a legacy plaintext key from before this scrubbing feature existed) in the cache — and that stale value is whatresolveConfig/resolveAIConfig/resolveAiConfigfall back to for subsequent requests, contradicting user intent and the PR's own goal of scrubbing non-reference keys from the cache.🐛 Proposed fix — distinguish "not provided" (preserve) from "explicitly cleared" (scrub)
model: typeof configRecord.model === "string" ? configRecord.model : current.aiConfig?.model, - apiKey: refOnly(configRecord.apiKey) ?? current.aiConfig?.apiKey, + apiKey: typeof configRecord.apiKey === "string" + ? (refOnly(configRecord.apiKey) ?? "") + : current.aiConfig?.apiKey,model: typeof configRecord.model === "string" ? configRecord.model : current.agentConfig?.model, - apiKey: refOnly(configRecord.apiKey) ?? current.agentConfig?.apiKey, + apiKey: typeof configRecord.apiKey === "string" + ? (refOnly(configRecord.apiKey) ?? "") + : current.agentConfig?.apiKey,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/config-cache.ts` around lines 91 - 131, Update the apiKey handling in the aiConfig and agentConfig branches of the cache update function to distinguish an omitted value from an explicit clear: preserve current.aiConfig?.apiKey or current.agentConfig?.apiKey only when configRecord.apiKey is absent, but store an empty value when it is explicitly provided and is not a valid secret:// reference. Continue storing valid references unchanged, ensuring non-reference keys and explicit "" clears remove stale cached credentials.
🧹 Nitpick comments (6)
electron/lib/config-cache.ts (1)
95-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
isSecretReffromelectron/lib/secure-store.tsinstead of re-deriving thesecret://prefix check here.Both files are main-process modules, so this duplication (unlike the analogous one in the renderer's
ui.ts, which can't import main-process code) is avoidable and centralizes the ref-prefix convention in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/config-cache.ts` around lines 95 - 96, Update the refOnly helper in config-cache.ts to reuse the existing isSecretRef utility from secure-store.ts instead of checking the "secret://" prefix locally, while preserving its string-or-undefined return behavior.src/store/slices/ui.ts (1)
105-172: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSolid migration logic overall; minor asymmetry in the AI-config mirror branch.
The AI top-level mirror (lines 137-140) overwrites
nextAi.apiKeywithout settingchanged = true, unlike the equivalent agent branch (lines 156-160) which gates the update on an explicit inequality check and setschanged = true. Since callers (src/store/index.ts) only persistmigrated.ai/migrated.agentwhenchangedis true, this mirrored update could be silently discarded if no provider needed raw-key migration but the top-level field was otherwise stale.const activeAi = migratedProviders.find((p) => p.id === nextAi.activeProviderId); if (activeAi) { - nextAi = { ...nextAi, apiKey: activeAi.apiKey }; + if (nextAi.apiKey !== activeAi.apiKey) { + nextAi = { ...nextAi, apiKey: activeAi.apiKey }; + changed = true; + } } else if (nextAi.apiKey && !isKeyRef(nextAi.apiKey)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/slices/ui.ts` around lines 105 - 172, The active-provider mirror branch in migrateLlmKeysToKeychain must detect when nextAi.apiKey differs from activeAi.apiKey, update the field, and set changed = true. Match the guarded update behavior already used for nextAgent so callers persist a stale AI top-level key.electron/ipc/chat.ts (1)
35-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame duplicated secret-ref caching guard as other IPC handlers.
See consolidated comment for the shared root cause across
ai-handlers.ts,flow-handlers.ts, andpi-agent.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/chat.ts` around lines 35 - 52, The config caching logic in the affected IPC handler duplicates the secret-reference guard used elsewhere. Reuse the shared caching helper or centralized implementation for persisting AI configuration, preserving ref-only API-key storage and connection-field updates without duplicating the guard.electron/ipc/flow-handlers.ts (1)
107-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame duplicated secret-ref caching guard.
Same shared pattern as
ai-handlers.ts,chat.ts, andpi-agent.ts— see consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/flow-handlers.ts` around lines 107 - 115, Update the AI configuration caching flow surrounding saveCachedConfig in flow-handlers.ts to use the shared secret-reference caching guard established by ai-handlers.ts, chat.ts, and pi-agent.ts. Remove the duplicated inline conditional and reuse the existing shared helper or pattern, while preserving the behavior of caching only secret references and non-secret base URL/model values.electron/ipc/pi-agent.ts (1)
196-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecret-ref caching guard duplicated three times in this file alone.
The
...(req.config?.apiKey && isSecretRef(req.config.apiKey) ? { apiKey: req.config.apiKey } : {})pattern appears atpi-agent:prompt,pi-agent:approve-plan, andpi-agent:compact-now, and the same shape recurs inai-handlers.ts,chat.ts, andflow-handlers.ts. See consolidated comment for a shared-helper proposal.Also applies to: 287-295, 369-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/pi-agent.ts` around lines 196 - 204, The secret-reference caching guard is duplicated across multiple handlers and files. Extract the conditional API-key field construction into a shared helper, then reuse it in the `prompt`, `approve-plan`, and `compact-now` handlers and the corresponding caching code in `ai-handlers.ts`, `chat.ts`, and `flow-handlers.ts`, preserving the behavior of caching only valid secret references and never raw keys.electron/ipc/ai-handlers.ts (1)
125-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated secret-ref caching guard.
This "cache
apiKeyonly when it's a secret ref, else cache without it" block is repeated verbatim (with minor field variations) inchat.ts,flow-handlers.ts, and three times inpi-agent.ts. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/ai-handlers.ts` around lines 125 - 141, The secret-reference caching guard in the ai:generatePrd handler is duplicated across chat.ts, flow-handlers.ts, and pi-agent.ts. Extract the shared cache-update logic into a reusable helper, then replace each repeated guard—including this handler—with that helper while preserving the existing behavior of caching apiKey only when isSecretRef returns true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/ai-handlers.ts`:
- Around line 104-123: Add timeout handling to the ai:fetchModels handler around
its fetch call by creating an AbortController, scheduling it to abort after
roughly 10–15 seconds, and passing its signal to fetch. Ensure the timer is
cleared when the request settles while preserving the existing response parsing
and error handling.
In `@electron/lib/secure-store.ts`:
- Around line 322-336: Update resolveLlmApiKey so its literal-key branch applies
the same isPlaceholder and containsPlaceholder guards used by setSecret and
getSecretByRef. Return an empty string for placeholder values, while preserving
literal non-placeholder keys and unresolved secret references as currently
handled.
In `@src/components/settings/endpoint-components.tsx`:
- Around line 111-121: Update the fallback model-fetch flow surrounding the
fetch call in the endpoint component to enforce a request timeout, using the
same timeout mechanism and duration established for the corresponding flow in
ai-handlers.ts. Ensure timed-out requests are aborted and retain the existing
response parsing and model filtering behavior.
- Around line 96-134: Restore stale-request protection in fetchModels by adding
a generation/sequence counter that advances for each invocation and checking it
after every asynchronous fetchModels resolution, including success and error
handling. Ignore updates from any call whose generation is no longer current so
older requests cannot overwrite availableModels, testState, testError, or
loading state; keep the guard effective for both the Electron IPC and renderer
fetch paths.
In `@src/components/settings/ProviderManager.tsx`:
- Around line 138-160: Wrap the async save flow in the onSave handler with error
handling covering secrets.set, secrets.delete, and subsequent updates. On any
keychain failure, prevent the save from being treated as successful, provide
visible feedback through the existing UI mechanism, and ensure the newly created
provider does not remain as an empty-key record; only complete the normal
editing-close flow after all persistence operations succeed.
---
Outside diff comments:
In `@electron/lib/config-cache.ts`:
- Around line 91-131: Update the apiKey handling in the aiConfig and agentConfig
branches of the cache update function to distinguish an omitted value from an
explicit clear: preserve current.aiConfig?.apiKey or current.agentConfig?.apiKey
only when configRecord.apiKey is absent, but store an empty value when it is
explicitly provided and is not a valid secret:// reference. Continue storing
valid references unchanged, ensuring non-reference keys and explicit "" clears
remove stale cached credentials.
---
Nitpick comments:
In `@electron/ipc/ai-handlers.ts`:
- Around line 125-141: The secret-reference caching guard in the ai:generatePrd
handler is duplicated across chat.ts, flow-handlers.ts, and pi-agent.ts. Extract
the shared cache-update logic into a reusable helper, then replace each repeated
guard—including this handler—with that helper while preserving the existing
behavior of caching apiKey only when isSecretRef returns true.
In `@electron/ipc/chat.ts`:
- Around line 35-52: The config caching logic in the affected IPC handler
duplicates the secret-reference guard used elsewhere. Reuse the shared caching
helper or centralized implementation for persisting AI configuration, preserving
ref-only API-key storage and connection-field updates without duplicating the
guard.
In `@electron/ipc/flow-handlers.ts`:
- Around line 107-115: Update the AI configuration caching flow surrounding
saveCachedConfig in flow-handlers.ts to use the shared secret-reference caching
guard established by ai-handlers.ts, chat.ts, and pi-agent.ts. Remove the
duplicated inline conditional and reuse the existing shared helper or pattern,
while preserving the behavior of caching only secret references and non-secret
base URL/model values.
In `@electron/ipc/pi-agent.ts`:
- Around line 196-204: The secret-reference caching guard is duplicated across
multiple handlers and files. Extract the conditional API-key field construction
into a shared helper, then reuse it in the `prompt`, `approve-plan`, and
`compact-now` handlers and the corresponding caching code in `ai-handlers.ts`,
`chat.ts`, and `flow-handlers.ts`, preserving the behavior of caching only valid
secret references and never raw keys.
In `@electron/lib/config-cache.ts`:
- Around line 95-96: Update the refOnly helper in config-cache.ts to reuse the
existing isSecretRef utility from secure-store.ts instead of checking the
"secret://" prefix locally, while preserving its string-or-undefined return
behavior.
In `@src/store/slices/ui.ts`:
- Around line 105-172: The active-provider mirror branch in
migrateLlmKeysToKeychain must detect when nextAi.apiKey differs from
activeAi.apiKey, update the field, and set changed = true. Match the guarded
update behavior already used for nextAgent so callers persist a stale AI
top-level key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e963f75-1cc2-4955-a73e-47ba5e9af769
📒 Files selected for processing (16)
changelogs/v2.5.9.mdelectron/ipc/ai-handlers.tselectron/ipc/chat.tselectron/ipc/flow-handlers.tselectron/ipc/pi-agent.tselectron/ipc/tools.tselectron/lib/config-cache.tselectron/lib/secure-store.test.tselectron/lib/secure-store.tselectron/preload.tssrc/components/settings/AISettings.tsxsrc/components/settings/AgentSettings.tsxsrc/components/settings/ProviderManager.tsxsrc/components/settings/endpoint-components.tsxsrc/store/index.tssrc/store/slices/ui.ts
Mirror the licenses.json pattern for the What's New feature registry: - scripts/features.config.js: curated FEATURES source of truth - scripts/generate-features.js: validates + writes src/generated/new-features.json (unique ids, required fields, version shape, non-empty highlights) - new-features-registry.ts reads the generated JSON instead of a hardcoded array - run the generator in build.js, the dev script, and CI + release workflows (next to generate-licenses.js) - AGENTS.md: rule to add major user-facing features to features.config.js - PR template: checklist item for adding a major-feature registry entry - fix stale entry: v2.1.0 "Radial Knowledge Graph" (actually a refactor; radial shipped in v0.7.0) -> v2.1.1 "Local Semantic Search"
- ai:fetchModels + renderer fallback fetch now time out after 12s (AbortController) - fetchModels: stale-request guard via a generation counter (no overwrite by a slower older request); web fallback aborts on timeout - resolveLlmApiKey: reject unfilled placeholders on the literal-key path - ProviderManager: surface keychain-save failures inline, keep the form open, and delete a half-created provider on failure (no orphaned empty-key record) - config-cache: distinguish an omitted apiKey (preserve) from an explicit non-ref value (clear stale cached credential); scrub per-provider keys - migrateLlmKeysToKeychain: guard the active-AI mirror so a refreshed top-level key is actually persisted (changed=true), matching the agent branch - dedupe the ref-only cache guard into cacheLlmConnection(), reused by chat / ai-handlers / flow / pi-agent Skipped: reusing isSecretRef from secure-store in config-cache — secure-store imports electron (safeStorage) and config-cache must load in the MCP runtime.
src/generated/new-features.json is git-ignored and imported by app code that the unit tests load (complete-onboarding → new-features-registry), so the Unit and E2E jobs failed with ERR_MODULE_NOT_FOUND. - npm test now runs generate-licenses + generate-features before compile/vitest, so it's self-sufficient locally and in CI - E2E job generates both JSON files before test:e2e (Playwright builds the app)
1 participant
What does this PR do?
Moves the AI provider API keys off plaintext storage and into the OS keychain. Keys for the cloud/local AI providers (chat + coding agent, one per saved provider) are now stored encrypted via the existing
safeStorage-backed secure-store (the same one used for MCP/service credentials) instead of in the Zustand store,localStorage, andai-settings-cache.json. The renderer only ever holds an opaquesecret://llm:<id>/apiKeyreference; the raw key is resolved in the main process at request time.Type of change
Screenshots / recording
Checklist
npm run type-check:allpassesnpm run lintpassesnpm testpasses (runsnpm run compilefirst soelectron/bundle-guard.test.tsactually executes —npm run test:bundleto run just that)npm run test:e2epasses (run before merging UI changes or cutting a release)var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)handle()and returnIpcResult<T>(ai:fetchModels; thesecrets:*channels already existed)dependencies/devDependencies— n/a (none added)--external:<pkg>flag changes — n/aNotes for reviewer
Depends on the saved-providers work (PR #95, already merged). Design:
"llm"ToolKindinelectron/lib/secure-store.ts, plussetLlmApiKey/deleteLlmApiKey/resolveLlmApiKey/llmSecretRef. The genericsecrets:set|has|deleteIPC + preload bridge now accept"llm"(identity guard updated). No renderer-facingget— decryption is main-process only, matching the existing pattern.chat.tsresolveAIConfig,ai-handlers.tsresolveConfig,flow-handlers.tsresolveAiConfig,pi-agent.ts×3) — so the ~7 downstreamAuthorization: Bearerheader-build sites are unchanged. The key exists only in main memory for the duration of a request.config-cache.tsscrubs anyapiKeythat isn't asecret://ref (top-level and per-provider), so a raw key can never be written toai-settings-cache.json.secrets.set("llm", id, "apiKey", raw)and keeps only the returned ref; the field shows•••• stored — type to replaceand never renders the key. Delete clears the keychain entry.ai:fetchModelsmain IPC resolves the ref and does the/v1/modelsrequest in main, so the key isn't used in a rendererfetch(also keeps it off the CSP path).useEndpointConfig.fetchModelsroutes through it, with a renderer-fetch fallback for the web/no-electron context.migrateLlmKeysToKeychainruns once duringhydrateFromElectron: it relocates any existing plaintext key (legacy top-level or per-provider) into the keychain, replaces it with a ref, and re-persists the scrubbed configs. If the keychain is unavailable, the raw key is dropped rather than left in plaintext.Summary by CodeRabbit
Security
Improvements