fix(xai): validate OAuth state before settling callbacks - #2228
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (3)Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny.⚙️ CodeRabbit configuration file Files:
Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.⚙️ CodeRabbit configuration file Files:
Apply the OpenClaude maintainer review rubric from AGENTS.md.⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe xAI OAuth loopback callback now validates the expected ChangesxAI OAuth state validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The callback now validates OAuth state before handling errors or codes, preventing unsolicited callbacks from ending an active login flow. No remaining merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR hardens the xAI OAuth loopback callback by validating the expected state before processing either an OAuth error or authorization code.
Confidence Score: 5/5The PR appears safe to merge; the callback validation is consistently wired into the production flow and preserves valid completion, recovery, and cancellation behavior. No actionable failure remains: the expected state is generated once, round-trips through URL-safe encoding, is checked before callback settlement, and is independently checked again before token exchange.
|
| Filename | Overview |
|---|---|
| src/services/api/xaiOAuthCallback.ts | Adds exact expected-state validation before any callback error or authorization code can settle the pending flow. |
| src/services/api/xaiOAuth.ts | Supplies the same generated state to the callback server and authorization URL while retaining downstream validation. |
| src/services/api/xaiOAuthCallback.test.ts | Adds focused regression coverage showing invalid-state requests cannot consume a login and that all supported completion paths remain functional. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Loopback callback received] --> B{Correct method and path?}
B -- No --> C[Return 404 or 405]
B -- Yes --> D{State exactly matches expected state?}
D -- No --> E[Return 400; keep login pending]
D -- Yes --> F{OAuth error present?}
F -- Yes --> G[Return 400; reject pending callback]
F -- No --> H{Authorization code present?}
H -- No --> I[Return 400; reject pending callback]
H -- Yes --> J[Return success; resolve callback]
J --> K[Service validates state again]
K --> L[Exchange authorization code]
Reviews (1): Last reviewed commit: "fix(xai): validate OAuth state before se..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Summary
AGENTS.mdandCONTRIBUTING.md.Impact
Testing
bun install --frozen-lockfile --network-concurrency 8,bun run typecheck,bun run typecheck:type-tests,node bin/openclaude --version,NODE_DISABLE_COMPILE_CACHE=1 node bin/openclaude --version,npm run test:provider-recommendation, andbun run security:pr-scan -- --base FETCH_HEAD --head HEADafter fetching canonical upstream main. Exact-scope lint andgit diff --checkpassed.bun test src/services/api/xaiOAuthCallback.test.ts src/services/api/xaiOAuthShared.test.ts— 36 passed. Eight invalid-state regressions failed before the fix. Real loopback HTTP tests cover recovery by valid callback, manual code, and cancellation; discovery and token exchange are mocked.bun run checkbuilt the CLI/SDK and passed smoke/deadcode, but printed 65 unique failing test names despite exiting 0 without a full-suite summary. The same command on untouched basee2b021d8bbda66b64cef7dbe1e089fea3eaa563freproduced the identical 65 names (fact extraction, registry append, attribution, pricing, model limits, and fast mode). This is not a green full-suite result.bun run test:provider: 1,657 passed, one failed:Claude stream watchdog > falls back when the top-level stream iterator never settles. Untouched base reproduced the same failure (1,646 passed, one failed). The change does not touch these failing paths; delivery follows the contribution guide's verified pre-existing failure exception.Notes
Summary by CodeRabbit