From fa0a37882747ea728effb62daea95b6fd7ac304e Mon Sep 17 00:00:00 2001 From: GGThed Date: Fri, 7 Aug 2026 12:56:13 -0400 Subject: [PATCH] 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 --- audit/01-securite.md | 672 ++++++++++++++++++++++++++++++++++++ audit/02-maintenabilite.md | 585 +++++++++++++++++++++++++++++++ audit/03-standards-stack.md | 393 +++++++++++++++++++++ audit/README.md | 69 ++++ audit/plan-remediation.md | 122 +++++++ 5 files changed, 1841 insertions(+) create mode 100644 audit/01-securite.md create mode 100644 audit/02-maintenabilite.md create mode 100644 audit/03-standards-stack.md create mode 100644 audit/README.md create mode 100644 audit/plan-remediation.md diff --git a/audit/01-securite.md b/audit/01-securite.md new file mode 100644 index 0000000..4750f60 --- /dev/null +++ b/audit/01-securite.md @@ -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=`, `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//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('
'.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('
').join(escape(str(value)).splitlines()) +``` + +`escape()` neutralise le HTML utilisateur ; seuls les `
` 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 `