fix(auth): add OAuth state parameter to prevent login CSRF (#268)#301
fix(auth): add OAuth state parameter to prevent login CSRF (#268)#301Shevilll wants to merge 3 commits into
Conversation
…g#268) The GitHub OAuth verification flow did not generate or validate the standard OAuth `state` parameter (RFC 6749 Section 10.12). The flow relied solely on a `session` id in the redirect URI to bind the Discord verification request to the GitHub callback, leaving it open to login CSRF / session fixation: an attacker could trick a victim into completing a callback that links the victim's Discord account to the attacker's GitHub account. Generate a cryptographically-random state with secrets.token_urlsafe(32) bound to the verification session, thread it through the OAuth authorize URL, and validate it in the callback with a constant-time comparison, rejecting any request with a missing or mismatched state. - verification.py: create_verification_session now returns (session_id, oauth_state) and stores the state with the session; add validate_oauth_state() for constant-time validation. - cogs.py: pass the generated state to login_with_github in both the /verify_github command and the onboarding flow. - auth.py: accept the state query param in the callback and reject the request when validation fails.
|
Warning Review limit reached
More reviews will be available in 55 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR closes a Login CSRF vulnerability in the GitHub OAuth flow by introducing a cryptographically random ChangesOAuth CSRF State Protection
Sequence Diagram(s)sequenceDiagram
participant User as Discord User
participant Cog as Discord Cog
participant VerSvc as verification.py
participant GitHub as GitHub OAuth
participant Callback as auth_callback
User->>Cog: /verify_github
Cog->>VerSvc: create_verification_session(discord_id)
VerSvc-->>Cog: (session_id, oauth_state)
Cog->>GitHub: login_with_github(redirect_to=callback_url, state=oauth_state)
GitHub-->>User: OAuth authorization URL
User->>GitHub: authorize
GitHub->>Callback: GET /callback?code=...&state=oauth_state&session=session_id
Callback->>VerSvc: validate_oauth_state(session_id, state)
VerSvc-->>Callback: True / False
alt state valid
Callback->>Callback: proceed with code exchange
else state invalid
Callback-->>User: HTML error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/services/auth/verification.py (1)
114-114: 💤 Low valuePrefix unused variable with underscore for consistency.
expiry_timeis unpacked but never used (expiry is validated via the database query). Use_expiry_timeto match the pattern used elsewhere in this file (e.g., lines 22, 89, 200).- discord_id, expiry_time, _oauth_state = session_data + discord_id, _expiry_time, _oauth_state = session_data🤖 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 `@backend/app/services/auth/verification.py` at line 114, The variable expiry_time is unpacked from session_data in the statement but is never used in the code since expiry validation happens via database query. Rename expiry_time to _expiry_time to follow the Python convention of prefixing unused variables with an underscore, which is consistent with the pattern already used elsewhere in the file such as with _oauth_state in the same unpacking statement.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@backend/app/services/auth/verification.py`:
- Line 114: The variable expiry_time is unpacked from session_data in the
statement but is never used in the code since expiry validation happens via
database query. Rename expiry_time to _expiry_time to follow the Python
convention of prefixing unused variables with an underscore, which is consistent
with the pattern already used elsewhere in the file such as with _oauth_state in
the same unpacking statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 392216ae-3919-4fa8-9260-c1fb00098e19
📒 Files selected for processing (3)
backend/app/api/v1/auth.pybackend/app/services/auth/verification.pybackend/integrations/discord/cogs.py
Expiry is validated through the database query, so the unpacked expiry_time is unused; underscore-prefix it for consistency with the rest of the module.
Shevilll
force-pushed
the
fix/oauth-csrf-state
branch
from
4a829e5 to
1e6ffea
Compare
|
Hi team! I've resolved the CodeRabbit linter nitpick on the verification session unpacking, ensured a completely clean run on the CI checks, and verified that all security validations pass. Please let me know if there is anything else needed, or if this is ready to merge! |
1 participant
Fixes #268
What
Adds the OAuth
stateparameter (RFC 6749 §10.12) to the GitHub verification flow: a cryptographically-random state is generated when the OAuth URL is created, bound to the verification session, and validated in the callback before any account linking happens.Why
The flow previously bound the Discord verification request to the GitHub callback using only a
sessionid in the redirect URI, and never generated or validated astate. That leaves it open to login CSRF / session fixation — a victim could be tricked into completing a callback that links their Discord account to an attacker's GitHub account.login_with_githubalready accepted an optionalstate, but no caller passed it and the callback never checked it.Changes
verification.py—create_verification_session()now generates a random state viasecrets.token_urlsafe(32), stores it alongside the existing in-memory verification session (same ~5-min TTL), and returns(session_id, oauth_state). Newvalidate_oauth_state()uses a constant-time comparison (secrets.compare_digest) and rejects a missing state, an unknown/expired session, or a mismatch.cogs.py— passes the generated state tologin_with_github(...)in both the/verify_githubcommand and the onboarding flow.auth.py— the callback now accepts thestatequery param and rejects the request when validation fails, before linking accounts.Design note
The issue suggested Redis for state storage, but this repo has no application-level Redis (it isn't a declared dependency and isn't used outside the vendored FalkorDB subproject). The established convention for verification sessions is the in-memory store in
verification.pywith a short TTL, so I bound the state there to keep the change minimal and consistent with the existing code. Migrating session/state storage to Redis would be a separate, broader change.Verification
python -m py_compilepasses for all changed files.validate_oauth_state: correct state accepted; forged, empty, andNonestates rejected; unknown session rejected.Summary by CodeRabbit
statetied to each verification session.statewhen generating OAuth links, reducing failed or mismatched callbacks.statevalidation fails, the flow now returns the existing verification error guidance instead of continuing.