From f4ecbbb2da33e653fa38acdb2d0355efbbfe88b5 Mon Sep 17 00:00:00 2001 From: adithya32 <163162210+KumarADITHYA123@users.noreply.github.com> Date: Thu, 5 Feb 2026 08:15:58 +0530 Subject: [PATCH 1/3] fix: implement OAuth CSRF protection (Bug A) #268 --- backend/app/api/v1/auth.py | 36 ++++++++++++++-- backend/app/services/auth/supabase.py | 62 +++++++++++++++++++++++++-- backend/integrations/discord/cogs.py | 38 +++++++++++++++- 3 files changed, 128 insertions(+), 8 deletions(-) diff --git a/backend/app/api/v1/auth.py b/backend/app/api/v1/auth.py index 0d0bacb7..f50b4b53 100644 --- a/backend/app/api/v1/auth.py +++ b/backend/app/api/v1/auth.py @@ -1,3 +1,4 @@ +from app.services.auth.supabase import validate_oauth_state from fastapi import APIRouter, Request, HTTPException, Query, Depends from fastapi.responses import HTMLResponse from app.database.supabase.client import get_supabase_client @@ -16,18 +17,23 @@ logger = logging.getLogger(__name__) router = APIRouter() + @router.get("/callback", response_class=HTMLResponse) async def auth_callback( request: Request, code: Optional[str] = Query(None), session: Optional[str] = Query(None), + state: Optional[str] = Query(None), # NEW: Accept state parameter app_instance: "DevRAIApplication" = Depends(get_app_instance), ): """ Handles the OAuth callback from Supabase after a user authorizes on GitHub. """ logger.info( - f"OAuth callback received with code: {'[PRESENT]' if code else '[MISSING]'}, session: {'[PRESENT]' if session else '[MISSING]'}") + f"OAuth callback received with code: {'[PRESENT]' if code else '[MISSING]'}, " + f"session: {'[PRESENT]' if session else '[MISSING]'}, " + f"state: {'[PRESENT]' if state else '[MISSING]'}" + ) if not code: logger.error("Missing authorization code in callback") @@ -37,6 +43,26 @@ async def auth_callback( logger.error("Missing session ID in callback") return _error_response("Missing session ID. Please try the /verify_github command again.") + # NEW SECURITY CHECK: Validate OAuth state parameter (RFC 6749) + if not state: + logger.error("Missing OAuth state parameter in callback - potential CSRF attack") + return _error_response( + "Missing security validation. This may indicate a CSRF attack. " + "Please try the /verify_github command again." + ) + + # Validate state matches stored value in Redis + state_valid = await validate_oauth_state(state, session) + if not state_valid: + logger.warning( + f"OAuth state validation failed for session: {session[:8]}... - " + f"Possible CSRF attempt or expired state" + ) + return _error_response( + "Security validation failed. Your verification session has expired or is invalid. " + "Please run the /verify_github command again." + ) + # Check if session is valid and not expired session_info = await get_verification_session_info(session) if not session_info: @@ -77,8 +103,12 @@ async def auth_callback( ) if not verified_user: - logger.error("User verification failed - no pending verification found") - return _error_response("No pending verification found or verification has expired. Please try the /verify_github command again.") + logger.error( + "User verification failed - no pending verification found" + ) + return _error_response( + "No pending verification found or verification has expired. Please try the /verify_github command again." + ) logger.info(f"Successfully verified user: {verified_user.id}!") diff --git a/backend/app/services/auth/supabase.py b/backend/app/services/auth/supabase.py index 6ee0d58e..41110fa9 100644 --- a/backend/app/services/auth/supabase.py +++ b/backend/app/services/auth/supabase.py @@ -1,16 +1,72 @@ -from typing import Optional +from typing import Optional, Dict, Any from app.database.supabase.client import get_supabase_client import logging +import secrets +from app.core.redis import get_redis_client logger = logging.getLogger(__name__) +async def store_oauth_state(session_id: str, ttl: int = 300) -> str: + """ + Generate and store a cryptographically secure OAuth state token in Redis. + """ + try: + state_token = secrets.token_urlsafe(32) + redis_client = await get_redis_client() + + # Store: oauth_state:{state_token} -> session_id + redis_key = f"oauth_state:{state_token}" + await redis_client.setex(redis_key, ttl, session_id) + + logger.info(f"OAuth state stored for session: {session_id[:8]}... (expires in {ttl}s)") + return state_token + + except Exception as e: + logger.error(f"Failed to store OAuth state: {str(e)}", exc_info=True) + raise Exception("OAuth state storage failed. Verification service temporarily unavailable.") from e + +async def validate_oauth_state(state_token: str, expected_session_id: str) -> bool: + """ + Validate and consume an OAuth state token. + """ + try: + redis_client = await get_redis_client() + redis_key = f"oauth_state:{state_token}" + + # Atomic retrieval and deletion using Lua script + lua_script = """ + local key = KEYS[1] + local expected = ARGV[1] + local stored = redis.call('GET', key) + + if stored == expected then + redis.call('DEL', key) + return 1 + else + return 0 + end + """ + + result = await redis_client.eval(lua_script, 1, redis_key, expected_session_id) + + if result == 1: + logger.info(f"OAuth state validated successfully for session: {expected_session_id[:8]}...") + return True + else: + logger.warning("OAuth state validation failed - mismatch or missing state") + return False + + except Exception as e: + logger.error(f"OAuth state validation error: {str(e)}", exc_info=True) + return False + async def login_with_oauth(provider: str, redirect_to: Optional[str] = None, state: Optional[str] = None): """ Generates an asynchronous OAuth sign-in URL. """ supabase = get_supabase_client() try: - options = {} + options: Dict[str, Any] = {} if redirect_to: options['redirect_to'] = redirect_to if state: @@ -41,7 +97,7 @@ 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="") + 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: diff --git a/backend/integrations/discord/cogs.py b/backend/integrations/discord/cogs.py index 64fd0b7f..f04676a4 100644 --- a/backend/integrations/discord/cogs.py +++ b/backend/integrations/discord/cogs.py @@ -146,8 +146,24 @@ async def verify_github(self, interaction: discord.Interaction): if not session_id: raise Exception("Failed to create verification session.") + # 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) + except Exception as e: + logger.error(f"Failed to generate OAuth state: {e}") + error_embed = discord.Embed( + title="❌ Verification Error", + description="Verification service is temporarily unavailable. Please try again.", + color=discord.Color.red() + ) + await interaction.followup.send(embed=error_embed, ephemeral=True) + return + callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" - auth_url_data = await login_with_github(redirect_to=callback_url) + + # Pass state parameter to OAuth flow (RFC 6749 compliant) + auth_url_data = await login_with_github(redirect_to=callback_url, state=state_token) auth_url = auth_url_data.get("url") if not auth_url: raise Exception("Failed to generate OAuth URL.") @@ -499,7 +515,25 @@ async def _send_onboarding_flow(self, user: discord.abc.User) -> str: # Generate GitHub OAuth URL via Supabase callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" - auth = await login_with_github(redirect_to=callback_url) + + # 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) + except Exception as e: + logger.error(f"Failed to generate OAuth state in onboarding: {e}") + try: + await user.send("Couldn't generate a verification link. Please use /verify_github.") + await user.send(build_encourage_verification_message(reminder_count=1)) + await user.send(embed=build_final_handoff_embed()) + except discord.Forbidden: + logger.warning(f"Cannot DM user {user.id} after auth state failure (DMs disabled)") + except Exception as send_error: + logger.exception( + f"Failed to send auth state failure fallback DM to user {user.id}: {send_error}") + return "auth_unavailable" + + auth = await login_with_github(redirect_to=callback_url, state=state_token) auth_url = auth.get("url") if not auth_url: try: From 2c599e6781c418b07e3c11b693f0aaaa23ade252 Mon Sep 17 00:00:00 2001 From: adithya32 <163162210+KumarADITHYA123@users.noreply.github.com> Date: Thu, 5 Feb 2026 09:44:51 +0530 Subject: [PATCH 2/3] refactor: apply CodeRabbit review (extract helper, improve log stacks) --- backend/integrations/discord/cogs.py | 71 ++++++++++++++-------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/backend/integrations/discord/cogs.py b/backend/integrations/discord/cogs.py index f04676a4..9d64ec5e 100644 --- a/backend/integrations/discord/cogs.py +++ b/backend/integrations/discord/cogs.py @@ -1,5 +1,6 @@ import logging import asyncio +from typing import Optional, Tuple import discord from discord import app_commands from discord.ext import commands, tasks @@ -14,7 +15,7 @@ from app.core.orchestration.queue_manager import AsyncQueueManager, QueuePriority from app.services.auth.management import get_or_create_user_by_discord -from app.services.auth.supabase import login_with_github +from app.services.auth.supabase import login_with_github, store_oauth_state from app.services.auth.verification import create_verification_session, cleanup_expired_tokens from app.services.codegraph.repo_service import RepoService from integrations.discord.bot import DiscordBot @@ -22,6 +23,26 @@ logger = logging.getLogger(__name__) +async def _generate_secure_github_auth_url(session_id: str, callback_url: str) -> Tuple[Optional[str], Optional[str]]: + """ + Generate OAuth URL with CSRF state protection. + Returns (auth_url, error_message) tuple. + """ + try: + # SECURITY FIX: Generate and store OAuth state parameter + 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." + + # 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") + if not auth_url: + return None, "Couldn't generate a verification link. Please use /verify_github." + + return auth_url, None + class DevRelCommands(commands.Cog): def __init__(self, bot: DiscordBot, queue_manager: AsyncQueueManager): self.bot = bot @@ -146,25 +167,20 @@ async def verify_github(self, interaction: discord.Interaction): if not session_id: raise Exception("Failed to create verification session.") - # 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) - except Exception as e: - logger.error(f"Failed to generate OAuth state: {e}") + callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" + + # Use helper to generate secure auth URL + auth_url, error_msg = await _generate_secure_github_auth_url(session_id, callback_url) + + if error_msg: error_embed = discord.Embed( title="❌ Verification Error", - description="Verification service is temporarily unavailable. Please try again.", + description=error_msg, color=discord.Color.red() ) await interaction.followup.send(embed=error_embed, ephemeral=True) return - callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" - - # Pass state parameter to OAuth flow (RFC 6749 compliant) - auth_url_data = await login_with_github(redirect_to=callback_url, state=state_token) - auth_url = auth_url_data.get("url") if not auth_url: raise Exception("Failed to generate OAuth URL.") @@ -516,34 +532,19 @@ async def _send_onboarding_flow(self, user: discord.abc.User) -> str: # Generate GitHub OAuth URL via Supabase callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" - # 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) - except Exception as e: - logger.error(f"Failed to generate OAuth state in onboarding: {e}") - try: - await user.send("Couldn't generate a verification link. Please use /verify_github.") - await user.send(build_encourage_verification_message(reminder_count=1)) - await user.send(embed=build_final_handoff_embed()) - except discord.Forbidden: - logger.warning(f"Cannot DM user {user.id} after auth state failure (DMs disabled)") - except Exception as send_error: - logger.exception( - f"Failed to send auth state failure fallback DM to user {user.id}: {send_error}") - return "auth_unavailable" + # Use helper to generate secure auth URL + auth_url, error_msg = await _generate_secure_github_auth_url(session_id, callback_url) - auth = await login_with_github(redirect_to=callback_url, state=state_token) - auth_url = auth.get("url") - if not auth_url: + if error_msg: + # Handle error (fallback logic) try: await user.send("Couldn't generate a verification link. Please use /verify_github.") await user.send(build_encourage_verification_message(reminder_count=1)) await user.send(embed=build_final_handoff_embed()) except discord.Forbidden: - logger.warning(f"Cannot DM user {user.id} after auth URL failure (DMs disabled)") - except Exception as e: - logger.exception(f"Failed to send auth failure fallback DM to user {user.id}: {e}") + logger.warning(f"Cannot DM user {user.id} after auth failure (DMs disabled)") + except Exception as send_error: + logger.exception(f"Failed to send auth failure fallback DM to user {user.id}") return "auth_unavailable" # Send welcome DM with actions (auth_url may be None when button disabled) From ed4a938be2ab87d742d9464c5ce3b0c852b021cf Mon Sep 17 00:00:00 2001 From: adithya32 <163162210+KumarADITHYA123@users.noreply.github.com> Date: Thu, 5 Feb 2026 09:52:09 +0530 Subject: [PATCH 3/3] style: remove unused exception variable (Ruff F841) --- backend/integrations/discord/cogs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/integrations/discord/cogs.py b/backend/integrations/discord/cogs.py index 9d64ec5e..4dcd67bf 100644 --- a/backend/integrations/discord/cogs.py +++ b/backend/integrations/discord/cogs.py @@ -543,7 +543,7 @@ async def _send_onboarding_flow(self, user: discord.abc.User) -> str: await user.send(embed=build_final_handoff_embed()) except discord.Forbidden: logger.warning(f"Cannot DM user {user.id} after auth failure (DMs disabled)") - except Exception as send_error: + except Exception: logger.exception(f"Failed to send auth failure fallback DM to user {user.id}") return "auth_unavailable"