chore(gitignore): ignorer les artefacts d assistants IA et corriger la regle *.html
- Ajout des dossiers/fichiers des assistants IA (Claude, ChatGPT/OpenAI, Cursor, Copilot, Aider, Windsurf, Gemini, Continue, Cline). - Ajout des notes de travail et de suivi generees par assistant (.ai/, audit/), retirees du suivi git via `git rm --cached` : les fichiers restent sur disque mais ne sont plus versionnes. - Retrait des regles `docs/` et `*.html`, qui ignoraient TOUT fichier .html du depot, y compris les templates Jinja2. Un nouveau template etait invisible pour git : fonctionnel en local, TemplateNotFound en production, sans signal dans `git status`. Les rapports de couverture sont desormais couverts par htmlcov/ et coverage_html_report/. - `.env` -> `.env*` avec exception `!.env.example`, pour couvrir .env.local, .env.production et les copies de sauvegarde. - Ajout des cles et certificats, des sauvegardes (backups/), des journaux (logs/), des environnements virtuels, des caches d outils et des fichiers d editeur/OS. - Deduplication des regles existantes (*.db et instance/ etaient en double, .instance/ n existait pas). Aucune modification du code applicatif. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
GGThed
+131
-17
@@ -1,27 +1,141 @@
|
|||||||
.env
|
# =============================================================================
|
||||||
|
# Secrets et configuration locale
|
||||||
|
# =============================================================================
|
||||||
|
# `.env*` couvre .env, .env.local, .env.production, .env.bak…
|
||||||
|
# L'exception laisse passer le seul fichier modèle, qui ne doit contenir
|
||||||
|
# que des valeurs factices.
|
||||||
|
.env*
|
||||||
|
!.env.example
|
||||||
|
|
||||||
.instance/
|
*.key
|
||||||
|
*.pem
|
||||||
|
*.pfx
|
||||||
|
*.p12
|
||||||
|
.certs/
|
||||||
|
id_rsa
|
||||||
|
id_ed25519
|
||||||
|
known_hosts
|
||||||
|
credentials.json
|
||||||
|
service-account*.json
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# Assistants IA — Claude
|
||||||
|
# =============================================================================
|
||||||
|
.claude/
|
||||||
|
.claude.json
|
||||||
|
.claude/settings.local.json
|
||||||
|
CLAUDE.md
|
||||||
|
CLAUDE.local.md
|
||||||
|
.anthropic/
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# Assistants IA — ChatGPT / OpenAI
|
||||||
|
# =============================================================================
|
||||||
|
.chatgpt/
|
||||||
|
.openai/
|
||||||
|
.codex/
|
||||||
|
AGENTS.md
|
||||||
|
chatgpt-*.md
|
||||||
|
openai-*.md
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# Assistants IA — autres outils
|
||||||
|
# =============================================================================
|
||||||
|
.cursor/
|
||||||
|
.cursorrules
|
||||||
|
.cursorignore
|
||||||
|
.windsurf/
|
||||||
|
.windsurfrules
|
||||||
|
.aider*
|
||||||
|
.continue/
|
||||||
|
.clinerules
|
||||||
|
.roo/
|
||||||
|
.gemini/
|
||||||
|
GEMINI.md
|
||||||
|
.github/copilot-instructions.md
|
||||||
|
.copilot/
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# Notes de travail et suivi générés par assistant IA
|
||||||
|
# =============================================================================
|
||||||
|
# Rapports d'audit, plans, brouillons et notes de session : documents de
|
||||||
|
# travail, non destinés à être versionnés dans le dépôt applicatif.
|
||||||
|
.ai/
|
||||||
|
audit/
|
||||||
|
*.audit.md
|
||||||
|
NOTES-IA.md
|
||||||
|
TODO-IA.md
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# Python
|
||||||
|
# =============================================================================
|
||||||
|
__pycache__/
|
||||||
|
*.py[cod]
|
||||||
|
*.so
|
||||||
|
*.egg-info/
|
||||||
|
.eggs/
|
||||||
|
build/
|
||||||
|
dist/
|
||||||
|
|
||||||
|
# Environnements virtuels
|
||||||
|
venv/
|
||||||
|
.venv/
|
||||||
|
env/
|
||||||
|
ENV/
|
||||||
|
|
||||||
|
# Outils
|
||||||
|
.pytest_cache/
|
||||||
|
.ruff_cache/
|
||||||
|
.mypy_cache/
|
||||||
|
.coverage
|
||||||
|
.coverage.*
|
||||||
|
htmlcov/
|
||||||
|
coverage_html_report/
|
||||||
|
coverage.xml
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# Données applicatives — ne jamais versionner
|
||||||
|
# =============================================================================
|
||||||
|
# Base SQLite locale (une base de démo a déjà été committée par le passé,
|
||||||
|
# voir l'historique du commit `4fd27ba`).
|
||||||
|
instance/
|
||||||
*.db
|
*.db
|
||||||
|
*.sqlite
|
||||||
|
*.sqlite3
|
||||||
|
|
||||||
|
# Contrats téléversés (données personnelles)
|
||||||
documents/
|
documents/
|
||||||
|
|
||||||
__pycache__/
|
# Sauvegardes produites par app/supporting_scripts/backup.py
|
||||||
*.cpython-313.pyc
|
backups/
|
||||||
*.cpython-312.pyc
|
|
||||||
*.pyc
|
|
||||||
|
|
||||||
.pytest_cache/
|
# Journaux applicatifs
|
||||||
.coverage
|
logs/
|
||||||
|
|
||||||
htmlcov/
|
|
||||||
.DS_Store
|
|
||||||
*.log
|
*.log
|
||||||
|
|
||||||
.certs/
|
# État d'exécution du bot Discord
|
||||||
*.pem
|
discord_pending.json
|
||||||
|
|
||||||
docs/
|
# =============================================================================
|
||||||
*.html
|
# Éditeurs et systèmes d'exploitation
|
||||||
|
# =============================================================================
|
||||||
|
.idea/
|
||||||
|
.vscode/
|
||||||
|
*.swp
|
||||||
|
*.swo
|
||||||
|
.DS_Store
|
||||||
|
Thumbs.db
|
||||||
|
desktop.ini
|
||||||
|
|
||||||
instance/
|
# =============================================================================
|
||||||
*.db
|
# NOTE — règles retirées volontairement
|
||||||
|
# =============================================================================
|
||||||
|
# `docs/` et `*.html` figuraient ici et ignoraient TOUT fichier .html du
|
||||||
|
# dépôt, y compris les templates Jinja2 de app/templates/. Un nouveau
|
||||||
|
# template était donc invisible pour git : l'application fonctionnait en
|
||||||
|
# local et cassait en production avec une TemplateNotFound, sans que
|
||||||
|
# `git status` ne signale quoi que ce soit.
|
||||||
|
#
|
||||||
|
# Les rapports de couverture HTML, qui étaient vraisemblablement la cible
|
||||||
|
# de ces règles, sont couverts ci-dessus par `htmlcov/` et
|
||||||
|
# `coverage_html_report/`.
|
||||||
|
|||||||
@@ -1,149 +0,0 @@
|
|||||||
# 0 — Provenance et écart entre les dépôts
|
|
||||||
|
|
||||||
## Deux dépôts, un seul fait autorité
|
|
||||||
|
|
||||||
| Dépôt | Rôle | HEAD `main` |
|
|
||||||
|---|---|---|
|
|
||||||
| `git.immortal.host/clubesportsudes/team-tryouts` | **Référence** — c'est celui-ci qui est audité | `bb0bc1c` |
|
|
||||||
| `github.com/cedrick2711/team-tryouts` | Miroir, **en retard de 16 commits** | `08f02f7` |
|
|
||||||
|
|
||||||
Les deux partagent un ancêtre commun (`d6fe505`). Le miroir GitHub porte en outre 2 commits qui ne sont pas sur le dépôt de référence (`0d5a408 fix registration` et son merge `08f02f7`) ; le correctif correspondant existe sur `immortal` sous la forme du commit `7d30aff Corriger erreur d'enregistrement`.
|
|
||||||
|
|
||||||
**La première passe de cet audit a été menée sur le miroir GitHub**, avant que la bonne source ne soit connue. L'ensemble des constats a été revérifié contre `immortal/main`. Le présent document liste ce qui a changé.
|
|
||||||
|
|
||||||
### Les 16 commits d'écart
|
|
||||||
|
|
||||||
```
|
|
||||||
bb0bc1c ajout de plateforme de base pour les url TRN
|
|
||||||
f89e4de régler problème avec mise a jour des status de message du discord bot
|
|
||||||
fcf10bc bug fix: Manager ne pouvait pas voir les tryouts. probleme avec discord bot
|
|
||||||
68e3da6 fix probleme avec dispos
|
|
||||||
d25b35c régler erreur 500 sur changement de role par admin
|
|
||||||
aeba4d7 régler problème de changement de rôle
|
|
||||||
7bb8022 added a clear_db to start fresh with only an admin
|
|
||||||
0dd4ecd Erreur de frappe dans un des dossiers
|
|
||||||
0f7788e Changement du layout de la page d'enregistrement
|
|
||||||
47d5ec4 added discord oauth2 to get basic user info to complete profile when registering
|
|
||||||
10af8c0 small change for prod
|
|
||||||
7d30aff Corriger erreur d'enregistrement
|
|
||||||
4097416 Update wsgi.py
|
|
||||||
056cea0 Update .gitea/workflows/git-to-ptero.yaml
|
|
||||||
d3b5700 Add .gitea/workflows/git-to-ptero.yaml
|
|
||||||
fd258de Update app/.env.exemple
|
|
||||||
```
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Constats de la première passe devenus caducs
|
|
||||||
|
|
||||||
Quatre constats ont été résolus par l'équipe entre les deux points de l'historique. Ils sont conservés ici pour mémoire, et retirés du décompte des documents 01 à 03.
|
|
||||||
|
|
||||||
### ✅ Seed automatique en production — résolu
|
|
||||||
|
|
||||||
Le bloc de `app/app.py` qui déclenchait le seed quand la table `users` était vide a été supprimé (commit `7bb8022`), et `app/supporting_scrits/seed.py` (460 lignes, comptes `manager1`, `coach1`, `scout1`… tous avec le mot de passe `password`) a été supprimé.
|
|
||||||
|
|
||||||
```diff
|
|
||||||
with app.app_context():
|
|
||||||
import app.models as models
|
|
||||||
- from app.models import User
|
|
||||||
db.create_all()
|
|
||||||
-
|
|
||||||
- # Seed database if empty
|
|
||||||
- if User.query.count() == 0:
|
|
||||||
- from app.supporting_scrits.seed import seed_database
|
|
||||||
- seed_database()
|
|
||||||
```
|
|
||||||
|
|
||||||
Un déploiement neuf ne crée donc plus de comptes par défaut.
|
|
||||||
|
|
||||||
> **Reste à traiter.** Le script de remplacement `clear_db.py` code toujours en dur `admin` / `password`, et il est bien plus dangereux que l'ancien seed sur un autre plan. Voir [SEC-22](01-securite.md#sec-22--).
|
|
||||||
>
|
|
||||||
> **À vérifier en production**, indépendamment du code : les comptes créés par l'ancien seed (`admin`, `manager1`, `manager2`, `coach1`, `coach2`, `coach3`, `scout1`) peuvent toujours exister en base avec le mot de passe `password`. La suppression du script ne supprime pas les comptes qu'il a créés.
|
|
||||||
|
|
||||||
### ✅ Token Discord imprimé sur stdout — résolu
|
|
||||||
|
|
||||||
La ligne `print(DISCORD_BOT_TOKEN or 'FAILED TO PRINT BOT TOKEN')` de `app/discord_bot.py:25` a été supprimée. Le module ne comporte plus aucun `print()`.
|
|
||||||
|
|
||||||
### ✅ `proxy_pass` vers `0.0.0.0` — résolu
|
|
||||||
|
|
||||||
`app/nginx.conf:122` : `proxy_pass http://0.0.0.0:5000;` → `proxy_pass http://127.0.0.1:5000;`
|
|
||||||
|
|
||||||
> **Reste à traiter.** L'incohérence de ports subsiste : `wsgi.py:24` utilise toujours `PORT` par défaut à **10000** alors que nginx envoie vers **5000** et que le docstring du même fichier annonce 5000. Voir [STD-06](03-standards-stack.md#std-06--).
|
|
||||||
|
|
||||||
### ✅ Faute de frappe `supporting_scrits` — résolu
|
|
||||||
|
|
||||||
Le dossier a été renommé `app/supporting_scripts/` (commit `0dd4ecd`).
|
|
||||||
|
|
||||||
> **Effet de bord non traité.** Le job CI « Security Scan » invoquait déjà un chemin erroné (`python security_scan.py` à la racine) ; le renommage l'éloigne encore. Voir [MNT-04](02-maintenabilite.md#mnt-04--).
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Constats aggravés ou requalifiés
|
|
||||||
|
|
||||||
### ⚠️ Secrets exposés — le nettoyage n'a pas révoqué
|
|
||||||
|
|
||||||
`app/.env.exemple` a été nettoyé (commit `fd258de`) : les valeurs réelles ont été remplacées par des marqueurs (`flask_app_secret_key`, `my_discord_bot_token`, `URI_vers_db_posgres`).
|
|
||||||
|
|
||||||
**Mais les secrets restent intégralement récupérables.** Vérification par recherche dans l'historique complet des deux dépôts :
|
|
||||||
|
|
||||||
```
|
|
||||||
$ git log --all --oneline -S "MTUyNzY3ODU3NjUyNTA1NDEyNQ"
|
|
||||||
fd258de Update app/.env.exemple ← retrait
|
|
||||||
2d3721b Ajout d'un .env.exemple pour simplifier la collaboration ← introduction
|
|
||||||
```
|
|
||||||
|
|
||||||
Le blob contenant le token Discord, le mot de passe PostgreSQL et la `SECRET_KEY` est toujours atteignable par `git show 2d3721b:app/.env.exemple` sur **les deux dépôts**. Il est de surcroît **toujours dans le HEAD du miroir GitHub**, donc visible par simple navigation dans l'interface web.
|
|
||||||
|
|
||||||
Le constat reste donc critique et sa correction inchangée : **révoquer, puis purger**. Voir [SEC-01](01-securite.md#sec-01--).
|
|
||||||
|
|
||||||
### ⚠️ En-têtes de proxy — le correctif a inversé le risque
|
|
||||||
|
|
||||||
`wsgi.py` a reçu un bloc de configuration proxy (commit `4097416`) :
|
|
||||||
|
|
||||||
```python
|
|
||||||
trusted_proxy='*',
|
|
||||||
trusted_proxy_count=1,
|
|
||||||
trusted_proxy_headers={'x-forwarded-for', 'x-forwarded-proto'},
|
|
||||||
clear_untrusted_proxy_headers=True
|
|
||||||
```
|
|
||||||
|
|
||||||
L'intention est bonne, et `clear_untrusted_proxy_headers=True` est le bon réflexe. Mais `trusted_proxy='*'` signifie « faire confiance aux en-têtes de proxy quelle qu'en soit la provenance », et `HOST` vaut toujours `0.0.0.0` par défaut (`wsgi.py:26`).
|
|
||||||
|
|
||||||
Avant ce changement, `request.remote_addr` valait toujours l'IP de nginx : le rate limiting était appliqué à un seau global — gênant, mais fermé. Désormais, un attaquant qui atteint directement le port applicatif contrôle `X-Forwarded-For` et peut donc **présenter une IP différente à chaque requête**, ce qui neutralise le rate limiting et le suivi de tentatives par IP.
|
|
||||||
|
|
||||||
Le constat change de nature et reste élevé. Voir [SEC-05](01-securite.md#sec-05--).
|
|
||||||
|
|
||||||
### ⚠️ Identifiant Discord — la vérification ajoutée n'en est pas une
|
|
||||||
|
|
||||||
Un flux OAuth2 Discord complet a été ajouté à l'inscription (commit `47d5ec4`, `auth.py:326-456`). Il obtient de Discord un identifiant authentifié et le place en session.
|
|
||||||
|
|
||||||
Mais cet identifiant est ensuite réinjecté dans le formulaire d'inscription **par un champ caché** :
|
|
||||||
|
|
||||||
```html
|
|
||||||
<!-- register.html:61 -->
|
|
||||||
<input type="hidden" name="discord_user_id" value="{{ discord_data.id }}">
|
|
||||||
```
|
|
||||||
|
|
||||||
et c'est cette valeur — passée par le client — qui est enregistrée (`auth.py:253`, `:290`). Un utilisateur peut la modifier avant envoi, ou poster directement le formulaire sans jamais passer par Discord.
|
|
||||||
|
|
||||||
`RegisterSchema` valide désormais le format de `discord_user_id` (17-20 chiffres, `validators.py:211-215`), ce qui est un progrès, mais ne prouve rien sur la propriété du compte.
|
|
||||||
|
|
||||||
La conséquence pratique est inchangée par rapport à la première passe — et le risque de fausse assurance est nouveau, puisque le flux *paraît* vérifié. Voir [SEC-07](01-securite.md#sec-07--).
|
|
||||||
|
|
||||||
### ⚠️ Limite d'inscription desserrée
|
|
||||||
|
|
||||||
`auth.py:190` : `@limiter.limit("3 per hour")` → `@limiter.limit("20 per hour")`.
|
|
||||||
|
|
||||||
Le CAPTCHA arithmétique étant trivial ([SEC-13](01-securite.md#sec-13--)), le rate limiting était la protection réellement efficace contre la création automatisée de comptes. Passer à 20/heure la divise par un facteur ~7, et [SEC-05](01-securite.md#sec-05--) la rend contournable.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Éléments nouveaux ajoutés à l'audit
|
|
||||||
|
|
||||||
| Élément | Constat |
|
|
||||||
|---|---|
|
|
||||||
| `auth.py:326-456` — flux OAuth2 Discord | [SEC-20](01-securite.md#sec-20--) absence de paramètre `state` · [SEC-07](01-securite.md#sec-07--) identité non contraignante |
|
|
||||||
| `.gitea/workflows/git-to-ptero.yaml` | [SEC-21](01-securite.md#sec-21--) déploiement de `.git/` par SFTP |
|
|
||||||
| `clear_db.py` | [SEC-22](01-securite.md#sec-22--) script destructif sans garde-fou |
|
|
||||||
| `migrations/add_tryout_coaches.py` | [STD-01](03-standards-stack.md#std-01--) migration manuelle hors outillage |
|
|
||||||
| `discord_pending.json` | [MNT-16](02-maintenabilite.md#mnt-16--) état d'exécution versionné |
|
|
||||||
@@ -1,809 +0,0 @@
|
|||||||
# 1 — Sécurité
|
|
||||||
|
|
||||||
22 constats. Références de lignes sur `immortal/main` @ `bb0bc1c`.
|
|
||||||
|
|
||||||
Les constats résolus par l'équipe (ancien SEC-02 seed automatique, ancien SEC-04 token imprimé) sont archivés dans [`00-provenance.md`](00-provenance.md). Les identifiants des constats subsistants n'ont pas été renumérotés, pour préserver la traçabilité.
|
|
||||||
|
|
||||||
| ID | Constat | Sévérité |
|
|
||||||
|---|---|---|
|
|
||||||
| [SEC-01](#sec-01--) | Secrets de production toujours récupérables dans l'historique | 🔴 Critique |
|
|
||||||
| [SEC-03](#sec-03--) | CORS ouvert à toutes les origines avec credentials par défaut | 🔴 Critique |
|
|
||||||
| [SEC-05](#sec-05--) | `trusted_proxy='*'` sur interface publique → en-têtes de proxy usurpables | 🟠 Élevé |
|
|
||||||
| [SEC-06](#sec-06--) | Schémas de validation importés mais jamais appliqués sur les routes utilisateurs | 🟠 Élevé |
|
|
||||||
| [SEC-07](#sec-07--) | Identifiant Discord modifiable malgré l'OAuth2 → détournement des notifications | 🟠 Élevé |
|
|
||||||
| [SEC-08](#sec-08--) | Rate limiting en mémoire, non partagé et réinitialisé à chaque redémarrage | 🟠 Élevé |
|
|
||||||
| [SEC-20](#sec-20--) | Flux OAuth2 Discord sans paramètre `state` | 🟠 Élevé |
|
|
||||||
| [SEC-21](#sec-21--) | Le déploiement SFTP pousse le répertoire `.git/` sur le serveur | 🟠 É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 trivial, et limite d'inscription desserrée à 20/heure | 🟡 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-22](#sec-22--) | `clear_db.py` : destruction totale sans garde-fou, identifiants en dur | 🟡 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 toujours récupérables dans l'historique**
|
|
||||||
|
|
||||||
Le fichier `app/.env.exemple` a été nettoyé au commit `fd258de` : les valeurs réelles y sont désormais remplacées par des marqueurs. **Cela ne change rien au fait que les secrets sont compromis.**
|
|
||||||
|
|
||||||
Recherche sur l'ensemble des références des deux dépôts :
|
|
||||||
|
|
||||||
```
|
|
||||||
$ git log --all --oneline -S "MTUyNzY3ODU3NjUyNTA1NDEyNQ"
|
|
||||||
fd258de Update app/.env.exemple
|
|
||||||
2d3721b Ajout d'un .env.exemple pour simplifier la collaboration
|
|
||||||
|
|
||||||
$ git log --all --oneline -S "0YO038Od2QcQCsCTlNDAMOAOlcPGrIND"
|
|
||||||
fd258de Update app/.env.exemple
|
|
||||||
2d3721b Ajout d'un .env.exemple pour simplifier la collaboration
|
|
||||||
```
|
|
||||||
|
|
||||||
Le blob introduit en `2d3721b` reste atteignable par `git show 2d3721b:app/.env.exemple` sur **`immortal` comme sur GitHub**. Il contient :
|
|
||||||
|
|
||||||
| Secret | Portée |
|
|
||||||
|---|---|
|
|
||||||
| `DISCORD_BOT_TOKEN` | Token du bot UdeS Esports — le commentaire du fichier le confirmait explicitement |
|
|
||||||
| `DATABASE_URL` | PostgreSQL Render, hôte public, **avec mot de passe** |
|
|
||||||
| `SECRET_KEY` | Clé de signature des sessions Flask |
|
|
||||||
|
|
||||||
Aggravant : **le miroir GitHub porte encore ces valeurs dans son HEAD**. Elles sont visibles par simple navigation dans l'interface web, sans même cloner.
|
|
||||||
|
|
||||||
**Impact.** Inchangé depuis la première passe :
|
|
||||||
- accès lecture/écriture complet à la base de production — identités, courriels, téléphones, notes personnelles, contrats ;
|
|
||||||
- contrôle du bot Discord, donc envoi de messages privés en usurpant l'identité de l'organisation ;
|
|
||||||
- avec la `SECRET_KEY`, **forge de cookies de session Flask valides** — authentification en tant que n'importe quel utilisateur, y compris `admin`, sans mot de passe.
|
|
||||||
|
|
||||||
**Correction.** Dans cet ordre :
|
|
||||||
|
|
||||||
1. **Révoquer**, avant toute manipulation de git — c'est la seule action qui rend les valeurs divulguées inoffensives :
|
|
||||||
- régénérer le token du bot (Discord Developer Portal → Bot → Reset Token) ;
|
|
||||||
- faire tourner le mot de passe PostgreSQL sur Render ;
|
|
||||||
- régénérer `SECRET_KEY` (`python -c "import secrets; print(secrets.token_hex(32))"`) — cela invalidera toutes les sessions en cours, ce qui est le comportement souhaité.
|
|
||||||
2. **Purger l'historique des deux dépôts** :
|
|
||||||
```bash
|
|
||||||
git filter-repo --path app/.env.exemple --invert-paths
|
|
||||||
```
|
|
||||||
puis forcer la réécriture sur toutes les branches, et prévenir les collaborateurs qu'ils doivent recloner.
|
|
||||||
3. **Traiter le miroir GitHub** : soit le synchroniser après purge, soit le supprimer. Un miroir en retard de 16 commits qui expose des secrets dans son HEAD n'apporte rien et coûte beaucoup.
|
|
||||||
4. Ajouter `.env*` au `.gitignore` avec une exception explicite : `!.env.example`.
|
|
||||||
5. Ajouter un scan de secrets à la CI (`gitleaks`, ou `detect-secrets` en pre-commit).
|
|
||||||
|
|
||||||
> Renommer aussi le fichier en `.env.example` — `exemple` est un francisme qui échappe à la détection de la plupart des outils.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-03 · 🔴
|
|
||||||
|
|
||||||
**CORS ouvert à toutes les origines avec credentials par défaut**
|
|
||||||
|
|
||||||
`app/app.py:76-95` — inchangé depuis la première passe.
|
|
||||||
|
|
||||||
```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 non 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` — y compris dans sa version nettoyée — **ne définit toujours 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` :
|
|
||||||
- `/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 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 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-05 · 🟠
|
|
||||||
|
|
||||||
**`trusted_proxy='*'` sur interface publique → en-têtes de proxy usurpables**
|
|
||||||
|
|
||||||
`wsgi.py:29-42`
|
|
||||||
|
|
||||||
```python
|
|
||||||
host = os.getenv('HOST', '0.0.0.0') # Bind to localhost by default (Nginx reverse proxy)
|
|
||||||
...
|
|
||||||
serve(
|
|
||||||
app, host=host, port=port, threads=threads,
|
|
||||||
channel_timeout=30, cleanup_interval=30,
|
|
||||||
# Waitress Proxy Settings
|
|
||||||
trusted_proxy='*',
|
|
||||||
trusted_proxy_count=1,
|
|
||||||
trusted_proxy_headers={'x-forwarded-for', 'x-forwarded-proto'},
|
|
||||||
clear_untrusted_proxy_headers=True
|
|
||||||
)
|
|
||||||
```
|
|
||||||
|
|
||||||
L'intention est correcte et `clear_untrusted_proxy_headers=True` est le bon réflexe. Deux réglages annulent le bénéfice :
|
|
||||||
|
|
||||||
- **`trusted_proxy='*'`** demande à Waitress d'accepter `X-Forwarded-For` et `X-Forwarded-Proto` **de n'importe quel pair**, pas seulement de nginx ;
|
|
||||||
- **`HOST` vaut `0.0.0.0`** — le commentaire de la ligne 26 annonce l'inverse de ce que fait le code. Le port applicatif est donc joignable directement, en contournant nginx.
|
|
||||||
|
|
||||||
**Impact.** La combinaison des deux rend les en-têtes de proxy entièrement contrôlables par un attaquant qui atteint le port applicatif :
|
|
||||||
|
|
||||||
1. **Contournement du rate limiting et du suivi de tentatives.** `app/extensions.py:16-19` utilise `key_func=get_remote_address`, qui lit `request.remote_addr` — valeur que Waitress renseigne désormais depuis `X-Forwarded-For`. Un attaquant qui change cet en-tête à chaque requête obtient **un seau de rate limiting neuf à chaque fois** : la limite de `10 per minute` sur `/auth/login` (`auth.py:96`) ne s'applique plus, ni les limites par défaut `200/jour, 50/heure`.
|
|
||||||
|
|
||||||
Il faut noter que ce changement **a inversé la nature du risque**. Avant, `remote_addr` valait toujours l'IP de nginx : la protection était mal calibrée (un seau global partagé par tous) mais fermée. Elle est maintenant ouverte.
|
|
||||||
|
|
||||||
Le verrouillage de compte après 5 échecs (`auth.py:167-181`) reste opérant, puisqu'il est indexé sur l'utilisateur et non sur l'IP — c'est aujourd'hui la seule barrière anti-bruteforce réellement en place.
|
|
||||||
|
|
||||||
2. **Contournement de la redirection HTTPS.** `app/app.py:182` teste `X-Forwarded-Proto`. En l'envoyant à `https` sur une connexion en clair, on désactive `force_https()` et l'application se comporte comme si la connexion était sécurisée. Même effet sur l'émission de HSTS (`app.py:163`).
|
|
||||||
|
|
||||||
3. **Journalisation empoisonnée.** Toute IP écrite dans les logs devient une valeur fournie par le client — ce qui rend l'analyse post-incident peu fiable (et affaiblit d'avance la correction proposée en [SEC-19](#sec-19--)).
|
|
||||||
|
|
||||||
**Correction.**
|
|
||||||
|
|
||||||
```python
|
|
||||||
host = os.getenv('HOST', '127.0.0.1')
|
|
||||||
...
|
|
||||||
trusted_proxy=os.getenv('TRUSTED_PROXY', '127.0.0.1'),
|
|
||||||
trusted_proxy_count=1,
|
|
||||||
trusted_proxy_headers={'x-forwarded-for', 'x-forwarded-proto'},
|
|
||||||
clear_untrusted_proxy_headers=True,
|
|
||||||
```
|
|
||||||
|
|
||||||
Les deux corrections sont nécessaires : `trusted_proxy` restreint *qui* peut envoyer ces en-têtes, `HOST` restreint *qui peut se connecter*. Si l'hébergement Pterodactyl impose une écoute sur `0.0.0.0`, renseigner alors l'adresse réelle du proxy dans `TRUSTED_PROXY` et filtrer le port au pare-feu.
|
|
||||||
|
|
||||||
Vérifier ensuite que `request.remote_addr` renvoie bien l'IP cliente, et non celle du proxy, avant de considérer le rate limiting comme fonctionnel.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 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`. 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 du mot de passe |
|
|
||||||
|---|---|---|
|
|
||||||
| `edit_user` | `:88-152` | `request.form.get('password')` → `hash_password()` si non vide |
|
|
||||||
| `create_user` | `:201-241` | `request.form.get('password')` → `hash_password()` sans contrôle |
|
|
||||||
| `edit_profile` | `:274-320` | idem, sur son propre compte |
|
|
||||||
|
|
||||||
Comparaison avec `auth.py:228-230`, où `RegisterSchema` **est** correctement chargé — l'inscription publique est validée, mais pas les trois 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` appelle `hash_password(password)` sans vérifier que `password` est non vide : le hachage d'une chaîne vide est stocké, et le compte devient accessible avec un mot de passe vide.
|
|
||||||
- Dans `edit_user`, `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. Retirer le champ du payload quand il est vide avant le `load()`.
|
|
||||||
|
|
||||||
Point d'attention pour `edit_user` : le bloc de changement de rôle par SQL brut (`:114-126`) exécute `db.session.remove()`. Toute validation doit intervenir **avant** ce bloc, sinon l'objet `user` manipulé ensuite n'est plus celui qui a été validé.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-07 · 🟠
|
|
||||||
|
|
||||||
**Identifiant Discord modifiable malgré l'OAuth2 → détournement des notifications**
|
|
||||||
|
|
||||||
Un flux OAuth2 Discord complet a été ajouté (`auth.py:326-456`). Il obtient de Discord un identifiant authentifié et le stocke en session :
|
|
||||||
|
|
||||||
```python
|
|
||||||
session['discord_oauth'] = {
|
|
||||||
'id': user_data.get('id'), # auth.py:448 — valeur authentifiée par Discord
|
|
||||||
'username': user_data.get('username'),
|
|
||||||
...
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
Mais cette valeur est ensuite **réinjectée dans le formulaire par un champ caché** :
|
|
||||||
|
|
||||||
```html
|
|
||||||
<!-- register.html:61 -->
|
|
||||||
<input type="hidden" name="discord_user_id" value="{{ discord_data.id }}">
|
|
||||||
```
|
|
||||||
|
|
||||||
et c'est la valeur **renvoyée par le client** qui est enregistrée (`auth.py:253` puis `:290`) :
|
|
||||||
|
|
||||||
```python
|
|
||||||
discord_user_id = validated.get('discord_user_id')
|
|
||||||
...
|
|
||||||
user = Player(..., discord_user_id=discord_user_id, ...)
|
|
||||||
```
|
|
||||||
|
|
||||||
**La preuve d'identité obtenue de Discord est perdue au passage.** Un utilisateur peut modifier le champ caché avant envoi, ou poster directement le formulaire sans jamais avoir ouvert le flux OAuth2. `RegisterSchema` valide désormais le format (`validators.py:211-215` : 17-20 chiffres), ce qui est un progrès, mais ne prouve rien sur la propriété du compte.
|
|
||||||
|
|
||||||
La route `edit_profile` (`users.py:284`, `:305`), accessible à **tout utilisateur authentifié**, permet de la même façon de définir un identifiant arbitraire — et n'applique même pas la validation de format ([SEC-06](#sec-06--)). Aucune contrainte d'unicité n'existe sur la colonne (`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 :
|
|
||||||
- demandes de sessions 1:1 avec leurs *« discussion points »*, souvent confidentiels ;
|
|
||||||
- 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 message privé pour accepter ou refuser une demande 1:1. L'attaquant obtient 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.
|
|
||||||
|
|
||||||
**Risque supplémentaire, nouveau :** le flux *paraît* vérifié. Un relecteur qui constate la présence d'un OAuth2 Discord conclura raisonnablement que l'identifiant est authentifié. Il ne l'est pas.
|
|
||||||
|
|
||||||
**Correction.** La brique manquante est courte — la valeur authentifiée existe déjà en session, il suffit de ne plus la faire transiter par le client :
|
|
||||||
|
|
||||||
```python
|
|
||||||
# auth.py, dans register() — ignorer ce que le formulaire prétend
|
|
||||||
discord_oauth = session.get('discord_oauth') or {}
|
|
||||||
discord_user_id = discord_oauth.get('id') # seule source de vérité
|
|
||||||
discord_username = discord_oauth.get('username')
|
|
||||||
```
|
|
||||||
|
|
||||||
et retirer le champ caché de `register.html:61` (l'afficher en lecture seule si l'on veut le montrer à l'utilisateur).
|
|
||||||
|
|
||||||
En complément :
|
|
||||||
1. Ajouter une contrainte d'unicité sur `User.discord_user_id` (nécessite [STD-01](03-standards-stack.md#std-01--)).
|
|
||||||
2. Pour `edit_profile`, soit passer par le même flux OAuth2, soit réserver la modification de ce champ aux administrateurs.
|
|
||||||
3. Traiter [SEC-20](#sec-20--) : sans paramètre `state`, le contenu de `session['discord_oauth']` n'est lui-même pas digne de confiance.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 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 sur son backend `memory://`, explicitement documenté comme non destiné à la production.
|
|
||||||
|
|
||||||
**Impact.**
|
|
||||||
- L'état est **par processus**. Toute évolution vers plusieurs workers ou plusieurs instances fragmente les compteurs et multiplie d'autant la limite effective.
|
|
||||||
- L'état est **perdu à chaque redémarrage** : les compteurs se réinitialisent à chaque redéploiement.
|
|
||||||
- Combiné à [SEC-05](#sec-05--), la protection est de toute façon indexée sur une clé que le client contrôle.
|
|
||||||
|
|
||||||
**Correction.** Adosser le limiteur à un stockage partagé — Redis de préférence, ou la base PostgreSQL déjà présente :
|
|
||||||
|
|
||||||
```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 rendue côté serveur, où chaque page consomme plusieurs requêtes (`/matches/api/events`, `/users/disponibilities`…). Exclure `/static` et `/health` du décompte.
|
|
||||||
|
|
||||||
Corriger [SEC-05](#sec-05--) d'abord : sans clé fiable, un stockage partagé ne sert à rien.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-20 · 🟠
|
|
||||||
|
|
||||||
**Flux OAuth2 Discord sans paramètre `state`**
|
|
||||||
|
|
||||||
`app/routes/auth.py:340-348`
|
|
||||||
|
|
||||||
```python
|
|
||||||
params = {
|
|
||||||
'client_id': DISCORD_CLIENT_ID,
|
|
||||||
'redirect_uri': DISCORD_REDIRECT_URI,
|
|
||||||
'response_type': 'code',
|
|
||||||
'scope': 'identify connections',
|
|
||||||
}
|
|
||||||
query = '&'.join(f'{k}={requests.utils.quote(v)}' for k, v in params.items())
|
|
||||||
auth_url = f'{DISCORD_API_BASE}/oauth2/authorize?{query}'
|
|
||||||
```
|
|
||||||
|
|
||||||
Aucun paramètre `state` n'est émis, et `discord_callback` (`:351-456`) n'en vérifie aucun : la requête de retour est acceptée sur la seule présence d'un `code` (`:363-366`).
|
|
||||||
|
|
||||||
`state` est le mécanisme anti-CSRF prévu par la RFC 6749 (§10.12) : une valeur aléatoire liée à la session, émise à l'aller et vérifiée au retour.
|
|
||||||
|
|
||||||
**Impact.** Un attaquant initie le flux avec **son propre** compte Discord, intercepte le `code` d'autorisation sans le consommer, puis amène la victime à visiter :
|
|
||||||
|
|
||||||
```
|
|
||||||
https://<app>/auth/discord/callback?code=<code_de_l_attaquant>
|
|
||||||
```
|
|
||||||
|
|
||||||
Le serveur échange ce code, obtient le profil Discord **de l'attaquant**, et le place dans `session['discord_oauth']` — la session **de la victime**. Celle-ci voit alors le formulaire d'inscription pré-rempli avec l'identité Discord de l'attaquant, et l'enregistre sans raison de se méfier (le bandeau affiche *« Discord account connected! »*, `auth.py:455`).
|
|
||||||
|
|
||||||
Combiné à [SEC-07](#sec-07--), c'est un chemin direct pour faire enregistrer à un tiers un `discord_user_id` contrôlé par l'attaquant — donc pour recevoir les notifications privées de ce compte.
|
|
||||||
|
|
||||||
Le flux ne servant aujourd'hui qu'à pré-remplir un formulaire d'inscription, la portée est bornée. Elle s'étendrait immédiatement si l'OAuth2 était réutilisé pour l'authentification ou pour la liaison de compte a posteriori, ce qui est l'évolution naturelle de ce code.
|
|
||||||
|
|
||||||
**Correction.**
|
|
||||||
|
|
||||||
```python
|
|
||||||
# auth.py — discord_login()
|
|
||||||
state = secrets.token_urlsafe(32)
|
|
||||||
session['discord_oauth_state'] = state
|
|
||||||
params = {..., 'state': state}
|
|
||||||
|
|
||||||
# auth.py — discord_callback()
|
|
||||||
expected = session.pop('discord_oauth_state', None)
|
|
||||||
if not expected or not secrets.compare_digest(request.args.get('state', ''), expected):
|
|
||||||
flash('Requête Discord invalide.', 'danger')
|
|
||||||
return redirect(url_for('auth.register'))
|
|
||||||
```
|
|
||||||
|
|
||||||
Deux points secondaires dans le même bloc :
|
|
||||||
- `requests.utils.quote(v)` (`:346`) lève un `TypeError` si `DISCORD_REDIRECT_URI` n'est pas défini — seul `DISCORD_CLIENT_ID` est vérifié (`:336`). Vérifier les trois variables, ou construire l'URL avec `urllib.parse.urlencode`.
|
|
||||||
- `:389` — `flash(f'Failed to connect to Discord. Please try again.', 'danger')` est une f-string sans substitution, et la variable `e` capturée à la ligne 388 n'est pas utilisée. Sans conséquence, mais signalé par Ruff (`F541`, `F841`).
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-21 · 🟠
|
|
||||||
|
|
||||||
**Le déploiement SFTP pousse le répertoire `.git/` sur le serveur**
|
|
||||||
|
|
||||||
`.gitea/workflows/git-to-ptero.yaml:41`
|
|
||||||
|
|
||||||
```yaml
|
|
||||||
mirror -R --verbose --parallel=4 ./ ./
|
|
||||||
```
|
|
||||||
|
|
||||||
`mirror -R` téléverse le répertoire courant du runner vers la racine distante. Après un `actions/checkout`, ce répertoire contient **`.git/` en entier** — objets, packfiles, refs, historique complet.
|
|
||||||
|
|
||||||
**Impact.** Si la racine du dépôt est aussi la racine servie par le serveur web — ce qui est le cas par défaut sur un hébergement de type Pterodactyl où l'application est déployée telle quelle — alors `.git/` devient téléchargeable. Les outils d'exploitation courants (`git-dumper` et équivalents) reconstruisent l'intégralité du dépôt à partir de `.git/config`, `.git/HEAD` et des objets, même sans listing de répertoire activé.
|
|
||||||
|
|
||||||
L'attaquant obtient alors le code source complet **et tout l'historique** — donc le commit `2d3721b` et les secrets de production de [SEC-01](#sec-01--). Les deux constats se composent : la purge d'historique demandée en SEC-01 perd son intérêt si le déploiement republie l'historique à chaque exécution.
|
|
||||||
|
|
||||||
Deux points secondaires du même workflow :
|
|
||||||
|
|
||||||
- **`StrictHostKeyChecking=no`** (`:35`) désactive la vérification de l'empreinte du serveur SFTP : le premier échange est vulnérable à une interception, et la clé privée de déploiement peut être présentée à un serveur usurpateur. Utiliser un `known_hosts` épinglé, alimenté par `ssh-keyscan` une fois et stocké en secret de dépôt.
|
|
||||||
- **Aucune exclusion.** Outre `.git/`, le miroir pousse `__pycache__/`, les éventuels `logs/` et tout fichier résiduel du runner.
|
|
||||||
|
|
||||||
**Correction.**
|
|
||||||
|
|
||||||
```yaml
|
|
||||||
mirror -R --verbose --parallel=4 \
|
|
||||||
--exclude-glob .git/ \
|
|
||||||
--exclude-glob .github/ \
|
|
||||||
--exclude-glob .gitea/ \
|
|
||||||
--exclude-glob __pycache__/ \
|
|
||||||
--exclude-glob '*.pyc' \
|
|
||||||
--exclude-glob logs/ \
|
|
||||||
--exclude-glob documents/ \
|
|
||||||
./ ./
|
|
||||||
```
|
|
||||||
|
|
||||||
Plus robuste : construire un artefact de déploiement propre (`git archive HEAD | tar -x -C dist/`) et ne téléverser que `dist/`. `git archive` n'inclut par construction ni `.git/` ni les fichiers ignorés.
|
|
||||||
|
|
||||||
Vérifier par ailleurs, sur le serveur actuel, si `.git/` est déjà présent et accessible — le workflow est déclenchable manuellement (`workflow_dispatch`, `:4`) et a pu être exécuté. Le cas échéant, supprimer le répertoire distant en plus d'appliquer le correctif.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 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.** Aucune utilisation de `|nl2br` dans les templates. Le filtre est enregistré (`app.py:125`) mais mort.
|
|
||||||
|
|
||||||
**Risque.** Piège en attente : le filtre porte un nom naturel, il est enregistré globalement, et la première personne qui écrira `{{ note.content|nl2br }}` — 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())
|
|
||||||
```
|
|
||||||
|
|
||||||
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; "
|
|
||||||
"img-src 'self' data: https://cdn.discordapp.com; "
|
|
||||||
```
|
|
||||||
|
|
||||||
`'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 est de bonne qualité (`frame-ancestors 'none'`, `base-uri 'self'`, `form-action 'self'`). L'ajout de `https://cdn.discordapp.com` en `img-src` pour les avatars Discord est correctement ciblé.
|
|
||||||
|
|
||||||
`'unsafe-inline'` sur `style-src` est nettement moins grave et généralement toléré.
|
|
||||||
|
|
||||||
**Correction.** Par étapes :
|
|
||||||
1. Extraire les `<script>` inline vers `static/js/` — `match_form.html` (45 Ko), `view_tryout.html`, `teams.html`, `calendar.html` et désormais `register.html` (312 lignes ajoutées) 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`) et passer à `script-src 'self' 'nonce-{nonce}'`.
|
|
||||||
3. Épingler les ressources CDN avec `integrity` (SRI).
|
|
||||||
|
|
||||||
En attendant, traiter [SEC-09](#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:598-625` (`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` (`:558-560`) qui contrôle `.pdf`. Les constantes `ALLOWED_CONTRACT_EXTENSIONS` et `ALLOWED_SIGNED_EXTENSIONS` (`:29-30`) **ne sont référencées nulle part**.
|
|
||||||
|
|
||||||
**Facteurs atténuants.** Le nom de fichier stocké est dérivé d'un UUID généré côté serveur, pas du nom fourni par le client — 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)`, 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 la constante déjà déclarée :
|
|
||||||
```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. Vérifier les octets d'en-tête (`%PDF-`) plutôt que la seule extension.
|
|
||||||
3. Conserver le stockage hors arborescence servie (`documents/`, gitignoré) — et vérifier que [SEC-21](#sec-21--) ne le republie pas.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-12 · 🟡
|
|
||||||
|
|
||||||
**Énumération d'utilisateurs via les messages de login**
|
|
||||||
|
|
||||||
`app/routes/auth.py:165-184`
|
|
||||||
|
|
||||||
```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 (`:131-138`) fuit également l'existence du compte, **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é — écart mesurable même sans lire les messages.
|
|
||||||
|
|
||||||
**Impact.** Constitution d'une liste d'identifiants valides, préalable à une 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 — d'autant que [SEC-05](#sec-05--) permet de contourner la limite de 10 tentatives par minute.
|
|
||||||
|
|
||||||
**Correction.**
|
|
||||||
- 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 :
|
|
||||||
```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 plutôt qu'à l'écran.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-13 · 🟡
|
|
||||||
|
|
||||||
**CAPTCHA trivial, et limite d'inscription desserrée à 20/heure**
|
|
||||||
|
|
||||||
`app/routes/auth.py:56-70`
|
|
||||||
|
|
||||||
```python
|
|
||||||
a = random.randint(1, 10)
|
|
||||||
b = random.randint(1, 10)
|
|
||||||
session['captcha_answer'] = a + b
|
|
||||||
return {'question': f'{a} + {b} = ?', 'id': captcha_id}
|
|
||||||
```
|
|
||||||
|
|
||||||
19 réponses possibles (2 à 20). La question figure 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é (`:67`) et stocké en session, mais **n'est jamais comparé** dans `verify_captcha` (`:73-89`) — le champ est décoratif.
|
|
||||||
|
|
||||||
**Aggravation.** La limite de la route `/auth/register` est passée de `3 per hour` à **`20 per hour`** (`:190`). Le rate limiting étant la seule protection réellement efficace ici, la barrière a été divisée par près de 7 — et [SEC-05](#sec-05--) la rend de toute façon contournable.
|
|
||||||
|
|
||||||
**Impact.** Création automatisée de comptes joueurs. Conséquences limitées en privilèges, mais pollution de la base, bruit dans les listes de sélection, et — combiné à [SEC-15](#sec-15--) — accès en masse aux coordonnées des membres.
|
|
||||||
|
|
||||||
**Correction.** Un CAPTCHA maison n'arrêtera pas de bots. Deux options :
|
|
||||||
- **Suffisant ici** : valider l'inscription par un lien envoyé au courriel institutionnel, en restreignant le domaine (`@usherbrooke.ca`). Cela résout simultanément les 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 [SEC-05](#sec-05--) et [SEC-08](#sec-08--), qui conditionnent l'efficacité du rate limiting.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-14 · 🟡
|
|
||||||
|
|
||||||
**`/health` expose l'erreur brute de la base de données**
|
|
||||||
|
|
||||||
`app/app.py:200-206`
|
|
||||||
|
|
||||||
```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 l'hôte, le port, le nom de la base et l'utilisateur. C'est de la reconnaissance d'infrastructure offerte gratuitement, précisément au moment où le système est en difficulté.
|
|
||||||
|
|
||||||
Incohérent avec le gestionnaire 500 (`:294-318`), qui fait 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
|
|
||||||
```
|
|
||||||
|
|
||||||
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:244-249`
|
|
||||||
|
|
||||||
```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 — peut énumérer `/users/1/view`, `/users/2/view`… et collecter les coordonnées de l'ensemble des membres : courriels, téléphones, identifiants Discord. Combiné à [SEC-13](#sec-13--), la barrière d'entrée est basse. Et la collecte des `discord_user_id` alimente directement [SEC-07](#sec-07--).
|
|
||||||
|
|
||||||
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.
|
|
||||||
2. **Restreindre l'accès** selon la relation :
|
|
||||||
|
|
||||||
```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-22 · 🟡
|
|
||||||
|
|
||||||
**`clear_db.py` : destruction totale sans garde-fou, identifiants en dur**
|
|
||||||
|
|
||||||
`clear_db.py` (racine du dépôt, 56 lignes) remplace l'ancien `seed.py`. Il supprime le contenu de **20 tables** puis crée un unique compte administrateur.
|
|
||||||
|
|
||||||
```python
|
|
||||||
tables = ['one_on_one_requests', …, 'users']
|
|
||||||
for table in tables:
|
|
||||||
db.session.execute(db.text(f'DELETE FROM {table}'))
|
|
||||||
db.session.commit()
|
|
||||||
|
|
||||||
admin = Admin(username='admin', password_hash=hash_password('password'), …)
|
|
||||||
```
|
|
||||||
|
|
||||||
C'est une nette amélioration sur l'ancien seed : il n'est plus déclenché automatiquement au démarrage. Quatre problèmes subsistent.
|
|
||||||
|
|
||||||
1. **Aucune confirmation, aucun garde-fou d'environnement.** `python clear_db.py` détruit l'intégralité des données de la base désignée par `DATABASE_URL`. Si le `.env` local pointe vers la base de production — ce qui était précisément la configuration livrée avant le nettoyage de `.env.exemple` ([SEC-01](#sec-01--)) — la commande efface la production sans poser de question. Le fichier est à la racine, sous un nom qui invite à l'exécution.
|
|
||||||
|
|
||||||
2. **Mot de passe administrateur en dur.** `admin` / `password` (`:37`), affiché en clair (`:46-48`). Ce mot de passe ne respecte pas la politique définie dans `validators.py:22-24`, que le script contourne en écrivant directement le hachage.
|
|
||||||
|
|
||||||
3. **Docstring erroné.** Il indique `python -m app.supporting_scripts.seed` (`:4`) alors que le fichier est `clear_db.py` à la racine. Personne ne peut suivre cette instruction.
|
|
||||||
|
|
||||||
4. **Effet de bord au chargement.** `create_app()` (`:54`) déclenche `db.create_all()` et **démarre le bot Discord** ([STD-02](03-standards-stack.md#std-02--), [STD-03](03-standards-stack.md#std-03--)). Réinitialiser la base ouvre une connexion Discord.
|
|
||||||
|
|
||||||
Le `f'DELETE FROM {table}'` (`:30`) interpole une liste codée en dur : pas d'injection possible, mais le motif est à éviter par principe.
|
|
||||||
|
|
||||||
**Impact.** Perte totale de données par erreur de manipulation, et compte administrateur à mot de passe connu sur toute base réinitialisée.
|
|
||||||
|
|
||||||
**Correction.**
|
|
||||||
|
|
||||||
```python
|
|
||||||
if os.getenv('ALLOW_DB_WIPE') != 'yes-i-am-sure':
|
|
||||||
sys.exit('Refus : définir ALLOW_DB_WIPE=yes-i-am-sure pour confirmer.')
|
|
||||||
|
|
||||||
db_url = os.environ['DATABASE_URL']
|
|
||||||
print(f'Cible : {db_url.split("@")[-1]}') # affiche l'hôte, jamais les identifiants
|
|
||||||
if input('Taper le nom de la base pour confirmer : ') != expected_db_name:
|
|
||||||
sys.exit('Annulé.')
|
|
||||||
|
|
||||||
password = secrets.token_urlsafe(16)
|
|
||||||
admin = Admin(username='admin', password_hash=hash_password(password), …)
|
|
||||||
print(f'Mot de passe admin (à noter maintenant) : {password}')
|
|
||||||
```
|
|
||||||
|
|
||||||
Déplacer le script hors de la racine (`app/supporting_scripts/`, conformément à son propre docstring) et l'exposer comme commande CLI Flask.
|
|
||||||
|
|
||||||
**Action indépendante du code :** vérifier en production si les comptes de l'ancien seed (`admin`, `manager1`, `manager2`, `coach1`, `coach2`, `coach3`, `scout1`) existent encore avec le mot de passe `password`. La suppression du script n'a pas supprimé les comptes.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-16 · 🔵
|
|
||||||
|
|
||||||
**Conversions `int()` non protégées sur entrées utilisateur**
|
|
||||||
|
|
||||||
De nombreuses routes convertissent des champs de formulaire sans garde :
|
|
||||||
|
|
||||||
| 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/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 ne divulgue pas de trace et fait le `db.session.rollback()`. Le problème est la qualité de service et le bruit dans les logs, qui masque les incidents réels. `matches.py:298-299` accepte de surcroît une liste d'identifiants sans borne.
|
|
||||||
|
|
||||||
**Correction.** Utiliser le convertisseur intégré, déjà employé ailleurs (`users.py:1110` : `request.form.get('player_id', type=int)`) :
|
|
||||||
|
|
||||||
```python
|
|
||||||
coach_id = request.form.get('coach_id', type=int)
|
|
||||||
if coach_id is None:
|
|
||||||
flash('Coach invalide.', 'danger')
|
|
||||||
return redirect(...)
|
|
||||||
```
|
|
||||||
|
|
||||||
À traiter globalement lors de la généralisation des schémas Marshmallow ([SEC-06](#sec-06--)).
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-17 · 🔵
|
|
||||||
|
|
||||||
**`add_to_team` ne vérifie pas la cohérence tryout / équipe / joueur**
|
|
||||||
|
|
||||||
`app/routes/tryouts.py` (route `/<int:tryout_id>/team/<int:team_id>/add`)
|
|
||||||
|
|
||||||
```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):
|
|
||||||
…
|
|
||||||
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`.** Rien ne vérifie non plus 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, 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
|
|
||||||
if team.tryout_id != tryout_id:
|
|
||||||
abort(404)
|
|
||||||
player_id = request.form.get('player_id', type=int)
|
|
||||||
if not TryoutRegistration.query.filter_by(tryout_id=tryout_id, player_id=player_id).first():
|
|
||||||
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` et `team_matches.py` le font correctement (`if participant.match_id != match_id`) et servent de référence.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## SEC-18 · 🔵
|
|
||||||
|
|
||||||
**Aucune réinitialisation de mot de passe ni MFA**
|
|
||||||
|
|
||||||
`app/routes/auth.py` expose `login`, `register`, `discord_login`, `discord_callback` 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 non validée, cf. [SEC-06](#sec-06--)) et devra le lui transmettre hors bande ;
|
|
||||||
- **aucune authentification à deux facteurs**, y compris pour le compte `Admin` ;
|
|
||||||
- **aucune expiration ni politique de rotation**.
|
|
||||||
|
|
||||||
Le verrouillage après 5 échecs est en revanche bien implémenté, avec réinitialisation du compteur au succès — et c'est aujourd'hui, compte tenu de [SEC-05](#sec-05--), la principale barrière anti-bruteforce.
|
|
||||||
|
|
||||||
**Impact.** Risque organisationnel : l'absence de réinitialisation en libre-service pousse vers des pratiques de contournement (mots de passe communiqués par Discord, 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 à 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 : `URLSafeTimedSerializer(app.config['SECRET_KEY'])`, jeton de 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 ([SEC-19](#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 fichier rotatif `auth.log`, son filtre de redaction et `propagate = False`. Une fabrique `get_auth_logger()` est fournie (`:148-154`).
|
|
||||||
|
|
||||||
**Aucun module n'importe `get_auth_logger` ni ne référence `team_tryouts.auth`** — vérifié par recherche sur l'ensemble de `app/`. Le docstring de `auth.py` annonce pourtant *« login with account lockout protection … and audit logging »*.
|
|
||||||
|
|
||||||
`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 à « qui s'est connecté au compte admin la semaine dernière ». Ce manque devient bloquant si un incident survient — et [SEC-01](#sec-01--) rend 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:157)
|
|
||||||
auth_logger.info('login success user=%s id=%s ip=%s', user.username, user.id, request.remote_addr)
|
|
||||||
# échec (auth.py:167)
|
|
||||||
auth_logger.warning('login failure user=%s ip=%s attempts=%s', username, request.remote_addr, …)
|
|
||||||
# verrouillage (auth.py:169)
|
|
||||||
auth_logger.warning('account locked user=%s ip=%s', user.username, request.remote_addr)
|
|
||||||
# déconnexion, création de compte, callback OAuth2, changement de mot de passe et de rôle
|
|
||||||
```
|
|
||||||
|
|
||||||
Étendre aux opérations sensibles de `users.py` : `create_user`, `edit_user` (surtout `role` et `is_active_account`), `delete_user`.
|
|
||||||
|
|
||||||
**Prérequis :** corriger [SEC-05](#sec-05--) d'abord. En l'état, `request.remote_addr` est une valeur fournie par le client — journaliser une IP usurpable donne une traçabilité illusoire, ce qui est pire que pas de traçabilité du tout.
|
|
||||||
@@ -1,631 +0,0 @@
|
|||||||
# 2 — Maintenabilité
|
|
||||||
|
|
||||||
16 constats sur la structure du code, l'outillage et la chaîne de livraison.
|
|
||||||
|
|
||||||
Références de lignes sur `immortal/main` @ `bb0bc1c`. Le renommage `supporting_scrits` → `supporting_scripts` relevé lors de la première passe a été effectué par l'équipe — voir [`00-provenance.md`](00-provenance.md).
|
|
||||||
|
|
||||||
| 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 | 🔵 Faible |
|
|
||||||
| [MNT-16](#mnt-16--) | Fichier d'état d'exécution `discord_pending.json` versionné | 🔵 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_scripts/security_scan.py`. Il n'y a pas de `security_scan.py` à la racine du dépôt.
|
|
||||||
|
|
||||||
> Le renommage du dossier (commit `0dd4ecd`, `supporting_scrits` → `supporting_scripts`) **n'a pas corrigé la CI** : le chemin invoqué était déjà erroné avant, il l'est toujours après.
|
|
||||||
|
|
||||||
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_scripts.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.
|
|
||||||
|
|
||||||
**Un quatrième point mérite une vérification urgente.** Le pilote PostgreSQL est passé de `psycopg2==2.9.12` à **`psycopg[binary]`, sans version épinglée** — seule entrée non épinglée d'un fichier qui l'est partout ailleurs. Deux conséquences :
|
|
||||||
|
|
||||||
- La reproductibilité est rompue : deux installations à quelques semaines d'intervalle n'obtiendront pas la même version du pilote.
|
|
||||||
- **`psycopg[binary]` est psycopg 3, pas psycopg2.** Or SQLAlchemy 2.0 résout le préfixe `postgresql://` vers le dialecte **psycopg2** par défaut. Si `DATABASE_URL` commence par `postgresql://` — ce qu'indiquait le fichier d'exemple avant son nettoyage — la création du moteur lèvera `ModuleNotFoundError: No module named 'psycopg2'` au démarrage.
|
|
||||||
|
|
||||||
Le fonctionnement actuel suppose donc que la variable de production utilise la forme explicite `postgresql+psycopg://…`. **À vérifier** : si l'application tourne, c'est le cas ; sinon, c'est la cause du dysfonctionnement. Dans les deux situations, la dépendance implicite entre le format de l'URL et le pilote installé doit être documentée, ou levée en normalisant l'URI au démarrage :
|
|
||||||
|
|
||||||
```python
|
|
||||||
url = os.getenv('DATABASE_URL', '')
|
|
||||||
if url.startswith('postgresql://'):
|
|
||||||
url = url.replace('postgresql://', 'postgresql+psycopg://', 1)
|
|
||||||
app.config['SQLALCHEMY_DATABASE_URI'] = url
|
|
||||||
```
|
|
||||||
|
|
||||||
**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", "psycopg[binary]~=3.2", "marshmallow~=4.3",
|
|
||||||
"python-dotenv~=1.2", "discord.py~=2.7", "APScheduler~=3.11",
|
|
||||||
"waitress~=3.0", "requests~=2.34",
|
|
||||||
]
|
|
||||||
```
|
|
||||||
En épinglant `psycopg`, retenir une contrainte de version — l'absence d'épinglage est le point le plus risqué de la liste actuelle.
|
|
||||||
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 265** |
|
|
||||||
| `matches.py` | 583 |
|
|
||||||
| `auth.py` | 473 |
|
|
||||||
| `teams.py` | 455 |
|
|
||||||
| `tryouts.py` | ~430 |
|
|
||||||
| `team_matches.py` | 309 |
|
|
||||||
| `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-241`) — CRUD administrateur ;
|
|
||||||
2. profil personnel (`:244-320`) ;
|
|
||||||
3. disponibilités (`:323-462`) — API JSON ;
|
|
||||||
4. contrats (`:465-650`) — téléversement et téléchargement de fichiers ;
|
|
||||||
5. sessions 1:1 (`:653-909`) — dont l'intégration Discord ;
|
|
||||||
6. notes (`:912-1265`) — personnelles et d'équipe, avec quatre routes de création presque identiques.
|
|
||||||
|
|
||||||
`auth.py` a par ailleurs gagné 177 lignes avec le flux OAuth2 Discord (`:326-456`) et suit la même trajectoire : authentification par mot de passe, CAPTCHA et fédération d'identité désormais dans un seul fichier.
|
|
||||||
|
|
||||||
**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**
|
|
||||||
|
|
||||||
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.
|
|
||||||
|
|
||||||
Le README ne documente par ailleurs **rien de ce qui a été ajouté depuis** : ni le flux OAuth2 Discord et ses trois variables d'environnement (`DISCORD_CLIENT_ID`, `DISCORD_CLIENT_SECRET`, `DISCORD_REDIRECT_URI`), ni `clear_db.py`, ni la chaîne de déploiement `.gitea/workflows/git-to-ptero.yaml`, ni le fait que le dépôt de référence n'est pas GitHub.
|
|
||||||
|
|
||||||
**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.
|
|
||||||
- Ajouter les prérequis (version de Python, PostgreSQL obligatoire au démarrage), l'installation, le démarrage, et le tableau des variables d'environnement.
|
|
||||||
- **Indiquer explicitement quel dépôt fait autorité.** C'est ce qui a manqué ici : l'absence de cette mention a conduit à auditer un miroir en retard de 16 commits. Un `README.md` sur le miroir GitHub renvoyant vers `git.immortal.host` — ou la suppression du miroir — éviterait la récidive.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## MNT-16 · 🔵
|
|
||||||
|
|
||||||
**Fichier d'état d'exécution `discord_pending.json` versionné**
|
|
||||||
|
|
||||||
Le fichier `discord_pending.json` est suivi par git à la racine du dépôt. Son contenu actuel :
|
|
||||||
|
|
||||||
```json
|
|
||||||
{}
|
|
||||||
```
|
|
||||||
|
|
||||||
Le nom et l'usage indiquent un état d'exécution du bot Discord — vraisemblablement le suivi des demandes en attente de réaction, équivalent persistant du dictionnaire `self.pending_requests` de `discord_bot.py:52`.
|
|
||||||
|
|
||||||
**Impact.** Un fichier d'état écrit par l'application et versionné pose trois problèmes :
|
|
||||||
|
|
||||||
1. **Conflits de fusion permanents.** Dès que le bot écrit dedans, le fichier apparaît modifié dans `git status`. Chaque développeur qui lance l'application localement produit une modification non intentionnelle, qu'il committera par inadvertance ou devra écarter à chaque fois.
|
|
||||||
2. **Écrasement au déploiement.** Le workflow SFTP ([SEC-21](01-securite.md#sec-21--)) pousse le contenu du dépôt sur le serveur : chaque déploiement **remet l'état du bot à `{}`**, perdant les demandes en attente. Les réactions ✅/❌ sur les messages Discord antérieurs cesseront d'être reconnues.
|
|
||||||
3. **Fuite potentielle.** Selon ce qui y est stocké (identifiants Discord, identifiants de demandes 1:1), le fichier peut contenir des données personnelles qui n'ont rien à faire dans un dépôt.
|
|
||||||
|
|
||||||
**Correction.**
|
|
||||||
- Retirer le fichier du suivi (`git rm --cached discord_pending.json`) et l'ajouter au `.gitignore`.
|
|
||||||
- Mieux : persister cet état **en base**, où vivent déjà les `OneOnOneRequest`. Un fichier JSON local ne survit ni au déploiement, ni à la mise à l'échelle ([STD-03](03-standards-stack.md#std-03--)), et n'est pas partageable entre plusieurs instances du bot.
|
|
||||||
- Vérifier au passage qu'aucune donnée personnelle n'a été committée dans ses versions antérieures (`git log -p -- discord_pending.json`).
|
|
||||||
@@ -1,406 +0,0 @@
|
|||||||
# 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`.
|
|
||||||
|
|
||||||
Un dossier `migrations/` existe désormais, mais il ne contient **pas** un outillage de migration : c'est un script unique écrit à la main, `migrations/add_tryout_coaches.py`, qui exécute du DDL en SQL brut et vérifie lui-même son idempotence en interrogeant `information_schema`.
|
|
||||||
|
|
||||||
Le script est correct dans ce qu'il fait — le SQL n'interpole aucune entrée utilisateur, la création de table est conditionnelle, et la migration de données vérifie qu'elle n'a pas déjà eu lieu. Il illustre surtout le manque : chaque évolution de schéma exige de réécrire à la main ce qu'Alembic fournit. Il n'y a ni table de version, ni ordonnancement, ni migration inverse, ni moyen de savoir quelles migrations ont été appliquées sur quelle base — l'idempotence est réimplémentée au cas par cas.
|
|
||||||
|
|
||||||
Il hérite en outre des effets de bord de la fabrique : `create_app()` à la ligne 15 déclenche `db.create_all()` et **démarre le bot Discord** ([STD-02](#std-02--), [STD-03](#std-03--)). Appliquer une migration ouvre une connexion Discord.
|
|
||||||
|
|
||||||
`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 |
|
|
||||||
|---|---|
|
|
||||||
| `:349` | `db.create_all()` — écriture dans le schéma |
|
|
||||||
| `:353-355` | Démarrage d'un thread portant un client Discord |
|
|
||||||
| `logging_config.py:66-67` | `os.makedirs('logs')` dans le répertoire de travail courant |
|
|
||||||
|
|
||||||
L'insertion automatique de données de démonstration a été retirée (voir [`00-provenance.md`](00-provenance.md)) — c'était le plus grave de ces effets de bord. Les deux autres subsistent, et leurs conséquences se voient désormais très concrètement dans les deux scripts utilitaires ajoutés depuis :
|
|
||||||
|
|
||||||
- `clear_db.py:54` appelle `create_app()` pour vider la base — **et démarre donc un bot Discord** au passage ;
|
|
||||||
- `migrations/add_tryout_coaches.py:15` fait de même pour appliquer une migration de schéma.
|
|
||||||
|
|
||||||
Aucun de ces deux scripts n'a besoin de Discord ; tous deux l'obtiennent, parce qu'il n'existe pas de moyen de construire l'application sans.
|
|
||||||
|
|
||||||
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:657`), `DISCORD_BOT_TOKEN` (`discord_bot.py:24`) et, depuis l'ajout de l'OAuth2, `DISCORD_CLIENT_ID`, `DISCORD_CLIENT_SECRET` et `DISCORD_REDIRECT_URI` (`auth.py:25-27`). `load_dotenv()` est appelé dans deux modules distincts (`app.py:16`, `discord_bot.py:23`).
|
|
||||||
|
|
||||||
Les trois dernières sont lues **au moment de l'import du module**, comme `DISCORD_WEBHOOK_URL`. Conséquence pratique : elles ne peuvent être ni surchargées en test, ni rechargées sans redémarrer le processus. Et rien ne vérifie leur présence au démarrage — `discord_login` teste `DISCORD_CLIENT_ID` (`auth.py:336`) mais pas les deux autres, si bien qu'un `DISCORD_REDIRECT_URI` manquant produit un `TypeError` à la ligne 346 au lieu d'un message clair (cf. [SEC-20](01-securite.md#sec-20--)).
|
|
||||||
|
|
||||||
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` | `:371` | `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` | `:31-32` | `HOST=127.0.0.1`, `PORT=5000` |
|
|
||||||
| `app/nginx.conf` | `:122` | `proxy_pass http://127.0.0.1:5000` ✅ corrigé |
|
|
||||||
|
|
||||||
Le `proxy_pass` a été corrigé (commit `10af8c0`) : il visait `0.0.0.0:5000`, ce qui n'a pas de sens comme adresse de destination. Il reste 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.
|
|
||||||
- **`wsgi.py` écoute sur 10000 par défaut, nginx envoie désormais vers 5000.** Sans `PORT=5000` dans l'environnement, le proxy ne trouve pas l'application. Le docstring du même fichier (`:11`) annonce pourtant *« PORT: Port to listen on (default: 5000) »* — la documentation et le code se contredisent dans le même fichier.
|
|
||||||
- **`HOST` par défaut à `0.0.0.0`** dans `wsgi.py:26`, avec un commentaire annonçant l'inverse. C'est un facteur direct de [SEC-05](01-securite.md#sec-05--).
|
|
||||||
- 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`, 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. 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.
|
|
||||||
@@ -1,82 +0,0 @@
|
|||||||
# Audit — Team Tryouts
|
|
||||||
|
|
||||||
Audit statique de l'application `team-tryouts` (Flask 3.1 / SQLAlchemy 2.0 / Jinja2 / PostgreSQL).
|
|
||||||
|
|
||||||
| | |
|
|
||||||
|---|---|
|
|
||||||
| **Branche** | `audit/securite-maintenabilite-standards` |
|
|
||||||
| **Base** | `immortal/main` @ `bb0bc1c` — dépôt de référence `git.immortal.host/clubesportsudes/team-tryouts` |
|
|
||||||
| **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, configuration CI/nginx/déploiement, `.gitignore`, dépendances). Aucun test dynamique, aucune exécution de l'application. |
|
|
||||||
|
|
||||||
> **Note de provenance.** Une première passe a été menée sur le miroir GitHub `cedrick2711/team-tryouts`, qui s'est révélé être **en retard de 16 commits**. L'audit a été rebasé sur le dépôt de référence et intégralement revérifié. Voir [`00-provenance.md`](00-provenance.md) pour le détail de l'écart et la liste des constats devenus caducs.
|
|
||||||
|
|
||||||
## Documents
|
|
||||||
|
|
||||||
| Fichier | Contenu |
|
|
||||||
|---|---|
|
|
||||||
| [`00-provenance.md`](00-provenance.md) | Écart entre les deux dépôts, constats résolus |
|
|
||||||
| [`01-securite.md`](01-securite.md) | 22 constats de sécurité |
|
|
||||||
| [`02-maintenabilite.md`](02-maintenabilite.md) | 16 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
|
|
||||||
|
|
||||||
Le socle est sain sur plusieurs points : ORM utilisé partout (**aucune injection SQL**, y compris dans le SQL brut ajouté récemment, qui est correctement paramétré), autoescape Jinja2 actif et **aucun `|safe`** dans les templates, CSRF activé globalement, hachage Werkzeug, modèle d'autorisation polymorphe cohérent, en-têtes de sécurité complets, verrouillage de compte après 5 échecs.
|
|
||||||
|
|
||||||
L'équipe a par ailleurs corrigé de sa propre initiative plusieurs points relevés lors de la première passe : le seed automatique en production a été supprimé, le token Discord n'est plus imprimé sur stdout, `proxy_pass` pointe désormais vers `127.0.0.1`, et la faute de frappe `supporting_scrits` a été corrigée. Ces constats sont archivés dans [`00-provenance.md`](00-provenance.md).
|
|
||||||
|
|
||||||
**Il reste deux constats critiques**, tous deux de configuration :
|
|
||||||
|
|
||||||
1. **Les secrets de production sont toujours récupérables.** Le fichier `app/.env.exemple` a bien été nettoyé (commit `fd258de`), mais les valeurs réelles — token du bot Discord, mot de passe PostgreSQL, `SECRET_KEY` — **restent dans l'historique git des deux dépôts** (commit `2d3721b`), et sont encore présentes **dans le HEAD du miroir GitHub**. Nettoyer un fichier ne révoque pas un secret.
|
|
||||||
2. **Le CORS accepte toutes les origines avec `supports_credentials=True`** dès que `CORS_ALLOWED_ORIGINS` n'est pas défini — et le `.env.exemple` fourni ne le définit pas.
|
|
||||||
|
|
||||||
Deux points méritent une attention particulière parce qu'ils **ressemblent à des correctifs mais n'en sont pas** :
|
|
||||||
|
|
||||||
- **Le paramétrage `trusted_proxy` de Waitress** (`wsgi.py:38-41`) a été ajouté pour traiter les en-têtes de proxy. Mais `trusted_proxy='*'` accepte ces en-têtes de **n'importe quelle source**, et le serveur écoute toujours sur `0.0.0.0`. Le résultat est l'inverse de l'intention : un attaquant joignant directement le port applicatif peut désormais **usurper `X-Forwarded-For` à chaque requête** et contourner intégralement le rate limiting et le suivi des tentatives de connexion. Voir [SEC-05](01-securite.md#sec-05--).
|
|
||||||
- **L'OAuth2 Discord** ajouté à l'inscription aurait pu résoudre le problème d'usurpation d'identifiant Discord. Mais l'identifiant vérifié par Discord est réinjecté dans le formulaire **via un champ caché** (`register.html:61`) avant d'être enregistré : il est donc modifiable par l'utilisateur. La vérification est cosmétique. Voir [SEC-07](01-securite.md#sec-07--).
|
|
||||||
|
|
||||||
Le flux OAuth2 est en outre **dépourvu de paramètre `state`**, ce qui l'expose à la CSRF classique de liaison de compte ([SEC-20](01-securite.md#sec-20--)).
|
|
||||||
|
|
||||||
Enfin, la chaîne de déploiement ajoutée (`.gitea/workflows/git-to-ptero.yaml`) **pousse le répertoire `.git/` complet sur le serveur SFTP** — donc l'historique contenant les secrets du point 1 ([SEC-21](01-securite.md#sec-21--)).
|
|
||||||
|
|
||||||
Côté outillage, rien n'a bougé : `requirements.txt` est toujours en UTF-16, le job CI « Security Scan » pointe toujours vers un chemin inexistant (et le renommage du dossier l'a même éloigné davantage), le job « Tests » reste un `echo`, et `.gitignore` contient toujours `*.html` — **tout nouveau template est silencieusement ignoré par git**.
|
|
||||||
|
|
||||||
## Répartition des constats
|
|
||||||
|
|
||||||
| Sévérité | Sécurité | Maintenabilité | Standards | Total |
|
|
||||||
|---|---|---|---|---|
|
|
||||||
| 🔴 Critique | 2 | — | — | **2** |
|
|
||||||
| 🟠 Élevé | 6 | 4 | 1 | **11** |
|
|
||||||
| 🟡 Moyen | 8 | 8 | 6 | **22** |
|
|
||||||
| 🔵 Faible | 6 | 4 | 3 | **13** |
|
|
||||||
| **Total** | **22** | **16** | **10** | **48** |
|
|
||||||
|
|
||||||
*(4 constats de la première passe ont été résolus par l'équipe — voir [`00-provenance.md`](00-provenance.md).)*
|
|
||||||
|
|
||||||
## Les 6 actions à mener en premier
|
|
||||||
|
|
||||||
| # | Action | Détail |
|
|
||||||
|---|---|---|
|
|
||||||
| 1 | **Révoquer les 3 secrets** — ils sont dans l'historique, le nettoyage du fichier n'a rien révoqué | [SEC-01](01-securite.md#sec-01--) |
|
|
||||||
| 2 | Purger l'historique des deux dépôts, et mettre le miroir GitHub à jour ou le supprimer | [SEC-01](01-securite.md#sec-01--) |
|
|
||||||
| 3 | Refuser le démarrage si `CORS_ALLOWED_ORIGINS` est vide | [SEC-03](01-securite.md#sec-03--) |
|
|
||||||
| 4 | `trusted_proxy='127.0.0.1'` + `HOST=127.0.0.1` | [SEC-05](01-securite.md#sec-05--) |
|
|
||||||
| 5 | Ajouter le paramètre `state` au flux OAuth2 Discord | [SEC-20](01-securite.md#sec-20--) |
|
|
||||||
| 6 | Exclure `.git/` du déploiement SFTP | [SEC-21](01-securite.md#sec-21--) |
|
|
||||||
|
|
||||||
## Ce qui est déjà bien fait
|
|
||||||
|
|
||||||
Pour éviter de le casser lors des corrections :
|
|
||||||
|
|
||||||
- **Aucune injection SQL.** L'ORM est utilisé sans exception. Les deux ajouts de SQL brut sont corrects : `users.py:120-123` utilise des paramètres liés, et `migrations/add_tryout_coaches.py` n'interpole aucune entrée utilisateur.
|
|
||||||
- **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 (`auth.py:150-155`).
|
|
||||||
- **Validation anti-open-redirect** sur le paramètre `next` (`auth.py:40-53`).
|
|
||||||
- **Filtre de redaction des secrets dans les logs** (`logging_config.py:18-50`).
|
|
||||||
- **Le changement de rôle par SQL brut** (`users.py:114-126`) est une solution correcte à un vrai problème SQLAlchemy : modifier un discriminateur polymorphe sur une instance chargée corrompt l'*identity map*. Le commentaire explique le raisonnement. À conserver.
|
|
||||||
@@ -1,135 +0,0 @@
|
|||||||
# Plan de remédiation
|
|
||||||
|
|
||||||
Ordre proposé. Les lots sont séquentiels : chacun lève des blocages du suivant.
|
|
||||||
|
|
||||||
Établi sur `immortal/main` @ `bb0bc1c`. Les constats déjà résolus par l'équipe sont listés dans [`00-provenance.md`](00-provenance.md) et absents de ce plan.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Lot 0 — Aujourd'hui, avant tout commit
|
|
||||||
|
|
||||||
Ces actions ne sont pas du code.
|
|
||||||
|
|
||||||
| # | 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 de l'ancien seed (`admin`, `manager1`, `manager2`, `coach1`, `coach2`, `coach3`, `scout1`) existent avec le mot de passe `password` ; les désactiver ou changer leur mot de passe | [SEC-22](01-securite.md#sec-22--) |
|
|
||||||
| 0.5 | **Vérifier si `.git/` est présent et téléchargeable** sur le serveur de production (`curl https://<app>/.git/HEAD`) ; le supprimer le cas échéant | [SEC-21](01-securite.md#sec-21--) |
|
|
||||||
| 0.6 | Consulter les logs Discord et PostgreSQL pour détecter un éventuel accès non autorisé | SEC-01 |
|
|
||||||
|
|
||||||
> **Le nettoyage de `app/.env.exemple` (commit `fd258de`) n'a rien révoqué.** Les valeurs restent lisibles dans l'historique des deux dépôts, et dans le HEAD du miroir GitHub. 0.1 à 0.3 restent nécessaires et sont indépendantes de toute manipulation de git.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Lot 1 — Correctifs critiques
|
|
||||||
|
|
||||||
| # | Action | Réf. | Effort |
|
|
||||||
|---|---|---|---|
|
|
||||||
| 1.1 | Purger l'historique des **deux** dépôts (`git filter-repo --path app/.env.exemple --invert-paths`), prévenir l'équipe de recloner | [SEC-01](01-securite.md#sec-01--) | 2 h |
|
|
||||||
| 1.2 | Synchroniser ou supprimer le miroir GitHub — il expose encore les secrets dans son HEAD | SEC-01 | 30 min |
|
|
||||||
| 1.3 | Ajouter `.env*` au `.gitignore` avec `!.env.example` ; renommer `.env.exemple` → `.env.example` | SEC-01 | 15 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 | `trusted_proxy='127.0.0.1'` + `HOST` par défaut à `127.0.0.1` dans `wsgi.py` | [SEC-05](01-securite.md#sec-05--) | 20 min |
|
|
||||||
| 1.6 | Exclure `.git/` (et `.github/`, `__pycache__/`, `logs/`) du miroir SFTP ; épingler `known_hosts` | [SEC-21](01-securite.md#sec-21--) | 1 h |
|
|
||||||
| 1.7 | 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 | **Vérifier le pilote PostgreSQL** — `psycopg[binary]` est psycopg 3 ; confirmer que `DATABASE_URL` utilise `postgresql+psycopg://`, ou normaliser l'URI au démarrage. Épingler la version. | [MNT-05](02-maintenabilite.md#mnt-05--) | 1 h |
|
|
||||||
| 2.2 | Réencoder `requirements.txt` en UTF-8, ajouter `.gitattributes` | [MNT-02](02-maintenabilite.md#mnt-02--) | 15 min |
|
|
||||||
| 2.3 | Corriger `.gitignore` (`*.html`, `docs/`), vérifier les fichiers déjà perdus | [MNT-01](02-maintenabilite.md#mnt-01--) | 30 min |
|
|
||||||
| 2.4 | 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.5 | Ajouter `pyproject.toml` avec la configuration Ruff, formater en un commit isolé | [MNT-06](02-maintenabilite.md#mnt-06--) | 2 h |
|
|
||||||
| 2.6 | Retirer `dotenv`, `login`, `discord` des dépendances | MNT-05 | 30 min |
|
|
||||||
| 2.7 | Retirer `discord_pending.json` du suivi git | [MNT-16](02-maintenabilite.md#mnt-16--) | 10 min |
|
|
||||||
| 2.8 | 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 | Ajouter le paramètre `state` au flux OAuth2 Discord | [SEC-20](01-securite.md#sec-20--) | 1 h |
|
|
||||||
| 3.2 | Prendre `discord_user_id` depuis la session OAuth2, pas depuis le champ caché du formulaire | [SEC-07](01-securite.md#sec-07--) | 2 h |
|
|
||||||
| 3.3 | Brancher `CreateUserSchema`, `EditUserSchema`, `EditProfileSchema` sur leurs routes | [SEC-06](01-securite.md#sec-06--) | 3 h |
|
|
||||||
| 3.4 | Garde-fous sur `clear_db.py` : confirmation, variable d'environnement, mot de passe aléatoire | [SEC-22](01-securite.md#sec-22--) | 1 h |
|
|
||||||
| 3.5 | Adosser Flask-Limiter à Redis ou PostgreSQL ; réévaluer les seuils (dont `register`, passé à 20/h) | [SEC-08](01-securite.md#sec-08--) · [SEC-13](01-securite.md#sec-13--) | 2 h |
|
|
||||||
| 3.6 | Restreindre `view_user` et réduire les données transmises au template | [SEC-15](01-securite.md#sec-15--) | 2 h |
|
|
||||||
| 3.7 | Valider les téléversements de contrats signés | [SEC-11](01-securite.md#sec-11--) | 1 h |
|
|
||||||
| 3.8 | Uniformiser les messages de login, égaliser les temps de réponse | [SEC-12](01-securite.md#sec-12--) | 1 h |
|
|
||||||
| 3.9 | Corriger `nl2br` (échapper avant `Markup`) ou le supprimer | [SEC-09](01-securite.md#sec-09--) | 15 min |
|
|
||||||
| 3.10 | Masquer l'erreur brute de `/health` | [SEC-14](01-securite.md#sec-14--) | 10 min |
|
|
||||||
| 3.11 | Alimenter le logger `team_tryouts.auth` — **après** 1.5 | [SEC-19](01-securite.md#sec-19--) | 2 h |
|
|
||||||
| 3.12 | Contrôler la cohérence tryout/équipe dans `add_to_team` | [SEC-17](01-securite.md#sec-17--) | 30 min |
|
|
||||||
| 3.13 | 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` ; reprendre `migrations/add_tryout_coaches.py` comme première révision Alembic | [STD-01](03-standards-stack.md#std-01--) | 3 h |
|
|
||||||
| 4.2 | Objets de configuration par environnement ; un seul `load_dotenv()` ; vérifier les 3 variables Discord OAuth2 au démarrage | [STD-05](03-standards-stack.md#std-05--) | 4 h |
|
|
||||||
| 4.3 | Sortir les effets de bord de `create_app()` — bénéficie aussi à `clear_db.py` et aux migrations | [STD-02](03-standards-stack.md#std-02--) | 3 h |
|
|
||||||
| 4.4 | Séparer le bot Discord en processus distinct ; persister son état en base | [STD-03](03-standards-stack.md#std-03--) · [MNT-16](02-maintenabilite.md#mnt-16--) | 1 j |
|
|
||||||
| 4.5 | *(dépend de 4.1)* Unicité sur `discord_user_id` | [SEC-07](01-securite.md#sec-07--) | 2 h |
|
|
||||||
| 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 |
|
|
||||||
| 4.9 | Aligner les ports (`wsgi.py` défaut 5000, `run.py` en `127.0.0.1`) | [STD-06](03-standards-stack.md#std-06--) | 30 min |
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 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` (1 265 lignes) en cinq blueprints ; sortir l'OAuth2 de `auth.py` | [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` ; remplacer le miroir SFTP par un artefact | [STD-10](03-standards-stack.md#std-10--) |
|
|
||||||
| 5.13 | Réécrire le README, en indiquant quel dépôt fait autorité | [MNT-15](02-maintenabilite.md#mnt-15--) |
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Dépendances entre lots
|
|
||||||
|
|
||||||
```
|
|
||||||
Lot 0 (révocation, vérifications en prod)
|
|
||||||
└─→ Lot 1 (correctifs critiques)
|
|
||||||
└─→ Lot 2 (outillage : pilote DB, CI verte, premiers tests)
|
|
||||||
├─→ Lot 3 (sécurité applicative)
|
|
||||||
│ └─ 3.11 journal d'audit (bloqué par 1.5 — sinon on journalise des IP usurpables)
|
|
||||||
└─→ 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)
|
|
||||||
```
|
|
||||||
|
|
||||||
Deux dépendances méritent attention :
|
|
||||||
|
|
||||||
- **2.1 avant tout le reste du Lot 2** : si le pilote PostgreSQL est mal apparié à l'URI, l'application ne démarre pas, et aucun test d'intégration ne peut être écrit.
|
|
||||||
- **1.5 avant 3.11** : journaliser `request.remote_addr` tant que `trusted_proxy='*'` produit une traçabilité illusoire — pire que pas de traçabilité, parce qu'on lui fera confiance.
|
|
||||||
Reference in New Issue
Block a user