fix(oauth): stop OAuth login from deleting a stored provider API key#491
fix(oauth): stop OAuth login from deleting a stored provider API key#491wonsh42 wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesOAuth credential preservation
Release version
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
wonsh42
changed the title
github-actions
Bot
changed the title
|
@lidge-jun 수정해주삼 |
wonsh42
changed the title
github-actions
Bot
changed the title
|
ㄱㄷㄱㄷ 지금 클로드 데스크탑이랑 그록 작업중 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ㄳㄳ 나 또 버그(?) 발견했는데 수정해서 pr 올려드림 |
클로드 데스크톱도 되게 할꺼임? cli만 돼서 불편하던데 (내가 기능을 못찾았던건가?) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
wonsh42
changed the title
8eca4eb to
73aa7f2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/oauth/index.ts (1)
618-631: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve key mode when
authModeis omitted
src/providers/api-keys.tsnever writesauthModefor API-key providers, so persistedxai/github-copilotentries can legitimately have an undefined mode. Insrc/oauth/index.ts:628-629, the strictexisting.authMode === "key"check then skips restoration and the provider falls back to the OAuth preset, even though a validapiKey/apiKeyPoolis present.Treat an omitted
authModeas key-mode here, e.g. restore"key"whenstoredApiKeyexists andexisting.authModeis either"key"orundefined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oauth/index.ts` around lines 618 - 631, The key-mode restoration in upsertOAuthProvider currently requires existing.authMode to equal "key", so persisted API-key providers with an omitted mode lose their key authentication. Update the condition in upsertOAuthProvider to set next.authMode to "key" when storedApiKey exists and existing.authMode is either "key" or undefined, while preserving other provider behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/oauth/index.ts`:
- Around line 618-631: The key-mode restoration in upsertOAuthProvider currently
requires existing.authMode to equal "key", so persisted API-key providers with
an omitted mode lose their key authentication. Update the condition in
upsertOAuthProvider to set next.authMode to "key" when storedApiKey exists and
existing.authMode is either "key" or undefined, while preserving other provider
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e42901d8-dbc9-4d27-a5d7-3748cd2e5e89
📒 Files selected for processing (2)
src/oauth/index.tstests/oauth-upsert-preserves-api-key.test.ts
There was a problem hiding this comment.
Verdict
Request changes — do not merge until the PR is rebased/merged onto current dev and the package.json conflict is resolved without regressing the published version. Optional follow-ups are not accepted for this PR; all quality items below must ship in this PR before merge.
Summary
Target branch is correctly dev. The OAuth upsert fix itself is sound and well-tested: for allowKeyAuthOverride providers (xai, github-copilot), upsertOAuthProvider now preserves sanitized apiKey / apiKeyPool and keeps an explicit authMode: "key" only while a valid active key remains, matching router.ts's authMode === "key" gate. The follow-up commit correctly restores OAuth mode after final key removal. GitHub currently reports mergeable: CONFLICTING, mergeStateStatus: DIRTY.
Blocking findings
- Merge conflict against current
dev(mergeable=CONFLICTING,mergeStateStatus=DIRTY).- Conflicted file (verified via local merge of
e0debe22intodev@58f0fab):package.jsononly. - Conflict hunk:
dev:"version": "2.7.41"- this PR:
"version": "2.7.40"
src/oauth/index.tsandtests/oauth-upsert-preserves-api-key.test.tsapply cleanly.- Required: rebase or merge latest
devinto this branch and resolve in this PR before merge. When resolving, keep2.7.41(currentdev) — do not downgrade to2.7.40. Prefer dropping therelease: v2.7.40/ version-bump commit from this bugfix entirely; version bumps belong to maintainer release promotion, not a contributor OAuth fix.
- Conflicted file (verified via local merge of
Additional required quality changes (fix in this PR before merge)
- Drop / neutralize the version bump. The three-dot diff still changes
package.json2.7.35 → 2.7.40via commit66de25ad(release: v2.7.40). That is out of scope for this fix and is what conflicts withdev's2.7.41. Resolve by takingdev's version (no version change in this PR). - Re-run full CI on the rebased tip. Latest Cross-platform CI for
e0debe22isaction_required(fork PR), not a green suite on the current tip. After rebase/conflict resolution, ensure Cross-platform CI / typecheck / tests pass on the mergeable SHA (earlier success on8eca4ebdoes not cover the follow-up commit + rebase). - Omitted
authModewith a stored key (CodeRabbit / related).src/oauth/index.ts:629only restoresauthMode: "key"whenexisting.authMode === "key". Fix in this PR so a preserved/sanitized active key (or equivalent key material that should keep key billing) also results in explicitauthMode: "key"when mode was omitted, and align withrouter.ts/ account semantics as needed so key override is not lost. Cover with a focused regression test. - Unsafe active key + valid pool alternate (
tests/..."rejects an unsafe active key…"): today this drops the active key, keeps the pool, and falls back to oauth. In this PR, promote the first safe pool entry intoapiKeyand keep key mode when a valid pool alternate exists (instead of falling back to oauth while pool keys remain usable). - Share / reuse sanitization helpers with
src/providers/api-keys.tsCRLF/trim checks where practical on this write path, rather than leaving a parallel defensive copy as follow-up polish.
Optional / non-blocking
None. Do not defer quality work to a follow-up PR; ship the items above in this PR.
Standards / Spec / Security
- Standards: Focused Bun-native change with a dedicated regression suite adjacent to the OAuth/key surface. Hard violation: shipping a release version bump inside a bug-fix PR (and conflict-regressing
2.7.41 → 2.7.40). No Node-only APIs introduced. - Spec: Matches the PR-stated bug (OAuth login must not delete stored keys / flip explicit key billing for
allowKeyAuthOverrideproviders). OAuth-only providers still get a plain preset overwrite. No linked GitHub issue; PR body is the effective spec — implementation aligns. No docs change needed for this internal write-path fix. - Security / privacy: Credential fields are sanitized (reject non-strings / blank / CRLF) before write-back; no request-body or key logging added. Touches OAuth config write path — worth a maintainer glance, but not a token-exfiltration or workflow-permission concern.
CI / process gates
| Gate | Status |
|---|---|
| Base branch | dev (correct; wrong-branch enforcer already corrected earlier) |
mergeable / mergeStateStatus |
CONFLICTING / DIRTY — blocking |
| Conflicted files | package.json |
Cross-platform CI (tip e0debe22) |
action_required (needs fork approval / not green on tip) |
| Enforce PR target / label / CodeRabbit | pass |
| Prior full CI success | 8eca4eb only (pre follow-up) |
Event: request-changes
After fixes
Once all blocking + required quality changes above are addressed (and conflicts/rebase done), mark this PR Ready for review (leave draft only until then). Then ping for re-review / CI.
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.
`upsertOAuthProvider` replaced the provider entry with the bare preset on every OAuth login. For providers that can be billed either way (`allowKeyAuthOverride`: xai, github-copilot) this deleted `apiKey` and `apiKeyPool` from config.json and silently flipped an explicit `authMode: "key"` billing choice back to the subscription — the key was gone and had to be pasted again. Carry the key fields over and keep an explicitly chosen `authMode: "key"`, so logging in never changes which credential bills the request. OAuth-only providers and fresh logins still get the untouched preset.
…omitted Address review follow-ups so OAuth login preserves key billing when a usable key remains, including pool promotion and shared API-key sanitization.
e0debe2 to
aa4b99b
Compare
|
Maintainer takeover update for PR #491:
Waiting on CI on the rebased tip before merge. This path touches OAuth config writes — still needs maintainer security glance before merge. |
3 participants
Summary
upsertOAuthProviderreplaced a provider's config entry with the bare preset on every OAuth login:For providers that can be billed either way — registry entries with
allowKeyAuthOverride(xai,github-copilot) — this deletedapiKeyandapiKeyPoolfromconfig.jsonand silently flipped an explicitauthMode: "key"billing choice back to the subscription. The stored key was gone and had to be pasted again, with no message saying so.router.tsalready honorsauthMode: "key"for exactly these providers, so the two credentials are meant to coexist. This makes the write path agree with the routing path.Change
Carry
apiKey/apiKeyPoolover, and keep an explicitly chosenauthMode: "key", so logging in never changes which credential bills the request. OAuth-only providers and fresh logins still get the untouched preset — onlyallowKeyAuthOverrideentries with an existing config are treated specially.Testing
New
tests/oauth-upsert-preserves-api-key.test.ts, 5 cases:authMode: "key"all survive a loginauthMode: "oauth"untouched when the user did not pick key billinganthropic) still gets the plain preset, stale user fields droppedRan locally against this branch:
bun test tests/oauth-upsert-preserves-api-key.test.ts— 5 passbun teston the adjacent OAuth/key suites (oauth-provider-reconcile,oauth-accounts-api,provider-api-keys,oauth-public-surface,xai-oauth-retry,github-copilot-oauth) — 32 pass, 0 failbun x tsc --noEmit— cleanbun scripts/privacy-scan.ts— passedNot run: the full
bun scripts/test.tssuite.Notes
I hit this while looking into a report that Claude's API key and OAuth login could not be signed in at the same time.
anthropicis not anallowKeyAuthOverrideentry, so that case is separate — the dedicatedanthropic-apikeyregistry entry already covers it, and this PR does not touch that. This fix is only about the key-deleting write path on the providers where both credentials are supported.Summary by CodeRabbit