From 835a394d3f1a1e47e5fe2891f1e20028ad1c5fdd Mon Sep 17 00:00:00 2001 From: GGThed Date: Sat, 8 Aug 2026 15:04:46 -0400 Subject: [PATCH] fix(auth): enumeration, verrou, deconnexion en GET, redirection ouverte Quatre constats de la liste des gains rapides, tous sur auth.py. SEC-017 -- enumeration de comptes Le formulaire repondait "il vous reste 3 tentative(s)" a un compte connu et "verifiez le nom d utilisateur et le mot de passe" a un inconnu. Le decompte lui-meme etait la fuite : la meme information, etalee sur cinq requetes. S y ajoutait un ecart de temps de reponse, check_password n etant appele que si la ligne existait -- scrypt est cher, l ecart est mesurable. Un seul message pour tous les echecs, et la verification s execute desormais sur les deux branches : contre un hachage aleatoire tire une fois par processus quand l identifiant n existe pas. SEC-018 -- verrou de compte declenchable par un tiers Cinq mauvaises reponses mettaient un compte connu hors service pendant quinze minutes, indefiniment renouvelables. Sur un compte president, c est toute l administration, et aucun ecran ne permettait de defaire. Le compteur et la fenetre restent -- ce sont la trace qu un administrateur lit quand un compte est pilonne, et la fenetre double jusqu a un plafond. Ce qui change : de bons identifiants passent, fenetre ouverte ou non, et remettent le compteur a zero. Le proprietaire du compte ne peut plus etre bloque par un tiers. Ce que cela coute, dit franchement : un verrou dur n arretait de toute facon pas un attaquant ayant trouve le mot de passe -- il lui suffisait d attendre. Le debit de tentatives reste borne par la limite de 10/minute par IP. Une limite par couple (compte, IP) demanderait un stockage dedie ; elle attend Alembic. L evenement account.locked devient account.throttled : "locked" affirmait plus que ce qui se passe. SEC-019 -- deconnexion en GET /auth/logout n avait pas de methods, donc GET, donc hors protection CSRF : n importe quelle page pouvait deconnecter un visiteur avec une balise img. La route passe en POST et l entree de navigation devient un formulaire avec jeton. Le style suit -- les regles .nav-links visaient les liens seuls. SEC-020 -- validation de redirection is_safe_url interrogeait urlparse().netloc. urlparse lit /\evil.com comme un chemin, sans netloc ; plusieurs navigateurs normalisent l antislash en barre oblique avant de resoudre, ce qui en fait //evil.com. La fonction refuse maintenant antislash et caracteres de controle, exige un chemin enracine, et compare l origine explicitement. Le xfail(strict) qui documentait SEC-017 est leve. 40 tests dans test_auth_session.py, dont la table des cibles refusees. Co-Authored-By: Claude Opus 5 --- app/routes/auth.py | 138 +++- app/static/css/style.css | 20 +- app/templates/layouts/base.html | 14 +- app/translations/en/LC_MESSAGES/messages.mo | Bin 44112 -> 43614 bytes app/translations/en/LC_MESSAGES/messages.po | 780 ++++++++++-------- app/translations/fr/LC_MESSAGES/messages.mo | Bin 48303 -> 47792 bytes app/translations/fr/LC_MESSAGES/messages.po | 824 ++++++++++++-------- tests/test_audit_logging.py | 8 +- tests/test_auth_session.py | 177 ++++- tests/test_i18n.py | 2 +- tests/test_role_change.py | 2 +- 11 files changed, 1245 insertions(+), 720 deletions(-) diff --git a/app/routes/auth.py b/app/routes/auth.py index 4c7f708..b9c972e 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -24,9 +24,17 @@ import requests #: Session key holding the pending OAuth2 anti-forgery token. DISCORD_STATE_KEY = 'discord_oauth_state' -# Account lockout settings +# Failed-attempt tracking. The tally is kept for the audit trail and for the +# cool-off marker below; it no longer refuses a correct password (SEC-018). MAX_LOGIN_ATTEMPTS = 5 LOCKOUT_DURATION_MINUTES = 15 +#: Ceiling on the doubling cool-off window. +MAX_LOCKOUT_MINUTES = 240 + +#: Hash of a value nobody can submit. Verifying against it when the username +#: is unknown makes that path cost the same scrypt work as a real one, so the +#: response time stops telling a caller which usernames exist (SEC-017). +_ABSENT_USER_HASH = None # Discord OAuth2 configuration DISCORD_CLIENT_ID = os.getenv('DISCORD_CLIENT_ID') @@ -47,17 +55,64 @@ DISCORD_PLATFORM_TO_GAMES = { def is_safe_url(url): """Validate that a URL is safe for redirection (same origin). + Accepts an absolute URL on this host, or a path beginning with exactly + one slash. Everything else is refused, including the two forms that + read differently to urlparse and to a browser: + + /\\evil.com several browsers normalise the backslash to a slash, + turning this into the protocol-relative //evil.com. + urlparse reports no netloc at all, so the old check + let it through and the redirect left the site. + /\\n//evil.com control characters are stripped before parsing. + Args: url: The URL to validate. Returns: - bool: True if the URL is safe (relative or same origin). + bool: True if the URL is safe. """ if not url: return False + if any(ord(char) < 0x20 or char in '\\\x7f' for char in url): + return False + parsed = urlparse(url) - # Allow relative URLs (no netloc) or same-origin URLs - return not parsed.netloc or parsed.netloc == request.host + if parsed.netloc: + return (parsed.netloc == request.host + and parsed.scheme in ('', 'http', 'https')) + # Relative targets must be rooted. 'dashboard' or 'javascript:...' are + # not paths on this site. + return url.startswith('/') + + +def _absent_user_hash(): + """A hash to verify against when the submitted username does not exist. + + check_password() used to be reached only when a user row was found, so + an unknown username answered as fast as the database lookup, and a known + one as slowly as scrypt. The gap is measurable and enumerates accounts. + Computed once per process, from a random secret, so no submitted password + can ever match it. + """ + global _ABSENT_USER_HASH + if _ABSENT_USER_HASH is None: + _ABSENT_USER_HASH = hash_password(secrets.token_urlsafe(32)) + return _ABSENT_USER_HASH + + +def cooloff_minutes(failed_attempts): + """Length of the cool-off window earned by this many failed attempts. + + Doubles every MAX_LOGIN_ATTEMPTS further failures, up to a ceiling. + + Args: + failed_attempts: Consecutive failures recorded on the account. + + Returns: + int: Minutes. + """ + steps = max(failed_attempts // MAX_LOGIN_ATTEMPTS - 1, 0) + return min(LOCKOUT_DURATION_MINUTES * (2 ** steps), MAX_LOCKOUT_MINUTES) def generate_captcha(): @@ -102,16 +157,17 @@ auth_bp = Blueprint('auth', __name__, url_prefix='/auth') @auth_bp.route('/login', methods=['GET', 'POST']) @limiter.limit("10 per minute") def login(): - """Handle user login authentication with account lockout protection. + """Handle user login authentication. GET: Render the login form. - POST: Authenticate user credentials with lockout check and audit logging. + POST: Authenticate user credentials, with audit logging. - Account lockout: After 5 consecutive failed attempts, the account is - locked for 15 minutes. Successful login resets the counter. - - Redirects authenticated users to dashboard. Validates credentials and checks - account status before login. + Failed attempts are counted and open a cool-off window, recorded in + ``locked_until`` and in the authentication log. The window does not + refuse correct credentials: when it did, five wrong guesses against a + known username took that account out of service for fifteen minutes, + repeatably, and on a president's account that meant no administration + at all. Guess rate is bounded by the rate limit on this view. Returns: Response: Login form or redirect to dashboard/next page. @@ -134,25 +190,23 @@ def login(): password = validated['password'] user = User.query.filter_by(username=username).first() - # 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( - _('Account is locked due to too many failed attempts. ' - 'Please try again in %(remaining)s minute(s).', remaining=remaining), - 'danger' - ) - return render_template('pages/login.html') + # Verified before anything else is decided, and on both branches. + # Reaching this only when a row exists made the response time a + # reliable oracle for which usernames are registered (SEC-017). + credentials_ok = check_password( + user.password_hash if user else _absent_user_hash(), password + ) - if user and check_password(user.password_hash, password): + if user and credentials_ok: 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') - # Reset failed login attempts on successful login + # Correct credentials clear the tally, cool-off window included. + # The window used to refuse them too, which is what turned it + # into a way to lock a known account out at will (SEC-018). user.failed_login_attempts = 0 user.locked_until = None db.session.commit() @@ -188,33 +242,26 @@ def login(): flash(_('Welcome back, %(username)s!', username=user.username), 'success') return redirect(next_page) if next_page else redirect(url_for('main.dashboard')) else: - # Track failed login attempt + # One message for every failure. The old code said "N attempts + # remaining" to a real account and "check username and password" + # to an unknown one, which listed the club's accounts to anyone + # who asked (SEC-017). 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( - _('Account locked after %(attempts)s failed attempts. ' - 'Please try again in %(minutes)s minutes.', - attempts=MAX_LOGIN_ATTEMPTS, - minutes=LOCKOUT_DURATION_MINUTES), - 'danger' - ) - else: - remaining = MAX_LOGIN_ATTEMPTS - user.failed_login_attempts - flash( - _('Login unsuccessful. %(remaining)s attempt(s) remaining ' - 'before lockout.', remaining=remaining), - 'danger' - ) + minutes = cooloff_minutes(user.failed_login_attempts) + user.locked_until = datetime.utcnow() + timedelta(minutes=minutes) + log_auth_event('account.throttled', username=username, + user_id=user.id, minutes=minutes, + attempts=user.failed_login_attempts) db.session.commit() else: log_auth_event('login.failure.unknown_user', username=username) - flash(_('Login unsuccessful. Please check username and password.'), 'danger') + + flash(_('Login unsuccessful. Please check your username and ' + 'password, or ask a president for help.'), 'danger') return render_template('pages/login.html') @@ -513,11 +560,16 @@ def discord_callback(): return redirect(url_for('auth.register')) -@auth_bp.route('/logout') +@auth_bp.route('/logout', methods=['POST']) @login_required def logout(): """Log out the current user and clear the session. + POST, not GET: a GET route is not covered by CSRF protection, so any + page on the internet could sign a user out with an tag pointing + here. A nuisance rather than a compromise, but it costs one form to + close (SEC-019). + Clears the user session and regenerates session ID to prevent session fixation/replay after logout. diff --git a/app/static/css/style.css b/app/static/css/style.css index 44fad61..48cff1e 100644 --- a/app/static/css/style.css +++ b/app/static/css/style.css @@ -138,7 +138,10 @@ a:hover { color: var(--primary-dark); } padding: 10px 0; } -.nav-links li a { +/* Logging out is a POST, so its nav entry is a button inside a form + rather than a link. It has to read as one of the entries above it. */ +.nav-links li a, +.nav-links li .nav-form button { display: flex; align-items: center; gap: 12px; @@ -148,7 +151,17 @@ a:hover { color: var(--primary-dark); } font-size: 0.9rem; } -.nav-links li a:hover, .nav-links li a.active { +.nav-links li .nav-form button { + width: 100%; + background: none; + border: 0; + font-family: inherit; + text-align: left; + cursor: pointer; +} + +.nav-links li a:hover, .nav-links li a.active, +.nav-links li .nav-form button:hover { background: rgba(255,255,255,0.08); color: white; } @@ -158,7 +171,8 @@ a:hover { color: var(--primary-dark); } padding-left: 17px; } -.nav-links li a i { width: 20px; text-align: center; font-size: 1.1rem; } +.nav-links li a i, +.nav-links li .nav-form button i { width: 20px; text-align: center; font-size: 1.1rem; } .nav-divider { height: 1px; diff --git a/app/templates/layouts/base.html b/app/templates/layouts/base.html index 9c0dd63..2d29dc3 100644 --- a/app/templates/layouts/base.html +++ b/app/templates/layouts/base.html @@ -112,10 +112,16 @@
  • - - - {{ _('Logout') }} - + {# A form, not a link: logging out is a state change, and a + GET route carries no CSRF token — any site could sign the + user out with an tag. Styled as a nav entry. #} +