fix(providers): refuse keychain restore across provider ownership

[luvs01]

Summary

• refuse restoreProviderKeyFromKeychain when a provider's config references a keychain account it does not own
• keep every legitimate restore working, including a provider's own active and pool accounts

storeProviderKeyInKeychain writes exactly two account shapes for a provider: <name> for the active key and <name>/<pool id> for pool entries. restoreProviderKeyFromKeychain never checked that the references it follows match the provider it was asked to restore. It reads each referenced secret, writes it back into that provider's config as plaintext, and then deletes the keychain item.

So a config where provider a carries keychain:b turns one restore of a into two problems: b's secret is disclosed as plaintext under a, and b's credential is deleted from the OS keychain while b's own config still points at the now-empty account. The refusal happens before anything is read or removed, so config and keychain are both left untouched.

This is deliberately scoped to the ownership check in one file. Threading a provider name through resolveProviderApiKey touches ~28 request-time call sites and belongs in a separate change; this one stands alone and needs no caller updates.

Verification

Run on agent/keychain-reference-owner-scope-20260907, one commit ahead of dev bf85e675484a2391b94b2135bbebe739813a9621 with nothing behind.

• bun test ./tests/providers/provider-key-store.test.ts — 8 pass, 0 fail
• bun run typecheck — passed
• bun run privacy:scan — passed
• git diff --check — passed

The new regression was confirmed to catch the defect: with src/providers/key-store.ts reverted to dev, the same file reports 7 pass / 1 fail, and passes with the fix in place. A second test asserts the ordinary own-account restore still round-trips.

No GUI change, so no screenshot applies.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• Bug Fixes
  • Provider key restoration now rejects keychain references belonging to unrelated providers or accounts.
  • Invalid restoration attempts return an error without changing configuration or deleting existing credentials.
  • Valid references for the provider and its pool accounts continue to restore successfully.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 410cca6f-a73d-4c75-8a15-538631079836

📥 Commits

Reviewing files that changed from the base of the PR and between bf85e67 and 73abec9.

📒 Files selected for processing (2)

• src/providers/key-store.ts
• tests/providers/provider-key-store.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

The restore flow now validates provider ownership of keychain references before credential access. Tests cover rejection of foreign references, preservation of secrets and configuration, and successful restoration of owned active and pool accounts.

Changes

Provider keychain restoration

Layer / File(s)	Summary
Validate keychain account ownership src/providers/key-store.ts	Lines 67–76 recognize the provider’s active and pool keychain account prefixes. Lines 198–209 reject foreign references before reading or deleting credentials and return HTTP 400 without changing configuration.
Test accepted and rejected references tests/providers/provider-key-store.test.ts	Lines 159–179 verify that foreign references preserve the configuration and owner secret. Lines 181–188 verify successful restoration for owned active and pool accounts. Line 225 contains no code change.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 73abe

Provider keychain restoration now blocks cross-provider credential references without disrupting valid owned accounts. The covered behavior preserves configuration and secrets for rejected references, with no remaining concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing keychain restoration across provider ownership boundaries.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

• ✅ All CI tests are green on my local testing.
• ✅ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

[lidge-jun]

리뷰 · 우선순위 75 / 80

