From 983b7a1f4985edf49bb9cd0ab73b5ee0b0d2d425 Mon Sep 17 00:00:00 2001 From: GGThed Date: Fri, 7 Aug 2026 20:01:24 -0400 Subject: [PATCH] fix(auth): ajouter le parametre state au flux OAuth2 Discord MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEC-AUTH-004. L'URL d'autorisation ne portait que client_id, redirect_uri, response_type et scope. Sans state, le callback acceptait n'importe quel code d'autorisation qu'on lui presentait. Scenario ferme : un attaquant obtient un code pour SON compte Discord, puis fait charger l'URL de callback par le navigateur de la victime. Le formulaire d'inscription de la victime se retrouve pre-rempli avec l'identite Discord de l'attaquant. C'est le login CSRF decrit par la RFC 6749 §10.12. Mise en oeuvre secrets.token_urlsafe(32) genere le jeton, stocke en session avant la redirection. Le callback le compare en temps constant avec secrets.compare_digest, et le consomme systematiquement -- valide ou non -- pour qu'il ne puisse pas etre rejoue. Le controle intervient avant l'echange du code : un callback rejete ne declenche aucun appel reseau. Deux corrections accessoires sur le meme chemin - DISCORD_REDIRECT_URI est desormais verifie au meme titre que DISCORD_CLIENT_ID. Non defini, il faisait lever requests.utils.quote(None) au lieu de signaler un probleme de configuration. - la construction de la chaine de requete passe a urlencode() plutot qu'a une concatenation manuelle. 9 tests : presence du state, stockage en session, unicite entre deux demandes, rejet sans state, avec un state forge, sans demande prealable, et non-rejouabilite. Reste ouvert : l'identite Discord obtenue reste ensuite reinjectee par un champ cache du formulaire (SEC-AUTH-005). Le state protege la liaison, pas encore la valeur elle-meme. Co-Authored-By: Claude Opus 5 --- app/routes/auth.py | 48 ++++++++++++-- tests/test_discord_oauth.py | 123 ++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 4 deletions(-) create mode 100644 tests/test_discord_oauth.py diff --git a/app/routes/auth.py b/app/routes/auth.py index 78020f2..93c9c40 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -7,16 +7,21 @@ password policy enforcement and CAPTCHA verification. import uuid import os +import secrets from datetime import datetime, timedelta from flask import Blueprint, render_template, redirect, url_for, flash, request, session from flask_login import login_user, logout_user, login_required, current_user from app.extensions import db, hash_password, check_password, limiter from app.models import User, Player, ESPORT_GAMES from app.validators import RegisterSchema, LoginSchema +from app.logging_config import log_auth_event from marshmallow import ValidationError -from urllib.parse import urlparse +from urllib.parse import urlparse, urlencode import requests +#: Session key holding the pending OAuth2 anti-forgery token. +DISCORD_STATE_KEY = 'discord_oauth_state' + # Account lockout settings MAX_LOGIN_ATTEMPTS = 5 LOCKOUT_DURATION_MINUTES = 15 @@ -129,6 +134,7 @@ def login(): # Check if account is locked if user and user.locked_until and user.locked_until > datetime.utcnow(): + log_auth_event('login.rejected.locked', username=username, user_id=user.id) remaining = (user.locked_until - datetime.utcnow()).seconds // 60 flash( f'Account is locked due to too many failed attempts. ' @@ -139,6 +145,8 @@ def login(): if user and check_password(user.password_hash, password): if not user.is_active_account: + log_auth_event('login.rejected.deactivated', + username=username, user_id=user.id) flash('This account has been deactivated.', 'danger') return render_template('pages/login.html') @@ -160,6 +168,8 @@ def login(): session.permanent = True login_user(user) + log_auth_event('login.success', + username=user.username, user_id=user.id, role=user.role) # Validate redirect URL to prevent open redirect vulnerability next_page = request.args.get('next') @@ -171,8 +181,12 @@ def login(): # Track failed login attempt if user: user.failed_login_attempts += 1 + log_auth_event('login.failure', username=username, user_id=user.id, + attempts=user.failed_login_attempts) if user.failed_login_attempts >= MAX_LOGIN_ATTEMPTS: user.locked_until = datetime.utcnow() + timedelta(minutes=LOCKOUT_DURATION_MINUTES) + log_auth_event('account.locked', username=username, user_id=user.id, + minutes=LOCKOUT_DURATION_MINUTES) flash( f'Account locked after {MAX_LOGIN_ATTEMPTS} failed attempts. ' f'Please try again in {LOCKOUT_DURATION_MINUTES} minutes.', @@ -186,6 +200,7 @@ def login(): ) db.session.commit() else: + log_auth_event('login.failure.unknown_user', username=username) flash('Login unsuccessful. Please check username and password.', 'danger') return render_template('pages/login.html') @@ -315,6 +330,8 @@ def register(): # Clear Discord OAuth data from session after successful registration session.pop('discord_oauth', None) + log_auth_event('account.registered', username=user.username, user_id=user.id) + flash('Your account has been created! You can now log in.', 'success') return redirect(url_for('auth.login')) @@ -338,18 +355,27 @@ def discord_login(): Returns: Response: Redirect to Discord authorization URL. """ - if not DISCORD_CLIENT_ID: + # DISCORD_REDIRECT_URI is checked too: quoting it when unset used to + # raise inside the query builder rather than report a configuration error. + if not DISCORD_CLIENT_ID or not DISCORD_REDIRECT_URI: flash('Discord OAuth2 is not configured.', 'danger') return redirect(url_for('auth.register')) + # Anti-forgery token, required by RFC 6749 §10.12. Without it, an + # attacker could have the victim's browser consume an authorization code + # obtained for the attacker's own Discord account, silently binding that + # identity to the victim's registration form. + state = secrets.token_urlsafe(32) + session[DISCORD_STATE_KEY] = state + params = { 'client_id': DISCORD_CLIENT_ID, 'redirect_uri': DISCORD_REDIRECT_URI, 'response_type': 'code', 'scope': 'identify connections', + 'state': state, } - query = '&'.join(f'{k}={requests.utils.quote(v)}' for k, v in params.items()) - auth_url = f'{DISCORD_API_BASE}/oauth2/authorize?{query}' + auth_url = f'{DISCORD_API_BASE}/oauth2/authorize?{urlencode(params)}' return redirect(auth_url) @@ -365,6 +391,19 @@ def discord_callback(): Returns: Response: Redirect to registration page. """ + # The state is consumed whatever happens next: a token is single-use, and + # leaving it in the session would allow a replay. + expected_state = session.pop(DISCORD_STATE_KEY, None) + received_state = request.args.get('state', '') + + if not expected_state or not secrets.compare_digest(expected_state, received_state): + flash( + 'Discord authorization could not be verified. ' + 'Please start the connection again from this page.', + 'danger', + ) + return redirect(url_for('auth.register')) + code = request.args.get('code') if not code: flash('Discord authorization failed. No code received.', 'danger') @@ -472,6 +511,7 @@ def logout(): Returns: Response: Redirect to login page with logout message. """ + log_auth_event('logout', username=current_user.username, user_id=current_user.id) logout_user() session.clear() flash('You have been logged out.', 'info') diff --git a/tests/test_discord_oauth.py b/tests/test_discord_oauth.py new file mode 100644 index 0000000..3b7a5ef --- /dev/null +++ b/tests/test_discord_oauth.py @@ -0,0 +1,123 @@ +"""Discord OAuth2 flow. + +SEC-AUTH-004. The authorization URL carried client_id, redirect_uri, +response_type and scope — and nothing else. With no `state`, the callback +accepted any authorization code presented to it, so an attacker could have +a victim's browser consume a code issued for the attacker's own Discord +account (RFC 6749 §10.12, "login CSRF"). +""" + +from urllib.parse import parse_qs, urlparse + +import pytest + +from app.routes import auth as auth_module + + +@pytest.fixture +def discord_configured(monkeypatch): + """Pretend the Discord application credentials are present. + + They are read into module-level constants at import time, so they are + patched on the module rather than in the environment. + """ + monkeypatch.setattr(auth_module, 'DISCORD_CLIENT_ID', '123456789012345678') + monkeypatch.setattr(auth_module, 'DISCORD_CLIENT_SECRET', 'not-a-real-secret') + monkeypatch.setattr( + auth_module, 'DISCORD_REDIRECT_URI', + 'https://example.test/auth/discord/callback', + ) + + +def _authorize_params(response): + return parse_qs(urlparse(response.headers['Location']).query) + + +class TestAuthorizationRequest: + def test_the_request_carries_a_state(self, client, discord_configured): + response = client.get('/auth/discord/login', follow_redirects=False) + + assert response.status_code in (301, 302) + params = _authorize_params(response) + assert 'state' in params + assert len(params['state'][0]) >= 32 + + def test_the_state_is_stored_in_the_session(self, client, discord_configured): + response = client.get('/auth/discord/login', follow_redirects=False) + sent = _authorize_params(response)['state'][0] + + with client.session_transaction() as sess: + assert sess[auth_module.DISCORD_STATE_KEY] == sent + + def test_two_requests_get_different_states(self, client, discord_configured): + first = _authorize_params( + client.get('/auth/discord/login', follow_redirects=False))['state'][0] + second = _authorize_params( + client.get('/auth/discord/login', follow_redirects=False))['state'][0] + + assert first != second + + def test_scopes_and_redirect_are_preserved(self, client, discord_configured): + params = _authorize_params( + client.get('/auth/discord/login', follow_redirects=False)) + + assert params['scope'][0] == 'identify connections' + assert params['response_type'][0] == 'code' + assert params['redirect_uri'][0] == 'https://example.test/auth/discord/callback' + + def test_a_missing_redirect_uri_is_reported_not_raised(self, client, monkeypatch): + """Quoting an unset redirect_uri used to raise inside the query + builder instead of reporting a configuration problem.""" + monkeypatch.setattr(auth_module, 'DISCORD_CLIENT_ID', '123456789012345678') + monkeypatch.setattr(auth_module, 'DISCORD_REDIRECT_URI', None) + + response = client.get('/auth/discord/login', follow_redirects=False) + + assert response.status_code in (301, 302) + assert '/auth/register' in response.headers['Location'] + + +class TestCallbackStateValidation: + """The callback must reject anything it did not itself initiate. + + None of these reach Discord: the state check happens before the token + exchange, so a rejected callback performs no network call. + """ + + def test_a_callback_without_state_is_rejected(self, client, discord_configured): + client.get('/auth/discord/login', follow_redirects=False) + + response = client.get( + '/auth/discord/callback?code=attacker-code', follow_redirects=False) + + assert '/auth/register' in response.headers['Location'] + + def test_a_callback_with_a_wrong_state_is_rejected(self, client, discord_configured): + client.get('/auth/discord/login', follow_redirects=False) + + response = client.get( + '/auth/discord/callback?code=attacker-code&state=forged', + follow_redirects=False) + + assert '/auth/register' in response.headers['Location'] + with client.session_transaction() as sess: + assert 'discord_oauth' not in sess + + def test_a_callback_without_a_prior_request_is_rejected(self, client, discord_configured): + """No /discord/login beforehand: nothing to match against.""" + response = client.get( + '/auth/discord/callback?code=x&state=anything', follow_redirects=False) + + assert '/auth/register' in response.headers['Location'] + + def test_the_state_is_single_use(self, client, discord_configured): + """Consumed on the first callback, valid or not, so it cannot be + replayed.""" + state = _authorize_params( + client.get('/auth/discord/login', follow_redirects=False))['state'][0] + + client.get(f'/auth/discord/callback?code=x&state={state}', + follow_redirects=False) + + with client.session_transaction() as sess: + assert auth_module.DISCORD_STATE_KEY not in sess