Repository navigation
fix(rate-limits): keep the OpenCode Go key in a main-owned store - #22630
kespineira wants to merge 2 commits into
Conversation
stablyai#22551 stores the OpenCode Go key in GlobalSettings, so settings:get, settings:get-sync and settings:changed hand the decrypted key to the renderer. Move it into a main-owned encrypted file behind write-only opencodeGoCredentials IPC that only reports whether a key is saved. The store is shared with the MiniMax API key without changing its on-disk format. Migrate a key saved under stablyai#22551 once at desktop startup. Store load keeps the sealed value on disk for every consumer, orcad included, and drops it only after the new store holds a key. Resolve the credential table before auth.json, since OpenCode 2 stops writing auth.json after its one-time import. Invalidate and refresh OpenCode Go usage after a key save or clear and after cookie or workspace changes. Skip an undecryptable saved key so the other sources still run. Refs stablyai#22512
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe OpenCode Go API key is now stored outside global settings in a provider-specific file store. Legacy keys are retained as sealed values and migrated to the store. Desktop IPC and settings UI provide status, save, and clear operations. Rate-limit resolution now checks the credential database before Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to A saved API key could be exposed if file hardening fails while encryption is unavailable. Make credential saves fail closed before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the user impact, implementation, motivation, migration, testing, security, and compatibility considerations. However, the change includes UI behavior and the required before/after visual proof is missing. The linked issue also uses "Refs Resolution Attach before and after screenshots or a video for the Settings UI change. Use the required linked-issue format, such as "Fixes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a62beff1-8ebb-4016-8b84-4eb9ef6ba4bf
📒 Files selected for processing (45)
src/main/credentials/encrypted-api-key-file-store.tssrc/main/ipc/credential-change-rate-limit-refresh.test.tssrc/main/ipc/credential-change-rate-limit-refresh.tssrc/main/ipc/minimax-credentials.tssrc/main/ipc/opencode-go-credentials.test.tssrc/main/ipc/opencode-go-credentials.tssrc/main/ipc/register-core-handlers/register-core-handlers.test.tssrc/main/ipc/register-core-handlers/register-core-handlers.tssrc/main/minimax/minimax-api-key-store.tssrc/main/opencode/opencode-go-api-key-store.test.tssrc/main/opencode/opencode-go-api-key-store.tssrc/main/persistence/applying-settings/settings-update.tssrc/main/persistence/applying-settings/terminal-settings-migrations.tssrc/main/persistence/leasing-ssh-ptys/secret-validation.tssrc/main/persistence/loading-store/legacy-opencode-go-api-key-migration.test.tssrc/main/persistence/loading-store/legacy-opencode-go-api-key-migration.tssrc/main/persistence/loading-store/loaded-state-parsing.tssrc/main/persistence/loading-store/state-serialization-secret-handling.tssrc/main/persistence/loading-store/state-write-round-trip.test.tssrc/main/persistence/loading-store/store.tssrc/main/protected-secret-persistence.tssrc/main/rate-limits/opencode-go-api-key-source.test.tssrc/main/rate-limits/opencode-go-api-key-source.tssrc/main/rate-limits/service-opencode-go-credentials.test.tssrc/main/rate-limits/service-refresh-orchestration.test.tssrc/main/rate-limits/service/service-account-refresh.tssrc/main/rate-limits/service/service-configuration.tssrc/main/rate-limits/service/service-fetch-targets.tssrc/main/rate-limits/service/service-full-cycle-preparation.tssrc/main/rate-limits/service/service-state.tssrc/main/rate-limits/service/service-types.tssrc/main/startup/main-process-account-services.tssrc/preload/api-types.tssrc/preload/api/opencode-go-credentials-bridge.tssrc/preload/index.tssrc/preload/opencode-go-credentials.test.tssrc/renderer/src/components/settings/accounts-pane-opencode-credentials.test.tssrc/renderer/src/components/settings/accounts-pane-opencode-credentials.tsxsrc/renderer/src/components/settings/accounts-pane-provider-setting-sections.tsxsrc/renderer/src/web/preload-api/web-agent-accounts-api.tssrc/renderer/src/web/web-preload-api-agent-providers.test.tssrc/renderer/src/web/web-preload-api-composition.test.tssrc/renderer/src/web/web-preload-api.tssrc/shared/default-global-settings.tssrc/shared/global-settings-types.ts
💤 Files with no reviewable changes (4)
- src/main/persistence/leasing-ssh-ptys/secret-validation.ts
- src/main/persistence/applying-settings/settings-update.ts
- src/shared/global-settings-types.ts
- src/shared/default-global-settings.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — a couple of rough edges in the new store worth a look.
Reviewed changes
- New generic encrypted store
encrypted-api-key-file-store.ts— safeStorage envelope under~/.orca, now backing both OpenCode Go and MiniMax; the MiniMax port is byte-identical (file name, prefix, messages, warnings, cache isolation). - Write-only IPC
opencodeGoCredentials:getStatus|saveApiKey|clearApiKeyreturns only{apiKeyConfigured}; preload bridge added and the web client fails closed. No reader channel exists. - Migration of the #22551 settings-slot key: the field is parked sealed at load (
retainLegacyOpenCodeGoApiKey), moved into the new store at desktop startup, and written back verbatim until it succeeds. - Settings removal:
opencodeGoApiKeyleavesGlobalSettings, defaults,PROTECTED_SECRET_SLOT, and secret validation; it is stripped from old-client updates. - Precedence change: the
credentialtable now outranksauth.json(OpenCode 2 imports auth.json once). - Refresh/invalidation: shared
refreshAfterCredentialChange, generation-guardedonApiKeyResolved, and anapiKeyErrorthat only replaces anunavailableresult.
ℹ️ A retained legacy plaintext value is re-emitted as plaintext
The retention design writes the parked blob back byte-for-byte, which is correct for a safeStorage-encrypted value. But #22551 also accepted a raw sk-… value that failed to decrypt, and this path parks that plaintext string and re-emits it verbatim. On a desktop launch the migration immediately moves it into the encrypted store, so the window is small — but a profile that only ever runs a Store consumer (orcad) never runs that migration, and unlike the old load path it will no longer re-encrypt the value on the next save. If that is intended for the legacy-plaintext case, ignore this.
Technical details
# Legacy plaintext is retained, not re-encrypted
## Affected sites
- `src/main/persistence/loading-store/legacy-opencode-go-api-key-migration.ts:39-41` — `retainSealed` parks the raw settings value, plaintext included.
- `src/main/credentials/encrypted-api-key-file-store.ts` / `src/main/persistence/protected-secret-persistence.ts:79-81` — serialization calls `encrypt(slot, '')`, whose `!plaintext && this.sealedSlots.has(slot)` branch returns the retained blob as-is.
- Desktop-only `store.migrateLegacyOpenCodeGoApiKey(...)` in `src/main/startup/main-process-account-services.ts:132`; orcad and other `Store` consumers skip it.
## Required outcome
- A `sk-…` legacy plaintext value should not be persisted in cleartext indefinitely on any profile. Either re-encrypt it while parked, or accept it explicitly as a documented degraded state.
## Suggested approach (optional)
- Park the ciphertext only for the encrypted case; for the accepted-legacy-plaintext case (`isLegacyPlaintextOpenCodeGoApiKey`), encrypt before retaining so the re-emitted blob is sealed regardless of which consumer loads the profile.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Keep the legacy key when the saved store file exists but this build cannot read it, since the store is machine-wide and safeStorage keys follow the app identity. Write the store durably so it survives the same power loss as the state write that drops the legacy field. Report a transient read failure as unreadable rather than as a key to re-enter, and skip the saved key for that cycle. Seal a legacy plaintext key before parking it so orcad-only profiles don't keep it in cleartext. Refs stablyai#22512
|
@pullfrog the legacy-plaintext note from your review is fixed in 515c342. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Follow-up commit 515c34288b hardening the OpenCode Go key store and the #22551 legacy migration. Both findings from the previous Pullfrog review are addressed.
- Durable credential write —
save()now passes{ durable: true }to bothwriteSecureFilecalls, so the new key is fsynced before the durable state write drops the legacy field. - Read vs. decrypt failure split —
read()distinguishes anisUnreadableErrorreadFileSyncfailure from a decode/decrypt failure and throws a newApiKeyFileUnreadableError. The OpenCode Go resolver treats it as transient (apiKeyReadSkipped): it skips the saved key for the cycle, keepsopencodeGoApiKeyConfiguredtrue, and shows no "re-enter or clear" message. - Legacy plaintext is sealed on retention —
retainLegacyOpenCodeGoApiKeynow encrypts ask-/oc_sk_plaintext value before parking it (degrading to plaintext only when the secret store has no encryption), so it is no longer re-emitted in cleartext by a consumer that never runs the desktop migration. - Unreadable store probe in migration —
migrateLegacyOpenCodeGoApiKeyreads an existing store file before trusting it; if this build cannot read or decrypt it, the legacy value stays on disk.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the hardening commit 515c342 since the prior pullfrog review; the credential-boundary port itself was covered previously.
- Sealed legacy plaintext —
retainLegacyOpenCodeGoApiKeynow re-encrypts an accepted#22551sk-/oc_sk_plaintext value throughprotectedSecrets.encryptbefore parking it, so aStore-only consumer such as orcad no longer writes it back in cleartext. When encryption is unavailable it is kept verbatim, matching#22551's limit, and the desktop migration still recovers it (covered by the new test). - Durable store writes — both
writeSecureFilecalls in the key store'ssave()pass{ durable: true }, so the new key is fsynced before the durable state write drops the legacy field. - Transient read failure — a failed
readFileSyncnow throwsApiKeyFileUnreadableError("API key file could not be read"), distinct from a decode/decrypt failure; the OpenCode Go resolver skips the saved key for that cycle, shows no "re-enter or clear" message, and keeps the configured flag. - Machine-wide store guard — the migration reads an existing store file and keeps the legacy settings key when this build cannot read another identity's file.
I traced both the encrypted-blob and accepted-plaintext retention paths through buildStateToSave and migrateLegacyOpenCodeGoApiKey and found no path that strands or misclassifies a key; the affected suites (legacy-opencode-go-api-key-migration, opencode-go-api-key-store, minimax-api-key-store, service-opencode-go-credentials, state-write-round-trip) pass with 38 tests.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 04ccd4f4-542c-4e8c-b84d-7edfdb89c27c
📒 Files selected for processing (12)
src/main/credentials/api-key-file-unreadable-error.tssrc/main/credentials/encrypted-api-key-file-store.tssrc/main/minimax/minimax-api-key-store.test.tssrc/main/opencode/opencode-go-api-key-store.test.tssrc/main/persistence/loading-store/legacy-opencode-go-api-key-migration.test.tssrc/main/persistence/loading-store/legacy-opencode-go-api-key-migration.tssrc/main/persistence/loading-store/state-write-round-trip.test.tssrc/main/rate-limits/service-opencode-go-credentials.test.tssrc/main/rate-limits/service/service-fetch-targets.tssrc/main/rate-limits/service/service-full-cycle-preparation.tssrc/main/rate-limits/service/service-types.tssrc/main/startup/main-process-account-services.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/rate-limits/service/service-types.ts
- src/main/startup/main-process-account-services.ts
- src/main/persistence/loading-store/state-write-round-trip.test.ts
- src/main/opencode/opencode-go-api-key-store.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
|
Superseded by merged #24615, which preserves the complete credential-storage change and credits @kespineira and @kevimux. The replacement keeps secret overrides in the main process, preserves retryable migration, refreshes usage when credentials change, and selects automatic credentials according to the execution backend. It passed fresh focused validation, independent review and public CI before merge. Selected-account caller integration remains in #24770. |
ELI5
#22551 made Orca read OpenCode Go usage with your API key. It saved that key inside Orca's general settings, and the app's UI layer can read those settings. This PR puts the key in a separate locked box that only Orca's main process can open. The Settings screen can put a key in or take it out, and can only ask whether a key is there, never read it. It also moves a key you already saved into that box, fixes which saved key Orca picks when OpenCode 2 has a newer one, and refreshes the bar as soon as you change a credential.
What Changed
This is a follow-up to #22551. It ports the credential boundary from #22514 onto main at #22551's author's request.
Before (main after #22551):
GlobalSettings.opencodeGoApiKey, sealed throughPROTECTED_SECRET_SLOT.settings:get,settings:get-syncandsettings:changedhand the decrypted key to the renderer, and the field is atype="password"input bound to it.auth.jsonoutranks OpenCode'scredentialtable.After:
src/main/credentials/encrypted-api-key-file-store.tsis a generic encrypted file store under~/.orca(safeStorage,writeSecureFile, versioned envelope).opencode-go-api-key-store.tsuses it, and MiniMax's API-key store now does too, with a byte-identical file name, envelope and messages. Its existing tests pass unchanged.opencodeGoCredentials:getStatus|saveApiKey|clearApiKeyreturns only{ apiKeyConfigured }. The Settings field shows Saved/Not saved plus Save and Clear, and never echoes the key. The web client has no such IPC and fails closed.opencodeGoApiKeyleavesGlobalSettings, the defaults,PROTECTED_SECRET_SLOT, serialization, secret validation and settings-update. A key saved under fix(rate-limits): read OpenCode Go usage with the Go API key #22551 is moved once at desktop startup. Store load parks the sealed value without decrypting it and keeps writing it back, so orcad and any otherStoreconsumer keep it on disk without ever holding the plaintext. It is dropped only after the new store holds a key. If safeStorage is unavailable, the save throws, or decryption fails, the sealed value is kept and the next startup retries.credentialtable, thenauth.json, thenOPENCODE_API_KEY. OpenCode 2 importsauth.jsononce and then writes only to SQLite, so a key re-connected in v2 was being shadowed by a staleauth.json. On 1.x the table exists but is empty, soauth.jsonstill wins there.invalidateOpenCodeGoCredentialState()and thenrefresh(). The generation bump drops a result fetched with the old credential. The configured flag is reset only when the key is cleared.src/main/ipc/credential-change-rate-limit-refresh.tsis now shared with MiniMax.auth.json, env and cookie still run. Its "re-enter or clear" error only replaces anunavailableresult, so a real cookie error stays visible.Why
type="password"hides the value on screen, but the renderer still holds it in memory and in every settings broadcast. MiniMax already stores its API key main-side behind write-only IPC. This applies the same boundary to OpenCode Go and moves both onto one store instead of two copies.I rejected keeping the settings slot with redaction. Every settings read path (IPC, remote
settings.get,settings:changed, dashboard and popout listeners) would need its own filter, and any path that got missed would leak the key.Linked Issue
Refs #22512. That issue was closed by #22551, and this follows up on it.
Visual Proof
Before: after #22551, the Settings field is a password input that holds the saved key.
After, with a fresh profile and nothing saved in Settings (the key is auto-detected from OpenCode's credential table):
Settings > Accounts > OpenCode Go with the write-only field:
Testing
I ran
pnpm devon macOS withORCA_BACKGROUND_LAUNCH=1and an empty user-data profile, then drove it over CDP. The usage popover and status bar showed OpenCode Go's 5h, weekly and 30-day windows, and the Settings field showed "Not saved". I didn't click Save by hand, because~/.orcais machine-wide and would overwrite my own key. The save and clear paths are covered by the IPC and store tests.New or updated tests:
encrypted-api-key-file-storeviaopencode-go-api-key-store.test.ts, plus the unchangedminimax-api-key-store.test.tsopencode-go-credentials.test.ts(IPC) andpreload/opencode-go-credentials.test.ts(exposed surface)legacy-opencode-go-api-key-migration.test.ts: retention when unavailable, on a save throw and on decrypt failure; an orcad-style load and flush that keeps the value; release after a successful save; idempotence. Plus aStoreround trip instate-write-round-trip.test.tsopencode-go-api-key-source.test.ts: the credential table beats a differentauth.jsonkeyservice-opencode-go-credentials.test.ts: stale in-flight result dropped, chip kept across a cookie change, unreadable key falls through, cookie error not overwrittencredential-change-rate-limit-refresh.test.ts, the web preload composition tests, andaccounts-pane-opencode-credentials.test.tsLocal checks on macOS:
pnpm lintandpnpm tcpass.pnpm run check:code-quality:changedpasses.pnpm testacrosssrc/main/{persistence,orcad,rate-limits,ipc,credentials,opencode,opencode-usage,minimax,startup},src/preload,src/shared,src/renderer/src/{components/status-bar,components/settings,web}and the proxy fetch audit: 1765 files, 17617 tests passed. I ran it withOPENCODE_API_KEY=sk-should-not-leak, and the sentinel never appears in the output.I didn't run full
pnpm testorpnpm buildon this branch, so CI covers the rest.AI Disclosure
I coordinated this with Orca orchestration. Claude (
claude-opus-5-5) wrote the port and the review fixes. Claude (claude-fable-5-1) reviewed it twice: the first pass found a migration that could drop the key, and the second confirmed the fix. Claude Code verified the OpenCode 2 credential table on a real install, ran the manual test and re-ran the checks. I reviewed the diff.Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
GlobalSettings, so no settings read, broadcast or remotesettings.getcarries it. The migration never decrypts outside the desktop startup path, and orcad keeps the sealed value without the plaintext. Log lines are fixed strings.opencodeGoApiKeyhas it stripped on update, so it is never persisted or broadcast.opencodeGoApiKeyConfiguredis unchanged from fix(rate-limits): read OpenCode Go usage with the Go API key #22551.~/.orcais shared by every user-data directory on the machine, like the MiniMax store. If two profiles each saved a different key under fix(rate-limits): read OpenCode Go usage with the Go API key #22551, the first to start wins, and the second logs that its key was dropped.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred).lintandtypecheckpass locally, and so do the affected test suites. I didn't run fulltestandbuildhere.