Skip to content

fix(kiro): support browser-based multi-account login - #447

Draft
coseung2 wants to merge 1 commit into
lidge-jun:devfrom
coseung2:fix/kiro-multiauth
Draft

fix(kiro): support browser-based multi-account login#447
coseung2 wants to merge 1 commit into
lidge-jun:devfrom
coseung2:fix/kiro-multiauth

Conversation

@coseung2

@coseung2 coseung2 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make the existing Add account action start a fresh kiro-cli browser login instead of re-importing the cached identity
  • persist Kiro profile, region, and refresh-registration metadata per OCX account so account switching cannot mix credentials
  • carry the selected account metadata through request building, refresh, and OAuth 401 replay
  • make refresh persistence generation-safe so an in-flight refresh cannot overwrite a newer reauthentication
  • keep refresh-only client secrets inside the auth store and expose only the non-secret routing subset at request time
  • document the Kiro multi-account behavior

Root cause

The shared provider UI already sent addAccount: true, but the Kiro provider ignored forceLogin and 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 logout followed by the supported kiro-cli login browser 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 by kiro-cli, while previously stored OCX accounts remain available for switching.

Verification

  • remaining review fixes consolidated into d972876 as one commit
  • added focused regressions for config-persistence rollback, same-PID process restart recovery, explicit environment routing, and email-to-profile-ARN reauthentication
  • isolated the existing local-CLI non-borrowing fixture from explicit environment routing
  • bun x tsc --noEmit
  • bun test --isolate tests
  • Cross-platform CI #1711 passed on Ubuntu, macOS, and Windows, including full tests, GUI tests/lint/build, privacy scan, release-helper syntax, CLI help smoke, and npm-global install smokes
  • React Doctor #887 passed
  • all inline review threads are resolved; explicit maintainer security approval is still required before merge

Security 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

  • 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.

Summary by CodeRabbit

  • New Features
    • Added account-scoped Kiro metadata (profile ARN / API region / SSO region) to drive Kiro request routing and OAuth refresh/replay behavior.
    • Improved forced-login so “add account” switches the installed CLI account while preserving existing identities and rolling back on cancel/failure.
  • Bug Fixes
    • Hardened token refresh/recovery to avoid cross-account/session mixing, handle terminal OAuth failures as reauthentication-required, and prevent stale refresh overwrites.
  • Documentation
    • Updated Kiro provider and credential import guidance to clearly separate first login vs add-account behavior and rollback details (multi-language docs).
  • Tests
    • Expanded Kiro adapter, OAuth store, forced-login, refresh, and 401 replay test coverage.

@github-actions github-actions Bot added the bug Something isn't working label Jul 25, 2026
@coderabbitai

This comment was marked as outdated.

