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. #} +