diff --git a/backend/app/api/v1/auth.py b/backend/app/api/v1/auth.py index 0d0bacb7..a9182087 100644 --- a/backend/app/api/v1/auth.py +++ b/backend/app/api/v1/auth.py @@ -1,7 +1,11 @@ from fastapi import APIRouter, Request, HTTPException, Query, Depends from fastapi.responses import HTMLResponse from app.database.supabase.client import get_supabase_client -from app.services.auth.verification import find_user_by_session_and_verify, get_verification_session_info +from app.services.auth.verification import ( + find_user_by_session_and_verify, + get_verification_session_info, + validate_oauth_state, +) from app.services.github.user.profiling import profile_user_from_github from typing import Optional import logging @@ -21,6 +25,7 @@ async def auth_callback( request: Request, code: Optional[str] = Query(None), session: Optional[str] = Query(None), + state: Optional[str] = Query(None), app_instance: "DevRAIApplication" = Depends(get_app_instance), ): """ @@ -37,6 +42,12 @@ async def auth_callback( logger.error("Missing session ID in callback") return _error_response("Missing session ID. Please try the /verify_github command again.") + # Validate the OAuth state parameter to protect against login CSRF (RFC 6749). + # The state is bound to the verification session when the OAuth URL is created. + if not validate_oauth_state(session, state): + logger.error(f"OAuth state validation failed for session ID: {session}") + return _error_response("Invalid or missing security token. 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: diff --git a/backend/app/services/auth/verification.py b/backend/app/services/auth/verification.py index cbfa156c..239e05b1 100644 --- a/backend/app/services/auth/verification.py +++ b/backend/app/services/auth/verification.py @@ -1,4 +1,5 @@ import uuid +import secrets from datetime import datetime, timedelta from typing import Optional, Dict, Tuple from app.database.supabase.client import get_supabase_client @@ -7,8 +8,8 @@ logger = logging.getLogger(__name__) -# session_id -> (discord_id, expiry_time) -_verification_sessions: Dict[str, Tuple[str, datetime]] = {} +# session_id -> (discord_id, expiry_time, oauth_state) +_verification_sessions: Dict[str, Tuple[str, datetime, str]] = {} SESSION_EXPIRY_MINUTES = 5 @@ -18,21 +19,25 @@ def _cleanup_expired_sessions(): """ current_time = datetime.now() expired_sessions = [ - session_id for session_id, (discord_id, expiry_time) in _verification_sessions.items() + session_id for session_id, (discord_id, expiry_time, _state) in _verification_sessions.items() if current_time > expiry_time ] for session_id in expired_sessions: - discord_id, _ = _verification_sessions[session_id] + discord_id, _, _ = _verification_sessions[session_id] del _verification_sessions[session_id] logger.info(f"Cleaned up expired verification session {session_id} for Discord user {discord_id}") if expired_sessions: logger.info(f"Cleaned up {len(expired_sessions)} expired verification sessions") -async def create_verification_session(discord_id: str) -> Optional[str]: +async def create_verification_session(discord_id: str) -> Optional[Tuple[str, str]]: """ - Create a verification session with expiry and return session ID. + Create a verification session with expiry and return (session_id, oauth_state). + + The OAuth ``state`` is a cryptographically-random token bound to the session. + It must be threaded through the OAuth authorize URL and validated in the + callback to protect against login CSRF (RFC 6749, Section 10.12). """ supabase = get_supabase_client() @@ -40,6 +45,7 @@ async def create_verification_session(discord_id: str) -> Optional[str]: token = str(uuid.uuid4()) session_id = str(uuid.uuid4()) + oauth_state = secrets.token_urlsafe(32) expiry_time = datetime.now() + timedelta(minutes=SESSION_EXPIRY_MINUTES) try: @@ -50,16 +56,44 @@ async def create_verification_session(discord_id: str) -> Optional[str]: }).eq("discord_id", discord_id).execute() if update_res.data: - _verification_sessions[session_id] = (discord_id, expiry_time) + _verification_sessions[session_id] = (discord_id, expiry_time, oauth_state) logger.info( f"Created verification session {session_id} for Discord user {discord_id}, expires at {expiry_time}") - return session_id + return session_id, oauth_state logger.error(f"Failed to set verification token for Discord ID: {discord_id}. User not found.") return None except Exception as e: logger.error(f"Error creating verification session for Discord ID {discord_id}: {str(e)}") return None +def validate_oauth_state(session_id: str, state: Optional[str]) -> bool: + """ + Validate the OAuth ``state`` returned in the callback against the value + bound to the verification session. + + Uses a constant-time comparison and rejects missing/expired sessions or any + mismatch. Does not consume the session (the session itself is consumed by + :func:`find_user_by_session_and_verify`). + """ + _cleanup_expired_sessions() + + if not state: + logger.warning(f"OAuth state missing in callback for session ID: {session_id}") + return False + + session_data = _verification_sessions.get(session_id) + if not session_data: + logger.warning(f"No verification session found while validating state for session ID: {session_id}") + return False + + _discord_id, _expiry_time, expected_state = session_data + + if not secrets.compare_digest(state, expected_state): + logger.warning(f"OAuth state mismatch for session ID: {session_id}") + return False + + return True + async def find_user_by_session_and_verify( session_id: str, github_id: str, github_username: str, email: Optional[str] ) -> Optional[User]: @@ -77,7 +111,7 @@ async def find_user_by_session_and_verify( logger.warning(f"No verification session found for session ID: {session_id}") return None - discord_id, expiry_time = session_data + discord_id, _expiry_time, _oauth_state = session_data current_time = datetime.now().isoformat() user_res = await supabase.table("users").select("*").eq( @@ -163,7 +197,7 @@ async def get_verification_session_info(session_id: str) -> Optional[Dict[str, s if not session_data: return None - discord_id, expiry_time = session_data + discord_id, expiry_time, _oauth_state = session_data if datetime.now() > expiry_time: del _verification_sessions[session_id] diff --git a/backend/integrations/discord/cogs.py b/backend/integrations/discord/cogs.py index 64fd0b7f..747bcd47 100644 --- a/backend/integrations/discord/cogs.py +++ b/backend/integrations/discord/cogs.py @@ -142,12 +142,13 @@ async def verify_github(self, interaction: discord.Interaction): await interaction.followup.send(embed=embed, ephemeral=True) return - session_id = await create_verification_session(str(interaction.user.id)) - if not session_id: + session = await create_verification_session(str(interaction.user.id)) + if not session: raise Exception("Failed to create verification session.") + session_id, oauth_state = session callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" - auth_url_data = await login_with_github(redirect_to=callback_url) + auth_url_data = await login_with_github(redirect_to=callback_url, state=oauth_state) auth_url = auth_url_data.get("url") if not auth_url: raise Exception("Failed to generate OAuth URL.") @@ -485,8 +486,8 @@ async def _send_onboarding_flow(self, user: discord.abc.User) -> str: if show_oauth_button: # Create verification session - session_id = await create_verification_session(str(user.id)) - if not session_id: + session = await create_verification_session(str(user.id)) + if not session: try: await user.send("I couldn't start verification right now. You can use /verify_github anytime.") await user.send(build_encourage_verification_message(reminder_count=1)) @@ -496,10 +497,11 @@ async def _send_onboarding_flow(self, user: discord.abc.User) -> str: except Exception as e: logger.exception(f"Failed to send session failure fallback DM to user {user.id}: {e}") return "session_unavailable" + session_id, oauth_state = session # 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) + auth = await login_with_github(redirect_to=callback_url, state=oauth_state) auth_url = auth.get("url") if not auth_url: try: diff --git a/tests/test_oauth_verification.py b/tests/test_oauth_verification.py new file mode 100644 index 00000000..bdfa0fea --- /dev/null +++ b/tests/test_oauth_verification.py @@ -0,0 +1,94 @@ +import pytest +from unittest.mock import AsyncMock, MagicMock, patch +from datetime import datetime, timedelta +from app.services.auth.verification import ( + create_verification_session, + validate_oauth_state, + find_user_by_session_and_verify, + get_verification_session_info, + _verification_sessions +) + +@pytest.fixture(autouse=True) +def clean_sessions(): + _verification_sessions.clear() + yield + _verification_sessions.clear() + +@pytest.mark.asyncio +async def test_create_verification_session_success(): + mock_supabase = MagicMock() + mock_execute = AsyncMock() + mock_execute.data = [{"id": "user-123"}] + + mock_table = MagicMock() + mock_update = MagicMock() + mock_eq = MagicMock() + + mock_supabase.table.return_value = mock_table + mock_table.update.return_value = mock_update + mock_update.eq.return_value = mock_eq + mock_eq.execute = mock_execute + + with patch("app.services.auth.verification.get_supabase_client", return_value=mock_supabase): + result = await create_verification_session("discord-id-123") + assert result is not None + session_id, oauth_state = result + assert len(session_id) > 0 + assert len(oauth_state) > 0 + + # Verify it was stored in memory sessions + assert session_id in _verification_sessions + discord_id, expiry, state = _verification_sessions[session_id] + assert discord_id == "discord-id-123" + assert state == oauth_state + +@pytest.mark.asyncio +async def test_validate_oauth_state(): + mock_supabase = MagicMock() + mock_execute = AsyncMock() + mock_execute.data = [{"id": "user-123"}] + mock_supabase.table.return_value.update.return_value.eq.return_value.execute = mock_execute + + with patch("app.services.auth.verification.get_supabase_client", return_value=mock_supabase): + result = await create_verification_session("discord-id-123") + assert result is not None + session_id, oauth_state = result + + # Validation with correct state + assert validate_oauth_state(session_id, oauth_state) is True + + # Validation with incorrect state + assert validate_oauth_state(session_id, "wrong-state") is False + + # Validation with missing state + assert validate_oauth_state(session_id, None) is False + + # Validation with non-existent session + assert validate_oauth_state("wrong-session-id", oauth_state) is False + +@pytest.mark.asyncio +async def test_get_verification_session_info(): + mock_supabase = MagicMock() + mock_execute = AsyncMock() + mock_execute.data = [{"id": "user-123"}] + mock_supabase.table.return_value.update.return_value.eq.return_value.execute = mock_execute + + with patch("app.services.auth.verification.get_supabase_client", return_value=mock_supabase): + result = await create_verification_session("discord-id-123") + assert result is not None + session_id, _ = result + + info = await get_verification_session_info(session_id) + assert info is not None + assert info["discord_id"] == "discord-id-123" + + # Test expired session + _verification_sessions[session_id] = ( + "discord-id-123", + datetime.now() - timedelta(minutes=1), + "state" + ) + expired_info = await get_verification_session_info(session_id) + assert expired_info is None + assert session_id not in _verification_sessions