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

fix(auth): enumeration, verrou, deconnexion en GET, redirection ouverte

Browse Source 
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 <[email protected]>
This commit is contained in:
GGThed
2026-08-08 15:05:13 -04:00
co-authored by Claude Opus 5
parent d85da32ef5
commit 835a394d3f
11 changed files with 1245 additions and 720 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/routes/auth.py
+94 -42
View File
@@ -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'
# 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
)
return render_template('pages/login.html')
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 <img> 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.
app/static/css/style.css
+17 -3
View File
@@ -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;
app/templates/layouts/base.html
+8 -2
View File
@@ -112,10 +112,16 @@
</a>
</li>
<li>
<a href="{{ url_for('auth.logout') }}" class="logout-link">
{# 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 <img> tag. Styled as a nav entry. #}
<form method="POST" action="{{ url_for('auth.logout') }}" class="nav-form">
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}"/>
<button type="submit" class="logout-link">
<i class="fas fa-sign-out-alt"></i>
<span>{{ _('Logout') }}</span>
</a>
</button>
</form>
</li>
<li class="nav-divider"></li>
<li class="nav-language">
app/translations/en/LC_MESSAGES/messages.mo
BIN
View File
Binary file not shown.
app/translations/en/LC_MESSAGES/messages.po
+463 -317
View File
File diff suppressed because it is too large Load Diff
app/translations/fr/LC_MESSAGES/messages.mo
BIN
View File
Binary file not shown.
app/translations/fr/LC_MESSAGES/messages.po
+482 -342
View File
File diff suppressed because it is too large Load Diff
tests/test_audit_logging.py
+5 -3
View File
@@ -70,7 +70,9 @@ class TestLoginEvents:
login('no-such-account', password='WrongPassword1')
assert _has_event(auth_log, 'login.failure.unknown_user')
def test_a_lockout_is_recorded(self, app, client, make_user, login, auth_log):
def test_a_run_of_failures_is_recorded(self, app, client, make_user, login, auth_log):
"""Renamed from account.locked with SEC-018: the window no longer
refuses the account owner, so 'locked' overstated what happened."""
user_id = make_user('player')
with app.app_context():
username = db.session.get(User, user_id).username
@@ -78,7 +80,7 @@ class TestLoginEvents:
for _ in range(5):
login(username, password='WrongPassword1')
assert _has_event(auth_log, 'account.locked')
assert _has_event(auth_log, 'account.throttled')
def test_a_deactivated_account_attempt_is_recorded(
self, app, client, make_user, login, auth_log
@@ -96,7 +98,7 @@ class TestLoginEvents:
def test_logout_is_recorded(self, client, as_role, auth_log):
as_role('player')
client.get('/auth/logout')
client.post('/auth/logout')
assert _has_event(auth_log, 'logout')
tests/test_auth_session.py
+171 -6
View File
@@ -100,12 +100,44 @@ class TestLogout:
as_role('player')
assert client.get('/users/profile').status_code == 200
client.get('/auth/logout')
client.post('/auth/logout')
response = client.get('/users/profile', follow_redirects=False)
assert response.status_code in (301, 302)
assert '/auth/login' in response.headers.get('Location', '')
def test_a_get_no_longer_logs_anyone_out(self, client, as_role):
"""SEC-019 — a GET route carries no CSRF token, so any page could
sign the user out with <img src=".../auth/logout">."""
as_role('player')
assert client.get('/auth/logout').status_code == 405
assert client.get('/users/profile').status_code == 200
def test_the_logout_control_carries_a_csrf_token(self, app_with_csrf, make_user, login):
"""The nav entry is a form now; without the token it would 400 on
every user, and only in production where CSRF is on."""
client = app_with_csrf.test_client()
with app_with_csrf.app_context():
from app.extensions import hash_password
from app.models import Player
user = Player(username='navtest', password_hash=hash_password('Password123'),
role='player', full_name='Nav Test', email='[email protected]')
db.session.add(user)
db.session.commit()
page = client.get('/auth/login').get_data(as_text=True)
token = re.search(r'name="csrf_token" value="([^"]+)"', page).group(1)
client.post('/auth/login', data={'username': 'navtest',
'password': 'Password123',
'csrf_token': token})
body = client.get('/users/profile').get_data(as_text=True)
form = re.search(
r'<form method="POST" action="/auth/logout".*?</form>', body, re.S)
assert form is not None, 'no logout form in the navigation'
assert 'name="csrf_token"' in form.group(0)
class TestLoginRejection:
def test_wrong_password_is_refused(self, client, make_user, login, app):
@@ -118,11 +150,6 @@ class TestLoginRejection:
response = client.get('/users/profile', follow_redirects=False)
assert response.status_code in (301, 302)
@pytest.mark.xfail(
strict=True,
reason='SEC-AUTH-006: the two branches emit different messages, '
'which lets an unauthenticated caller enumerate accounts',
)
def test_login_failure_message_does_not_reveal_account_existence(
self, client, make_user, app
):
@@ -143,3 +170,141 @@ class TestLoginRejection:
return _flash_messages(body)
assert failure_message(username) == failure_message('no-such-account')
def test_the_message_stays_the_same_past_the_attempt_threshold(
self, client, make_user, app
):
"""The tally used to be counted out loud — "3 attempt(s) remaining"
— which is the same disclosure, spread over five requests."""
user_id = make_user('player')
with app.app_context():
username = db.session.get(User, user_id).username
seen = set()
for _ in range(7):
body = client.post(
'/auth/login',
data={'username': username, 'password': 'WrongPassword1'},
follow_redirects=True,
).get_data(as_text=True)
seen.update(_flash_messages(body))
assert len(seen) == 1, f'the message varies with the tally: {seen}'
class TestFailedAttemptThrottle:
"""SEC-018 — five wrong guesses used to take a known account out of
service for fifteen minutes, repeatably. On a president's account that
is the whole administration, and nothing in the product could undo it."""
@staticmethod
def _exhaust(client, username, times=6):
for _ in range(times):
client.post('/auth/login',
data={'username': username, 'password': 'WrongPassword1'},
follow_redirects=True)
def test_the_owner_still_gets_in_after_the_threshold(
self, app, client, make_user, login
):
user_id = make_user('player')
with app.app_context():
username = db.session.get(User, user_id).username
self._exhaust(client, username)
login(username)
assert client.get('/users/profile').status_code == 200
def test_a_successful_login_clears_the_tally(self, app, client, make_user, login):
user_id = make_user('player')
with app.app_context():
username = db.session.get(User, user_id).username
self._exhaust(client, username)
login(username)
with app.app_context():
user = db.session.get(User, user_id)
assert user.failed_login_attempts == 0
assert user.locked_until is None
def test_the_window_is_still_recorded(self, app, client, make_user):
"""It no longer gates anything, but it is the trace an administrator
reads when an account is being hammered."""
user_id = make_user('player')
with app.app_context():
username = db.session.get(User, user_id).username
self._exhaust(client, username)
with app.app_context():
user = db.session.get(User, user_id)
assert user.failed_login_attempts >= 5
assert user.locked_until is not None
def test_the_window_grows_with_repetition(self):
from app.routes.auth import MAX_LOCKOUT_MINUTES, cooloff_minutes
assert cooloff_minutes(5) == 15
assert cooloff_minutes(10) == 30
assert cooloff_minutes(15) == 60
assert cooloff_minutes(1000) == MAX_LOCKOUT_MINUTES
class TestRedirectValidation:
"""SEC-020 — is_safe_url() asked urlparse for a netloc, and urlparse
reads '/\evil.com' as a path. Several browsers normalise the backslash
to a slash first, which makes it the protocol-relative '//evil.com'."""
REFUSED = [
'//evil.example',
'/\evil.example',
'\\evil.example',
'https://evil.example/phish',
'javascript:alert(1)',
'dashboard',
'/dash\nboard',
'',
None,
]
ACCEPTED = [
'/dashboard',
'/users/profile?tab=games',
'/teams#roster',
]
@pytest.mark.parametrize('target', REFUSED)
def test_a_hostile_target_is_refused(self, app, target):
from app.routes.auth import is_safe_url
with app.test_request_context('/auth/login'):
assert not is_safe_url(target)
@pytest.mark.parametrize('target', ACCEPTED)
def test_an_internal_path_is_accepted(self, app, target):
from app.routes.auth import is_safe_url
with app.test_request_context('/auth/login'):
assert is_safe_url(target)
def test_the_same_host_spelled_out_is_accepted(self, app):
from app.routes.auth import is_safe_url
with app.test_request_context('/auth/login'):
from flask import request
assert is_safe_url(f'http://{request.host}/dashboard')
def test_the_login_redirect_refuses_to_leave_the_site(self, client, make_user, app):
user_id = make_user('player')
with app.app_context():
username = db.session.get(User, user_id).username
response = client.post(
'/auth/login?next=/%5Cevil.example',
data={'username': username, 'password': 'Password123'},
follow_redirects=False,
)
assert 'evil.example' not in response.headers.get('Location', '')
tests/test_i18n.py
+1 -1
View File
@@ -222,7 +222,7 @@ class TestLocaleSurvivesSessionRotation:
def test_the_choice_survives_logging_out(self, client, as_role):
client.get('/lang/en')
as_role('player')
client.get('/auth/logout')
client.post('/auth/logout')
body = client.get('/auth/login').get_data(as_text=True)
assert 'lang="en"' in body
tests/test_role_change.py
+1 -1
View File
@@ -55,7 +55,7 @@ class TestPolymorphicIdentity:
with app.app_context():
username = db.session.get(User, target_id).username
client.get('/auth/logout')
client.post('/auth/logout')
login(username)
# Coach-only, and it is a coach's own page rather than a redirect.