chatgpt-codex-connector[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

@coseung2

This comment was marked as outdated.

@lidge-jun

This comment was marked as outdated.

@lidge-jun

This comment was marked as outdated.

@coseung2
coseung2 force-pushed the fix/kiro-multiauth branch from 0c73bd1 to b25f03c Compare July 25, 2026 18:11
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as resolved.

@Wibias
Wibias marked this pull request as draft July 26, 2026 16:20
Wibias

This comment was marked as outdated.

@coseung2
coseung2 force-pushed the fix/kiro-multiauth branch from 977ca7a to 48adb2b Compare July 26, 2026 21:04
@coseung2
coseung2 marked this pull request as ready for review July 26, 2026 21:13

This comment was marked as outdated.

chatgpt-codex-connector[bot]

This comment was marked as outdated.

Wibias

This comment was marked as outdated.

@Wibias
Wibias marked this pull request as draft July 26, 2026 23:34
lidge-jun added a commit that referenced this pull request Jul 27, 2026
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.
@coseung2

Copy link
Copy Markdown
Contributor Author

@Wibias 머지 차단 항목 2건을 b1aedd1d에서 처리했습니다.

1. 스냅샷 실패 시 logout 거부 (src/oauth/kiro.ts forceLogin)

세션 저장소가 존재하지만 캡처할 수 없으면 kiro-cli logout 이전에 중단합니다. snapshotKiroCliSession()이 단순히 null인지만 보면 "세션이 아예 없음"과 "세션이 있는데 못 읽음"을 구분할 수 없어서, inspectKiroCliSessionSnapshot()이 진단과 함께 blocked를 보고하도록 했습니다.

  • blocked 대상: unreadable, schema_mismatch, invalid_json, token_ambiguous, token_key_missing, token_found(직렬화 실패)
  • missing / token_missing만 "잃을 것이 없는" 상태로 취급 → 기존 세션이 없는 최초 로그인 경로는 그대로 통과

2. 다른 계정 리전 차용 차단 (src/oauth/kiro.ts refresh)

저장 크리덴셜이 있는데 kiro 메타데이터가 없으면 account-scoped 빈 마커 {}를 사용해 기본 리전에 고정합니다. 기존에는 resolveKiroRegion(undefined)로 빠져 KIRO_REGION 또는 로컬 CLI 가져오기를 읽었고, 계정 전환 후에는 무관한 계정의 리전으로 라우팅될 수 있었습니다. 크리덴셜 자체가 없는 진짜 accountless refresh만 기존 env/로컬 폴백을 유지합니다.

회귀 4건

  • 스키마 불일치 저장소에서 logout이 호출되지 않고 DB가 보존됨 (runner 호출 0회)
  • 세션이 전혀 없을 때 최초 강제 로그인은 정상 진행
  • 레거시 메타데이터가 로컬 CLI 리전(ap-southeast-1)을 빌리지 않음
  • 레거시 메타데이터가 KIRO_REGION을 빌리지 않되, accountless refresh는 여전히 env 폴백 사용

소스 수정을 stash로 되돌리면 이 중 3건이 실패하는 것을 확인했습니다(47 pass / 3 fail → 50 pass / 0 fail).

검증

  • tests/kiro-oauth.test.ts 50/50
  • Kiro + OAuth refresh/store 인접 스위트 합계 163 pass / 0 fail
  • bun run typecheck, bun run privacy:scan, git diff --check 통과

참고로 tests/codex-auth-modal-status.test.ts가 대량 배치 실행 시 간헐적으로 실패하는데, 이 커밋 이전 48adb2b6에서도 동일 배치를 두 번 돌려 0 fail / 2 fail로 재현됐습니다. 제 변경과 무관한 기존 순서 의존성으로 보이며, 단독 실행 시에는 항상 통과합니다.

문서도 갱신했습니다: 스냅샷 불가 시 로그아웃을 거부한다는 내용을 en/ko/ja/ru/zh-cn providers.md에 추가했습니다.

@Wibias Wibias left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FixedinspectKiroCliSessionSnapshot() + 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

  1. 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.
  2. Draft → ready when the rebase is done (currently isDraft: true; CodeRabbit is skipping for that reason).
  3. Cross-platform CI green on the rebased tip — Ubuntu/macOS already green on this tip; finish/confirm Windows, then re-run after rebase.
  4. 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.

@coseung2
coseung2 marked this pull request as ready for review July 27, 2026 15:01
chatgpt-codex-connector[bot]

This comment was marked as resolved.

Wibias commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Follow-up re-review — current head b1aedd1

I rechecked the current head and the newest Codex comments. The two previously requested fixes are present: forced login now refuses an uncapturable CLI store, and account-scoped refresh no longer borrows another local Kiro account's region. Cross-platform and npm-global CI are green.

Verdict: changes still requested. Three functional blockers remain, plus one broken recovery instruction.

1. Forced-login rollback can still snapshot the wrong database

inspectKiroCliSessionSnapshot() finds the first candidate containing a parseable credential, not necessarily the database that kiro-cli logout will mutate. An existing primary Kiro database with no currently recognized token row is classified as token_missing, treated as safe, and skipped. A later Amazon Q/Kiro fallback database can then be snapshotted instead.

If the primary CLI token schema or key changes, Add account can therefore back up a different store, run logout against the real active store, and restore the wrong database after failure.

Before logout, identify and snapshot the exact active CLI database independently of successful credential parsing. If the active store cannot be identified unambiguously, abort. Add a regression with an unrecognized primary Kiro DB and a valid later fallback DB, proving logout is never called and the primary DB remains unchanged.

2. Metadata-less credentials now lose documented routing

The newest Codex P1 is valid. accessSnapshot() emits kiro: {} for every selected Kiro account without stored metadata, so the adapter treats the empty account context as authoritative and ignores KIRO_API_REGION, KIRO_REGION, and KIRO_PROFILE_ARN.

This breaks documented KIRO_ACCESS_TOKEN / manual-token setups and can also leave a preserved pre-upgrade CLI-backed account selectable but unusable after Add account switches the local CLI identity.

Preserve the safety boundary without restoring unrelated local-CLI fallback:

  • When an environment/manual credential is created, persist its configured routing metadata with that account.
  • Before forced account switching, generation-safely backfill a matching legacy CLI-backed account from the current CLI credential when its refresh token identifies the same session.
  • Keep metadata-less stored accounts isolated from whichever unrelated account the CLI is currently using.

Add end-to-end regressions for an environment-token account with region/profile overrides and for a non-default-region legacy OIDC account that remains routable and refreshable after adding a second account.

3. Reauthentication compares mismatched identity fields

The newest Codex P2 is also valid. runLogin() currently compares:

existing.accountId ?? existing.email
cred.accountId ?? cred.email

An existing email-only Kiro slot therefore fails reauthentication when the same login later gains a profile ARN: the stored email is compared with the new account ID even when the emails still match.

Compare account IDs only when both sides have one; otherwise compare normalized emails when both are present. Reject only when corresponding available identity fields conflict. Add a regression for an email-only Kiro account gaining a profile ARN during reauthentication.

4. Recovery guidance names a nonexistent command

The backup refusal tells the user to run ocx account diagnose kiro, but diagnose is not a supported ocx account subcommand. Replace it with an actual supported diagnostic or concrete recovery instruction.

The older Codex config-settlement and same-PID-restart comments remain non-blocking for this review. The former risks worse credential/CLI desynchronization if rolled back after credential persistence; the latter is a narrow container/process-identity edge with a documented manual recovery path.

@Wibias
Wibias marked this pull request as draft July 27, 2026 15:19
@coseung2
coseung2 force-pushed the fix/kiro-multiauth branch 2 times, most recently from 5e16e3b to d972876 Compare July 27, 2026 17:09

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review findings in d972876:

  • transaction settlement now waits for both credential and config persistence; config failures roll back the Kiro CLI switch
  • recovery format v2 distinguishes same-PID process restarts with a per-process instance marker while retaining v1 compatibility
  • metadata-less credentials preserve explicit KIRO_* routing without borrowing local CLI account state
  • reauthentication compares account IDs only when both exist, otherwise falling back to case-insensitive email matching
  • isolated the existing local-CLI non-borrowing fixture from explicit environment routing

Added focused regressions for all four review paths. The review-fix delta is one commit touching only:

  • src/oauth/index.ts
  • src/oauth/kiro-credentials.ts
  • tests/kiro-adapter.test.ts
  • tests/kiro-review-regressions.test.ts

Validation:

  • typecheck and full bun test --isolate tests passed
  • Cross-platform CI #1711 passed on Ubuntu, macOS, and Windows, including GUI, privacy, build, CLI, and npm-global smoke checks
  • React Doctor #887 passed
  • all inline review threads are resolved

@coderabbitai review

@coderabbitai

This comment was marked as off-topic.

@coseung2
coseung2 marked this pull request as ready for review July 27, 2026 17:20
Rebased onto current upstream dev and includes all review fixes through d972876.
@coseung2
coseung2 force-pushed the fix/kiro-multiauth branch from d972876 to 7c181cd Compare July 27, 2026 17:27
chatgpt-codex-connector[bot]

This comment was marked as outdated.

@Wibias
Wibias marked this pull request as draft July 27, 2026 17:39

Wibias commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Re-review on current head 7c181cd: changes requested. Three real blockers remain:

  1. Forced-login rollback can target the wrong SQLite store. inspectKiroCliSessionSnapshot() chooses a store through sqliteEntries()/the import selectors, including KIROCLI_DB_PATH and KIRO_CLI_DB_FILE, while the subsequent kiro-cli logout / kiro-cli login commands mutate the CLI's own active store. With a custom selector or multiple discovered stores, OCX can snapshot and later restore a different database, leaving the actual CLI session switched or lost. Snapshot/restore must target the exact store the CLI commands mutate; otherwise reject the forced-login flow. Add a regression with two stores and an explicit custom DB selector.

  2. The preserved legacy Kiro account can become unusable. preserveIdentityless keeps the old metadata-less row when adding the new account. After kiro-cli switches identities, that row has no stable identity or account-scoped Kiro routing metadata, cannot safely borrow the new local CLI state, and reauthAccountId explicitly refuses accounts without accountId/email. It therefore remains selectable but cannot be reliably used or repaired. Migrate/bind the legacy row before switching, or mark/remove it and prevent selection. Add an end-to-end regression for adding an account on top of a legacy identity-less Kiro credential and then switching back.

  3. The recovery guidance references a nonexistent command. The error tells users to run ocx account diagnose kiro, but that command does not exist. Replace it with supported, actionable guidance, or implement and test the command plus CLI help/docs.

Not merge-ready until these are fixed.

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.

3 participants