Skip to content

fix(providers): refuse keychain restore across provider ownership (carry #3845) - #3869

Merged
lidge-jun merged 3 commits into
devfrom
codex/rt-m5-3845
Sep 7, 2026
Merged

fix(providers): refuse keychain restore across provider ownership (carry #3845)#3869
lidge-jun merged 3 commits into
devfrom
codex/rt-m5-3845

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Summary

Carries #3845 by @luvs01: restoring a provider from the keychain now rejects references that belong to another provider, while a provider's own active and pool accounts still restore. Security-sensitive (credential handling): the independent security review verdict is appended below before merge.

(carried/reimplemented from #3845; Co-authored-by trailer in the commit)

Verification

  • Regression: key-store foreign-reference rejection and own-account restore tests run in the chain-top CI.
  • Independent security review: pending (verdict will be pasted here).
  • Local checks NOT RUN by maintainer instruction.

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.

Manual review chain (integrate bottom-up; stack: null, no native stack)

Layer Branch Base Source
1 codex/rt-m1-3532 dev #3532 (Ingwannu)
2 codex/rt-m2-3840 layer 1 #3840 (chilung-cgu)
3 codex/rt-m3-3837 layer 2 #3837 (luvs01) + test isolation fix
4 codex/rt-m4-3843 layer 3 #3843 (luvs01) + same-delta fix
5 codex/rt-m5-3845 layer 4 #3845 (luvs01)
6 codex/rt-m6-2033 layer 5 #2033 (louis-tepe) reimplemented — chain top

Verification policy (maintainer instruction, this train): local test suite / typecheck / build were NOT RUN; branches pushed with --no-verify. Lower layers carry [skip ci]; the full Cross-platform CI (lane=all, Windows shards included) runs once at the chain top head and is the exact-head evidence for the cumulative tree.

Layer 5 of 6. Review this PR's diff only.


Independent security review (astra explorer, read-only, 2026-09-07) — PASS

src/providers/key-store.ts:72 accepts exactly the provider account or its slash-prefixed pool namespace. Provider-name validation excludes slashes, preventing cross-provider prefix ambiguity for valid configurations. At src/providers/key-store.ts:197, every active/pool reference is checked before any keychain read, mutation, or deletion. No alternate foreign-account path exists within restore, and restore never writes keychain passwords. Legitimate active-only, pool, and shared active/pool references remain supported. The new error contains no secret; successful restore's plaintext config serialization is existing, intentional behavior.

Chain-wide static review of all six layers (source-intent match, Co-authored-by attribution, TypeScript imports/signatures, citation-marker test trace): PASS, no blockers.


Maintainer integration decision (MAINTAINERS.md, dev-only admin integration): @lidge-jun integrates this manual chain into dev bottom-up. Exact chain-top evidence: Cross-platform CI run 34106345180 at head 6eadb1658 (lane=all: Linux 4/4, macOS 2/2 + control, Windows 6/6, gates, storage policy, api usage, keyring ×3, npm-global ×3, docker smoke, aggregate ci = success). Tested tree 7621cac89 equals the prospective merge tree of origin/dev@ece556a6e + chain top. Independent chain review PASS; #3845 security review PASS (see #3869). Local suites NOT RUN by maintainer instruction. This is maintainer integration, not self-approval. Lower-layer PR runs are skipped/cancelled by design ([skip ci]); they are not passing evidence on their own.

luvs01 and others added 3 commits September 7, 2026 18:22
(cherry picked from commit 8ef77f7)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…er marker in the same delta [skip ci]

Resolves the unresolved major finding on #3843 (discussion_r3946034145):
lastIndexOf selected the later START, its END made the whole-string strip
pair the first START with that END, and the malformed text vanished. Walk
START-delimited segments independently so a superseded or over-bound span is
emitted verbatim and only a bounded trailing span is held for the next delta.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…ip ci]

(cherry picked from commit 73abec9)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 7, 2026 09:24
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • ^dev$
  • ^preview$

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 2d73a223-d337-42cc-bd87-977baf2e7ab0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T09:28:41.648893Z 924b657 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the bug Something isn't working label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

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: 924b65799d

ℹ️ 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".

Comment thread src/providers/key-store.ts
@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 66 / 80

이 PR은 기여자 #3845(luvs01)을 릴리스 트레인 레이어 5(codex/rt-m5-3845, base codex/rt-m4-3843)로 옮긴 캐리입니다. 지금 dev HEAD의 restoreProviderKeyFromKeychain은 provider 이름과 상관없이 apiKey / pool 안의 keychain:… 참조를 읽어 평문으로 되돌린 뒤 키체인 항목을 삭제합니다. 누군가(또는 잘못된 편집이) provider A의 설정에 keychain:B를 넣으면, A에 대해 restore를 누르는 순간 B의 비밀이 A 설정에 평문으로 쓰이고 B의 키체인 항목까지 지워집니다. 유출과 파괴가 한 경로에 같이 있는 보안 버그입니다. 저장(storeProviderKeyInKeychain)은 원래 그 provider의 active 이름과 name/… pool 네임스페이스만 쓰므로, restore도 같은 소유 규칙을 강제해야 대칭이 맞습니다.

이 캐리는 keychainReferenceBelongsToProvider를 추가해 account가 name 자체이거나 name/ 접두사인 경우만 허용합니다. restore 루프에 들어가기 전에 외래 참조를 모아 하나라도 있으면 400으로 거절하고 config·키체인을 손대지 않습니다. 에러 메시지에는 비밀 값이 없고 개수만 나갑니다. PR 본문에 붙은 독립 보안 리뷰(astra explorer, 2026-09-07)도 PASS로, 슬래시 없는 provider 이름 규칙 덕분에 접두사 모호성이 없고, 읽기/삭제 전에 모든 참조를 검사한다고 확인했습니다.

테스트는 두 축입니다. (1) other에 키를 저장한 뒤 relay.apiKey = "keychain:other"로 바꿔 restore하면 ok: false / status 400, config 파일에 비밀이 없고 키체인에 other 비밀이 남으며 resolveProviderApiKey(other)는 여전히 동작. (2) 자기 active+pool 참조는 예전처럼 restore 성공. 자격 증명 경로라 우선순위가 높고, types/config 분할과도 무관합니다. 체인 top CI가 누적 증거인 정책은 동일합니다.

주의할 점은 소유 판별이 문자열 접두사라는 점입니다. provider 이름에 /가 못 들어간다는 기존 검증이 전제입니다. 그 전제가 깨지면 alicealice/foo 경계가 흔들릴 수 있으니, 이름 검증이 계속 막아주는지가 이 가드의 바닥입니다. 보안 리뷰도 그 점을 전제로 PASS를 줬습니다.

src/providers/key-store.ts keychainReferenceBelongsToProvider - active name 또는 pool name/…만 소유로 인정.

src/providers/key-store.ts restoreProviderKeyFromKeychain 사전 거절 - 외래 참조면 읽기·평문 기록·삭제 전에 400. config 불변.

tests/providers/provider-key-store.test.ts 외래 거절 + 자기 계정 restore - 유출/파괴 경로와 정상 경로를 둘 다 잠금.

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

  • 보안 리뷰 PASS를 충분 증거로 바로 머지할지, 체인 top CI 초록까지 기다릴지
  • 외래 참조가 여러 개일 때 메시지에 account 이름을 더 드러낼지(지금은 개수만 — 비밀은 안 나가서 안전)
  • 랜딩 후 원본 #3845를 바로 landed-via-maintainer로 닫을지

너의 추천
레이어 5로 머지하세요. restore 전에 소유를 막는 최소 가드이고, 테스트와 독립 보안 리뷰가 같은 결론입니다. 머지 후 원본 #3845에 Landed via #3869 at <commit> + landed-via-maintainer로 닫으세요.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants