fix(kiro): support browser-based multi-account login#447
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
0c73bd1 to
b25f03c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
977ca7a to
48adb2b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Verdict
Request changes — Jun’s and the prior maintainer checklist are addressed on 48adb2b6, Cross-platform CI is green, and the branch is mergeable. Two Codex findings still need fixes in this PR before merge (auth-boundary / documented rollback). After those land and CI is green on the tip, we will re-run Cross-platform CI as needed and merge.
Required before merge (from Codex, still valid on tip)
- Refuse logout when the existing CLI session cannot be snapshotted (
src/oauth/kiro.tsforceLogin path). IfsnapshotKiroCliSession()returnsnull(e.g. unreadable / schema-mismatched DB), do not callkiro-cli logout. Abort first so a later failure cannot destroy a session the docs promise to restore. Add a schema-mismatch / unreadable-DB regression. - Do not borrow another account’s refresh region (
src/oauth/kiro.tsrefresh path). Request routing already uses account-scoped emptykiro: {}, but refresh of a credential with missingkirometadata still falls throughresolveKiroRegion(undefined)and can hit the post-switch local CLI region. After a local refresh-token mismatch, use an account-scoped empty/default metadata marker (or equivalent) so refresh does not borrow unrelated local region. Cover the cross-region upgrade / legacy-metadata path.
Explicitly not required for merge (Codex)
- Delay settlement until
saveConfig— soft; credential persist already succeeded; rolling CLI back would desync worse. - Same-PID restart vs recovery owner — rare container edge; acceptable follow-up.
Already satisfied (no rework)
- Jun: transactional snapshot/restore + cancel / callback / credential-save regressions; rebase onto
devwith BizRouter / Kiro contracts preserved. - Maintainer: conflicts cleared; account-scoped
refreshKiroToken(..., cred);preserveIdentityless/ no unscoped active-CLI overwrite; docs + locales + registry note; draft cleared; CI green on tip. - Codex P1 legacy preserve + P2 matching rotated CLI refresh — already fixed.
Process
- Security-boundary OAuth change: final maintainer security approve happens after the two fixes above + green CI.
- Then: CI green → merge.
Event: request-changes
Two read-only audit lenses returned 30 contradictions; the verified ones are applied here and recorded in 006_corrections.md. Factual corrections: - claude-code#1124 closed 2025-05-16, not 2025-08-10 - 13 open enhancement issues, not 11 - #529 is merged, so issue #42 phase 2 is done - #418 has no PR behind it; it was miscounted as in-flight - #528 does not contain #424's current head, so it needs a rebase - #527's retarget is manual and independent of #526 Framing corrections: - stale-needs-info.yml is absent from the default branch, so bundle C had no real deadline - the debug switch #543's reporter asked for already exists - whether #545 is our defect is answerable from code, not owner judgment - #491 is an OAuth credential change, so it is a security boundary rather than a warm-up; #533 and #447 keep their security classification too Structure: bundle G added for roadmap honesty, bundle F rescoped to the security boundary, #498 split out of bundle B, cycle order rebuilt, and the loop archetype corrected to decision-elicitation.
메인테이너 리뷰의 머지 차단 항목 2건을 처리한다. 1. 세션 저장소가 존재하지만 스냅샷을 만들 수 없으면(파일 판독 불가, 스키마 불일치, 토큰 선택 모호) `kiro-cli logout`을 호출하지 않고 먼저 중단한다. 문서가 약속한 정확 복원은 스냅샷에서만 가능하므로, 캡처 실패 상태에서 로그아웃하면 이후 실패 시 세션이 영구 소실된다. `inspectKiroCliSessionSnapshot()`이 `blocked`를 함께 보고하고, `missing`/`token_missing`만 "잃을 것이 없는" 상태로 취급해 최초 로그인 경로는 그대로 통과시킨다. 2. `kiro` 메타데이터가 없는 저장 크리덴셜의 refresh가 `resolveKiroRegion(undefined)`로 빠지면서 KIRO_REGION이나 로컬 CLI 가져오기를 읽어 계정 전환 후 무관한 리전을 빌려오는 경로를 막는다. 저장 크리덴셜이 있으면 account-scoped 빈 마커를 사용해 기본 리전에 고정하고, 크리덴셜이 없는 진짜 accountless refresh만 기존 env/로컬 폴백을 유지한다. 회귀 4건 추가: 스키마 불일치 저장소에서 logout 미호출, 세션 부재 시 최초 로그인 정상 진행, 레거시 메타데이터가 로컬 CLI 리전/KIRO_REGION을 빌리지 않음. 소스 수정을 되돌리면 3건이 실패하는 것을 확인했다.
|
@Wibias 머지 차단 항목 2건을 1. 스냅샷 실패 시 logout 거부 ( 세션 저장소가 존재하지만 캡처할 수 없으면
2. 다른 계정 리전 차용 차단 ( 저장 크리덴셜이 있는데 회귀 4건
소스 수정을 stash로 되돌리면 이 중 3건이 실패하는 것을 확인했습니다(47 pass / 3 fail → 50 pass / 0 fail). 검증
참고로 문서도 갱신했습니다: 스냅샷 불가 시 로그아웃을 거부한다는 내용을 en/ko/ja/ru/zh-cn |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Follow-up re-review @ b1aedd1d
Verdict: request changes for process, not for the two prior code blockers. Tip closes both merge-blocking Codex items from my last review. Before merge: rebase onto current dev, leave draft / mark ready, and keep Cross-platform CI green on the rebased tip. Maintainer security approval still follows that.
Required items from prior review
| # | Item | Status on b1aedd1d |
|---|---|---|
| 1 | Refuse logout when CLI session cannot be snapshotted | Fixed — inspectKiroCliSessionSnapshot() + blocked for unreadable / schema_mismatch / invalid_json / token_ambiguous / token_key_missing / token_found; logout only after a capturable or truly empty session. Regression: schema-mismatch DB → zero runner calls, DB preserved; empty session still proceeds. Docs mention refuse-to-sign-out when store is present but uncapturable. |
| 2 | Do not borrow another account’s refresh region | Fixed — stored credential with missing kiro metadata uses account-scoped empty {} so resolveKiroRegion pins the default instead of KIRO_REGION / local CLI import. Truly accountless refresh keeps env/local fallback. Regressions cover local CLI ap-southeast-1 and KIRO_REGION=eu-central-1. |
The two Codex items I previously marked not required (delay settlement until saveConfig; same-PID restart vs recovery owner) remain acceptable follow-ups.
Still required before merge
- Rebase onto current
dev— tip is 2 ahead / 178 behind. That is a merge blocker for this OAuth boundary PR even with the code fixes in place. - Draft → ready when the rebase is done (currently
isDraft: true; CodeRabbit is skipping for that reason). - Cross-platform CI green on the rebased tip — Ubuntu/macOS already green on this tip; finish/confirm Windows, then re-run after rebase.
- Maintainer security approval for the Kiro OAuth / CLI session mutation path.
Once 1–3 are done, re-request review; I expect to approve the code side unless rebase introduces new conflicts or regressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1aedd1dee
ℹ️ 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".
| kiro: { | ||
| ...(cred.kiro?.profileArn ? { profileArn: cred.kiro.profileArn } : {}), | ||
| ...(cred.kiro?.apiRegion ? { apiRegion: cred.kiro.apiRegion } : {}), | ||
| ...(cred.kiro?.ssoRegion ? { ssoRegion: cred.kiro.ssoRegion } : {}), |
There was a problem hiding this comment.
When login uses the documented KIRO_ACCESS_TOKEN/manual fallback, or an upgraded credential has no stored kiro metadata, this snapshot always supplies an empty account context. The adapter therefore treats the context as authoritative and ignores KIRO_API_REGION, KIRO_REGION, and KIRO_PROFILE_ARN, whereas the previous request path honored those settings. Even without any CLI account switch, such credentials are now sent to us-east-1 without their configured profile and can fail authentication; capture the environment routing metadata when persisting these credentials or retain an explicit safe fallback mode, with an end-to-end regression for the environment login path.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
| const expected = existing.accountId ?? existing.email; | ||
| const got = cred.accountId ?? cred.email; |
There was a problem hiding this comment.
A forced Kiro login is explicitly allowed to identify an account by email when its SQLite data lacks a profile ARN. If that account is later reauthenticated after the CLI starts returning both the same email and a profile ARN, expected is the stored email while got becomes the new accountId, so this rejects the same user as an identity mismatch and rolls back the login. Compare account IDs when both exist and otherwise compare emails, rather than coalescing each side independently, and add a regression for an email-only Kiro slot gaining a profile ARN.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
Follow-up re-review — current head
|
3 participants
Summary
kiro-clibrowser login instead of re-importing the cached identityRoot cause
The shared provider UI already sent
addAccount: true, but the Kiro provider ignoredforceLoginand always imported the current local cache. Kiro request and refresh metadata was also resolved globally from the current local session, which could pair one stored account's token with another account's profile or region.User impact
Using Add account for Kiro now runs
kiro-cli logoutfollowed by the supportedkiro-cli loginbrowser flow, imports only the newly authenticated CLI SQLite session, and appends the stable identity to the OCX account pool. This switches the active account used bykiro-cli, while previously stored OCX accounts remain available for switching.Verification
devat58f0fabbun run typecheckbun run privacy:scanSecurity review
This changes authentication and credential handling and therefore requires explicit maintainer security review. Client registration secrets remain confined to the protected auth-store boundary and are not returned by management APIs or attached to parsed requests.
Checklist
Summary by CodeRabbit