Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughOAuth failover now rebinds credential identity, replay scope, and Cursor state after rotation. Copilot OAuth 401 refreshes validate the refreshed credential’s API origin and fall back to the canonical origin. Regression tests cover all rotation and refresh paths. ChangesOAuth failover identity consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟠 High · up to OAuth 429 recovery can still retry a request with one account’s credential against another account’s approved Copilot origin when the rotated account lacks its own endpoint, creating a credential-boundary security risk. The PR is not merge-ready until the fallback is made account-scoped or otherwise explicitly accepted by the responsible security owner. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation in Resolution Replace the source-text assertions in Full details: Out of Scope Changes checkExplanation The changes in
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
리뷰 · 우선순위 78 / 80이 PR은 OAuth 계정 로테이션(특히 github-copilot)에서 토큰만 바꾸고 신원(identity)은 옛 계정을 가리키던 구멍을 한곳에서 막는다. 지금 이 브랜치는 그걸 라인 ~995-1015 ( 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Rotating a credential without rotating the IDENTITY of that credential left the two naming different accounts, and both consumers of that identity then misbehaved. applyFailoverSnapshot swapped the bearer, the Copilot origin, the Antigravity project and the Kiro routing metadata, but never updated sentOAuthSnapshot or replayOAuthCredentialSnapshot. sentOAuthSnapshot feeds the 401 forced refresh, which refreshes the SNAPSHOT's account rather than whichever account is on the route now - so a later 401 renewed the account the request had just rotated away from. replayOAuthCredentialSnapshot scopes the reasoning replay cache, so a stale value let continuation state minted under one account be addressed under another. Both rebinds now live inside the helper. Three rotation sites already disagreed about which of them to update, and the one that got it right (runTurn) got it right by hand; a fourth site would have to remember too. In the helper it cannot forget. The other two sites are brought up to runTurn's standard: the HTTP 429 path had no replay rebind at all (sealing the attempt identity records what happened, it does not re-scope the cache), and the sidecar bound WITHOUT the credential snapshot, which fails the OAuth replay key closed - safe-looking, but it leaves the scope carrying no account identity to distinguish. Both 401 rebuilds now prefer refreshed.apiBaseUrl over getOAuthCredentialApiBaseUrl. The latter reads the ACTIVE credential, and generic 429 failover never promotes the active account, so after a rotation it still named the rate-limited account - pairing a refreshed bearer with the wrong account's origin. On reachability, an independent audit was clear: on today's control flow a 401 cannot follow a 429 rotation back into the refresh path, because the HTTP 429 branch does not `continue recovery` and Copilot Responses models take a passthrough return with no rotator at all. So this is latent identity drift, not a live cross-account send. That missing continue is an ACCIDENTAL guard: adding it - an obvious future improvement to 429 recovery - would activate the defect. Which is the argument for fixing the identity first. Three assertions added, each driven red on its own by reverting only its fix. The replay-scope one is bounded to each rotation's own recovery call: an unbounded forward search finds the NEXT site's bind and passes with this site's deleted, and that weaker form was written first and survived the deletion.
…r too The Cursor conversation and identity scope are credential-scoped exactly like the snapshot and the replay scope, and they were cleared at only one of the three rotation sites - runTurn, by hand. The same rotation that swapped the bearer could therefore carry the previous account's conversation forward at the other two. For image turns that is not theoretical: src/images/loop.ts copies the conversation id back onto the outer request, which is how it outlives the rotation that should have ended it. Moving the clear into the helper removes the per-site copy rather than adding two more, and the test pins that there is exactly one - a duplicate is how these three drifted apart in the first place. 862 Cursor tests across 52 files stay green.
…tation Closes blocker 1 of the #2745 review. The fallback was 'refreshed.apiBaseUrl ?? getOAuthCredentialApiBaseUrl(provider)', and the second arm is account-blind: getOAuthCredentialApiBaseUrl reads the ACTIVE credential, and a generic 429 rotation never promotes the account it rotated to. So a legacy account B carrying no allowlisted apiBaseUrl was rebuilt as B's bearer paired with A's origin — one account's token sent to another account's host. copilotOriginForRefreshedCredential resolves from the refreshed snapshot alone and otherwise fails closed to the canonical Copilot origin. It never consults or retains another account's route. Behaviour across every arm: B has its own allowlisted origin -> that origin B is legacy with no origin -> canonical, never A's B has a non-allowlisted origin -> canonical B origin is empty -> canonical Also closes blocker 2 for this site. The old test asserted the source text 'refreshed.apiBaseUrl ?? getOAuthCredentialApiBaseUrl' appears exactly twice — it asserted the DEFECT existed, and would have passed whether the assignment was reachable or whether the resulting pair was correct. Replaced with a behavioural test that drives the resolver, plus a topology guard that strips comments first so the helper's own doc comment explaining the removed expression is not read as the defect. Mutation-verified: restoring the account-blind fallback fails the guard (18 pass / 1 fail); with the fix, 19 pass / 0 fail. tsc exit 0.
3de2eef to
1c61a7e
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 1c61a7e8c. One credential/origin blocker remains in the generic OAuth 429 path.
applyFailoverSnapshot clones the existing route provider (including account A's baseUrl) and calls resolveProviderTransport(..., snapshot.apiBaseUrl) at src/server/responses/core.ts:2851-2858. When rotated account B has no stored apiBaseUrl, resolveGithubCopilotTransport falls through from the missing argument to validateCopilotApiBaseUrl(provider.baseUrl). Because that provider is the cloned A route, B's bearer is still paired with A's accepted Copilot origin. The new copilotOriginForRefreshedCredential helper closes the two 401 rebuild sites, but it is not used by this generic 429 snapshot path.
Please make the 429 rotation resolve exclusively from B's snapshot (for example, pass copilotOriginForRefreshedCredential(snapshot) or otherwise clear the previous account's origin before resolution). Add an executable A -> 429 -> B regression through the actual recovery/fetch path that asserts both the second dispatch bearer and origin. The current resolve = validate(...) ?? default unit merely reimplements the intended rule and cannot catch this reachable fallback through provider.baseUrl. Re-request review on the updated head.
Four merged on the authorization (lidge-jun#2794, lidge-jun#2747, lidge-jun#2806, lidge-jun#2740), each already green — it removed a process gate, not a verification one. Two credential-path PRs were not merged despite the rights being available. lidge-jun#2638's hygiene gate asks whether a human reviewed auth-context.ts, and maintainer-sponsored IS that judgement, so applying it forges the gate. lidge-jun#2807 is the interesting case: hygiene PASSES because core.ts is absent from RESTRICTED_FILES, while CODEOWNERS:46 assigns it to me and MAINTAINERS.md:60 requires security review for credential handling. The gate is narrower than the rule it encodes, and a passing check is not permission when the rule applies. Left as a follow-up rather than fixed here: widening a security gate while holding admin rights and an open PR the widening would block is the kind of self-serving edit that needs its own review.
|
Superseded by #2841, which carries this work forward on current This branch is
Closing in favour of #2841. |
…#2841) * fix(responses): bind a rotated OAuth bearer to its own Copilot origin On a generic-OAuth 429 rotation, `applyFailoverSnapshot` clones the FAILED account's provider and re-resolves Copilot transport with the rotated account's `snapshot.apiBaseUrl`. When that account has no stored origin the bare `undefined` reached the transport resolver, whose own fallback chain is validateCopilotApiBaseUrl(apiBaseUrl) // undefined for account B ?? validateCopilotApiBaseUrl(provider.baseUrl) // still account A's origin ?? GITHUB_COPILOT_DEFAULT_API_BASE so the second step returned the cloned host and account B's bearer was sent to account A's accepted origin. An account legitimately has no stored origin whenever its login response carried no `endpoints.api` — `saveCredential` only records `apiBaseUrl` when it is present and validates — so this is reachable with ordinary credentials rather than crafted ones. Both values were individually valid, which is why nothing caught it: the defect is in the pairing, and the helper's own doc comment already claimed the behavior the code did not implement. Resolving the snapshot value before handing it over closes the fall-through. A rotated account with its own regional origin keeps it; one without gets the canonical default; a crafted non-Copilot origin is still refused. The existing guard asserted the source contained `snapshot.apiBaseUrl`, which is precisely the buggy expression — a text assertion that pinned the defect in place. It now requires the resolved form, and four behavioral tests observe the pairing directly, including one that reproduces the leak through the unresolved call so the others cannot pass vacuously. Verification: bun test tests/generic-oauth-failover.test.ts 19 pass 0 fail (was 18 pass 1 fail) bun x tsc --noEmit exit 0 Reimplements #2807 on current dev; that branch was 80 commits behind and conflicting. Credit to the original investigation there. * fix(responses): keep Copilot origin snapshot-atomic Resolve every initial and refreshed Copilot transport origin from the same OAuth access snapshot that supplied its bearer. Add account-switch regression coverage for Chat, Responses, 401 replay, and 429 account failover. * test(oauth): keep Copilot bearer fixtures privacy-safe Build expected Authorization values from separate fixture parts so the repository privacy scan does not mistake synthetic Copilot tokens for secrets. --------- Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
Four merged on the authorization (lidge-jun#2794, lidge-jun#2747, lidge-jun#2806, lidge-jun#2740), each already green — it removed a process gate, not a verification one. Two credential-path PRs were not merged despite the rights being available. lidge-jun#2638's hygiene gate asks whether a human reviewed auth-context.ts, and maintainer-sponsored IS that judgement, so applying it forges the gate. lidge-jun#2807 is the interesting case: hygiene PASSES because core.ts is absent from RESTRICTED_FILES, while CODEOWNERS:46 assigns it to me and MAINTAINERS.md:60 requires security review for credential handling. The gate is narrower than the rule it encodes, and a passing check is not permission when the rule applies. Left as a follow-up rather than fixed here: widening a security gate while holding admin rights and an open PR the widening would block is the kind of self-serving edit that needs its own review.
…lidge-jun#2841) * fix(responses): bind a rotated OAuth bearer to its own Copilot origin On a generic-OAuth 429 rotation, `applyFailoverSnapshot` clones the FAILED account's provider and re-resolves Copilot transport with the rotated account's `snapshot.apiBaseUrl`. When that account has no stored origin the bare `undefined` reached the transport resolver, whose own fallback chain is validateCopilotApiBaseUrl(apiBaseUrl) // undefined for account B ?? validateCopilotApiBaseUrl(provider.baseUrl) // still account A's origin ?? GITHUB_COPILOT_DEFAULT_API_BASE so the second step returned the cloned host and account B's bearer was sent to account A's accepted origin. An account legitimately has no stored origin whenever its login response carried no `endpoints.api` — `saveCredential` only records `apiBaseUrl` when it is present and validates — so this is reachable with ordinary credentials rather than crafted ones. Both values were individually valid, which is why nothing caught it: the defect is in the pairing, and the helper's own doc comment already claimed the behavior the code did not implement. Resolving the snapshot value before handing it over closes the fall-through. A rotated account with its own regional origin keeps it; one without gets the canonical default; a crafted non-Copilot origin is still refused. The existing guard asserted the source contained `snapshot.apiBaseUrl`, which is precisely the buggy expression — a text assertion that pinned the defect in place. It now requires the resolved form, and four behavioral tests observe the pairing directly, including one that reproduces the leak through the unresolved call so the others cannot pass vacuously. Verification: bun test tests/generic-oauth-failover.test.ts 19 pass 0 fail (was 18 pass 1 fail) bun x tsc --noEmit exit 0 Reimplements lidge-jun#2807 on current dev; that branch was 80 commits behind and conflicting. Credit to the original investigation there. * fix(responses): keep Copilot origin snapshot-atomic Resolve every initial and refreshed Copilot transport origin from the same OAuth access snapshot that supplied its bearer. Add account-switch regression coverage for Chat, Responses, 401 replay, and 429 account failover. * test(oauth): keep Copilot bearer fixtures privacy-safe Build expected Authorization values from separate fixture parts so the repository privacy scan does not mistake synthetic Copilot tokens for secrets. --------- Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
Four merged on the authorization (lidge-jun#2794, lidge-jun#2747, lidge-jun#2806, lidge-jun#2740), each already green — it removed a process gate, not a verification one. Two credential-path PRs were not merged despite the rights being available. lidge-jun#2638's hygiene gate asks whether a human reviewed auth-context.ts, and maintainer-sponsored IS that judgement, so applying it forges the gate. lidge-jun#2807 is the interesting case: hygiene PASSES because core.ts is absent from RESTRICTED_FILES, while CODEOWNERS:46 assigns it to me and MAINTAINERS.md:60 requires security review for credential handling. The gate is narrower than the rule it encodes, and a passing check is not permission when the rule applies. Left as a follow-up rather than fixed here: widening a security gate while holding admin rights and an open PR the widening would block is the kind of self-serving edit that needs its own review.
…lidge-jun#2841) * fix(responses): bind a rotated OAuth bearer to its own Copilot origin On a generic-OAuth 429 rotation, `applyFailoverSnapshot` clones the FAILED account's provider and re-resolves Copilot transport with the rotated account's `snapshot.apiBaseUrl`. When that account has no stored origin the bare `undefined` reached the transport resolver, whose own fallback chain is validateCopilotApiBaseUrl(apiBaseUrl) // undefined for account B ?? validateCopilotApiBaseUrl(provider.baseUrl) // still account A's origin ?? GITHUB_COPILOT_DEFAULT_API_BASE so the second step returned the cloned host and account B's bearer was sent to account A's accepted origin. An account legitimately has no stored origin whenever its login response carried no `endpoints.api` — `saveCredential` only records `apiBaseUrl` when it is present and validates — so this is reachable with ordinary credentials rather than crafted ones. Both values were individually valid, which is why nothing caught it: the defect is in the pairing, and the helper's own doc comment already claimed the behavior the code did not implement. Resolving the snapshot value before handing it over closes the fall-through. A rotated account with its own regional origin keeps it; one without gets the canonical default; a crafted non-Copilot origin is still refused. The existing guard asserted the source contained `snapshot.apiBaseUrl`, which is precisely the buggy expression — a text assertion that pinned the defect in place. It now requires the resolved form, and four behavioral tests observe the pairing directly, including one that reproduces the leak through the unresolved call so the others cannot pass vacuously. Verification: bun test tests/generic-oauth-failover.test.ts 19 pass 0 fail (was 18 pass 1 fail) bun x tsc --noEmit exit 0 Reimplements lidge-jun#2807 on current dev; that branch was 80 commits behind and conflicting. Credit to the original investigation there. * fix(responses): keep Copilot origin snapshot-atomic Resolve every initial and refreshed Copilot transport origin from the same OAuth access snapshot that supplied its bearer. Add account-switch regression coverage for Chat, Responses, 401 replay, and 429 account failover. * test(oauth): keep Copilot bearer fixtures privacy-safe Build expected Authorization values from separate fixture parts so the repository privacy scan does not mistake synthetic Copilot tokens for secrets. --------- Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
2 participants
Summary
Supersedes #2745 by carrying its two commits onto current
devand closing both blockers from @Ingwannu's security-boundary review.Blocker 1 — the account-blind Copilot fallback
The defect was one
??:getOAuthCredentialApiBaseUrlisvalidateCopilotApiBaseUrl(getCredential(provider)?.apiBaseUrl)— it reads the active credential, with no account scoping. A generic 429 rotation never promotes the account it rotated to, so for a legacy account B carrying no allowlistedapiBaseUrl, that arm silently reached account A. B's bearer paired with A's origin — one account's token sent to another account's host.copilotOriginForRefreshedCredentialnow resolves from the refreshed snapshot alone and otherwise fails closed to the canonical Copilot origin. It never consults, and never retains, another account's route:Blocker 2 — the source-text assertions
The review called them "not request-path behaviour". It was worse than that: one asserted the defect existed.
That passes whether the assignment is reachable, whether the right snapshot arrives, and whether the resulting bearer/origin pair is correct — and fixing the bug would have broken it.
Replaced with a behavioural test driving the resolver across all four arms above, plus a topology guard that strips comments before scanning. The new helper's doc comment quotes the removed expression to explain why it was wrong, and the first version of the guard read that explanation as the defect.
Verification
Mutation-verified: restore the account-blind fallback and the guard fails (18 pass / 1 fail); with the fix, 19 / 0.
Still open from the original review — not claimed as closed
applyFailoverSnapshotaudit, where an undefined snapshot origin can leave the previous routed base URL on the provider object.Both are worth doing. Neither is done here, and I would rather say so than let two closed blockers imply a clean sweep.
Integration-line disposition
No Go runtime counterpart. This is a credential-boundary change:
MAINTAINERS.mdrequires explicit security review, and I authored it, so it needs a non-author maintainer.Checklist
dev.Closes #2745.
Summary by CodeRabbit