fix: implement OAuth CSRF protection (Bug A) #268 #269
Conversation
📝 WalkthroughWalkthroughAdds Redis-backed OAuth state generation and validation: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiscordBot as Discord Bot (cogs)
participant Backend as Auth Service
participant Redis
participant Provider as OAuth Provider (GitHub)
participant Callback as Callback Handler
User->>DiscordBot: /verify_github (initiate)
DiscordBot->>Backend: request auth URL with session_id
Backend->>Redis: store_oauth_state(session_id) -> state_token
Redis-->>Backend: OK
Backend->>Provider: redirect_to=callback_url, state=state_token
Provider-->>User: auth_url
User->>Provider: authorize
Provider->>Callback: GET /callback?code=...&state=...
Callback->>Redis: validate_oauth_state(state, session_id)
Redis-->>Callback: ok / failed (state consumed)
Callback->>Backend: on valid state, exchange code and link accounts
Callback-->>User: success / error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/auth/supabase.py (1)
96-105:⚠️ Potential issue | 🟠 MajorRemove
set_session()call; usesign_out()alone for logout.According to Supabase Python documentation,
set_session()requires a realrefresh_tokenvalue—passing an empty string will fail. For logout, callsign_out()directly; it properly revokes the session and refresh token without needing to set a session first. Remove theset_session()line entirely.Current code
async def logout(access_token: str): """Logs out a user by revoking their session.""" supabase = get_supabase_client() try: await supabase.auth.set_session(access_token, refresh_token="") # nosec await supabase.auth.sign_out() return {"message": "User logged out successfully"} except Exception as e: logger.error(f"Logout failed: {e}", exc_info=True) raise
🧹 Nitpick comments (4)
backend/integrations/discord/cogs.py (4)
149-166: Move the import to module level to avoid repeated import overhead and improve readability.The inline import of
store_oauth_stateinside the function body works but is unconventional. Sincelogin_with_githubis already imported at module level from the same module,store_oauth_stateshould be imported alongside it.♻️ Proposed fix
At line 17, update the import:
-from app.services.auth.supabase import login_with_github +from app.services.auth.supabase import login_with_github, store_oauth_stateThen remove the inline import at line 150:
# SECURITY FIX: Generate and store OAuth state parameter - from app.services.auth.supabase import store_oauth_state try: state_token = await store_oauth_state(session_id, ttl=300)
153-161: Uselogger.exceptionfor automatic stack trace inclusion.Per static analysis hint TRY400,
logger.exceptionautomatically includes the exception info, making the code cleaner and ensuring consistent stack trace logging.♻️ Proposed fix
except Exception as e: - logger.error(f"Failed to generate OAuth state: {e}") + logger.exception("Failed to generate OAuth state") error_embed = discord.Embed(
518-536: Duplicate OAuth state generation logic - consider extracting a helper.This block is nearly identical to lines 149-161 in
verify_github. Both generate state, handle errors similarly, and pass state tologin_with_github. A private helper method would reduce duplication and ensure consistent behavior.♻️ Optional: Extract helper method
async def _generate_oauth_url_with_state( self, session_id: str, callback_url: str ) -> tuple[str | None, str | None]: """ Generate OAuth URL with CSRF state protection. Returns (auth_url, error_message) tuple. """ try: state_token = await store_oauth_state(session_id, ttl=300) except Exception: logger.exception("Failed to generate OAuth state") return None, "Verification service is temporarily unavailable. Please try again." auth = await login_with_github(redirect_to=callback_url, state=state_token) auth_url = auth.get("url") if not auth_url: return None, "Couldn't generate a verification link. Please use /verify_github." return auth_url, NoneThis helper can then be used in both
verify_githuband_send_onboarding_flow.
523-534: Uselogger.exceptionand remove redundant exception argument.Static analysis correctly identifies that
logging.exceptionalready includes the exception, making{send_error}redundant at line 533.♻️ Proposed fix
except Exception as e: - logger.error(f"Failed to generate OAuth state in onboarding: {e}") + logger.exception("Failed to generate OAuth state in onboarding") try: ... except Exception as send_error: - logger.exception( - f"Failed to send auth state failure fallback DM to user {user.id}: {send_error}") + logger.exception( + f"Failed to send auth state failure fallback DM to user {user.id}")
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/integrations/discord/cogs.py`:
- Around line 534-548: The except block is catching Exception as send_error but
never uses send_error, causing Ruff F841; update the fallback error handling in
the verification flow (the block around _generate_secure_github_auth_url and the
user.send calls) to remove the unused variable by changing "except Exception as
send_error:" to "except Exception:" (keep the logger.exception call unchanged so
the exception is still logged), ensuring the rest of the logic (sending fallback
DMs with build_encourage_verification_message and build_final_handoff_embed)
remains the same.
🧹 Nitpick comments (1)
backend/integrations/discord/cogs.py (1)
26-44: Handle OAuth URL generation failures inside the helper.If
login_with_githubraises or returns a falsy payload, the caller falls back to generic exception handling. Wrapping it here keeps error messaging consistent and prevents leaking stack traces into higher-level flows.♻️ Suggested update
- # Pass state parameter to OAuth flow (RFC 6749 compliant) - auth = await login_with_github(redirect_to=callback_url, state=state_token) - auth_url = auth.get("url") + # Pass state parameter to OAuth flow (RFC 6749 compliant) + try: + auth = await login_with_github(redirect_to=callback_url, state=state_token) + except Exception: + logger.exception("Failed to generate GitHub OAuth URL") + return None, "Couldn't generate a verification link. Please use /verify_github." + + auth_url = auth.get("url") if auth else None if not auth_url: return None, "Couldn't generate a verification link. Please use /verify_github."
1 participant
Closes #268
📝 Description
This PR fixes a critical Login CSRF / Session Fixation vulnerability (Bug A) in the OAuth flow.
Previously, the state parameter was missing from the Discord-to-GitHub verification process, allowing attackers to theoretically force a victim to link the attacker's GitHub account. This fix implements full RFC 6749 compliance by generating, storing, and strictly validating a cryptographically secure state token.
🔧 Changes Made
/verify_githuband onboarding) to generate and pass the state parameter to GitHub.📷 Screenshots or Visual Changes (if applicable)
Backend security fix only - no visual UI changes.
✅ Checklist
ruff,mypy, andbanditchecks).Summary by CodeRabbit
New Features
Bug Fixes