From b277c453f97e3bbcece16b50994cf97b79c1254c Mon Sep 17 00:00:00 2001 From: GGThed Date: Fri, 7 Aug 2026 19:47:00 -0400 Subject: [PATCH] fix(security): appliquer les schemas de validation sur la gestion des comptes SEC-AUTHZ-001. CreateUserSchema, EditUserSchema et EditProfileSchema etaient importes dans users.py et jamais appeles : chaque nom n'apparaissait qu'une fois dans le fichier, sur sa ligne d'import. Les trois routes lisaient request.form directement. Consequences levees : - aucune politique de mot de passe sur create_user, edit_user et edit_profile. Un mot de passe d'un caractere etait accepte pour un compte administrateur. - aucune validation de format sur username, email, phone, discord_user_id. - edit_user ne verifiait pas l'unicite du courriel avant affectation : la contrainte unique remontait en IntegrityError, donc en erreur 500. Un controle explicite excluant l'utilisateur courant est ajoute. C'est aussi le point d'injection de la chaine de XSS stocke SEC-XSS-001 : edit_profile acceptait n'importe quel nom d'utilisateur, charge HTML comprise, qui ressortait ensuite en JSON via /matches/api/events et etait injectee par innerHTML dans le calendrier. Deux details de formulaire imposaient un adaptateur, _form_payload : - request.form.to_dict() ne conserve que la premiere valeur d'une cle repetee, donc games doit etre relu avec getlist(). - une case a cocher non cochee est absente de la soumission, ce qui n'est pas la meme chose qu'un load_default. Sans injection explicite, decocher is_active_account aurait cesse de desactiver le compte. - un mot de passe vide signifie "conserver l'actuel" et non "definir le mot de passe vide" : le champ est retire avant validation. Le controle manuel du role devient redondant, le schema le contraint deja par OneOf(USER_TYPES). Verifie par quatre tests qui echouaient avant ce changement. Co-Authored-By: Claude Opus 5 --- app/routes/users.py | 134 ++++++++++++++++++++++++++++++++------------ 1 file changed, 98 insertions(+), 36 deletions(-) diff --git a/app/routes/users.py b/app/routes/users.py index 4a0a38b..15c4e78 100644 --- a/app/routes/users.py +++ b/app/routes/users.py @@ -7,7 +7,7 @@ import os import uuid from flask import Blueprint, render_template, redirect, url_for, flash, request, jsonify, send_file from flask_login import login_required, current_user -from app.extensions import db, hash_password, csrf +from app.extensions import db, hash_password from app.models import ( Admin, Manager, Coach, Player, Scout, User, USER_TYPES, ESPORT_GAMES, @@ -18,11 +18,11 @@ from app.models import ( MatchParticipant, Tryout, TryoutRegistration, TeamPlayer, ) from werkzeug.utils import secure_filename -from datetime import datetime, timedelta, date as date_type +from datetime import datetime, timedelta from marshmallow import ValidationError from app.validators import ( CreateUserSchema, EditUserSchema, EditProfileSchema, - UploadContractSchema, OneOnOneRequestSchema, + UploadContractSchema, ) import requests @@ -31,6 +31,38 @@ ALLOWED_SIGNED_EXTENSIONS = {'pdf'} users_bp = Blueprint('users', __name__, url_prefix='/users') + +# --------------------------------------------------------------------------- +# Form helpers shared by the schema-validated routes +# --------------------------------------------------------------------------- + +def _flash_validation_errors(err): + """Surface marshmallow errors the same way auth.py already does.""" + for field, messages in err.messages.items(): + for msg in messages: + flash(f'{field}: {msg}', 'danger') + + +def _form_payload(*, checkboxes=(), list_fields=('games',), optional_blank=('password',)): + """Turn the multi-valued request form into a plain dict for marshmallow. + + request.form.to_dict() keeps only the first value of a repeated key, so + list fields have to be re-read with getlist(). Unchecked HTML checkboxes + are simply absent from the submission, which is not the same as a schema + default, so they are injected explicitly. Blank optional fields are + dropped rather than sent as '' — an empty password means "leave the + current one alone", not "set the password to the empty string". + """ + payload = request.form.to_dict() + for name in list_fields: + payload[name] = request.form.getlist(name) + for name in checkboxes: + payload[name] = name in request.form + for name in optional_blank: + if not payload.get(name): + payload.pop(name, None) + return payload + # --------------------------------------------------------------------------- # Gamertag helper (shared) # --------------------------------------------------------------------------- @@ -96,21 +128,37 @@ def edit_user(user_id): user = User.query.get_or_404(user_id) if request.method == 'POST': - full_name = request.form.get('full_name') - email = request.form.get('email') - phone = request.form.get('phone') - role = request.form.get('role') - is_active = request.form.get('is_active_account') == 'on' - - if role not in USER_TYPES: - flash('Invalid role selected.', 'danger') + def _rerender(): return render_template('pages/edit_user.html', user=user, roles=USER_TYPES, - esport_games=ESPORT_GAMES, game_platforms=GAME_PLATFORMS) + esport_games=ESPORT_GAMES, game_platforms=GAME_PLATFORMS, + user_gamertags={gt.game: {'gamertag': gt.gamertag, + 'platform': gt.platform} + for gt in user.gamertags}) - selected_games = request.form.getlist('games') - discord_username = request.form.get('discord_username', '').strip() - discord_user_id = request.form.get('discord_user_id', '').strip() - league_os_profile = request.form.get('league_os_profile', '').strip() + try: + validated = EditUserSchema().load( + _form_payload(checkboxes=('is_active_account',)) + ) + except ValidationError as err: + _flash_validation_errors(err) + return _rerender() + + full_name = validated['full_name'] + email = validated['email'] + phone = validated.get('phone') + role = validated['role'] + is_active = validated['is_active_account'] + selected_games = validated.get('games', []) + discord_username = validated.get('discord_username') + discord_user_id = validated.get('discord_user_id') + league_os_profile = validated.get('league_os_profile') + + # Previously absent: the column is unique, so assigning a taken + # address surfaced as an IntegrityError, i.e. a 500. + clash = User.query.filter(User.email == email, User.id != user.id).first() + if clash: + flash('Email already in use by another account.', 'danger') + return _rerender() # Change role via raw SQL to avoid polymorphic identity corruption. # Must discard the entire session because the polymorphic discriminator @@ -137,7 +185,9 @@ def edit_user(user_id): update_user_gamertags(user, selected_games) - password = request.form.get('password') + # Blank means "keep the current password"; anything else has already + # been checked against the policy by the schema. + password = validated.get('password') if password: user.password_hash = hash_password(password) @@ -207,17 +257,21 @@ def create_user(): return redirect(url_for('main.dashboard')) if request.method == 'POST': - username = request.form.get('username') - email = request.form.get('email') - password = request.form.get('password') - full_name = request.form.get('full_name') - phone = request.form.get('phone') - role = request.form.get('role') - - if role not in USER_TYPES: - flash('Invalid role selected.', 'danger') + try: + validated = CreateUserSchema().load(request.form) + except ValidationError as err: + _flash_validation_errors(err) return render_template('pages/create_user.html', roles=USER_TYPES) + username = validated['username'] + email = validated['email'] + password = validated['password'] + full_name = validated['full_name'] + phone = validated.get('phone') + # The schema constrains role with OneOf(USER_TYPES), so the former + # manual membership check is now redundant. + role = validated['role'] + if User.query.filter_by(username=username).first(): flash('Username already exists.', 'danger') return render_template('pages/create_user.html', roles=USER_TYPES) @@ -274,15 +328,22 @@ def profile(): def edit_profile(): """Edit the current user's profile.""" if request.method == 'POST': - username = request.form.get('username') - full_name = request.form.get('full_name') - email = request.form.get('email') - phone = request.form.get('phone') + try: + validated = EditProfileSchema().load(_form_payload()) + except ValidationError as err: + _flash_validation_errors(err) + return render_template('pages/edit_profile.html', user=current_user, + esport_games=ESPORT_GAMES, game_platforms=GAME_PLATFORMS, + user_gamertags=current_user.get_gamertags()) - selected_games = request.form.getlist('games') - discord_username = request.form.get('discord_username', '').strip() - discord_user_id = request.form.get('discord_user_id', '').strip() - league_os_profile = request.form.get('league_os_profile', '').strip() + username = validated['username'] + full_name = validated['full_name'] + email = validated['email'] + phone = validated.get('phone') + selected_games = validated.get('games', []) + discord_username = validated.get('discord_username') + discord_user_id = validated.get('discord_user_id') + league_os_profile = validated.get('league_os_profile') if username != current_user.username and User.query.filter_by(username=username).first(): flash('Username already taken.', 'danger') @@ -307,7 +368,9 @@ def edit_profile(): update_user_gamertags(current_user, selected_games) - password = request.form.get('password') + # Blank means "keep the current password"; anything else has already + # been checked against the policy by the schema. + password = validated.get('password') if password: current_user.password_hash = hash_password(password) @@ -1043,7 +1106,6 @@ def notes_dashboard(): ).order_by(OneOnOneRequest.created_at.desc()).all() # For context selectors in the form - from app.models import Team as MatchTeam matches = Match.query.filter( db.or_(Match.created_by == current_user.id, Match.status == 'scheduled'), ).order_by(Match.date.desc()).limit(20).all()