GGThedandClaude Opus 5
ed586233f6
Le miroir GitHub audite en premiere passe etait en retard de 16 commits
sur git.immortal.host/clubesportsudes/team-tryouts. L'audit est rebase
sur immortal/main @ bb0bc1c et l'ensemble des constats reverifie.
Resolus par l'equipe (archives dans 00-provenance.md) :
- seed automatique en production supprime
- print du token Discord supprime
- proxy_pass nginx corrige vers 127.0.0.1
- dossier supporting_scrits renomme
Nouveaux constats :
- SEC-20 flux OAuth2 Discord sans parametre state (CSRF de liaison)
- SEC-21 le deploiement SFTP pousse .git/ sur le serveur
- SEC-22 clear_db.py destructif sans garde-fou, admin/password en dur
- MNT-16 discord_pending.json versionne
Requalifies :
- SEC-01 secrets retires du fichier mais toujours dans l'historique des
deux depots, et dans le HEAD du miroir GitHub -> revocation requise
- SEC-05 trusted_proxy='*' + bind 0.0.0.0 rend X-Forwarded-For usurpable,
ce qui ouvre le rate limiting au lieu de le corriger
- SEC-07 l'OAuth2 ajoute ne contraint pas l'identite : le discord_user_id
transite par un champ cache du formulaire
- MNT-05 psycopg[binary] non epingle, incompatible avec les URI
postgresql:// que SQLAlchemy resout vers psycopg2
48 constats. Aucune modification du code applicatif.
Co-Authored-By: Claude Opus 5 <[email protected]>
83 lines
6.9 KiB
Markdown
83 lines
6.9 KiB
Markdown
# 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.
|