docs(audit): audit securite, maintenabilite et standards de la stack
Revue statique de l'ensemble du code Python, de la configuration CI/nginx, du .gitignore et des dependances. 44 constats documentes avec references fichier:ligne, impact et correctif propose. - audit/01-securite.md 19 constats (4 critiques) - audit/02-maintenabilite.md 15 constats - audit/03-standards-stack.md 10 ecarts aux conventions Flask/SQLAlchemy - audit/plan-remediation.md ordre de traitement en 6 lots Points critiques : secrets de production reels committes dans app/.env.exemple, seed automatique en production avec mot de passe password, CORS ouvert a toutes les origines avec credentials par defaut, token du bot Discord imprime sur stdout au demarrage. Aucune modification du code applicatif. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
GGThed
@@ -0,0 +1,672 @@
|
||||
# 1 — Sécurité
|
||||
|
||||
19 constats. Les références de lignes correspondent à `main` @ `08f02f7`.
|
||||
|
||||
| ID | Constat | Sévérité |
|
||||
|---|---|---|
|
||||
| [SEC-01](#sec-01--) | Secrets de production réels committés dans le dépôt | 🔴 Critique |
|
||||
| [SEC-02](#sec-02--) | Seed automatique en production avec mot de passe `password` | 🔴 Critique |
|
||||
| [SEC-03](#sec-03--) | CORS ouvert à toutes les origines avec credentials par défaut | 🔴 Critique |
|
||||
| [SEC-04](#sec-04--) | Token du bot Discord imprimé sur stdout à l'import | 🔴 Critique |
|
||||
| [SEC-05](#sec-05--) | En-têtes de proxy non validés → contournement HTTPS + rate limiting inopérant | 🟠 Élevé |
|
||||
| [SEC-06](#sec-06--) | Schémas de validation importés mais jamais appliqués sur les routes utilisateurs | 🟠 Élevé |
|
||||
| [SEC-07](#sec-07--) | `discord_user_id` arbitraire → détournement des notifications privées | 🟠 Élevé |
|
||||
| [SEC-08](#sec-08--) | Rate limiting en mémoire, non partagé et réinitialisé à chaque redémarrage | 🟠 Élevé |
|
||||
| [SEC-09](#sec-09--) | `nl2br` marque du HTML utilisateur non échappé comme sûr | 🟡 Moyen |
|
||||
| [SEC-10](#sec-10--) | CSP avec `'unsafe-inline'` sur `script-src` | 🟡 Moyen |
|
||||
| [SEC-11](#sec-11--) | Aucune validation de type sur l'upload de contrat signé | 🟡 Moyen |
|
||||
| [SEC-12](#sec-12--) | Énumération d'utilisateurs via les messages de login | 🟡 Moyen |
|
||||
| [SEC-13](#sec-13--) | CAPTCHA arithmétique trivial | 🟡 Moyen |
|
||||
| [SEC-14](#sec-14--) | `/health` expose l'erreur brute de la base de données | 🟡 Moyen |
|
||||
| [SEC-15](#sec-15--) | Profil complet de tout utilisateur visible par tout compte authentifié | 🟡 Moyen |
|
||||
| [SEC-16](#sec-16--) | Conversions `int()` non protégées sur entrées utilisateur | 🔵 Faible |
|
||||
| [SEC-17](#sec-17--) | `add_to_team` ne vérifie pas la cohérence tryout/équipe/joueur | 🔵 Faible |
|
||||
| [SEC-18](#sec-18--) | Aucune réinitialisation de mot de passe ni MFA | 🔵 Faible |
|
||||
| [SEC-19](#sec-19--) | Journal d'audit d'authentification déclaré mais jamais alimenté | 🔵 Faible |
|
||||
|
||||
---
|
||||
|
||||
## SEC-01 · 🔴
|
||||
|
||||
**Secrets de production réels committés dans le dépôt**
|
||||
|
||||
`app/.env.exemple` — fichier **suivi par git** — ne contient pas des valeurs d'exemple mais des identifiants réels :
|
||||
|
||||
| Ligne | Secret |
|
||||
|---|---|
|
||||
| `:6` | `SECRET_KEY=65476749453935` — clé de signature des sessions Flask |
|
||||
| `:18` | `DISCORD_BOT_TOKEN=MTUyNzY3ODU3NjUyNTA1NDEyNQ.G1gPNQ.LeFV…` — token du bot UdeS Esports |
|
||||
| `:21` | `DATABASE_URL=postgresql://team_tryouts_db_user:0YO038Od2QcQ…@dpg-…render.com/team_tryouts_db` — base PostgreSQL Render, hôte public, avec mot de passe |
|
||||
|
||||
Le commentaire ligne 16-17 confirme explicitement qu'il s'agit du token de production : *« This is the UdeS Esports BOT token »*.
|
||||
|
||||
**Impact.** Toute personne ayant accès au dépôt — y compris via un fork, un clone, ou si le dépôt devient public — obtient :
|
||||
- un accès lecture/écriture complet à la base de production (l'hôte Render est joignable depuis Internet) : identités, courriels, téléphones, notes personnelles des joueurs, contrats ;
|
||||
- le contrôle du bot Discord (envoi de DM en usurpant l'identité de l'organisation) ;
|
||||
- la capacité de **forger des cookies de session Flask valides** grâce à la `SECRET_KEY`, donc de s'authentifier en tant que n'importe quel utilisateur, y compris `admin`, sans mot de passe.
|
||||
|
||||
Le `.gitignore` ignore bien `.env`, mais le fichier a été committé sous le nom `.env.exemple`, qui échappe à la règle.
|
||||
|
||||
**Présent dans l'historique** depuis le commit `2d3721b` (*« Ajout d'un .env.exemple pour simplifier la collaboration »*). Supprimer le fichier ne suffira pas.
|
||||
|
||||
**Correction.**
|
||||
1. **Révoquer immédiatement, avant toute autre action** : régénérer le token du bot dans le Discord Developer Portal, faire tourner le mot de passe PostgreSQL sur Render, générer une nouvelle `SECRET_KEY` (`python -c "import secrets; print(secrets.token_hex(32))"`). La rotation de la `SECRET_KEY` invalidera toutes les sessions en cours, ce qui est le comportement souhaité ici.
|
||||
2. Remplacer le contenu du fichier par des valeurs factices (`SECRET_KEY=<générer avec …>`, `DATABASE_URL=postgresql://user:password@host:5432/dbname`).
|
||||
3. Purger l'historique (`git filter-repo --path app/.env.exemple --invert-paths`, ou BFG), puis forcer la réécriture sur toutes les branches et prévenir les collaborateurs qu'ils doivent recloner.
|
||||
4. Ajouter `.env*` (avec l'astérisque) au `.gitignore`, en gardant une exception explicite pour le modèle : `!.env.example`.
|
||||
5. Ajouter un scan de secrets à la CI (`gitleaks`, ou `detect-secrets` en pre-commit) pour empêcher la récidive.
|
||||
|
||||
> Renommer aussi le fichier en `.env.example` — l'orthographe actuelle est un francisme qui casse la détection automatique de la plupart des outils.
|
||||
|
||||
---
|
||||
|
||||
## SEC-02 · 🔴
|
||||
|
||||
**Seed automatique en production avec mot de passe `password`**
|
||||
|
||||
`app/app.py:348-356`
|
||||
|
||||
```python
|
||||
with app.app_context():
|
||||
import app.models as models
|
||||
from app.models import User
|
||||
db.create_all()
|
||||
|
||||
if User.query.count() == 0:
|
||||
from app.supporting_scrits.seed import seed_database
|
||||
seed_database()
|
||||
```
|
||||
|
||||
Ce bloc s'exécute **à chaque appel de `create_app()`**, sans distinction d'environnement — donc aussi via `wsgi.py`, c'est-à-dire en production.
|
||||
|
||||
`app/supporting_scrits/seed.py` crée alors des comptes de démonstration dont le mot de passe est la chaîne littérale `password` :
|
||||
|
||||
```python
|
||||
username='admin', password_hash=hash_password('password'), # :42
|
||||
username='manager1', password_hash=hash_password('password'), # :48
|
||||
username='coach1', password_hash=hash_password('password'), # :60
|
||||
username='scout1', password_hash=hash_password('password'), # :79
|
||||
```
|
||||
|
||||
…et les affiche en clair au démarrage (`seed.py:449-453`).
|
||||
|
||||
**Impact.** Tout déploiement neuf, toute restauration sur base vide, toute migration vers une nouvelle instance crée un compte `admin` / `password` accessible depuis Internet. C'est un contournement complet de l'authentification. Le compte `admin` a `can_manage_users() == True` : création, modification et suppression de tous les utilisateurs.
|
||||
|
||||
Le mot de passe `password` ne respecte d'ailleurs pas la politique définie dans `validators.py:22-24` (8 caractères, majuscule, minuscule, chiffre) — ce qui montre que le seed contourne toute la couche de validation.
|
||||
|
||||
**Correction.**
|
||||
- Conditionner le seed : `if os.getenv('SEED_DEMO_DATA', 'false').lower() == 'true' and User.query.count() == 0:`.
|
||||
- Mieux : sortir le seed du factory et en faire une commande CLI Flask (`flask seed-demo`), exécutée explicitement en développement.
|
||||
- Faire générer les mots de passe de démo aléatoirement (`secrets.token_urlsafe(16)`) et les afficher une seule fois, plutôt que d'utiliser une constante.
|
||||
- Vérifier immédiatement en production si les comptes `admin`, `manager1`, `manager2`, `coach1`, `coach2`, `coach3`, `scout1` existent avec ces mots de passe, et les désactiver le cas échéant.
|
||||
|
||||
---
|
||||
|
||||
## SEC-03 · 🔴
|
||||
|
||||
**CORS ouvert à toutes les origines avec credentials par défaut**
|
||||
|
||||
`app/app.py:76-95`
|
||||
|
||||
```python
|
||||
allowed_origins = os.getenv('CORS_ALLOWED_ORIGINS', '').split(',')
|
||||
allowed_origins = [origin.strip() for origin in allowed_origins if origin.strip()]
|
||||
|
||||
if allowed_origins:
|
||||
CORS(app, origins=allowed_origins, supports_credentials=True, …)
|
||||
else:
|
||||
# When no origins specified, allow all (development) or none (production)
|
||||
# In production with a reverse proxy, CORS is handled at the Nginx level
|
||||
CORS(app, supports_credentials=True, methods=[…], max_age=3600)
|
||||
```
|
||||
|
||||
Le commentaire décrit une intention qui n'est pas implémentée : la branche `else` **autorise toutes les origines**, en développement comme en production. `flask-cors` avec `supports_credentials=True` et sans `origins` reflète l'en-tête `Origin` de la requête dans `Access-Control-Allow-Origin` et ajoute `Access-Control-Allow-Credentials: true`.
|
||||
|
||||
Le commentaire renvoie la responsabilité à nginx, mais `app/nginx.conf` **ne contient aucune directive CORS**. Et `app/.env.exemple` **ne définit pas `CORS_ALLOWED_ORIGINS`** : la configuration livrée aux équipes tombe donc systématiquement dans la branche permissive.
|
||||
|
||||
**Impact.** N'importe quel site tiers visité par un utilisateur connecté peut lire, avec ses cookies de session, le contenu de toutes les routes `GET` — notamment :
|
||||
- `/users/disponibilities` : disponibilités de tous les joueurs actifs, avec noms d'utilisateur ;
|
||||
- `/matches/api/events` : calendrier complet, participants, sessions 1:1 approuvées ;
|
||||
- `/users/profile`, `/users/<id>/view` : données personnelles.
|
||||
|
||||
La protection CSRF (`CSRFProtect`) limite les écritures, mais n'empêche pas ces lectures.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```python
|
||||
allowed_origins = [o.strip() for o in os.getenv('CORS_ALLOWED_ORIGINS', '').split(',') if o.strip()]
|
||||
if allowed_origins:
|
||||
CORS(app, origins=allowed_origins, supports_credentials=True, methods=[...], max_age=3600)
|
||||
elif app.debug:
|
||||
CORS(app, origins=['http://localhost:5000'], supports_credentials=True)
|
||||
# sinon : pas de CORS du tout — même origine uniquement
|
||||
```
|
||||
|
||||
L'application étant rendue côté serveur (Jinja2) et consommant ses propres API en même-origine, **le cas nominal est de ne pas activer CORS du tout**. Documenter `CORS_ALLOWED_ORIGINS` dans le fichier d'exemple.
|
||||
|
||||
---
|
||||
|
||||
## SEC-04 · 🔴
|
||||
|
||||
**Token du bot Discord imprimé sur stdout à l'import**
|
||||
|
||||
`app/discord_bot.py:24-25`
|
||||
|
||||
```python
|
||||
DISCORD_BOT_TOKEN = os.getenv('DISCORD_BOT_TOKEN')
|
||||
print(DISCORD_BOT_TOKEN or 'FAILED TO PRINT BOT TOKEN')
|
||||
```
|
||||
|
||||
Le token est écrit en clair sur la sortie standard **à chaque import du module**, donc à chaque démarrage de l'application.
|
||||
|
||||
**Impact.** Le secret se retrouve dans les logs du superviseur de processus, les journaux de la plateforme d'hébergement, les logs de conteneur, et la sortie des jobs CI. Ces destinations sont typiquement conservées longtemps, indexées, et accessibles à un public plus large que les variables d'environnement elles-mêmes.
|
||||
|
||||
À noter : le `SensitiveDataFilter` de `logging_config.py` ne peut rien ici — il filtre les enregistrements du module `logging`, pas les appels à `print()`.
|
||||
|
||||
**Correction.** Supprimer la ligne. Si un diagnostic de configuration est nécessaire au démarrage :
|
||||
|
||||
```python
|
||||
logger.info('Discord bot token: %s', 'configuré' if DISCORD_BOT_TOKEN else 'ABSENT')
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## SEC-05 · 🟠
|
||||
|
||||
**En-têtes de proxy non validés → contournement HTTPS et rate limiting inopérant**
|
||||
|
||||
L'application lit `X-Forwarded-Proto` pour décider d'appliquer HSTS et la redirection HTTPS :
|
||||
|
||||
`app/app.py:163` — `is_https = request.is_secure or request.headers.get('X-Forwarded-Proto') == 'https'`
|
||||
`app/app.py:182` — `if not request.is_secure and request.headers.get('X-Forwarded-Proto') != 'https':`
|
||||
|
||||
Or **`ProxyFix` n'est jamais appliqué** et aucune liste de proxys de confiance n'est configurée. L'en-tête est accepté tel quel, quelle que soit sa provenance.
|
||||
|
||||
Ce défaut est amplifié par la configuration réseau :
|
||||
|
||||
- `wsgi.py:26` — `host = os.getenv('HOST', '0.0.0.0')`, avec le commentaire trompeur *« Bind to localhost by default »*. Le serveur Waitress écoute en réalité sur **toutes les interfaces**.
|
||||
- `app/nginx.conf:122` — `proxy_pass http://0.0.0.0:5000;` — `0.0.0.0` n'est pas une adresse de destination valide comme cible amont ; ce devrait être `127.0.0.1`.
|
||||
|
||||
**Impact.**
|
||||
1. Le port de l'application est joignable directement, en contournant nginx — donc sans TLS, sans les en-têtes de sécurité ajoutés par nginx.
|
||||
2. En envoyant `X-Forwarded-Proto: https` sur cette connexion en clair, on désactive la redirection HTTPS de `force_https()` et l'application se comporte comme si la connexion était sécurisée.
|
||||
3. **Corollaire plus grave — le rate limiting est neutralisé.** `app/extensions.py:16-19` utilise `key_func=get_remote_address`, qui lit `request.remote_addr`. Sans `ProxyFix`, cette valeur est l'IP de nginx pour *toutes* les requêtes proxifiées. Conséquences :
|
||||
- la limite de `10 per minute` sur `/auth/login` (`auth.py:79`) devient un **seau global partagé par tous les utilisateurs** — la protection anti-bruteforce ne fonctionne pas par attaquant ;
|
||||
- inversement, un seul client peut consommer le quota global et **bloquer le login de toute l'organisation** (déni de service trivial) ;
|
||||
- les limites par défaut `200/jour, 50/heure` s'appliquent à l'ensemble du trafic, ce qui rendra l'application inutilisable en usage normal dès quelques utilisateurs simultanés.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```python
|
||||
from werkzeug.middleware.proxy_fix import ProxyFix
|
||||
|
||||
# après la création de l'app, uniquement si l'on est réellement derrière un proxy
|
||||
if os.getenv('BEHIND_PROXY', 'false').lower() == 'true':
|
||||
app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1, x_port=1)
|
||||
```
|
||||
|
||||
`x_for=1` indique de ne faire confiance qu'au dernier saut — celui de nginx. Ne jamais activer ce middleware si l'application n'est pas derrière un proxy, sinon `X-Forwarded-For` devient falsifiable par le client.
|
||||
|
||||
En complément :
|
||||
- `wsgi.py` : passer le défaut de `HOST` à `127.0.0.1` (ce que le commentaire annonce déjà) ;
|
||||
- `nginx.conf:122` : `proxy_pass http://127.0.0.1:5000;` ;
|
||||
- filtrer au pare-feu le port applicatif.
|
||||
|
||||
---
|
||||
|
||||
## SEC-06 · 🟠
|
||||
|
||||
**Schémas de validation importés mais jamais appliqués sur les routes utilisateurs**
|
||||
|
||||
`app/routes/users.py:23-26` importe `CreateUserSchema`, `EditUserSchema` et `EditProfileSchema`. Vérification par comptage d'occurrences : **chacun de ces trois noms n'apparaît qu'une seule fois dans le fichier — sur la ligne d'import**. Ils ne sont jamais instanciés.
|
||||
|
||||
Les trois routes concernées lisent le formulaire brut :
|
||||
|
||||
| Route | Lignes | Traitement |
|
||||
|---|---|---|
|
||||
| `create_user` | `:196-226` | `request.form.get('password')` → `hash_password(password)` directement |
|
||||
| `edit_user` | `:98-133` | `password = request.form.get('password')` → `hash_password(password)` si non vide |
|
||||
| `edit_profile` | `:255-295` | idem, sur son propre compte |
|
||||
|
||||
Comparaison avec `auth.py:206-218`, où `RegisterSchema` **est** correctement chargé — l'inscription publique est donc validée, mais pas les trois autres chemins de création/modification de compte.
|
||||
|
||||
**Impact.**
|
||||
- **Aucune politique de mot de passe** sur ces routes : `a` est accepté. Un président créant les comptes de l'équipe peut leur attribuer des mots de passe d'un caractère, et n'importe quel utilisateur peut affaiblir le sien via `edit_profile`.
|
||||
- **Aucune validation de format** sur `email` (le champ n'est même pas vérifié comme étant une adresse), `username`, `phone`.
|
||||
- `create_user` (`:216`) appelle `hash_password(password)` sans vérifier que `password` est non vide : un `password_hash` d'une chaîne vide est stocké, et le compte devient accessible avec un mot de passe vide.
|
||||
- Dans `edit_user` (`:115-118`), `full_name` et `email` sont assignés sans contrôle de nullité, alors que les colonnes sont `nullable=False` (`models/user_model/user.py:20-21`) → `IntegrityError` non gérée → 500.
|
||||
|
||||
**Correction.** Appliquer les schémas déjà écrits, sur le modèle de `auth.py` :
|
||||
|
||||
```python
|
||||
schema = EditProfileSchema()
|
||||
try:
|
||||
validated = schema.load(request.form)
|
||||
except ValidationError as err:
|
||||
for field, messages in err.messages.items():
|
||||
for msg in messages:
|
||||
flash(f'{field}: {msg}', 'danger')
|
||||
return render_template('pages/edit_profile.html', ...)
|
||||
```
|
||||
|
||||
Attention : `EditUserSchema` et `EditProfileSchema` déclarent `password` avec `load_default=''` et `validate=validate_password` — un mot de passe vide échouera donc la validation. Il faut soit passer `validate=validate.And(...)` conditionnel, soit retirer le champ du payload quand il est vide avant le `load()`.
|
||||
|
||||
---
|
||||
|
||||
## SEC-07 · 🟠
|
||||
|
||||
**`discord_user_id` arbitraire → détournement des notifications privées**
|
||||
|
||||
`app/routes/users.py:262-263` et `:283-284` (route `edit_profile`, accessible à **tout utilisateur authentifié**) :
|
||||
|
||||
```python
|
||||
discord_user_id = request.form.get('discord_user_id', '').strip()
|
||||
...
|
||||
current_user.discord_user_id = discord_user_id or None
|
||||
```
|
||||
|
||||
Aucune validation (conséquence de SEC-06 : `validate_discord_user_id` existe dans `validators.py:83-97` mais n'est pas appelée), et **aucune vérification de propriété** : rien ne prouve que l'utilisateur contrôle réellement ce compte Discord. Aucune contrainte d'unicité sur la colonne non plus (`models/user_model/user.py:32`).
|
||||
|
||||
**Impact.** Un joueur peut renseigner l'identifiant Discord d'une autre personne — un coach, un membre de la direction. Il reçoit alors à sa place les messages privés du bot. Selon les flux décrits dans le README, cela inclut :
|
||||
- les demandes de sessions 1:1 avec leurs *« discussion points »*, souvent confidentiels ;
|
||||
- les notifications de matchs et d'entraînements ;
|
||||
- surtout, **la capacité de répondre à la place de la cible** : `discord_bot.py` traite les réactions ✅/❌ en DM pour accepter ou refuser une demande 1:1 (`on_reaction_add`, `:118`). L'attaquant obtient donc un pouvoir de décision qui ne lui appartient pas.
|
||||
|
||||
Deux utilisateurs peuvent en outre déclarer le même identifiant, ce qui rend le comportement non déterministe.
|
||||
|
||||
**Correction.**
|
||||
1. Appliquer `validate_discord_user_id` (corrigé par SEC-06) — nécessaire mais très insuffisant : il ne vérifie que le format 17-20 chiffres.
|
||||
2. Ajouter une contrainte d'unicité sur `User.discord_user_id`.
|
||||
3. **Implémenter une vérification de possession** : à la saisie, envoyer un code à usage unique en DM sur l'identifiant déclaré et exiger sa saisie sur la plateforme avant d'activer le lien. C'est la seule correction qui traite réellement le problème.
|
||||
4. En attendant, réserver la modification de ce champ aux administrateurs.
|
||||
|
||||
---
|
||||
|
||||
## SEC-08 · 🟠
|
||||
|
||||
**Rate limiting en mémoire, non partagé et réinitialisé à chaque redémarrage**
|
||||
|
||||
`app/extensions.py:16-19`
|
||||
|
||||
```python
|
||||
limiter = Limiter(
|
||||
key_func=get_remote_address,
|
||||
default_limits=["200 per day", "50 per hour"]
|
||||
)
|
||||
```
|
||||
|
||||
Aucun `storage_uri` n'est fourni. Flask-Limiter bascule alors sur son backend `memory://`, qui est explicitement documenté comme non destiné à la production (la bibliothèque émet d'ailleurs un avertissement au démarrage).
|
||||
|
||||
**Impact.**
|
||||
- L'état est **par processus**. `wsgi.py:25` démarre Waitress avec `cpu_count() * 2 + 1` threads — cela reste un processus, donc le compteur est partagé ici ; mais toute évolution vers plusieurs workers ou plusieurs instances (montée en charge, déploiement bleu-vert) fragmente les compteurs et multiplie d'autant la limite effective.
|
||||
- L'état est **perdu à chaque redémarrage** : un attaquant peut réinitialiser les compteurs si un redéploiement survient, et le verrouillage anti-bruteforce ne survit pas aux mises à jour.
|
||||
- Combiné à SEC-05, la protection est de toute façon appliquée à la mauvaise clé.
|
||||
|
||||
**Correction.** Adosser le limiteur à un stockage partagé — Redis de préférence, ou la base PostgreSQL déjà présente si l'on veut éviter une dépendance supplémentaire :
|
||||
|
||||
```python
|
||||
limiter = Limiter(
|
||||
key_func=get_remote_address,
|
||||
default_limits=["200 per day", "50 per hour"],
|
||||
storage_uri=os.getenv('RATELIMIT_STORAGE_URI', 'memory://'),
|
||||
)
|
||||
```
|
||||
|
||||
Réévaluer aussi les valeurs : `50 per hour` par IP est très bas pour une application web rendue côté serveur, où chaque page consomme plusieurs requêtes (`/matches/api/events`, `/users/disponibilities`…). Exclure les routes `/static` et `/health` du décompte.
|
||||
|
||||
---
|
||||
|
||||
## SEC-09 · 🟡
|
||||
|
||||
**`nl2br` marque du HTML utilisateur non échappé comme sûr**
|
||||
|
||||
`app/app.py:19-30`
|
||||
|
||||
```python
|
||||
def nl2br(value):
|
||||
if value:
|
||||
return markupsafe.Markup('<br>'.join(str(value).splitlines()))
|
||||
return ''
|
||||
```
|
||||
|
||||
`markupsafe.Markup()` **désactive l'échappement automatique de Jinja2** pour la chaîne produite. Le contenu utilisateur est inséré tel quel, sans passer par `escape()`.
|
||||
|
||||
**Statut actuel : non exploitable.** Une recherche sur l'ensemble des templates ne trouve **aucune utilisation de `|nl2br`**. Le filtre est enregistré (`app.py:125`) mais mort.
|
||||
|
||||
**Risque.** C'est un piège en attente : le filtre porte un nom naturel, il est enregistré globalement, et la première personne qui écrira `{{ note.content|nl2br }}` — un usage évident sur `PersonalNote`, `TeamNote` ou les *discussion points* des 1:1 — introduira une XSS stockée sans s'en rendre compte. Ces contenus sont saisis par des coachs et joueurs et affichés à d'autres utilisateurs.
|
||||
|
||||
**Correction.** Échapper avant de marquer :
|
||||
|
||||
```python
|
||||
from markupsafe import Markup, escape
|
||||
|
||||
def nl2br(value):
|
||||
if not value:
|
||||
return ''
|
||||
return Markup('<br>').join(escape(str(value)).splitlines())
|
||||
```
|
||||
|
||||
`escape()` neutralise le HTML utilisateur ; seuls les `<br>` insérés par le filtre restent actifs. Alternative sans code : supprimer le filtre et utiliser `white-space: pre-line` en CSS.
|
||||
|
||||
---
|
||||
|
||||
## SEC-10 · 🟡
|
||||
|
||||
**CSP avec `'unsafe-inline'` sur `script-src`**
|
||||
|
||||
`app/app.py:149-159`
|
||||
|
||||
```python
|
||||
"script-src 'self' 'unsafe-inline' https://cdn.jsdelivr.net; "
|
||||
"style-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com https://cdn.jsdelivr.net; "
|
||||
```
|
||||
|
||||
`'unsafe-inline'` sur `script-src` **annule l'essentiel du bénéfice de la CSP** : c'est précisément l'injection de `<script>` inline que la directive est censée bloquer. La politique ne protège plus que contre le chargement de scripts externes.
|
||||
|
||||
Le reste de la politique est de bonne qualité (`frame-ancestors 'none'`, `base-uri 'self'`, `form-action 'self'`, `object-src` implicitement couvert par `default-src 'self'`).
|
||||
|
||||
`'unsafe-inline'` sur `style-src` est nettement moins grave et généralement toléré.
|
||||
|
||||
**Correction.** Chemin réaliste, par étapes :
|
||||
1. Extraire les `<script>` inline des templates vers des fichiers sous `static/js/` — un audit rapide montre que `match_form.html` (44 Ko), `view_tryout.html` (31 Ko), `teams.html` (23 Ko) et `calendar.html` (16 Ko) en concentrent la majorité.
|
||||
2. Pour ce qui doit rester inline, générer un nonce par requête (`secrets.token_urlsafe(16)` dans un `before_request`, injecté dans le contexte Jinja) et passer à `script-src 'self' 'nonce-{nonce}'`.
|
||||
3. Épingler les ressources CDN avec `integrity` (SRI) plutôt que de faire confiance à l'origine seule.
|
||||
|
||||
En attendant, traiter SEC-09 : sans point d'injection HTML, la CSP affaiblie n'est pas exploitable.
|
||||
|
||||
---
|
||||
|
||||
## SEC-11 · 🟡
|
||||
|
||||
**Aucune validation de type sur l'upload de contrat signé**
|
||||
|
||||
`app/routes/users.py:577-604` (`upload_signed_contract`) :
|
||||
|
||||
```python
|
||||
file = request.files['signed_file']
|
||||
if file.filename == '':
|
||||
flash('No file selected.', 'danger')
|
||||
return redirect(url_for('users.list_contracts'))
|
||||
|
||||
signed_filename = f"signed_{contract.stored_filename}"
|
||||
file.save(contract.file_path.replace(contract.stored_filename, signed_filename))
|
||||
```
|
||||
|
||||
Aucune vérification d'extension ni de type — contrairement à `upload_contract` (`:537-539`) qui contrôle `.pdf`. Les constantes `ALLOWED_CONTRACT_EXTENSIONS` et `ALLOWED_SIGNED_EXTENSIONS` (`:29-30`) **ne sont référencées nulle part** (une seule occurrence chacune : leur déclaration).
|
||||
|
||||
**Facteurs atténuants.** Le nom de fichier stocké est dérivé d'un UUID généré côté serveur (`:556-557`), pas du nom fourni par le client — il n'y a donc pas de traversée de répertoire, et le fichier est toujours écrit avec le suffixe `.pdf`. Les fichiers sont servis par `send_file(..., as_attachment=True)` (`:615`, `:629`), donc téléchargés plutôt qu'interprétés. Le risque d'exécution est faible.
|
||||
|
||||
**Impact réel.** Stockage de contenu arbitraire (jusqu'à 16 Mo) sous une extension `.pdf` trompeuse : usage du serveur comme relais de distribution de fichiers malveillants vers des utilisateurs légitimes, qui recevront un « contrat » qui n'en est pas un. Et corruption fonctionnelle des dossiers de contrats.
|
||||
|
||||
**Correction.**
|
||||
1. Appliquer la même vérification que `upload_contract`, en utilisant les constantes déjà déclarées :
|
||||
```python
|
||||
ext = file.filename.rsplit('.', 1)[-1].lower() if '.' in file.filename else ''
|
||||
if ext not in ALLOWED_SIGNED_EXTENSIONS:
|
||||
flash('Seuls les fichiers PDF sont acceptés.', 'danger')
|
||||
return redirect(url_for('users.list_contracts'))
|
||||
```
|
||||
2. Ne pas se fier à l'extension seule : vérifier les octets d'en-tête (`%PDF-`) après lecture, ou utiliser `python-magic`.
|
||||
3. Servir les téléchargements avec `Content-Type: application/pdf` explicite et `Content-Disposition: attachment` (déjà le cas via `as_attachment=True`).
|
||||
4. Stocker les documents hors de l'arborescence servie par le serveur web — c'est déjà le cas (`documents/`, gitignoré), à préserver.
|
||||
|
||||
---
|
||||
|
||||
## SEC-12 · 🟡
|
||||
|
||||
**Énumération d'utilisateurs via les messages de login**
|
||||
|
||||
`app/routes/auth.py:148-167`
|
||||
|
||||
```python
|
||||
if user:
|
||||
user.failed_login_attempts += 1
|
||||
if user.failed_login_attempts >= MAX_LOGIN_ATTEMPTS:
|
||||
flash(f'Account locked after {MAX_LOGIN_ATTEMPTS} failed attempts. …')
|
||||
else:
|
||||
remaining = MAX_LOGIN_ATTEMPTS - user.failed_login_attempts
|
||||
flash(f'Login unsuccessful. {remaining} attempt(s) remaining before lockout.', 'danger')
|
||||
else:
|
||||
flash('Login unsuccessful. Please check username and password.', 'danger')
|
||||
```
|
||||
|
||||
Le message diffère selon que le compte existe ou non. Le message de verrouillage (`:114-121`) fuit également l'existence du compte, et de surcroît **avant toute vérification du mot de passe**.
|
||||
|
||||
S'y ajoute une différence de temps de réponse : quand l'utilisateur n'existe pas, `check_password` n'est jamais appelé, donc le coût du hachage n'est pas payé — un écart mesurable même sans lire les messages.
|
||||
|
||||
**Impact.** Constitution d'une liste d'identifiants valides, préalable à une attaque par pulvérisation de mots de passe ou à de l'hameçonnage ciblé. Sur une organisation étudiante dont les noms d'utilisateurs sont devinables, l'impact est réel.
|
||||
|
||||
**Correction.**
|
||||
- Retourner un message identique dans tous les cas : *« Identifiants invalides. »*, sans compteur de tentatives restantes ni mention de verrouillage.
|
||||
- Exécuter systématiquement `check_password` contre un hachage factice quand l'utilisateur n'existe pas, pour égaliser les temps de réponse :
|
||||
```python
|
||||
DUMMY_HASH = generate_password_hash('dummy-password-for-timing-equalization')
|
||||
...
|
||||
check_password(user.password_hash if user else DUMMY_HASH, password)
|
||||
```
|
||||
- Notifier le verrouillage par courriel au titulaire du compte plutôt qu'à l'écran.
|
||||
|
||||
---
|
||||
|
||||
## SEC-13 · 🟡
|
||||
|
||||
**CAPTCHA arithmétique trivial**
|
||||
|
||||
`app/routes/auth.py:39-53`
|
||||
|
||||
```python
|
||||
a = random.randint(1, 10)
|
||||
b = random.randint(1, 10)
|
||||
session['captcha_answer'] = a + b
|
||||
return {'question': f'{a} + {b} = ?', 'id': captcha_id}
|
||||
```
|
||||
|
||||
L'espace des réponses possibles est de 19 valeurs (2 à 20). La question est présente en clair dans le HTML sous forme `N + M = ?`, donc résoluble par une expression régulière de trois lignes. `random` est le générateur pseudo-aléatoire standard, non cryptographique.
|
||||
|
||||
Le `captcha_id` est généré (`:50`) et stocké en session, mais **n'est jamais comparé** lors de la vérification (`verify_captcha`, `:56-72`, ne fait que dépiler `captcha_answer` et ignorer `captcha_id`) — le champ est décoratif.
|
||||
|
||||
**Facteur atténuant.** La route `/auth/register` est limitée à `3 per hour` (`:173`), ce qui borne fortement l'exploitation — sous réserve que le rate limiting fonctionne (voir SEC-05 et SEC-08, qui le compromettent).
|
||||
|
||||
**Impact.** Création automatisée de comptes joueurs. Conséquences limitées (un joueur n'a pas de privilèges), mais pollution de la base et bruit dans les listes de sélection.
|
||||
|
||||
**Correction.** Si l'objectif est réellement d'arrêter des bots, un CAPTCHA maison n'y parviendra pas. Deux options selon l'ambition :
|
||||
- **Suffisant ici** : valider l'inscription par un lien envoyé au courriel institutionnel, en restreignant le domaine (`@usherbrooke.ca`). Cela résout simultanément le problème des comptes jetables et vérifie l'appartenance à l'organisation.
|
||||
- Sinon, intégrer un service dédié (hCaptcha, Turnstile) — au prix d'une dépendance externe et d'un assouplissement de la CSP.
|
||||
|
||||
Dans tous les cas, corriger le rate limiting (SEC-05/SEC-08), qui est ici la protection réellement efficace.
|
||||
|
||||
---
|
||||
|
||||
## SEC-14 · 🟡
|
||||
|
||||
**`/health` expose l'erreur brute de la base de données**
|
||||
|
||||
`app/app.py:206-212`
|
||||
|
||||
```python
|
||||
except Exception as e:
|
||||
health_data['status'] = 'unhealthy'
|
||||
health_data['database'] = f'error: {str(e)}'
|
||||
return jsonify(health_data), 503
|
||||
```
|
||||
|
||||
La route `/health` **n'est pas protégée par `@login_required`** — elle est publique, comme attendu d'un endpoint de supervision. Mais en cas d'incident, l'exception SQLAlchemy est renvoyée telle quelle au client.
|
||||
|
||||
**Impact.** Les erreurs `psycopg2` incluent typiquement le nom d'hôte, le port, le nom de la base et le nom d'utilisateur (par exemple `could not connect to server: … host "dpg-….render.com" port 5432 … database "team_tryouts_db" user "team_tryouts_db_user"`). C'est de la reconnaissance d'infrastructure offerte gratuitement, précisément au moment où le système est en difficulté.
|
||||
|
||||
C'est d'autant plus incohérent que le gestionnaire d'erreur 500 (`:300-324`) fait exactement l'inverse, et le documente : *« Never exposes stack traces to users »*.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```python
|
||||
except Exception:
|
||||
app.logger.error('Health check: échec de connexion à la base', exc_info=True)
|
||||
return jsonify({'status': 'unhealthy', 'database': 'unreachable'}), 503
|
||||
```
|
||||
|
||||
Le détail part dans les logs, où il est utile ; le client reçoit un statut binaire, qui est tout ce dont un équilibreur de charge a besoin. Envisager aussi de restreindre `/health` au réseau interne via nginx.
|
||||
|
||||
---
|
||||
|
||||
## SEC-15 · 🟡
|
||||
|
||||
**Profil complet de tout utilisateur visible par tout compte authentifié**
|
||||
|
||||
`app/routes/users.py:231-236`
|
||||
|
||||
```python
|
||||
@users_bp.route('/<int:user_id>/view')
|
||||
@login_required
|
||||
def view_user(user_id):
|
||||
user = User.query.get_or_404(user_id)
|
||||
return render_template('pages/view_user.html', profile_user=user)
|
||||
```
|
||||
|
||||
Seule l'authentification est vérifiée. Aucun contrôle d'appartenance à la même équipe, ni de rôle.
|
||||
|
||||
L'objet `User` transmis au template porte `email`, `phone`, `discord_username`, `discord_user_id`, `full_name`, `league_os_profile` (`models/user_model/user.py:16-33`).
|
||||
|
||||
**Impact.** Tout compte — y compris un compte joueur créé par inscription publique (`/auth/register`, ouverte à tous) — peut énumérer `/users/1/view`, `/users/2/view`… et collecter les coordonnées personnelles de l'ensemble des membres de l'organisation : courriels, téléphones, identifiants Discord. Combiné à SEC-13 (inscription faiblement protégée), la barrière d'entrée est basse.
|
||||
|
||||
Note : `list_users` (`:76-85`) est bien réservé aux `Admin`. C'est la vue de détail unitaire qui a été oubliée.
|
||||
|
||||
**Correction.** Deux niveaux, à combiner :
|
||||
1. **Limiter les données transmises au template** plutôt que de passer l'objet ORM complet — les coordonnées n'ont pas à figurer sur un profil consulté par un pair.
|
||||
2. **Restreindre l'accès** selon la relation : membres d'une même `OrgTeam`, ou rôles d'encadrement (`Admin`, `Manager`, `Coach` de l'équipe du joueur, `Scout`).
|
||||
|
||||
```python
|
||||
def can_view_profile(viewer, target):
|
||||
if isinstance(viewer, (Admin, Manager, Scout)) or viewer.id == target.id:
|
||||
return True
|
||||
viewer_teams = {t.id for t in viewer.get_org_teams()}
|
||||
return bool(viewer_teams & {t.id for t in target.get_org_teams()})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## SEC-16 · 🔵
|
||||
|
||||
**Conversions `int()` non protégées sur entrées utilisateur**
|
||||
|
||||
De nombreuses routes convertissent des champs de formulaire sans garde. Exemples :
|
||||
|
||||
| Fichier | Ligne | Code |
|
||||
|---|---|---|
|
||||
| `routes/teams.py` | `:120-121` | `int(coach_id) if coach_id else None` |
|
||||
| `routes/teams.py` | `:241`, `:272` | `User.query.get_or_404(int(coach_id))` |
|
||||
| `routes/tryouts.py` | `:70`, `:72-74` | `int(max_players)`, `int(target_org_team_id)`… |
|
||||
| `routes/tryouts.py` | `:428` | `TeamMember(team_id=team_id, player_id=int(player_id), …)` |
|
||||
| `routes/matches.py` | `:298-299` | `[int(p) for p in team1_player_ids.split(',') if p]` |
|
||||
| `routes/matches.py` | `:314`, `:318` | `int(pid)` sur `request.form.getlist('player_ids')` |
|
||||
|
||||
Une valeur non numérique lève `ValueError`, non interceptée → **HTTP 500** au lieu d'un 400.
|
||||
|
||||
**Impact.** Faible en confidentialité : le gestionnaire 500 (`app.py:300-324`) ne divulgue pas de trace et fait bien le `db.session.rollback()`. Le problème est la qualité de service et le bruit dans les logs d'erreur, qui masque les incidents réels. `matches.py:298-299` accepte de surcroît une liste d'identifiants sans borne — un `player_ids` très long entraîne autant d'INSERT.
|
||||
|
||||
**Correction.** Utiliser le convertisseur intégré de Flask, déjà employé ailleurs dans le code (`users.py:1089` : `request.form.get('player_id', type=int)`), qui renvoie `None` au lieu de lever :
|
||||
|
||||
```python
|
||||
coach_id = request.form.get('coach_id', type=int)
|
||||
if coach_id is None:
|
||||
flash('Coach invalide.', 'danger')
|
||||
return redirect(...)
|
||||
```
|
||||
|
||||
Pour les listes, valider chaque élément et plafonner la taille. À traiter globalement lors de la généralisation des schémas Marshmallow (SEC-06).
|
||||
|
||||
---
|
||||
|
||||
## SEC-17 · 🔵
|
||||
|
||||
**`add_to_team` ne vérifie pas la cohérence tryout / équipe / joueur**
|
||||
|
||||
`app/routes/tryouts.py:412-432`
|
||||
|
||||
```python
|
||||
def add_to_team(tryout_id, team_id):
|
||||
team = Team.query.get_or_404(team_id)
|
||||
tryout = Tryout.query.get_or_404(tryout_id)
|
||||
if not current_user.can_manage_this_tryout(tryout):
|
||||
…
|
||||
player_id = request.form.get('player_id')
|
||||
…
|
||||
member = TeamMember(team_id=team_id, player_id=int(player_id), position=position)
|
||||
```
|
||||
|
||||
Le contrôle d'autorisation porte sur `tryout_id`, mais l'écriture porte sur `team_id`. **Il n'est jamais vérifié que `team.tryout_id == tryout_id`.** De même, rien ne vérifie que `player_id` correspond à un joueur inscrit à ce tryout — ni même à un `Player`.
|
||||
|
||||
**Impact.** Un coach ou manager légitime sur le tryout A peut, en modifiant `team_id` dans la requête, ajouter des joueurs à une équipe rattachée au tryout B qu'il ne gère pas. Il peut aussi rattacher un utilisateur non inscrit, ou un compte non-joueur (coach, admin), ce qui produira des incohérences en cascade dans les matchs et les évaluations.
|
||||
|
||||
Exploitation limitée aux comptes d'encadrement — d'où la sévérité faible — mais c'est un contournement du modèle d'autorisation par ailleurs bien construit.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```python
|
||||
team = Team.query.get_or_404(team_id)
|
||||
tryout = Tryout.query.get_or_404(tryout_id)
|
||||
if team.tryout_id != tryout_id:
|
||||
abort(404)
|
||||
if not current_user.can_manage_this_tryout(tryout):
|
||||
...
|
||||
player_id = request.form.get('player_id', type=int)
|
||||
registered = TryoutRegistration.query.filter_by(
|
||||
tryout_id=tryout_id, player_id=player_id).first()
|
||||
if not registered:
|
||||
flash("Ce joueur n'est pas inscrit à ce tryout.", 'danger')
|
||||
return redirect(url_for('tryouts.view_tryout', tryout_id=tryout_id))
|
||||
```
|
||||
|
||||
Ce motif — *vérifier que la ressource enfant appartient bien au parent sur lequel porte l'autorisation* — mérite d'être appliqué systématiquement. `matches.py:562-575` et `team_matches.py:288-301` le font correctement (`if participant.match_id != match_id`) et peuvent servir de référence.
|
||||
|
||||
---
|
||||
|
||||
## SEC-18 · 🔵
|
||||
|
||||
**Aucune réinitialisation de mot de passe ni MFA**
|
||||
|
||||
`app/routes/auth.py` expose `login`, `register` et `logout`. Il n'existe :
|
||||
- **aucun mécanisme de réinitialisation de mot de passe** — un utilisateur qui oublie le sien doit passer par un administrateur, qui le définira via `edit_user` (route par ailleurs non validée, cf. SEC-06) et devra le lui transmettre par un canal hors bande ;
|
||||
- **aucune authentification à deux facteurs**, y compris pour le compte `Admin` qui a un contrôle total sur les utilisateurs ;
|
||||
- **aucune expiration ni politique de rotation** des mots de passe.
|
||||
|
||||
Le verrouillage après 5 échecs (`auth.py:19-20`, `:150-165`) est en revanche bien implémenté, avec réinitialisation du compteur au succès.
|
||||
|
||||
**Impact.** Risque organisationnel plus que technique : l'absence de réinitialisation en libre-service pousse vers des pratiques de contournement (mots de passe communiqués par Discord, mots de passe partagés, comptes non désactivés au départ d'un membre). Pour une application détenant des contrats et des évaluations nominatives, l'absence de MFA sur le compte président est un point à documenter comme risque accepté, à défaut d'être corrigé.
|
||||
|
||||
**Correction.** Par ordre de rapport valeur/effort :
|
||||
1. Réinitialisation par courriel avec jeton signé à durée limitée — `itsdangerous` est déjà présent dans les dépendances (transitif de Flask) : `URLSafeTimedSerializer(app.config['SECRET_KEY'])`, jeton valable 30 minutes, à usage unique.
|
||||
2. TOTP (`pyotp`) sur les rôles `Admin` et `Manager` uniquement, pour limiter la friction.
|
||||
3. Journaliser les changements de mot de passe dans `team_tryouts.auth` (voir SEC-19).
|
||||
|
||||
---
|
||||
|
||||
## SEC-19 · 🔵
|
||||
|
||||
**Journal d'audit d'authentification déclaré mais jamais alimenté**
|
||||
|
||||
`app/logging_config.py:102-115` configure un logger dédié `team_tryouts.auth`, avec son propre fichier rotatif `auth.log`, son filtre de redaction et `propagate = False`. Une fabrique `get_auth_logger()` est fournie (`:148-154`).
|
||||
|
||||
Or **aucun module n'importe `get_auth_logger` ni ne référence `team_tryouts.auth`**. Le docstring de `auth.py` annonce pourtant *« login with account lockout protection … and audit logging »* — la journalisation d'audit n'existe pas.
|
||||
|
||||
`auth.log` est donc créé vide à chaque démarrage et le restera.
|
||||
|
||||
**Impact.** Aucune traçabilité des événements d'authentification : impossible de détecter une campagne de bruteforce, d'identifier l'origine d'une compromission, ou de répondre à une question aussi simple que « qui s'est connecté au compte admin la semaine dernière ». Ce manque devient bloquant si un incident survient — et les constats SEC-01 et SEC-02 rendent un incident plausible.
|
||||
|
||||
**Correction.** Alimenter le logger aux points de décision de `auth.py` :
|
||||
|
||||
```python
|
||||
from app.logging_config import get_auth_logger
|
||||
auth_logger = get_auth_logger()
|
||||
|
||||
# succès (auth.py:140)
|
||||
auth_logger.info('login success user=%s id=%s ip=%s', user.username, user.id, request.remote_addr)
|
||||
# échec (auth.py:149)
|
||||
auth_logger.warning('login failure user=%s ip=%s attempts=%s', username, request.remote_addr, …)
|
||||
# verrouillage (auth.py:152)
|
||||
auth_logger.warning('account locked user=%s ip=%s', user.username, request.remote_addr)
|
||||
# déconnexion, création de compte, changement de mot de passe, changement de rôle
|
||||
```
|
||||
|
||||
Étendre aux opérations sensibles de `users.py` : `create_user`, `edit_user` (surtout les changements de `role` et `is_active_account`), `delete_user`.
|
||||
|
||||
Attention : `request.remote_addr` n'aura de valeur qu'une fois SEC-05 corrigé — sans `ProxyFix`, toutes les entrées porteront l'IP de nginx.
|
||||
@@ -0,0 +1,585 @@
|
||||
# 2 — Maintenabilité
|
||||
|
||||
15 constats sur la structure du code, l'outillage et la chaîne de livraison.
|
||||
|
||||
| ID | Constat | Sévérité |
|
||||
|---|---|---|
|
||||
| [MNT-01](#mnt-01--) | `.gitignore` ignore `*.html` : tout nouveau template est invisible pour git | 🟠 Élevé |
|
||||
| [MNT-02](#mnt-02--) | `requirements.txt` encodé en UTF-16 | 🟠 Élevé |
|
||||
| [MNT-03](#mnt-03--) | Aucun test, et le job CI « Tests » est un leurre | 🟠 Élevé |
|
||||
| [MNT-04](#mnt-04--) | Le job CI « Security Scan » pointe vers un fichier inexistant | 🟠 Élevé |
|
||||
| [MNT-05](#mnt-05--) | Trois dépendances parasites, dont un doublon de `python-dotenv` | 🟡 Moyen |
|
||||
| [MNT-06](#mnt-06--) | Aucune configuration Ruff alors que la CI exige `ruff format --check` | 🟡 Moyen |
|
||||
| [MNT-07](#mnt-07--) | `backup.py` cible SQLite alors que l'application impose PostgreSQL | 🟡 Moyen |
|
||||
| [MNT-08](#mnt-08--) | `users.py` : 1 245 lignes, six responsabilités distinctes | 🟡 Moyen |
|
||||
| [MNT-09](#mnt-09--) | Code mort : imports et constantes jamais utilisés | 🟡 Moyen |
|
||||
| [MNT-10](#mnt-10--) | Requêtes N+1 systématiques dans six modules de routes | 🟡 Moyen |
|
||||
| [MNT-11](#mnt-11--) | `delete_team` laisse des références orphelines | 🟡 Moyen |
|
||||
| [MNT-12](#mnt-12--) | Duplication du parsing date/heure dans quatre modules | 🔵 Faible |
|
||||
| [MNT-13](#mnt-13--) | `datetime.utcnow()` déprécié — 12 occurrences | 🔵 Faible |
|
||||
| [MNT-14](#mnt-14--) | Aucune pagination sur les listes | 🔵 Faible |
|
||||
| [MNT-15](#mnt-15--) | README en décalage avec le code, et dossier `supporting_scrits` mal orthographié | 🔵 Faible |
|
||||
|
||||
---
|
||||
|
||||
## MNT-01 · 🟠
|
||||
|
||||
**`.gitignore` ignore `*.html` : tout nouveau template est invisible pour git**
|
||||
|
||||
`.gitignore:23-24`
|
||||
|
||||
```
|
||||
docs/
|
||||
*.html
|
||||
```
|
||||
|
||||
Ces deux règles n'ont pas de portée restreinte. Vérification :
|
||||
|
||||
```
|
||||
$ git check-ignore -v --no-index app/templates/pages/newpage.html
|
||||
.gitignore:24:*.html app/templates/pages/newpage.html
|
||||
|
||||
$ git check-ignore -v --no-index docs/newdoc.html
|
||||
.gitignore:23:docs/ docs/newdoc.html
|
||||
```
|
||||
|
||||
Les 40 templates existants restent suivis (git conserve ce qui est déjà indexé), et les fichiers de `docs/` ont manifestement été ajoutés en forçant. Mais **tout nouveau fichier `.html`, où qu'il soit dans l'arborescence, est silencieusement ignoré**.
|
||||
|
||||
**Impact.** C'est le constat le plus insidieux du rapport. Une personne qui ajoute une page à l'application la verra fonctionner en local, fera son `git add .`, `git commit`, `git push` — sans aucun avertissement — et l'application sera cassée en production avec une `TemplateNotFound`. Le diagnostic est difficile : le fichier existe bien sur le poste de développement, la revue de PR ne montre rien d'anormal, et `git status` reste propre.
|
||||
|
||||
Le même piège s'applique à toute documentation ajoutée sous `docs/` (d'où le choix de placer le présent audit dans `audit/` et non dans `docs/`).
|
||||
|
||||
**Correction.** Remplacer les deux lignes par des règles ciblées sur ce qui était réellement visé — vraisemblablement les rapports de couverture générés et les documents d'architecture exportés :
|
||||
|
||||
```gitignore
|
||||
# rapports générés
|
||||
htmlcov/
|
||||
coverage_html_report/
|
||||
```
|
||||
|
||||
Puis vérifier ce qui manque déjà :
|
||||
|
||||
```bash
|
||||
git status --ignored --short | grep '\.html$'
|
||||
git add -f docs/ # si l'on souhaite versionner la documentation existante
|
||||
```
|
||||
|
||||
Passer en revue le reste du fichier au passage : `*.db` y figure deux fois (`:5` et `:33`), `instance/` et `.instance/` cohabitent.
|
||||
|
||||
---
|
||||
|
||||
## MNT-02 · 🟠
|
||||
|
||||
**`requirements.txt` encodé en UTF-16**
|
||||
|
||||
Les premiers octets du fichier sont `FF FE 61 00` : nomenclature UTF-16 LE, puis `a` codé sur deux octets. Le fichier fait 1 708 octets pour 47 lignes — soit environ le double de la taille attendue.
|
||||
|
||||
C'est le résultat classique d'un `pip freeze > requirements.txt` exécuté depuis PowerShell, dont la redirection produit de l'UTF-16 par défaut sur Windows PowerShell 5.1.
|
||||
|
||||
**Impact.**
|
||||
- `pip install -r requirements.txt` fonctionne sur les versions récentes de pip (qui détectent la nomenclature), mais échoue ou produit des noms de paquets corrompus sur des versions plus anciennes et dans certaines images de conteneurs.
|
||||
- **`pip-audit` ne sait pas parser ce format** : le job CI « Security Audit » (`ci.yml:30-31`) est donc au mieux inopérant. Sa ligne de commande le masque d'ailleurs : `pip-audit --require-hashes --no-deps || pip-audit` — le `||` avale l'échec du premier appel, et le fichier ne contient aucun hachage, donc `--require-hashes` ne pouvait de toute façon pas réussir.
|
||||
- Les diffs git sont illisibles, ce qui rend toute revue de changement de dépendance impossible.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```bash
|
||||
python -c "open('requirements.txt','w',encoding='utf-8',newline='\n').write(open('requirements.txt',encoding='utf-16').read())"
|
||||
```
|
||||
|
||||
Puis ajouter un `.gitattributes` pour éviter la récidive :
|
||||
|
||||
```gitattributes
|
||||
* text=auto eol=lf
|
||||
requirements.txt text eol=lf working-tree-encoding=UTF-8
|
||||
```
|
||||
|
||||
Et sous PowerShell, préférer `pip freeze | Out-File -Encoding utf8 requirements.txt`.
|
||||
|
||||
---
|
||||
|
||||
## MNT-03 · 🟠
|
||||
|
||||
**Aucun test, et le job CI « Tests » est un leurre**
|
||||
|
||||
`.github/workflows/ci.yml:74-94`
|
||||
|
||||
```yaml
|
||||
test:
|
||||
name: Tests
|
||||
needs: [security-audit, lint]
|
||||
steps:
|
||||
…
|
||||
- name: Run tests
|
||||
run: |
|
||||
echo "No tests configured yet. Add tests to the project."
|
||||
# python -m pytest tests/ --cov=. --cov-report=xml
|
||||
continue-on-error: true
|
||||
```
|
||||
|
||||
Il n'existe aucun répertoire `tests/`, aucun fichier `test_*.py`, et ni `pytest` ni `pytest-cov` ne figurent dans `requirements.txt`.
|
||||
|
||||
**Impact.** Le job affiche un statut vert dans l'interface GitHub. Pour quiconque regarde la liste des checks d'une PR, l'application est « testée ». Le `continue-on-error: true` garantit en plus que ce job ne pourra jamais bloquer une fusion, même une fois de vrais tests ajoutés — il faudra penser à retirer ce drapeau.
|
||||
|
||||
L'absence de tests est particulièrement coûteuse ici, parce que le code contient exactement le type de logique qui en réclame : une matrice de permissions à cinq rôles, avec des méthodes `can_manage_this_tryout` / `can_manage_this_org_team` dont le comportement diffère par sous-classe. C'est vérifiable en quelques dizaines de lignes de tests, et invérifiable à la main.
|
||||
|
||||
**Correction.** Commencer par la valeur maximale — la matrice d'autorisation :
|
||||
|
||||
```python
|
||||
# tests/test_permissions.py
|
||||
import pytest
|
||||
|
||||
@pytest.mark.parametrize('role,expected', [
|
||||
('admin', True), ('manager', True), ('coach', False),
|
||||
('player', False), ('scout', False),
|
||||
])
|
||||
def test_can_manage_teams(role, expected, user_factory):
|
||||
assert user_factory(role).can_manage_teams() is expected
|
||||
```
|
||||
|
||||
Puis, par ordre de rendement :
|
||||
1. les méthodes `can_*` de chaque sous-classe de `User` (test unitaire pur, sans base) ;
|
||||
2. les schémas Marshmallow de `validators.py` (idem) ;
|
||||
3. des tests d'intégration sur les routes sensibles avec le client de test Flask : chaque rôle contre chaque route, en vérifiant les 403/redirections — c'est ce qui aurait détecté SEC-15 et SEC-17.
|
||||
|
||||
Ajouter `pytest`, `pytest-cov` et `factory-boy` à un `requirements-dev.txt`, puis activer réellement le job (`continue-on-error` retiré, seuil de couverture progressif).
|
||||
|
||||
---
|
||||
|
||||
## MNT-04 · 🟠
|
||||
|
||||
**Le job CI « Security Scan » pointe vers un fichier inexistant**
|
||||
|
||||
`.github/workflows/ci.yml:72`
|
||||
|
||||
```yaml
|
||||
- name: Run security scan
|
||||
run: python security_scan.py --skip-http
|
||||
```
|
||||
|
||||
Le script se trouve en réalité à `app/supporting_scrits/security_scan.py`. Il n'y a pas de `security_scan.py` à la racine du dépôt.
|
||||
|
||||
Le job échoue donc à chaque exécution avec `can't open file 'security_scan.py': [Errno 2] No such file or directory`. Contrairement au job `test`, celui-ci n'a pas de `continue-on-error` : il apparaît **en rouge en permanence**.
|
||||
|
||||
**Impact.** Une CI qui est toujours rouge cesse d'être un signal. L'équipe s'habitue à fusionner malgré l'échec, et le jour où un vrai problème est détecté, il passe inaperçu. C'est un coût de maintenance négatif : le job consomme des minutes CI et détruit la confiance dans le tableau de bord.
|
||||
|
||||
Deux problèmes secondaires dans le même job :
|
||||
- l'exécution du script importe la configuration de l'application, qui exige `DATABASE_URL` (`app.py:58-62`) ; or seul `SECRET_KEY` est fourni (`ci.yml:70`) — le script échouerait aussi pour cette raison ;
|
||||
- le repli `secrets.CI_SECRET_KEY || 'test-key-not-for-production-1234567890'` fournit une clé de 34 caractères, ce qui passe le contrôle de longueur du script (`security_scan.py:42`) mais valide un scénario qui n'est pas celui de la production.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```yaml
|
||||
- name: Run security scan
|
||||
env:
|
||||
SECRET_KEY: ${{ secrets.CI_SECRET_KEY }}
|
||||
DATABASE_URL: postgresql://postgres:postgres@localhost:5432/ci
|
||||
FLASK_DEBUG: 'false'
|
||||
run: python -m app.supporting_scrits.security_scan --skip-http
|
||||
```
|
||||
|
||||
Et faire échouer le job si le script renvoie un code non nul, ce qu'il fait déjà via son `exit(main())`.
|
||||
|
||||
Vérifier aussi que le job `lint` passe (voir MNT-06) : dans l'état actuel, `test` dépend de `lint` via `needs`, donc un `lint` rouge empêche `test` de s'exécuter.
|
||||
|
||||
---
|
||||
|
||||
## MNT-05 · 🟡
|
||||
|
||||
**Trois dépendances parasites, dont un doublon de `python-dotenv`**
|
||||
|
||||
`requirements.txt` est un `pip freeze` brut de 47 entrées, mêlant dépendances directes et transitives. Trois entrées posent problème :
|
||||
|
||||
| Ligne | Paquet | Problème |
|
||||
|---|---|---|
|
||||
| `:15` | `dotenv==0.9.9` | Doublon confusant de `python-dotenv==1.2.2` (`:36`), déjà présent. Le paquet `dotenv` sur PyPI est un projet distinct et quasi vide ; c'est le nom vers lequel se trompent régulièrement les installations. Le code importe `from dotenv import load_dotenv`, qui est fourni par `python-dotenv` — `dotenv` est donc inutile. |
|
||||
| `:28` | `login==0.0.6` | Paquet PyPI sans rapport avec `Flask-Login` (`:19`, le vrai utilisé). Aucun `import login` dans le code. Installé par confusion de nom. |
|
||||
| `:13` | `discord==2.3.2` | Méta-paquet qui ne fait que réinstaller `discord.py`, déjà épinglé ligne `:14` en version 2.7.1. Les deux versions divergent. |
|
||||
|
||||
**Impact.** Surface d'approvisionnement élargie sans contrepartie : trois paquets supplémentaires exécutent leur `setup.py` à l'installation et sont dans le chemin d'import. Les paquets aux noms proches de bibliothèques populaires (`dotenv`, `login`) sont précisément la cible privilégiée des attaques par confusion de dépendances. Ils sont ici bénins, mais leur présence indique que la liste n'est pas relue.
|
||||
|
||||
Par ailleurs, `psycopg2==2.9.12` (`:35`) exige une chaîne de compilation C et les en-têtes PostgreSQL ; `psycopg2-binary` est le choix usuel pour un déploiement sans compilation, ou `psycopg[binary]` (v3) pour un projet neuf.
|
||||
|
||||
**Correction.**
|
||||
1. Retirer `dotenv`, `login` et `discord` — vérifier au préalable qu'aucun `import` ne les référence (aucun n'apparaît dans le code).
|
||||
2. Séparer les dépendances directes des transitives. Le standard actuel de l'écosystème est un `pyproject.toml` avec `[project] dependencies`, la résolution étant figée dans un fichier de verrouillage (`uv lock`, `pip-tools`) :
|
||||
```toml
|
||||
[project]
|
||||
dependencies = [
|
||||
"Flask~=3.1", "Flask-SQLAlchemy~=3.1", "Flask-Login~=0.6",
|
||||
"Flask-WTF~=1.3", "Flask-Limiter~=4.1", "flask-cors~=6.0",
|
||||
"SQLAlchemy~=2.0", "psycopg2-binary~=2.9", "marshmallow~=4.3",
|
||||
"python-dotenv~=1.2", "discord.py~=2.7", "APScheduler~=3.11",
|
||||
"waitress~=3.0", "requests~=2.34",
|
||||
]
|
||||
```
|
||||
3. Fournir un `requirements-dev.txt` (`pytest`, `pytest-cov`, `ruff`, `pip-audit`).
|
||||
4. Une fois MNT-02 corrigé, générer des hachages (`pip-compile --generate-hashes`) pour que `pip-audit --require-hashes` de `ci.yml:31` ait un sens.
|
||||
|
||||
---
|
||||
|
||||
## MNT-06 · 🟡
|
||||
|
||||
**Aucune configuration Ruff alors que la CI exige `ruff format --check`**
|
||||
|
||||
`.github/workflows/ci.yml:47-51`
|
||||
|
||||
```yaml
|
||||
- name: Run ruff linter
|
||||
run: ruff check . --output-format=github
|
||||
- name: Run ruff formatter check
|
||||
run: ruff format --check .
|
||||
```
|
||||
|
||||
Le dépôt ne contient **ni `pyproject.toml`, ni `ruff.toml`, ni `.ruff.toml`, ni `setup.cfg`**. Ruff s'exécute donc avec ses réglages par défaut : longueur de ligne 88, jeu de règles `E4,E7,E9,F`.
|
||||
|
||||
**Impact.**
|
||||
- `ruff format --check .` va signaler la quasi-totalité des fichiers. Le style du projet — alignement des arguments sur plusieurs lignes, virgules finales, guillemets simples — ne correspond pas à la sortie du formateur Ruff, qui normalise vers des guillemets doubles. C'est un job durablement rouge, avec le même effet de désensibilisation que MNT-04.
|
||||
- `ruff check` avec les règles par défaut inclut `F401` (import inutilisé) : il signalera les imports morts identifiés en MNT-09, ce qui est souhaitable — mais aussi les ré-exports volontaires de `app/models/__init__.py`, où seule la ligne `:28` porte un `# noqa: F401`. Les 20 autres imports du fichier n'en ont pas et seront signalés à tort.
|
||||
- La limite de 88 caractères est enfreinte à de nombreux endroits (`users.py:43`, `matches.py:183`, `team_matches.py:183`…).
|
||||
|
||||
**Correction.** Ajouter un `pyproject.toml` qui reflète le style réel du projet, puis appliquer le formatage en un commit isolé (à ajouter à `.git-blame-ignore-revs` pour ne pas polluer `git blame`) :
|
||||
|
||||
```toml
|
||||
[tool.ruff]
|
||||
line-length = 110
|
||||
target-version = "py312"
|
||||
exclude = ["app/supporting_scrits/seed.py"]
|
||||
|
||||
[tool.ruff.lint]
|
||||
select = ["E", "F", "W", "I", "B", "S", "N", "UP"]
|
||||
ignore = ["E501"]
|
||||
|
||||
[tool.ruff.lint.per-file-ignores]
|
||||
"app/models/__init__.py" = ["F401"]
|
||||
|
||||
[tool.ruff.format]
|
||||
quote-style = "single"
|
||||
```
|
||||
|
||||
`S` active les règles `flake8-bandit` — utile pour ce projet : elles auraient signalé plusieurs constats du volet sécurité. `I` trie les imports, `UP` détecte les constructions obsolètes comme `datetime.utcnow()` (MNT-13).
|
||||
|
||||
---
|
||||
|
||||
## MNT-07 · 🟡
|
||||
|
||||
**`backup.py` cible SQLite alors que l'application impose PostgreSQL**
|
||||
|
||||
`app/supporting_scrits/backup.py:17,23`
|
||||
|
||||
```python
|
||||
import sqlite3
|
||||
DATABASE_PATH = os.getenv('DATABASE_PATH', os.path.join(os.getcwd(), 'instance', 'team_tryouts.db'))
|
||||
```
|
||||
|
||||
Le script sauvegarde un fichier SQLite via `sqlite3.Connection.backup()` (`:47-51`) et vérifie son intégrité par `PRAGMA integrity_check` (`:124`).
|
||||
|
||||
Or `app/app.py:58-62` **refuse de démarrer** si `DATABASE_URL` n'est pas défini, avec le message *« must be set to a PostgreSQL connection string »*, et `.env.exemple:21` pointe vers une instance PostgreSQL hébergée sur Render.
|
||||
|
||||
**Impact.** Le script ne trouve jamais de fichier à sauvegarder. Il ne plante même pas : `backup_database()` (`:38-40`) détecte l'absence du fichier, affiche `[WARNING] Database not found … Skipping database backup.` et renvoie `None`. `main()` poursuit, sauvegarde les documents, et **affiche `=== Backup completed successfully ===`**.
|
||||
|
||||
C'est le pire cas possible pour une procédure de sauvegarde : elle rapporte un succès alors qu'aucune donnée n'a été sauvegardée. Si le script est planifié (le docstring évoque le Planificateur de tâches Windows), l'organisation croit disposer de sauvegardes qui n'existent pas — et ne le découvrira qu'au moment de restaurer.
|
||||
|
||||
Le script conserve toutefois une valeur réelle : `backup_documents()` (`:59-80`) archive correctement le dossier `documents/`, qui contient les contrats signés et n'est pas dans la base.
|
||||
|
||||
**Correction.**
|
||||
1. Remplacer la sauvegarde de base par un appel à `pg_dump`, en dérivant les paramètres de `DATABASE_URL` :
|
||||
```python
|
||||
subprocess.run(['pg_dump', '--no-owner', '--format=custom',
|
||||
'--file', backup_path, os.environ['DATABASE_URL']], check=True)
|
||||
```
|
||||
`check=True` est essentiel : c'est ce qui manque aujourd'hui, conceptuellement, au script.
|
||||
2. Faire renvoyer un code de sortie non nul à `main()` quand la sauvegarde de base a échoué ou été ignorée — aujourd'hui `success` n'est mis à `False` que si la vérification d'intégrité échoue (`:164-166`), jamais si la sauvegarde n'a pas eu lieu.
|
||||
3. Vérifier que Render, qui héberge la base, fournit déjà des sauvegardes automatiques — auquel cas ce script n'a besoin de couvrir que `documents/`, et doit le dire explicitement.
|
||||
|
||||
---
|
||||
|
||||
## MNT-08 · 🟡
|
||||
|
||||
**`users.py` : 1 245 lignes, six responsabilités distinctes**
|
||||
|
||||
Volumétrie des modules de routes :
|
||||
|
||||
| Module | Lignes |
|
||||
|---|---|
|
||||
| `users.py` | **1 245** |
|
||||
| `matches.py` | 583 |
|
||||
| `teams.py` | 455 |
|
||||
| `tryouts.py` | 432 |
|
||||
| `team_matches.py` | 309 |
|
||||
| `auth.py` | 296 |
|
||||
| `evaluations.py` | 233 |
|
||||
| `main.py` | 139 |
|
||||
|
||||
`users.py` regroupe six domaines fonctionnels sans lien entre eux, matérialisés par les commentaires de section du fichier lui-même :
|
||||
|
||||
1. gestion des comptes (`:76-228`) — CRUD administrateur ;
|
||||
2. profil personnel (`:231-299`) ;
|
||||
3. disponibilités (`:302-441`) — API JSON ;
|
||||
4. contrats (`:444-629`) — téléversement et téléchargement de fichiers ;
|
||||
5. sessions 1:1 (`:632-888`) — dont l'intégration Discord ;
|
||||
6. notes (`:891-1245`) — personnelles et d'équipe, avec quatre routes de création presque identiques.
|
||||
|
||||
**Impact.** Un fichier de cette taille concentre les conflits de fusion, ralentit la navigation, et rend la revue de code superficielle. Le symptôme visible ici : les trois schémas de validation importés en tête ont cessé d'être utilisés (SEC-06) sans que personne ne le remarque, parce que la déclaration et l'usage sont séparés par plusieurs centaines de lignes.
|
||||
|
||||
Le préfixe d'URL trahit le problème : `url_prefix='/users'` produit des routes comme `/users/one-on-one` et `/users/contracts/<id>/download`, qui ne concernent pas la gestion des utilisateurs.
|
||||
|
||||
**Correction.** Scinder en blueprints alignés sur les domaines, avec leurs propres préfixes :
|
||||
|
||||
```
|
||||
app/routes/
|
||||
users.py → /users (comptes + profil)
|
||||
availability.py → /availability (disponibilités joueurs + coachs)
|
||||
contracts.py → /contracts
|
||||
one_on_one.py → /one-on-one
|
||||
notes.py → /notes
|
||||
```
|
||||
|
||||
Les redirections des anciennes URL sont à prévoir, ou à assumer comme rupture si l'application n'a pas d'usagers externes.
|
||||
|
||||
Les quatre routes de création de note (`manage_personal_notes` `:1081`, `add_personal_note` `:1116`, `add_note_from_tryout` `:1157`, `add_note_from_match` `:1203`) partagent 80 % de leur corps et gagneraient à être unifiées derrière une seule route acceptant un contexte optionnel.
|
||||
|
||||
---
|
||||
|
||||
## MNT-09 · 🟡
|
||||
|
||||
**Code mort : imports et constantes jamais utilisés**
|
||||
|
||||
Vérifié par comptage d'occurrences dans `app/routes/users.py` — une seule occurrence signifie que le nom n'apparaît que sur sa ligne de déclaration :
|
||||
|
||||
| Nom | Ligne | Occurrences |
|
||||
|---|---|---|
|
||||
| `CreateUserSchema` | `:24` | 1 |
|
||||
| `EditUserSchema` | `:24` | 1 |
|
||||
| `EditProfileSchema` | `:24` | 1 |
|
||||
| `csrf` | `:10` | 1 |
|
||||
| `ALLOWED_CONTRACT_EXTENSIONS` | `:29` | 1 |
|
||||
| `ALLOWED_SIGNED_EXTENSIONS` | `:30` | 1 |
|
||||
| `date_type` | `:21` | 1 |
|
||||
|
||||
Ailleurs :
|
||||
- `app/app.py:19-30` — filtre `nl2br` enregistré, jamais employé dans aucun template (cf. SEC-09) ;
|
||||
- `app/logging_config.py:148-154` — `get_auth_logger()` jamais appelé (cf. SEC-19) ; le fichier `auth.log` et son handler sont configurés pour rien ;
|
||||
- `app/routes/users.py:636` — `DISCORD_WEBHOOK_URL` lu au moment de l'import du module, ce qui empêche toute modification par test ou rechargement de configuration.
|
||||
|
||||
**Impact.** Deux des entrées de cette liste ne sont pas de simples résidus : les trois schémas et les deux constantes d'extension **décrivent un comportement de sécurité que le lecteur croit implémenté**. Un relecteur voyant `ALLOWED_SIGNED_EXTENSIONS = {'pdf'}` en tête de fichier conclut raisonnablement que les téléversements sont filtrés. Ils ne le sont pas (SEC-11). Le code mort ment ici sur les propriétés du système.
|
||||
|
||||
**Correction.** Deux traitements distincts :
|
||||
- **Rebrancher** ce qui devait l'être : les trois schémas (SEC-06), les deux constantes d'extension (SEC-11), `get_auth_logger` (SEC-19).
|
||||
- **Supprimer** le reste : `csrf`, `date_type`, et le filtre `nl2br` s'il n'est pas destiné à servir.
|
||||
|
||||
`ruff check` avec la règle `F401` (active par défaut) détecte les imports inutilisés — le job `lint` de la CI les aurait signalés s'il s'exécutait (MNT-06). Les constantes de module ne sont pas couvertes par `F401` ; `vulture` peut compléter.
|
||||
|
||||
---
|
||||
|
||||
## MNT-10 · 🟡
|
||||
|
||||
**Requêtes N+1 systématiques dans six modules de routes**
|
||||
|
||||
Le motif « requête dans une boucle » est présent dans `evaluations.py`, `main.py`, `matches.py`, `teams.py`, `tryouts.py` et `users.py`. Exemples représentatifs :
|
||||
|
||||
`routes/tryouts.py:163` — une requête par joueur inscrit :
|
||||
```python
|
||||
registered_players = [User.query.get(r.player_id) for r in registrations if r.player_id]
|
||||
```
|
||||
|
||||
`routes/tryouts.py:168-172` — puis une requête d'évaluation par joueur, dans la même vue :
|
||||
```python
|
||||
for p in registered_players:
|
||||
existing = Evaluation.query.filter_by(tryout_id=…, player_id=p.id, evaluator_id=…).first()
|
||||
```
|
||||
|
||||
`routes/users.py:320-335` (`get_disponibilities`) — la liste complète des joueurs actifs, puis leurs disponibilités une par une via la relation.
|
||||
|
||||
`routes/matches.py:536-541` (`get_players_available_at_time`) — boucle sur tous les joueurs actifs avec une requête `PlayerDisponibility` chacun.
|
||||
|
||||
`routes/matches.py:226` — cas notable, la requête est exécutée **deux fois par joueur** :
|
||||
```python
|
||||
all_players = [User.query.get(r.player_id) for r in registrations if User.query.get(r.player_id)]
|
||||
```
|
||||
|
||||
**Impact.** À l'échelle actuelle d'une association étudiante (quelques dizaines de joueurs), l'effet est imperceptible. Le point d'attention est la trajectoire : `view_tryout` (`tryouts.py:134-252`) exécute déjà, pour un tryout de 30 inscrits avec 4 matchs, de l'ordre de 100 requêtes par affichage de page. Sur une base PostgreSQL distante (Render), chaque requête paie une latence réseau — c'est là que le coût devient visible, bien avant qu'il ne le soit en charge CPU.
|
||||
|
||||
**Correction.** Charger en une requête plutôt qu'en boucle :
|
||||
|
||||
```python
|
||||
# au lieu de [User.query.get(r.player_id) for r in registrations]
|
||||
player_ids = [r.player_id for r in registrations]
|
||||
players = {u.id: u for u in User.query.filter(User.id.in_(player_ids))}
|
||||
registered_players = [players[pid] for pid in player_ids if pid in players]
|
||||
```
|
||||
|
||||
Pour les relations, utiliser le chargement anticipé de SQLAlchemy :
|
||||
|
||||
```python
|
||||
from sqlalchemy.orm import selectinload
|
||||
registrations = TryoutRegistration.query.options(
|
||||
selectinload(TryoutRegistration.player)
|
||||
).filter_by(tryout_id=tryout_id).all()
|
||||
```
|
||||
|
||||
Plusieurs relations sont déclarées `lazy='dynamic'` (`models/user_model/user.py:42-52`), ce qui interdit le chargement anticipé. Ce mode n'a d'intérêt que si l'on filtre systématiquement la relation ; sinon `lazy='select'` (défaut) ou `lazy='selectin'` est préférable.
|
||||
|
||||
Activer `SQLALCHEMY_RECORD_QUERIES` en développement pour mesurer avant d'optimiser.
|
||||
|
||||
---
|
||||
|
||||
## MNT-11 · 🟡
|
||||
|
||||
**`delete_team` laisse des références orphelines**
|
||||
|
||||
`app/routes/teams.py:202-224`
|
||||
|
||||
```python
|
||||
tryouts = Tryout.query.filter_by(target_org_team_id=team_id).all()
|
||||
for t in tryouts:
|
||||
t.target_org_team_id = None
|
||||
db.session.commit()
|
||||
|
||||
TeamPlayer.query.filter_by(org_team_id=team_id).delete()
|
||||
db.session.commit()
|
||||
|
||||
db.session.delete(team)
|
||||
db.session.commit()
|
||||
```
|
||||
|
||||
Seuls `Tryout.target_org_team_id` et `TeamPlayer` sont traités. Trois modèles référencent pourtant `org_team_id` et ne sont pas nettoyés : `TeamNote`, `TeamMatch` et `OneOnOneRequest`. `OrgTeam.coaches` / `OrgTeam.managers` (tables d'association) ne sont pas vidés non plus.
|
||||
|
||||
**Impact.** Selon les contraintes effectivement créées par `db.create_all()`, la suppression lève une `IntegrityError` (violation de clé étrangère) → HTTP 500, l'équipe n'est pas supprimée, et l'utilisateur ne comprend pas pourquoi. Le gestionnaire 500 fait bien le `rollback` (`app.py:316`), donc pas de corruption — mais la fonctionnalité est inutilisable dès qu'une équipe a des notes ou des matchs, c'est-à-dire dans tous les cas réels.
|
||||
|
||||
Second problème, structurel : la fonction enchaîne **trois `commit()` successifs**. Si le troisième échoue, les deux premiers sont déjà persistés — les tryouts ont perdu leur équipe cible et les joueurs ont été retirés, alors que l'équipe existe toujours. L'opération n'est pas atomique.
|
||||
|
||||
À comparer avec `delete_user` (`users.py:142-185`), qui traite 15 modèles liés et ne commite qu'une fois à la fin — c'est le bon modèle à suivre.
|
||||
|
||||
**Correction.** À court terme, aligner sur `delete_user` : traiter toutes les tables liées, un seul `commit()` final.
|
||||
|
||||
À moyen terme, le problème de fond est que ces cascades sont écrites à la main dans les vues. Les déclarer sur les relations est plus sûr, parce que la base garantit alors la cohérence quel que soit le chemin de suppression :
|
||||
|
||||
```python
|
||||
# models/org_team/org_team.py
|
||||
notes = db.relationship('TeamNote', backref='org_team',
|
||||
cascade='all, delete-orphan', passive_deletes=True)
|
||||
```
|
||||
|
||||
avec `ondelete='CASCADE'` sur la `ForeignKey` correspondante. Cela suppose des migrations (cf. STD-01), les contraintes existantes ayant été créées sans.
|
||||
|
||||
---
|
||||
|
||||
## MNT-12 · 🔵
|
||||
|
||||
**Duplication du parsing date/heure dans quatre modules**
|
||||
|
||||
Le même bloc apparaît, à quelques variantes près, dans `matches.py` (`:244-263`, `:368-392`), `team_matches.py` (`:135-158`, `:216-236`), `users.py` (`:366-371`, `:732-741`, `:947-951`) et `tryouts.py` (`:60-65`, `:110-115`) :
|
||||
|
||||
```python
|
||||
try:
|
||||
date_obj = datetime.strptime(date_str, '%Y-%m-%d').date()
|
||||
start_time = datetime.strptime(start_time_str, '%H:%M').time()
|
||||
if end_time_str:
|
||||
end_time = datetime.strptime(end_time_str, '%H:%M').time()
|
||||
else:
|
||||
end_time = (datetime.combine(date_obj, start_time) + timedelta(minutes=30)).time()
|
||||
except (ValueError, TypeError):
|
||||
flash('Invalid date format.', 'danger')
|
||||
return …
|
||||
```
|
||||
|
||||
La règle métier « une plage sans heure de fin dure 30 minutes » est réimplémentée cinq fois. `add_30_minutes()` existe pourtant déjà dans `users.py:309-310`, mais n'est utilisée que localement.
|
||||
|
||||
Les traitements divergent d'ailleurs entre copies : `team_matches.py:227-236` avale silencieusement les heures invalides (`except ValueError: pass`), alors que `matches.py:261-263` affiche une erreur, et `matches.py:390-391` met `start_time` à `None`. Trois comportements différents pour la même saisie invalide.
|
||||
|
||||
**Correction.** Extraire dans un module `app/utils/datetimes.py` :
|
||||
|
||||
```python
|
||||
DEFAULT_SLOT_MINUTES = 30
|
||||
|
||||
def parse_date(value: str) -> date: ...
|
||||
def parse_time_range(start: str, end: str | None, on: date) -> tuple[time, time]: ...
|
||||
```
|
||||
|
||||
Ces fonctions ont l'avantage d'être testables sans base ni contexte de requête — un bon premier chantier de tests (MNT-03).
|
||||
|
||||
Mieux encore : ces conversions relèvent de la validation d'entrée. `validators.py` définit déjà `OneOnOneRequestSchema` avec des champs date/heure (`:407-441`), mais en `fields.String` avec `validate.Regexp` plutôt qu'en `fields.Date` / `fields.Time`, qui feraient la conversion et le contrôle en une étape.
|
||||
|
||||
---
|
||||
|
||||
## MNT-13 · 🔵
|
||||
|
||||
**`datetime.utcnow()` déprécié — 12 occurrences**
|
||||
|
||||
`datetime.utcnow()` est déprécié depuis Python 3.12. L'application cible Python 3.12 (`ci.yml:24`, `:42`, `:62`).
|
||||
|
||||
Le problème n'est pas seulement l'avertissement : la méthode renvoie un `datetime` **naïf** (sans fuseau) contenant une heure UTC. Comparé à un `datetime` local, le résultat est silencieusement faux.
|
||||
|
||||
Occurrences : `models/user_model/user.py:24`, `models/*` (valeurs par défaut de `created_at`), `routes/auth.py:114,115,153`, `routes/users.py:601,824,867`, `routes/teams.py:66`, `routes/tryouts.py:34,252`, `routes/team_matches.py:87`.
|
||||
|
||||
Le cas le plus sensible est `auth.py:114` :
|
||||
```python
|
||||
if user and user.locked_until and user.locked_until > datetime.utcnow():
|
||||
```
|
||||
La comparaison est correcte tant que `locked_until` est écrit par le même appel naïf (`:153`) — c'est le cas. Mais toute introduction d'une valeur consciente du fuseau, par exemple via le bot Discord qui utilise `ZoneInfo('America/Toronto')` (`discord_bot.py:55`), lèvera un `TypeError: can't compare offset-naive and offset-aware datetimes`.
|
||||
|
||||
**Correction.**
|
||||
|
||||
```python
|
||||
from datetime import datetime, timezone
|
||||
datetime.now(timezone.utc)
|
||||
```
|
||||
|
||||
Et déclarer les colonnes en `db.DateTime(timezone=True)`. Le changement doit être fait d'un bloc : mélanger valeurs naïves et conscientes dans une même colonne produit des comparaisons erronées. Les données existantes étant en UTC naïf, une migration les convertit sans perte.
|
||||
|
||||
La règle Ruff `DTZ` (`flake8-datetimez`) détecte ces appels — à ajouter au `select` de MNT-06.
|
||||
|
||||
---
|
||||
|
||||
## MNT-14 · 🔵
|
||||
|
||||
**Aucune pagination sur les listes**
|
||||
|
||||
Toutes les vues de liste chargent l'intégralité de la table :
|
||||
|
||||
| Route | Ligne | Requête |
|
||||
|---|---|---|
|
||||
| `users.list_users` | `users.py:84` | `User.query.order_by(…).all()` |
|
||||
| `evaluations.list_evaluations` | `evaluations.py:76-80` | trois `outerjoin` puis `.all()` |
|
||||
| `teams.list_teams` | `teams.py:48-50` | trois requêtes `.all()` (coachs, managers, **tous les joueurs**) |
|
||||
| `team_matches.list_matches` | `team_matches.py:69` | `.all()`, puis boucle sur les participants |
|
||||
| `matches.api_events` | `matches.py:46-136` | tous les tryouts visibles, tous leurs matchs, tous les participants |
|
||||
|
||||
`teams.list_teams` charge `all_players` (`:50`) pour peupler des listes déroulantes du formulaire — la totalité des joueurs de l'organisation est sérialisée dans le HTML à chaque affichage de la page équipes.
|
||||
|
||||
**Impact.** Nul aujourd'hui, à l'échelle d'une association étudiante. Le constat est consigné pour la trajectoire : combiné aux N+1 de MNT-10, `api_events` est la route qui se dégradera en premier, puisqu'elle cumule le chargement complet et les requêtes en boucle, et qu'elle est appelée à chaque ouverture du calendrier.
|
||||
|
||||
**Correction.** Différer, mais choisir dès maintenant le motif pour ne pas avoir à le rétro-adapter. Flask-SQLAlchemy fournit `paginate()` :
|
||||
|
||||
```python
|
||||
page = request.args.get('page', 1, type=int)
|
||||
pagination = User.query.order_by(User.role, User.username).paginate(page=page, per_page=50)
|
||||
return render_template('pages/users.html', users=pagination.items, pagination=pagination)
|
||||
```
|
||||
|
||||
Pour les listes déroulantes, préférer un point d'API filtré par saisie (autocomplétion) plutôt que l'injection de la table complète dans le HTML.
|
||||
|
||||
---
|
||||
|
||||
## MNT-15 · 🔵
|
||||
|
||||
**README en décalage avec le code, et dossier `supporting_scrits` mal orthographié**
|
||||
|
||||
**README.** La section « Security Features Implemented » (`README.md:11-21`) énonce des garanties qui ne correspondent pas au code :
|
||||
|
||||
| Affirmation | Réalité |
|
||||
|---|---|
|
||||
| *« Authorization Checks: Proper ownership validation on all sensitive operations »* | Faux pour `view_user` (SEC-15) et `add_to_team` (SEC-17) |
|
||||
| *« Rate Limiting: Login endpoint limited to 10 requests per minute »* | La limite existe mais s'applique à un seau global (SEC-05) |
|
||||
| *« Secure Session Cookies: HTTPSOnly… »* | Il s'agit de `HttpOnly` ; et `SESSION_COOKIE_SECURE` vaut `false` dans le fichier d'exemple |
|
||||
|
||||
Le README ne mentionne par ailleurs ni la commande de démarrage exacte, ni la version de Python requise, ni la procédure d'installation (`pip install -r requirements.txt`), ni le fait qu'une base PostgreSQL est obligatoire au démarrage. Un nouvel arrivant ne peut pas lancer le projet en suivant le document.
|
||||
|
||||
**Impact.** Une documentation de sécurité fausse est plus nuisible qu'absente : elle sert de base aux décisions de déploiement et coupe court aux vérifications. C'est vraisemblablement ce qui explique que les constats SEC-06 et SEC-15 aient survécu.
|
||||
|
||||
**Nommage.** Le dossier `app/supporting_scrits/` contient une faute (`scrits` → `scripts`). Elle se propage dans tous les imports (`app.py:355`, `ci.yml`, docstrings) et dans les chemins que les développeurs tapent quotidiennement.
|
||||
|
||||
**Correction.**
|
||||
- Réécrire la section sécurité pour décrire l'état réel, en distinguant ce qui est implémenté de ce qui est prévu. Y ajouter les prérequis, l'installation et le démarrage.
|
||||
- Renommer le dossier en `app/supporting_scripts/` (`git mv`), en mettant à jour `app/app.py:355` et `.github/workflows/ci.yml:72` — ce dernier étant de toute façon à corriger (MNT-04).
|
||||
@@ -0,0 +1,393 @@
|
||||
# 3 — Respect des standards de la stack
|
||||
|
||||
Écarts aux conventions établies de Flask, SQLAlchemy et du déploiement d'applications web Python.
|
||||
|
||||
| ID | Constat | Sévérité |
|
||||
|---|---|---|
|
||||
| [STD-01](#std-01--) | Pas de migrations : `db.create_all()` au démarrage | 🟠 Élevé |
|
||||
| [STD-02](#std-02--) | `create_app()` déclenche des effets de bord au démarrage | 🟡 Moyen |
|
||||
| [STD-03](#std-03--) | Le bot Discord est démarré depuis la fabrique d'application | 🟡 Moyen |
|
||||
| [STD-04](#std-04--) | Trois approches de validation coexistent ; Flask-WTF inutilisé | 🟡 Moyen |
|
||||
| [STD-05](#std-05--) | Aucun objet de configuration ni séparation d'environnements | 🟡 Moyen |
|
||||
| [STD-06](#std-06--) | Ports et adresses d'écoute incohérents entre cinq fichiers | 🟡 Moyen |
|
||||
| [STD-07](#std-07--) | Journalisation vers des fichiers locaux plutôt que stdout | 🟡 Moyen |
|
||||
| [STD-08](#std-08--) | Logique métier dans les vues, sans couche de service | 🔵 Faible |
|
||||
| [STD-09](#std-09--) | Gestion d'erreurs API par préfixe d'URL codé en dur | 🔵 Faible |
|
||||
| [STD-10](#std-10--) | Pas de conteneurisation ni de procédure de déploiement reproductible | 🔵 Faible |
|
||||
|
||||
---
|
||||
|
||||
## STD-01 · 🟠
|
||||
|
||||
**Pas de migrations : `db.create_all()` au démarrage**
|
||||
|
||||
`app/app.py:348-351`
|
||||
|
||||
```python
|
||||
with app.app_context():
|
||||
import app.models as models
|
||||
from app.models import User
|
||||
db.create_all()
|
||||
```
|
||||
|
||||
Ni `Flask-Migrate` ni `alembic` ne figurent dans `requirements.txt`, et il n'existe pas de dossier `migrations/`.
|
||||
|
||||
`db.create_all()` **ne crée que les tables absentes**. Il ne modifie jamais une table existante : ajouter une colonne, changer un type, ajouter une contrainte d'unicité ou une clé étrangère `ON DELETE CASCADE` n'a aucun effet sur une base déjà initialisée.
|
||||
|
||||
**Impact.** C'est le constat qui bloque le plus de corrections proposées dans ce rapport :
|
||||
- SEC-07 demande une contrainte d'unicité sur `User.discord_user_id` — impossible sans migration ;
|
||||
- MNT-11 demande des `ondelete='CASCADE'` sur les clés étrangères d'`OrgTeam` — impossible sans migration ;
|
||||
- MNT-13 demande de passer les colonnes en `DateTime(timezone=True)` — impossible sans migration.
|
||||
|
||||
Au-delà, le mode de fonctionnement actuel implique que **toute évolution du schéma se fait à la main en production**, par des `ALTER TABLE` non versionnés, non revus et non rejouables. Les environnements divergent silencieusement : une base créée aujourd'hui par `create_all()` n'a pas la même structure qu'une base créée il y a six mois puis modifiée manuellement. Il n'existe aucun moyen de savoir laquelle est correcte.
|
||||
|
||||
**Correction.** Introduire Flask-Migrate. La séquence sur une base existante :
|
||||
|
||||
```bash
|
||||
pip install Flask-Migrate
|
||||
flask db init
|
||||
flask db stamp head # déclare la base actuelle comme point de départ
|
||||
```
|
||||
|
||||
puis, pour chaque évolution :
|
||||
|
||||
```bash
|
||||
flask db migrate -m "unicité sur discord_user_id"
|
||||
flask db upgrade
|
||||
```
|
||||
|
||||
Dans `app.py`, remplacer le bloc `create_all()` par l'initialisation de l'extension :
|
||||
|
||||
```python
|
||||
from flask_migrate import Migrate
|
||||
migrate = Migrate() # dans extensions.py
|
||||
migrate.init_app(app, db)
|
||||
```
|
||||
|
||||
et retirer `db.create_all()` de la fabrique — les migrations s'appliquent lors du déploiement, pas au démarrage du processus.
|
||||
|
||||
**Relire systématiquement les migrations générées** : l'autogénération d'Alembic gère mal les modèles à héritage polymorphe de table unique comme `User`, et produit régulièrement des suppressions de colonnes non voulues.
|
||||
|
||||
---
|
||||
|
||||
## STD-02 · 🟡
|
||||
|
||||
**`create_app()` déclenche des effets de bord au démarrage**
|
||||
|
||||
`app/app.py:33-365`. La fabrique, en plus de configurer l'application, exécute :
|
||||
|
||||
| Ligne | Effet de bord |
|
||||
|---|---|
|
||||
| `:351` | `db.create_all()` — écriture dans le schéma |
|
||||
| `:354-356` | Insertion des données de démonstration si la table est vide |
|
||||
| `:359-361` | Démarrage d'un thread portant un client Discord |
|
||||
| `logging_config.py:66-67` | `os.makedirs('logs')` dans le répertoire de travail courant |
|
||||
|
||||
Le motif de fabrique d'application (*application factory*) existe précisément pour permettre de construire plusieurs instances configurées différemment — c'est ce qui rend une application Flask testable. Ici, chaque appel à `create_app()` écrit dans la base et ouvre une connexion Discord.
|
||||
|
||||
**Impact.**
|
||||
- **Les tests d'intégration sont impraticables.** Le motif standard est une fixture `app = create_app(TestConfig)` par session de test ; ici, chaque construction tenterait de se connecter à Discord et de peupler la base. C'est un facteur direct de MNT-03 (absence de tests) : l'architecture rend le premier test coûteux à écrire.
|
||||
- Le comportement dépend du répertoire de travail (`os.getcwd()` en `logging_config.py:66` et `users.py:541`), donc du mode de lancement — le dossier `logs/` n'apparaît pas au même endroit selon qu'on lance `python run.py` ou `python wsgi.py` depuis un autre répertoire.
|
||||
- Le démarrage n'est pas idempotent (cf. STD-03).
|
||||
|
||||
**Correction.** Faire accepter une configuration à la fabrique et sortir les effets de bord :
|
||||
|
||||
```python
|
||||
def create_app(config_object=None):
|
||||
app = Flask(__name__)
|
||||
app.config.from_object(config_object or ProductionConfig)
|
||||
...
|
||||
return app
|
||||
```
|
||||
|
||||
- `create_all` / seed → commandes CLI (`flask db upgrade`, `flask seed-demo`), exécutées au déploiement ;
|
||||
- bot Discord → processus séparé (cf. STD-03) ;
|
||||
- chemin des logs → configurable, dérivé de `app.root_path` plutôt que de `os.getcwd()`.
|
||||
|
||||
---
|
||||
|
||||
## STD-03 · 🟡
|
||||
|
||||
**Le bot Discord est démarré depuis la fabrique d'application**
|
||||
|
||||
`app/app.py:358-363`
|
||||
|
||||
```python
|
||||
try:
|
||||
from app.discord_bot import start_bot
|
||||
start_bot(flask_app=app)
|
||||
except Exception as e:
|
||||
app.logger.warning('Could not start Discord bot: %s', e)
|
||||
```
|
||||
|
||||
`discord_bot.py` lance un thread portant une boucle asyncio, un client Discord et un `AsyncIOScheduler` avec une tâche cron quotidienne à 18h00 (`:78-84`).
|
||||
|
||||
**Impact.** Le couplage entre le cycle de vie du serveur web et celui du bot pose trois problèmes concrets :
|
||||
|
||||
1. **Multiplication des instances.** Un seul processus est lancé aujourd'hui (`wsgi.py` avec Waitress multi-threads), donc un seul bot. Mais toute mise à l'échelle horizontale — plusieurs workers, deux instances derrière un équilibreur, un déploiement bleu-vert où l'ancienne et la nouvelle version tournent en parallèle — crée **autant de connexions Discord et autant de planificateurs**. Les rappels quotidiens partiraient alors en double ou en triple. Discord limite en outre le nombre de sessions simultanées par token.
|
||||
2. **Défaillance silencieuse.** Le `except Exception` avale toute erreur en un simple `warning`. Si le token est invalide ou révoqué — ce qui va arriver lors de la correction de SEC-01 — l'application démarre normalement et les notifications cessent, sans alerte.
|
||||
3. **Le web ne peut pas redémarrer sans couper les notifications**, et inversement.
|
||||
|
||||
**Correction.** Séparer les processus, ce qui est le modèle standard pour une tâche de fond persistante :
|
||||
|
||||
```
|
||||
web: python wsgi.py
|
||||
bot: python -m app.discord_bot
|
||||
```
|
||||
|
||||
La communication passe alors par la base de données partagée — plutôt que par la `Queue` en mémoire actuelle (`discord_bot.py:53`), qui impose justement le partage du processus.
|
||||
|
||||
Si la séparation est trop coûteuse à court terme, deux garde-fous minimaux :
|
||||
- conditionner le démarrage à une variable (`ENABLE_DISCORD_BOT`), pour pouvoir le désactiver en développement et en CI ;
|
||||
- poser un verrou consultatif PostgreSQL (`pg_advisory_lock`) pour garantir qu'une seule instance planifie les rappels.
|
||||
|
||||
Journaliser l'échec en `error`, pas en `warning`.
|
||||
|
||||
---
|
||||
|
||||
## STD-04 · 🟡
|
||||
|
||||
**Trois approches de validation coexistent ; Flask-WTF inutilisé**
|
||||
|
||||
Trois mécanismes se répartissent la validation des entrées, sans règle apparente :
|
||||
|
||||
| Approche | Où | Exemple |
|
||||
|---|---|---|
|
||||
| Schémas Marshmallow | `auth.py` uniquement, plus `upload_contract` | `auth.py:206-218` |
|
||||
| Contrôles manuels | partout ailleurs | `evaluations.py:20-30` (`validate_score`), `tryouts.py:296` (`if new_status in [...]`) |
|
||||
| Aucune validation | `users.py` (create/edit user, edit profile) | SEC-06 |
|
||||
|
||||
`Flask-WTF==1.3.0` est installé et `CSRFProtect` en est utilisé — mais **aucun `FlaskForm` n'est défini** dans le projet. La bibliothèque n'est présente que pour la protection CSRF, alors que sa fonction principale est la définition et le rendu de formulaires validés. `WTForms==3.2.2` est également installé et totalement inutilisé.
|
||||
|
||||
**Impact.** L'absence de convention est la cause structurelle de SEC-06 : quand trois approches sont acceptables, aucune n'est obligatoire, et une route peut n'en appliquer aucune sans que cela détonne à la lecture. Les règles sont par ailleurs dupliquées — la politique de mot de passe existe dans `validators.py:22-24`, mais rien n'oblige les routes à passer par là.
|
||||
|
||||
Les schémas Marshmallow sont bien écrits (`StripMixin` avec `unknown = EXCLUDE` pour ignorer proprement le `csrf_token`, validateurs réutilisables, messages explicites). Le problème n'est pas leur qualité mais leur application partielle.
|
||||
|
||||
**Correction.** Choisir une approche unique et l'appliquer partout. Deux options défendables :
|
||||
|
||||
- **Marshmallow partout** (recommandé ici, car les schémas existent déjà et couvrent presque tous les formulaires) : compléter les schémas manquants — `TeamCreateSchema`, `MatchCreateSchema`, `EvaluationSchema`, `TryoutSchema` — et faire passer chaque `request.form` par un `load()`. Un décorateur factorise le traitement des erreurs :
|
||||
```python
|
||||
@validate_form(EditProfileSchema, on_error='pages/edit_profile.html')
|
||||
def edit_profile(validated): ...
|
||||
```
|
||||
- **Flask-WTF partout** : plus idiomatique pour une application rendue côté serveur, puisque les formulaires se rendent et se repeuplent seuls dans Jinja2 en cas d'erreur, et que la protection CSRF est intégrée par champ. Coût de conversion plus élevé, car les 40 templates écrivent leurs `<input>` à la main.
|
||||
|
||||
Dans les deux cas, retirer la bibliothèque non retenue des dépendances (MNT-05).
|
||||
|
||||
---
|
||||
|
||||
## STD-05 · 🟡
|
||||
|
||||
**Aucun objet de configuration ni séparation d'environnements**
|
||||
|
||||
`app/app.py:54-95` affecte 12 clés de configuration une par une, en mêlant constantes et lectures d'environnement :
|
||||
|
||||
```python
|
||||
app.config['SECRET_KEY'] = os.getenv('SECRET_KEY')
|
||||
app.config['SQLALCHEMY_DATABASE_URI'] = os.getenv('DATABASE_URL')
|
||||
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
|
||||
app.config['MAX_CONTENT_LENGTH'] = 16 * 1024 * 1024
|
||||
app.config['SESSION_COOKIE_SECURE'] = os.getenv('SESSION_COOKIE_SECURE', 'true').lower() == 'true'
|
||||
...
|
||||
```
|
||||
|
||||
D'autres réglages sont lus directement via `os.getenv()` au fil du code, hors de `app.config` : `FORCE_HTTPS` (`app.py:183`), `LOG_LEVEL` et `FLASK_DEBUG` (`logging_config.py:73,133`), `DISCORD_WEBHOOK_URL` (`users.py:636`, au moment de l'import), `DISCORD_BOT_TOKEN` (`discord_bot.py:24`). `load_dotenv()` est appelé dans deux modules distincts (`app.py:16`, `discord_bot.py:23`).
|
||||
|
||||
Il n'existe **aucune notion d'environnement** : les mêmes valeurs par défaut s'appliquent en développement, en CI et en production. C'est la cause directe de SEC-02 (seed en production) et de SEC-03 (CORS permissif par défaut).
|
||||
|
||||
**Impact.** Impossible de répondre par la lecture à des questions comme « quelle configuration s'applique en production ? » ou « qu'est-ce qui change entre local et prod ? » — il faut parcourir cinq fichiers et reconstituer les valeurs par défaut de chaque `os.getenv`. Les valeurs par défaut sont d'ailleurs incohérentes entre elles : `FLASK_DEBUG` vaut `'false'` dans `app.py:371` et `'true'` dans `run.py:20`.
|
||||
|
||||
**Correction.** Motif standard Flask — des classes de configuration, sélectionnées par une variable d'environnement :
|
||||
|
||||
```python
|
||||
# app/config.py
|
||||
class BaseConfig:
|
||||
SQLALCHEMY_TRACK_MODIFICATIONS = False
|
||||
MAX_CONTENT_LENGTH = 16 * 1024 * 1024
|
||||
SESSION_COOKIE_HTTPONLY = True
|
||||
SESSION_COOKIE_SAMESITE = 'Lax'
|
||||
PERMANENT_SESSION_LIFETIME = 3600
|
||||
WTF_CSRF_ENABLED = True
|
||||
|
||||
class ProductionConfig(BaseConfig):
|
||||
SESSION_COOKIE_SECURE = True
|
||||
FORCE_HTTPS = True
|
||||
SEED_DEMO_DATA = False
|
||||
|
||||
def __init__(self):
|
||||
self.SECRET_KEY = _require('SECRET_KEY')
|
||||
self.SQLALCHEMY_DATABASE_URI = _require('DATABASE_URL')
|
||||
self.CORS_ALLOWED_ORIGINS = _require('CORS_ALLOWED_ORIGINS')
|
||||
|
||||
class DevelopmentConfig(BaseConfig):
|
||||
SESSION_COOKIE_SECURE = False
|
||||
FORCE_HTTPS = False
|
||||
SEED_DEMO_DATA = True
|
||||
```
|
||||
|
||||
`_require()` lève au démarrage si la variable manque — l'application refuse de démarrer mal configurée, ce que `app.py:56-62` fait déjà pour deux clés et qu'il suffit de généraliser. C'est ce mécanisme qui aurait empêché SEC-03.
|
||||
|
||||
Un seul `load_dotenv()`, au point d'entrée. Toute lecture de configuration passe ensuite par `current_app.config`, ce qui la rend surchargeable en test.
|
||||
|
||||
---
|
||||
|
||||
## STD-06 · 🟡
|
||||
|
||||
**Ports et adresses d'écoute incohérents entre cinq fichiers**
|
||||
|
||||
| Fichier | Ligne | Écoute / cible |
|
||||
|---|---|---|
|
||||
| `run.py` | `:26` | `app.run(host='127.0.0.2', port=5000)` |
|
||||
| `app/app.py` | `:377` | `app.run(host='0.0.0.0', port=10000)` |
|
||||
| `wsgi.py` | `:24-26` | `PORT` défaut **10000**, `HOST` défaut `0.0.0.0` |
|
||||
| `app/.env.exemple` | `:25-26` | `HOST=127.0.0.1`, `PORT=5000` |
|
||||
| `app/nginx.conf` | `:122` | `proxy_pass http://0.0.0.0:5000` |
|
||||
|
||||
Quatre combinaisons différentes pour trois points d'entrée. Points notables :
|
||||
|
||||
- **`127.0.0.2` dans `run.py`** est presque certainement une faute de frappe pour `127.0.0.1`. L'adresse est techniquement valide (tout le bloc `127.0.0.0/8` est en boucle locale sous Linux), mais elle n'est pas joignable sous Windows sans configuration supplémentaire — or le projet cible Windows (Waitress, `nginx.conf` avec des chemins `C:/`).
|
||||
- **`proxy_pass http://0.0.0.0:5000`** : `0.0.0.0` désigne « toutes les interfaces » comme adresse d'écoute, mais n'a pas de sens comme adresse de destination. La cible correcte est `127.0.0.1:5000`.
|
||||
- **`wsgi.py` écoute sur 10000 par défaut, nginx envoie vers 5000.** Sans `PORT=5000` dans l'environnement, le proxy ne trouve pas l'application.
|
||||
- Le commentaire de `wsgi.py:26` dit *« Bind to localhost by default (Nginx reverse proxy) »* alors que le défaut est `0.0.0.0` — l'intention est documentée, l'implémentation fait l'inverse (cf. SEC-05, dont c'est un facteur aggravant).
|
||||
|
||||
**Impact.** La chaîne nginx → Waitress ne fonctionne pas avec les valeurs par défaut : elle exige des variables d'environnement non documentées dans le README. Un déploiement suivant la documentation aboutit à un 502.
|
||||
|
||||
**Correction.**
|
||||
1. Un seul point d'entrée de production : `wsgi.py`, avec `HOST` par défaut à `127.0.0.1` et `PORT` par défaut à `5000` pour s'aligner sur nginx et sur `.env.exemple`.
|
||||
2. Supprimer le bloc `if __name__ == '__main__'` de `app/app.py:368-377`, qui fait doublon avec `run.py` et diverge de lui.
|
||||
3. Corriger `run.py:26` en `127.0.0.1` et lui faire lire `HOST`/`PORT`.
|
||||
4. `nginx.conf:122` → `proxy_pass http://127.0.0.1:5000;`.
|
||||
5. Documenter le tableau des ports dans le README.
|
||||
|
||||
---
|
||||
|
||||
## STD-07 · 🟡
|
||||
|
||||
**Journalisation vers des fichiers locaux plutôt que stdout**
|
||||
|
||||
`app/logging_config.py:66-128` configure trois `RotatingFileHandler` (10 Mo, 5 à 10 archives) écrivant dans `os.getcwd()/logs/`. Le handler console n'est ajouté **que si `FLASK_DEBUG=true`** (`:133-138`) — en production, l'application n'écrit donc **rien sur stdout**.
|
||||
|
||||
**Impact.** Cela contredit le principe des *logs comme flux d'événements* (facteur XI des douze facteurs), qui veut que le processus écrive sans tampon sur stdout et laisse l'environnement d'exécution router, agréger et archiver.
|
||||
|
||||
Conséquences concrètes :
|
||||
- `docker logs`, `journalctl`, et les collecteurs des plateformes d'hébergement (Render, entre autres, ne capture que stdout/stderr) ne voient rien. Sur un hébergement conteneurisé, **les logs sont écrits dans un système de fichiers éphémère et perdus à chaque redéploiement**.
|
||||
- Le chemin dépend du répertoire de travail (`os.getcwd()`), donc de la façon dont le processus a été lancé — les logs peuvent atterrir à des endroits différents selon le mode de démarrage (cf. STD-02).
|
||||
- Le format est textuel (`logging_config.py:81-84`), non structuré : pas d'agrégation ni de requêtage par champ.
|
||||
|
||||
Le reste du module est de bonne facture : la séparation erreurs / auth / application est pertinente, et le `SensitiveDataFilter` (`:18-50`) est une précaution que l'on voit rarement.
|
||||
|
||||
**Correction.** Conserver la structure, changer la destination :
|
||||
|
||||
```python
|
||||
handler = logging.StreamHandler(sys.stdout)
|
||||
handler.setFormatter(formatter)
|
||||
handler.addFilter(sensitive_filter)
|
||||
app.logger.addHandler(handler)
|
||||
```
|
||||
|
||||
Garder les handlers fichier optionnels, activés par `LOG_TO_FILES=true` pour le déploiement Windows sur serveur dédié où ils gardent leur intérêt.
|
||||
|
||||
Passer au format JSON (`python-json-logger`) si une agrégation est envisagée ; les canaux distincts deviennent alors un champ `logger` plutôt que trois fichiers.
|
||||
|
||||
---
|
||||
|
||||
## STD-08 · 🔵
|
||||
|
||||
**Logique métier dans les vues, sans couche de service**
|
||||
|
||||
L'application suit le découpage blueprints / modèles / templates, mais **toute la logique métier vit dans les fonctions de vue**. Exemples :
|
||||
|
||||
- `matches.create_match` (`matches.py:215-337`, 122 lignes) : validation, création du match, création des participants selon trois types de match, puis envoi des notifications Discord — le tout dans une seule fonction, avec des `db.session.flush()` intercalés dans les boucles ;
|
||||
- `users.delete_user` (`users.py:142-185`) : orchestration de la suppression de 15 modèles liés ;
|
||||
- `main.dashboard` (`main.py:26-139`) : cinq variantes de calcul de statistiques selon le rôle, avec des requêtes agrégées.
|
||||
|
||||
Conséquence : **cette logique n'est atteignable que par une requête HTTP**. Tester la règle « supprimer un joueur d'un tryout le retire aussi de ses équipes et de ses matchs » impose de monter un client de test, une session authentifiée et un jeton CSRF — pour vérifier une règle qui n'a rien de HTTP.
|
||||
|
||||
Le couplage aux notifications est le plus visible : `matches.py:325-331` et `team_matches.py:185-192` appellent `send_schedule_notification` directement dans la vue, en boucle sur les participants. Créer un match par un autre chemin (import, script, tâche planifiée) n'enverrait aucune notification.
|
||||
|
||||
**Impact.** Faible aujourd'hui — le code reste lisible et l'application fonctionne. C'est un constat de trajectoire : c'est ce qui rend l'écriture des tests de MNT-03 plus coûteuse qu'elle ne devrait l'être, et ce qui a permis les duplications de MNT-12.
|
||||
|
||||
**Correction.** Extraire progressivement, en commençant par ce qui est dupliqué ou testable :
|
||||
|
||||
```python
|
||||
# app/services/matches.py
|
||||
def create_match(tryout, form_data, author) -> Match:
|
||||
"""Crée un match, ses participants et déclenche les notifications."""
|
||||
```
|
||||
|
||||
La vue se réduit alors à : valider l'entrée → appeler le service → rendre le résultat. Inutile de tout convertir d'un coup ; appliquer la règle aux nouvelles fonctionnalités et aux zones déjà remaniées suffit à inverser la tendance.
|
||||
|
||||
---
|
||||
|
||||
## STD-09 · 🔵
|
||||
|
||||
**Gestion d'erreurs API par préfixe d'URL codé en dur**
|
||||
|
||||
`app/app.py:219-343`. Les sept gestionnaires d'erreur décident du format de réponse en testant le chemin :
|
||||
|
||||
```python
|
||||
if request.path.startswith('/users/disponibilities') or \
|
||||
request.path.startswith('/users/coach-availability') or \
|
||||
request.path.startswith('/users/api/'):
|
||||
return jsonify({'error': 'Bad request', 'message': str(error)}), 400
|
||||
return render_template('errors/400.html', error=error), 400
|
||||
```
|
||||
|
||||
Le bloc est répété sept fois, avec des listes de préfixes **qui divergent entre les gestionnaires** : le 400 teste trois préfixes, les 401/403/404/429/500 n'en testent que deux (`coach-availability` a été oublié).
|
||||
|
||||
Ces préfixes ne couvrent d'ailleurs pas toutes les routes JSON de l'application. `matches.py` expose `/matches/api/events`, `/matches/api/manageable-tryouts`, `/matches/api/available_players/…` ; `team_matches.py` expose `/team-matches/api/manageable-teams` ; `teams.py:392` et `team_matches.py:288` renvoient du JSON sur `/teams/<id>/toggle_status/<id>` et `/team-matches/<id>/toggle-presence/<id>`. **Aucune de ces routes n'est reconnue** : une erreur 403 sur `/matches/api/events` renvoie une page HTML à un appel `fetch()`, qui échouera au `response.json()` avec un message incompréhensible côté client.
|
||||
|
||||
**Impact.** Comportement d'erreur incohérent selon la route, et couplage fort entre la structure d'URL et la gestion d'erreurs : renommer un blueprint casse silencieusement le format de réponse. Chaque nouvelle route JSON doit penser à s'ajouter à sept listes.
|
||||
|
||||
**Correction.** Utiliser la négociation de contenu, qui est le mécanisme prévu par HTTP :
|
||||
|
||||
```python
|
||||
def wants_json():
|
||||
return (request.accept_mimetypes.best_match(['application/json', 'text/html'])
|
||||
== 'application/json' or request.path.startswith('/api/'))
|
||||
```
|
||||
|
||||
Plus propre encore : regrouper les routes JSON sous un blueprint `api_bp` avec `url_prefix='/api'` et lui attacher ses propres gestionnaires via `@api_bp.errorhandler` — Flask applique alors automatiquement le bon format selon le blueprint qui a traité la requête, sans test de chemin.
|
||||
|
||||
Factoriser en tout état de cause les sept blocs identiques en une fonction unique.
|
||||
|
||||
---
|
||||
|
||||
## STD-10 · 🔵
|
||||
|
||||
**Pas de conteneurisation ni de procédure de déploiement reproductible**
|
||||
|
||||
Le dépôt ne contient ni `Dockerfile`, ni `docker-compose.yml`, ni `Procfile`, ni manifeste de plateforme (`render.yaml`, `fly.toml`…). Le déploiement repose sur :
|
||||
|
||||
- `app/nginx.conf`, avec des chemins Windows absolus codés en dur (`C:/nginx/certs/fullchain.pem`, `:86-87`) ;
|
||||
- `app/supporting_scrits/run_https.py`, qui génère des certificats auto-signés via `subprocess` ;
|
||||
- `docs/deployment.md`, une procédure manuelle ;
|
||||
- `backup.py`, dont le docstring évoque le Planificateur de tâches Windows — et qui ne fonctionne pas (MNT-07).
|
||||
|
||||
`.gitignore:23` ignorant `docs/`, la documentation de déploiement est dans une situation ambiguë : présente dans l'index, mais toute mise à jour ultérieure risque de passer inaperçue (MNT-01).
|
||||
|
||||
**Impact.** L'environnement de production n'est pas reproductible : il est le produit d'une suite d'actions manuelles sur une machine Windows particulière. Reconstruire l'installation après une panne matérielle demande de retrouver la bonne version de Python, d'installer nginx manuellement, de placer les certificats aux chemins attendus, de configurer les variables d'environnement et de créer les tâches planifiées — sans que rien ne vérifie que le résultat correspond à l'existant.
|
||||
|
||||
Pour une association étudiante, où les personnes qui déploient changent chaque année, c'est un risque de continuité réel : la connaissance du déploiement n'est pas dans le dépôt.
|
||||
|
||||
**Correction.** Un `Dockerfile` couvre l'essentiel du besoin et supprime la dépendance à Windows :
|
||||
|
||||
```dockerfile
|
||||
FROM python:3.12-slim
|
||||
WORKDIR /app
|
||||
RUN apt-get update && apt-get install -y --no-install-recommends libpq5 \
|
||||
&& rm -rf /var/lib/apt/lists/*
|
||||
COPY requirements.txt .
|
||||
RUN pip install --no-cache-dir -r requirements.txt
|
||||
COPY . .
|
||||
RUN useradd --create-home appuser && chown -R appuser /app
|
||||
USER appuser
|
||||
EXPOSE 5000
|
||||
CMD ["python", "wsgi.py"]
|
||||
```
|
||||
|
||||
avec un `docker-compose.yml` réunissant l'application, PostgreSQL et le bot Discord en service distinct (STD-03).
|
||||
|
||||
Prérequis : corriger l'encodage de `requirements.txt` (MNT-02), sans quoi l'étape `pip install` peut échouer selon l'image de base ; et remplacer `psycopg2` par `psycopg2-binary` (MNT-05) pour éviter d'embarquer une chaîne de compilation.
|
||||
|
||||
Si le déploiement doit rester sur Windows, alors documenter la procédure de façon exécutable — un script PowerShell d'installation versionné plutôt qu'une suite d'étapes en prose.
|
||||
@@ -0,0 +1,69 @@
|
||||
# Audit — Team Tryouts
|
||||
|
||||
Audit statique de l'application `team-tryouts` (Flask 3.1 / SQLAlchemy 2.0 / Jinja2 / PostgreSQL).
|
||||
|
||||
| | |
|
||||
|---|---|
|
||||
| **Branche** | `audit/securite-maintenabilite-standards` |
|
||||
| **Base** | `main` @ `08f02f7` |
|
||||
| **Date** | 2026-08-07 |
|
||||
| **Périmètre** | Sécurité · Maintenabilité · Respect des standards de la stack |
|
||||
| **Méthode** | Revue de code statique (100 % du code Python, config CI/nginx, `.gitignore`, dépendances). Aucun test dynamique, aucune exécution de l'app. |
|
||||
|
||||
## Documents
|
||||
|
||||
| Fichier | Contenu |
|
||||
|---|---|
|
||||
| [`01-securite.md`](01-securite.md) | 19 constats de sécurité, du critique au faible |
|
||||
| [`02-maintenabilite.md`](02-maintenabilite.md) | 15 constats de maintenabilité et d'outillage |
|
||||
| [`03-standards-stack.md`](03-standards-stack.md) | 10 écarts aux conventions Flask / SQLAlchemy / 12-factor |
|
||||
| [`plan-remediation.md`](plan-remediation.md) | Ordre de traitement proposé |
|
||||
|
||||
---
|
||||
|
||||
## Synthèse exécutive
|
||||
|
||||
L'application est fonctionnellement riche et le socle est sain sur plusieurs points : ORM utilisé partout (**aucune injection SQL**), autoescape Jinja2 actif et **aucun `|safe`** dans les 40 templates, CSRF activé globalement, hachage de mots de passe via Werkzeug, modèle d'autorisation polymorphe cohérent (`can_manage_this_tryout`, `can_manage_this_org_team`), en-têtes de sécurité complets et verrouillage de compte après 5 échecs.
|
||||
|
||||
Ces bonnes bases sont **annulées par une poignée de défauts de configuration et de déploiement**, pas par la logique métier. Les trois plus graves sont indépendants du code applicatif :
|
||||
|
||||
1. **Des secrets de production réels sont publiés dans le dépôt** (`app/.env.exemple`) : token du bot Discord, URL PostgreSQL Render complète avec mot de passe, `SECRET_KEY`. Ils sont dans l'historique git depuis le commit `2d3721b`.
|
||||
2. **Un déploiement neuf s'auto-amorce avec des comptes `admin` / `password`** — le seed de démo se déclenche automatiquement en production quand la table `users` est vide.
|
||||
3. **Le CORS par défaut accepte toutes les origines avec `supports_credentials=True`** dès que `CORS_ALLOWED_ORIGINS` n'est pas défini — ce qui est le cas dans le `.env.exemple` fourni.
|
||||
|
||||
Le README affirme « Authorization Checks: **Proper ownership validation on all sensitive operations** ». C'est vrai pour les tryouts, matchs et équipes, mais **faux pour la gestion des utilisateurs** : les trois routes `create_user`, `edit_user` et `edit_profile` importent des schémas de validation Marshmallow… et ne les appellent jamais. Aucune politique de mot de passe ne s'applique sur ces chemins.
|
||||
|
||||
Côté outillage, la CI est en trompe-l'œil : le job « Security Scan » pointe vers un fichier qui n'existe pas à cet emplacement, le job « Tests » est un `echo` en `continue-on-error`, et `requirements.txt` est encodé en **UTF-16**, ce qui casse `pip-audit`. Enfin, `.gitignore` contient `*.html` : **tout nouveau template créé est silencieusement ignoré par git**.
|
||||
|
||||
## Répartition des constats
|
||||
|
||||
| Sévérité | Sécurité | Maintenabilité | Standards | Total |
|
||||
|---|---|---|---|---|
|
||||
| 🔴 Critique | 4 | — | — | **4** |
|
||||
| 🟠 Élevé | 4 | 4 | 1 | **9** |
|
||||
| 🟡 Moyen | 7 | 7 | 6 | **20** |
|
||||
| 🔵 Faible | 4 | 4 | 3 | **11** |
|
||||
| **Total** | **19** | **15** | **10** | **44** |
|
||||
|
||||
## Les 5 actions à mener en premier
|
||||
|
||||
| # | Action | Détail |
|
||||
|---|---|---|
|
||||
| 1 | **Révoquer les 3 secrets exposés** puis purger l'historique git | [SEC-01](01-securite.md#sec-01--) |
|
||||
| 2 | Désactiver le seed automatique en production | [SEC-02](01-securite.md#sec-02--) |
|
||||
| 3 | Refuser le démarrage si `CORS_ALLOWED_ORIGINS` est vide | [SEC-03](01-securite.md#sec-03--) |
|
||||
| 4 | Supprimer le `print()` du token Discord | [SEC-04](01-securite.md#sec-04--) |
|
||||
| 5 | Brancher les schémas Marshmallow sur les routes utilisateurs | [SEC-06](01-securite.md#sec-06--) |
|
||||
|
||||
## Ce qui est déjà bien fait
|
||||
|
||||
Pour éviter de le casser lors des corrections :
|
||||
|
||||
- **Aucune injection SQL** — l'ORM est utilisé sans exception ; le seul `text()` est un `SELECT 1` de healthcheck.
|
||||
- **Aucun `|safe`, aucun `{% autoescape false %}`** dans les templates ; les `innerHTML` de `main.js` ne manipulent que des chaînes littérales.
|
||||
- **Autorisation par dispatch polymorphe** (`Admin`/`Manager`/`Coach`/`Player`/`Scout`) plutôt que par comparaison de chaînes de rôles — approche propre et testable.
|
||||
- **Protection contre la fixation de session** au login (`session.clear()` avec préservation du token CSRF, ligne `auth.py:133-138`).
|
||||
- **Validation anti-open-redirect** sur le paramètre `next` (`auth.py:23-36`).
|
||||
- **En-têtes de sécurité complets** et HSTS conditionné à HTTPS réel.
|
||||
- **Filtre de redaction des secrets dans les logs** (`logging_config.py:18-50`).
|
||||
- **Découpage des modèles** en fichiers par entité, avec ordre d'import documenté par couches.
|
||||
@@ -0,0 +1,122 @@
|
||||
# Plan de remédiation
|
||||
|
||||
Ordre proposé. Les lots sont séquentiels : chacun lève des blocages du suivant.
|
||||
|
||||
---
|
||||
|
||||
## Lot 0 — Aujourd'hui, avant tout commit
|
||||
|
||||
Ces actions ne sont pas du code. Elles sont urgentes parce que les secrets sont exposés dès maintenant.
|
||||
|
||||
| # | Action | Réf. |
|
||||
|---|---|---|
|
||||
| 0.1 | **Révoquer le token du bot Discord** (Developer Portal → Bot → Reset Token) | [SEC-01](01-securite.md#sec-01--) |
|
||||
| 0.2 | **Faire tourner le mot de passe PostgreSQL** sur Render | SEC-01 |
|
||||
| 0.3 | **Générer une nouvelle `SECRET_KEY`** — invalide toutes les sessions, c'est voulu | SEC-01 |
|
||||
| 0.4 | **Vérifier en production** si les comptes `admin`, `manager1/2`, `coach1/2/3`, `scout1` existent avec le mot de passe `password` ; les désactiver ou changer leur mot de passe | [SEC-02](01-securite.md#sec-02--) |
|
||||
| 0.5 | Consulter les logs Discord et PostgreSQL pour détecter un éventuel accès non autorisé | SEC-01 |
|
||||
|
||||
> 0.1 à 0.3 sont indépendantes du code et peuvent être faites immédiatement. La purge de l'historique git (0.6, ci-dessous) vient après, et ne remplace pas la révocation : les secrets ont pu être clonés.
|
||||
|
||||
---
|
||||
|
||||
## Lot 1 — Correctifs critiques
|
||||
|
||||
| # | Action | Réf. | Effort |
|
||||
|---|---|---|---|
|
||||
| 1.1 | Vider `app/.env.exemple` de ses valeurs réelles, le renommer `.env.example`, ajouter `.env*` au `.gitignore` | SEC-01 | 15 min |
|
||||
| 1.2 | Purger l'historique git (`git filter-repo`), prévenir l'équipe de recloner | SEC-01 | 1 h |
|
||||
| 1.3 | Conditionner le seed à `SEED_DEMO_DATA` ; générer les mots de passe de démo aléatoirement | SEC-02 | 30 min |
|
||||
| 1.4 | Refuser le démarrage si `CORS_ALLOWED_ORIGINS` est vide hors développement | [SEC-03](01-securite.md#sec-03--) | 20 min |
|
||||
| 1.5 | Supprimer `print(DISCORD_BOT_TOKEN)` — `discord_bot.py:25` | [SEC-04](01-securite.md#sec-04--) | 2 min |
|
||||
| 1.6 | Ajouter un scan de secrets (`gitleaks`) à la CI | SEC-01 | 30 min |
|
||||
|
||||
---
|
||||
|
||||
## Lot 2 — Débloquer l'outillage
|
||||
|
||||
À faire tôt : sans CI fiable et sans tests, les lots suivants ne peuvent pas être validés.
|
||||
|
||||
| # | Action | Réf. | Effort |
|
||||
|---|---|---|---|
|
||||
| 2.1 | Réencoder `requirements.txt` en UTF-8, ajouter `.gitattributes` | [MNT-02](02-maintenabilite.md#mnt-02--) | 15 min |
|
||||
| 2.2 | Corriger `.gitignore` (`*.html`, `docs/`), vérifier les fichiers déjà perdus | [MNT-01](02-maintenabilite.md#mnt-01--) | 30 min |
|
||||
| 2.3 | Corriger le chemin de `security_scan.py` dans la CI et lui fournir `DATABASE_URL` | [MNT-04](02-maintenabilite.md#mnt-04--) | 20 min |
|
||||
| 2.4 | Ajouter `pyproject.toml` avec la configuration Ruff, formater en un commit isolé | [MNT-06](02-maintenabilite.md#mnt-06--) | 2 h |
|
||||
| 2.5 | Retirer `dotenv`, `login`, `discord` ; `psycopg2` → `psycopg2-binary` | [MNT-05](02-maintenabilite.md#mnt-05--) | 30 min |
|
||||
| 2.6 | Créer `tests/`, avec les tests de la matrice de permissions ; activer réellement le job CI | [MNT-03](02-maintenabilite.md#mnt-03--) | 1 j |
|
||||
|
||||
---
|
||||
|
||||
## Lot 3 — Sécurité applicative
|
||||
|
||||
| # | Action | Réf. | Effort |
|
||||
|---|---|---|---|
|
||||
| 3.1 | Appliquer `ProxyFix`, corriger `HOST` (`wsgi.py`) et `proxy_pass` (`nginx.conf`) | [SEC-05](01-securite.md#sec-05--) · [STD-06](03-standards-stack.md#std-06--) | 1 h |
|
||||
| 3.2 | Brancher `CreateUserSchema`, `EditUserSchema`, `EditProfileSchema` sur leurs routes | [SEC-06](01-securite.md#sec-06--) | 3 h |
|
||||
| 3.3 | Adosser Flask-Limiter à Redis ou PostgreSQL ; réévaluer les seuils | [SEC-08](01-securite.md#sec-08--) | 2 h |
|
||||
| 3.4 | Restreindre `view_user` et réduire les données transmises au template | [SEC-15](01-securite.md#sec-15--) | 2 h |
|
||||
| 3.5 | Valider les téléversements de contrats signés | [SEC-11](01-securite.md#sec-11--) | 1 h |
|
||||
| 3.6 | Uniformiser les messages de login, égaliser les temps de réponse | [SEC-12](01-securite.md#sec-12--) | 1 h |
|
||||
| 3.7 | Corriger `nl2br` (échapper avant `Markup`) ou le supprimer | [SEC-09](01-securite.md#sec-09--) | 15 min |
|
||||
| 3.8 | Masquer l'erreur brute de `/health` | [SEC-14](01-securite.md#sec-14--) | 10 min |
|
||||
| 3.9 | Alimenter le logger `team_tryouts.auth` | [SEC-19](01-securite.md#sec-19--) | 2 h |
|
||||
| 3.10 | Contrôler la cohérence tryout/équipe dans `add_to_team` | [SEC-17](01-securite.md#sec-17--) | 30 min |
|
||||
| 3.11 | Remplacer les `int()` nus par `request.form.get(..., type=int)` | [SEC-16](01-securite.md#sec-16--) | 2 h |
|
||||
|
||||
---
|
||||
|
||||
## Lot 4 — Fondations structurelles
|
||||
|
||||
Ce lot débloque des correctifs de sécurité qui exigent des changements de schéma.
|
||||
|
||||
| # | Action | Réf. | Effort |
|
||||
|---|---|---|---|
|
||||
| 4.1 | Introduire Flask-Migrate, `flask db stamp head` sur la base existante | [STD-01](03-standards-stack.md#std-01--) | 3 h |
|
||||
| 4.2 | Objets de configuration par environnement ; un seul `load_dotenv()` | [STD-05](03-standards-stack.md#std-05--) | 4 h |
|
||||
| 4.3 | Sortir les effets de bord de `create_app()` (create_all, seed, bot) | [STD-02](03-standards-stack.md#std-02--) | 3 h |
|
||||
| 4.4 | Séparer le bot Discord en processus distinct | [STD-03](03-standards-stack.md#std-03--) | 1 j |
|
||||
| 4.5 | *(dépend de 4.1)* Unicité sur `discord_user_id` + vérification de possession | [SEC-07](01-securite.md#sec-07--) | 1 j |
|
||||
| 4.6 | *(dépend de 4.1)* Cascades FK sur `OrgTeam` ; corriger `delete_team` | [MNT-11](02-maintenabilite.md#mnt-11--) | 3 h |
|
||||
| 4.7 | *(dépend de 4.1)* Migrer vers `datetime.now(timezone.utc)` et `DateTime(timezone=True)` | [MNT-13](02-maintenabilite.md#mnt-13--) | 4 h |
|
||||
| 4.8 | Journaliser sur stdout ; handlers fichier optionnels | [STD-07](03-standards-stack.md#std-07--) | 1 h |
|
||||
|
||||
---
|
||||
|
||||
## Lot 5 — Dette de conception
|
||||
|
||||
Sans urgence. À traiter au fil des évolutions plutôt qu'en chantier dédié.
|
||||
|
||||
| # | Action | Réf. |
|
||||
|---|---|---|
|
||||
| 5.1 | Découper `users.py` en cinq blueprints | [MNT-08](02-maintenabilite.md#mnt-08--) |
|
||||
| 5.2 | Choisir une approche de validation unique ; retirer Flask-WTF ou WTForms | [STD-04](03-standards-stack.md#std-04--) |
|
||||
| 5.3 | Extraire les utilitaires date/heure | [MNT-12](02-maintenabilite.md#mnt-12--) |
|
||||
| 5.4 | Résorber les N+1 sur `view_tryout` et `api_events` | [MNT-10](02-maintenabilite.md#mnt-10--) |
|
||||
| 5.5 | Blueprint `/api` avec ses propres gestionnaires d'erreur | [STD-09](03-standards-stack.md#std-09--) |
|
||||
| 5.6 | Extraire une couche de services | [STD-08](03-standards-stack.md#std-08--) |
|
||||
| 5.7 | Durcir la CSP (nonces, scripts externalisés) | [SEC-10](01-securite.md#sec-10--) |
|
||||
| 5.8 | Réinitialisation de mot de passe par courriel ; MFA sur les rôles d'encadrement | [SEC-18](01-securite.md#sec-18--) |
|
||||
| 5.9 | Remplacer le CAPTCHA par une validation du courriel institutionnel | [SEC-13](01-securite.md#sec-13--) |
|
||||
| 5.10 | Réparer `backup.py` (`pg_dump`) ou le restreindre aux documents | [MNT-07](02-maintenabilite.md#mnt-07--) |
|
||||
| 5.11 | Pagination des listes | [MNT-14](02-maintenabilite.md#mnt-14--) |
|
||||
| 5.12 | `Dockerfile` + `docker-compose.yml` | [STD-10](03-standards-stack.md#std-10--) |
|
||||
| 5.13 | Réécrire le README ; renommer `supporting_scrits` → `supporting_scripts` | [MNT-15](02-maintenabilite.md#mnt-15--) |
|
||||
|
||||
---
|
||||
|
||||
## Dépendances entre lots
|
||||
|
||||
```
|
||||
Lot 0 (révocation)
|
||||
└─→ Lot 1 (correctifs critiques)
|
||||
└─→ Lot 2 (outillage : CI verte, premiers tests)
|
||||
├─→ Lot 3 (sécurité applicative)
|
||||
└─→ Lot 4 (migrations, configuration)
|
||||
├─→ 4.5 unicité discord_user_id (bloqué par 4.1)
|
||||
├─→ 4.6 cascades FK (bloqué par 4.1)
|
||||
└─→ 4.7 datetimes conscients (bloqué par 4.1)
|
||||
└─→ Lot 5 (dette de conception)
|
||||
```
|
||||
|
||||
Le Lot 2 est délibérément placé avant les lots 3 et 4 : sans tests sur la matrice de permissions, les modifications d'autorisation du Lot 3 ne peuvent pas être validées autrement qu'à la main.
|
||||
Reference in New Issue
Block a user