이 PR은 OS 키체인에 넣어 둔 프로바이더 API 키를 다시 설정으로 되돌리는 restoreProviderKeyFromKeychain이, 자기 것이 아닌 키체인 계정을 따라가면 생기는 보안 구멍을 막는 수정입니다. 지금 dev(HEAD bf85e6754, 2.46.0 open, 최근 팁은 #3836 delivery-note privacy)에는 #3210으로 src/providers/key-store.ts의 키체인 저장/복원과 관리 라우트(src/server/management/oauth-account-routes.ts)가 이미 들어와 있습니다. 저장할 때 쓰는 계정 이름은 딱 두 가지입니다. 활성 키는 keychain:<프로바이더이름>, 풀 항목은 keychain:<프로바이더이름>/<풀id>입니다. 그런데 복원 쪽은 그 규칙이 맞는지를 검사하지 않았습니다.

복원이 하는 일은 위험합니다. 참조를 따라 키체인에서 비밀을 읽고, 그 값을 해당 프로바이더 설정의 평문 apiKey(또는 풀 키)로 다시 쓰고, 키체인 항목을 삭제합니다. 그래서 설정만 손으로 꼬아서 프로바이더 a의 apiKey가 keychain:b를 가리키게 해 둔 뒤 a를 복원하면, b의 비밀이 a 설정에 평문으로 드러나고, b의 키체인 항목은 지워지는데 b 설정은 여전히 그 빈 계정을 가리킵니다. 한 번의 복원으로 비밀 유출과 다른 프로바이더 자격 증명 파괴가 같이 일어납니다.

이 변경은 keychainReferenceBelongsToProvider를 추가해, 참조의 계정 부분이 name 자체이거나 name/으로 시작할 때만 자기 소유로 봅니다. 복원 시작 직후, 읽기·삭제 전에 자기 것이 아닌 참조가 하나라도 있으면 400으로 거절하고 설정과 키체인을 둘 다 건드리지 않습니다. 작성자가 밝힌 대로, 요청 시점의 resolveProviderApiKey에 프로바이더 이름을 끝까지 밀어 넣는 일은 호출처가 많아 이번 범위에서 빼고, 복원 경로의 소유권 검사만 단독으로 닫습니다. GUI·카탈로그·types/config 분할과는 무관합니다.

테스트도 결함을 직접 잠급니다. tests/providers/provider-key-store.test.ts에 (1) other가 키체인에 넣은 계정을 relay가 가리키게 한 뒤 복원하면 실패하고, 설정에 비밀이 쓰이지 않으며, 키체인과 other 해석은 그대로인지, (2) 자기 활성·풀 계정 복원은 예전처럼 성공하는지 두 케이스가 있습니다. 작성자 서술에 따르면 같은 파일을 dev 코드로 되돌리면 회귀 한 건만 실패하고, 수정본에서는 8 pass / typecheck / privacy:scan이 통과합니다. 파일 두 개·추가 약 53줄로 작고, base가 현재 tip과 같아서 rebase 부담도 거의 없습니다.

라인 src/providers/key-store.ts · keychainReferenceBelongsToProvider - 소유 판정은 account === name || account.startsWith(name + "/")입니다. storeProviderKeyInKeychain이 쓰는 두 형태와 맞습니다. 프로바이더 이름에 /가 들어가면 애매해질 수 있지만, 현재 저장 규약이 그 형태라 이번 픽스 범위에서는 합리적입니다.
라인 restoreProviderKeyFromKeychain · foreign 거절 - 거절이 읽기·삭제보다 앞에 있어서, 악의적/잘못된 참조가 있어도 키체인과 config가 반쯤만 바뀌는 상태가 되지 않습니다. 메시지도 참조 개수만 말해 비밀 내용을 흘리지 않습니다.
경로 resolveProviderApiKey - 요청 시점에는 여전히 참조 문자열만 보고 키체인을 읽습니다. 설정에 keychain:다른프로바이더가 남아 있으면 복원은 막히지만, 그 프로바이더로 요청을 보내면 남의 키를 쓸 수 있는 구멍은 남습니다. 작성자가 의도적으로 다음 작업으로 미룬 부분입니다.
경로 tests/providers/provider-key-store.test.ts - 교차 소유 거절과 정상 복원 회귀가 핵심을 잘 잡습니다. 풀 항목이 keychain:다른이름/풀id를 가리키는 경우도 같은 헬퍼로 거절되므로, 필요하면 그 한 케이스를 더 넣어도 되지만 필수는 아닙니다.
경로 types.ts / config.ts 분할 - 이 픽스와 무관합니다. close-don't-rebase 대상이 아닙니다.
경로 CI - hygiene·게이트가 초록인지 머지 전에 한 번 확인하면 됩니다.

메인테이너의 판단이 필요한 지점

• 이번 PR만 먼저 착지하고, resolveProviderApiKey 소유권 검사는 별도 PR로 둘지(작성자 제안)
• 손수 꼬인 keychain: 참조를 복원 거절만 할지, 관리 UI/진단에서 “소유하지 않는 참조”로 더 눈에 띄게 할지
• Store third-party provider API keys in the OS keychain (currently plaintext in config.json) #1221/feat(providers): opt-in OS keychain storage for provider API keys (#1221) #3210 키체인 열차의 보안 후속으로 빠르게 착지시킬지, 요청 경로 소유권까지 한 묶음으로 기다릴지

너의 추천
CI가 초록이면 dev에 머지하세요. 결함이 명확하고(비밀 유출 + 타인 자격 증명 삭제), 고치는 위치가 복원 한 함수로 좁으며, 회귀 테스트가 있습니다. 요청 경로 소유권은 별도 PR로 두고 이 픽스는 단독 착지하는 편이 맞습니다. 닫지 마세요.

이 댓글은 grok-bot이 작성했습니다

[lidge-jun]

Landed on dev via #3869 (merge 0719457d1; chain-top Cross-platform CI run 34106345180, all jobs green). carried (cherry-pick -x 73abec9); independent security review PASS recorded on #3869. Your authorship is preserved with a Co-authored-by: luvs01 trailer on the landed commit, so it counts toward your contribution graph. Closing this PR as superseded — thank you @luvs01!