clubesportsudes/team-tryouts
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(auth): ajouter le parametre state au flux OAuth2 Discord

Browse Source 
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 <[email protected]>
This commit is contained in:
GGThed
2026-08-07 20:01:24 -04:00
co-authored by Claude Opus 5
parent d05e9cde32
commit 983b7a1f49
2 changed files with 167 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/routes/auth.py
+44 -4
View File
@@ -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')
tests/test_discord_oauth.py
+123
View File
@@ -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