perun/AUDIT.md

# Audit de reprise — Perun for DCS World

> Revue menée pour décider d'une reprise du projet (upstream `szporwolik/perun`,
> **archivé le 2026-03-29**, licence MIT). Fork de travail : `DaKerboul/perun`,
> miroir Gitea : `git.kerboul.me/kerboul/perun`.
>
> **Méthode.** Revue à 5 casquettes (Architecte, Sécurité, Backend .NET, DCS/Lua,
> DevOps/DBA). **Périmètre audité** : le code réellement écrit par le projet —
> hook Lua (`01_DCS`), app C# WinForms (`02_Windows_App`), wrapper C++ (`03_*`,
> hors arbre Lua 5.1.5 vendoré), schéma MySQL (`04_MySQL`), exemple PHP
> (`05_Misc`). Les ~35 `.c`/`.h` de `lua-5.1.5/` sont du Lua amont vendoré et **ne
> sont pas audités**.
> Date : 2026-06-11.

---

## Synthèse — verdict

Projet **fonctionnellement riche et bien pensé sur le fond** (modèle de données
propre, couverture événementielle DCS complète, multi-instances, intégrations
SRS/LotATC). Mais **dette de sécurité critique** et code applicatif daté
(monolithes, threading artisanal, zéro test, zéro CI, Windows/.NET Framework
uniquement). C'est une **bonne base à reprendre**, à condition de traiter les
points P0 **avant** toute remise en production.

| # | Domaine | Gravité | Sévérité |
|---|---------|---------|----------|
| S1 | Injection SQL via données joueur (app C#) | **Critique** | 🔴 P0 |
| S2 | Port TCP sans authentification, bind `0.0.0.0` | **Critique** | 🔴 P0 |
| S3 | XSS stocké dans l'exemple PHP | Élevé | 🟠 P1 |
| S4 | Mot de passe MySQL stocké en clair | Élevé | 🟠 P1 |
| S5 | PII (IP, UCID) sans rétention ni consentement (RGPD) | Élevé | 🟠 P1 |
| B1 | Threading non synchronisé (buffer partagé) | Élevé | 🟠 P1 |
| B2 | `ExecuteReader` pour des INSERT/UPDATE, 1 connexion/frame | Moyen | 🟡 P2 |
| A1 | Monolithes, `dynamic`, aucun test, aucune CI | Moyen | 🟡 P2 |
| L1 | Fuite de variables globales dans le hook DCS | Moyen | 🟡 P2 |
| D1 | Lua 5.1.5 vendoré dans le repo, pas de build reproductible | Moyen | 🟡 P2 |
| D2 | Pas de FK, dépendance à `STRICT_TRANS_TABLES` off, pas de migrations | Faible | 🟢 P3 |

---

## 1. Architecte / Lead — structure & dette

**Points forts**
- Découpage fonctionnel clair en 5 dossiers numérotés, lisible d'emblée.
- Séparation nette des responsabilités : collecte (Lua) → transport (TCP/DLL) →
  persistance (C#) → restitution (PHP). Le protocole de trames (IDs 1/2/3/50…101)
  est documenté dans le README.
- Multi-instances supporté de bout en bout (champ `instance` partout).

**Points faibles**
- **Monolithes.** `DatabaseController.SendToMySql` fait ~290 lignes avec un `switch`
  géant mêlant construction SQL, mapping métier et logging
  (`01_Classes/DatabaseController.cs:12-304`). Idem `TCPController.StartListen`
  (boucles imbriquées sur ~170 lignes).
- **`dynamic` partout** pour le JSON entrant (`DatabaseController.cs:25`,
  `TCPController.cs:121`) : aucune validation de schéma, accès `TCPFrame.payload.x`
  qui lèvent au moindre champ manquant → exceptions au lieu d'un rejet propre.
- **Plateforme verrouillée** : VS2017, .NET Framework 4.8, WinForms → Windows
  uniquement, fin de vie. Or le serveur DCS est Windows, mais l'app de
  persistance n'a aucune raison d'y être clouée (elle ne parle que TCP + MySQL).
- **TODO laissé en dur** (commentaire polonais) :
  `DatabaseController.cs:92` « TUTAJ DODAC CATCH TBD » → gestion d'erreur inachevée.
- **Aucun test, aucune CI**, dérive de version (cf. §4 L-version).

**À faire**
- [ ] Extraire la construction SQL dans une couche dédiée (un handler par type de
      trame) + DTO typés à la place de `dynamic`.
- [ ] Cibler **.NET 8** + un worker headless multiplateforme ; garder l'UI WinForms
      en option (ou la remplacer par un petit panneau web/CLI).
- [ ] Introduire des tests unitaires (parsing de trames, génération SQL) et un
      pipeline CI.

## 2. Sécurité (AppSec) — **bloquant pour la prod**

**S1 — Injection SQL via données contrôlées par le joueur. 🔴**
L'app paramètre *certaines* valeurs (`@PAR_*`) mais **en concatène des dizaines
d'autres** directement dans le SQL, dont des champs que n'importe quel joueur
maîtrise (UCID, nom, hash de mission, IP, datetime, tous les compteurs `ps_*`) :
- `DatabaseController.cs:128-132` (chat) : `ucid`, `missionhash`, `all`, `datetime`.
- `DatabaseController.cs:159-165` (stats) : `stat_ucid`, `stat_missionhash` et
  ~30 valeurs `stat_data_perun.ps_*` injectées telles quelles.
- `DatabaseController.cs:175-177` (login) : `login_ucid`, `login_ipaddr`, `login_datetime`.
- `DatabaseController.cs:50-51, 77-89, 205-206` : `instance`/`type` concaténés.

Un UCID/nom forgé (ou un module client modifié) permet l'exfiltration ou la
destruction de la base. C'est le **défaut n°1 à corriger**.
→ **Tout** passer en requêtes paramétrées (`MySqlParameter`), sans exception.

**S2 — Listener TCP non authentifié, exposé. 🔴**
`new TcpListener(IPAddress.Any, intListenPort)` (`TCPController.cs:51`) écoute sur
**toutes** les interfaces, **sans authentification ni allowlist**. Couplé à S1,
n'importe quel hôte joignant le port (48621 par défaut) injecte des trames
arbitraires → compromission complète de la base.
→ Par défaut **bind `127.0.0.1`** (hook et app sont quasi toujours sur la même
machine), + secret partagé/HMAC sur les trames, + allowlist d'IP.

**S3 — XSS stocké (PHP). 🟠**
`05_Misc/05_PHP_Example/index.php` réinjecte en HTML des données joueur **sans
échappement** : message de chat (`:110`), nom (`:92, :109, :143`),
contenu d'événement (`:127`). Un joueur dont le nom vaut `<script>…</script>`
exécute du JS dans le navigateur de l'admin. Les requêtes elles-mêmes sont
statiques (pas de SQLi côté PHP), le risque est l'**XSS**.
→ `htmlspecialchars()` systématique sur toute sortie ; corriger aussi le HTML
invalide (`<h1>` fermé par `</h2>` ligne 44).

**S4 — Identifiants MySQL en clair. 🟠**
Le mot de passe est stocké en `String` dans les user settings .NET
(`02_Forms/form_Main.cs:119`, clé `MYSQL_Password` de `app.config`) → écrit en
clair dans `user.config`.
→ Chiffrer via **DPAPI** (`ProtectedData`) ou déléguer à un gestionnaire de
secrets ; a minima ne jamais journaliser la chaîne de connexion.

**S5 — Données personnelles (RGPD). 🟠**
Le hook collecte et stocke **adresses IP** et **UCID** des joueurs
(`Perun-hook.lua:364-366` → `pe_DataPlayers_lastip`, `pe_LogLogins_ip`), sans
politique de rétention ni information des joueurs.
→ Définir une durée de rétention + purge, anonymiser/hacher l'IP si non
nécessaire, documenter (mention serveur + Discord).

## 3. Backend / .NET — qualité & robustesse

**B1 — Threading artisanal non synchronisé. 🟠**
Le thread TCP écrit dans `Globals.arrMySQLSendBuffer` (tableau fixe) pendant que
le thread d'envoi le lit, **sans verrou** (`TCPController.cs:130-142`). Buffer
plein = paquets **silencieusement perdus** (`:139-141`), scan linéaire O(n) par
paquet. Accès concurrents → corruption/race.
→ Remplacer par une `BlockingCollection<T>`/`Channel<T>` thread-safe et bornée.

**B2 — Accès base inefficace. 🟡**
- `ExecuteReader()` utilisé pour exécuter des lots d'INSERT/UPDATE
  (`DatabaseController.cs:217`) : devrait être `ExecuteNonQuery()`.
- **Une nouvelle `MySqlConnection` ouverte/fermée par trame** (`:34, :292`) :
  pas de réutilisation du pool, surcoût réseau par paquet.
- Tout est **synchrone bloquant** (pas d'`async`/`await`).
→ Connexion/pool réutilisé, requêtes asynchrones, `ExecuteNonQueryAsync`.

**Autres**
- Dépendance `Newtonsoft.Json` → migrable vers `System.Text.Json`.
- Gestion d'erreurs par numéros MySQL en dur (`:246-279`) : utile mais fragile,
  à compléter (le catch manquant signalé en `:92`).

## 4. DCS / Lua / Intégration

**Points forts**
- Couverture événementielle DCS **très complète** : kill (avec catégorisation
  PvP/AI), friendly fire, crash, eject, takeoff/landing (airfield/ship/FARP),
  multicrew, change_slot, connect/disconnect, chat, MOTD. C'est le vrai actif du
  projet (`Perun-hook.lua:725-879`).
- Comptage de stats maison car les stats natives DCS sont peu fiables (choix
  assumé et pertinent).

**Points faibles**
- **L1 — Fuite de variables globales 🟡** : plusieurs variables sont assignées
  sans `local` dans l'environnement *hook* (privilégié et partagé) — ex.
  `_temp_killers`/`_temp_event_type` (`:754-756`), `_master_type`/`_master_slot`/
  `_sub_slot` (`:813`), `_temp_airfield` (`:849`). Risque de collision avec
  d'autres hooks installés sur le serveur.
- **L-version — dérive de version** : la version du hook est codée en dur
  `"v0.12.1"` (`:30`) et vit séparément de la version de l'app (`Globals.VersionPerun`).
  → source unique de vérité (tag git → injecté au build).
- **Coût par frame** : `onSimulationFrame` fait du `table.concat`/JSON à chaque
  frame ; sur serveur chargé, surveiller le budget temps (déjà mesuré en µs dans
  les logs — bon réflexe à conserver/exposer).
- **Dépendance à une DLL compilée** (`perun.dll`, issue de `03_Perun_Lua_Wrapper`)
  livrée hors repo : reproductibilité du build à fiabiliser (cf. D1).
- TCP **en clair**, pas de TLS (acceptable en loopback, à revoir si distant).

**À faire**
- [ ] `local` sur toutes les temporaires ; passe `luacheck`.
- [ ] Versionner hook + app depuis le tag git.
- [ ] Documenter/reproduire le build de `perun.dll` (CMake déjà présent).

## 5. DevOps / DBA / Release

**Base de données — plutôt saine.**
- InnoDB + `utf8mb4`/`unicode_ci`, PK/`AUTO_INCREMENT`, **clés UNIQUE**
  pertinentes (UCID, hash, type, stats par mission+ucid+type) et index sur les
  colonnes de tri (`datetime`, `instance`, `type`) — `04_MySQL/m1081_perun.sql`.
- **D2 🟢** : pas de **`FOREIGN KEY`** (intégrité référentielle non garantie),
  dépendance documentée à **`STRICT_TRANS_TABLES` désactivé** (README:42,84) —
  c'est-à-dire qu'on s'appuie sur la coercition/troncature silencieuse de MySQL,
  ce qui masque des bugs. Nom de fichier cryptique (`m1081_perun.sql`).
- Pas d'outil de **migration** (un seul dump). → introduire des migrations
  versionnées (Flyway/dbmate/sqitch) et, à terme, des FK + un mode strict assumé.

**Build / release / repo**
- **D1 🟡** : arbre **Lua 5.1.5 complet vendoré** dans `03_Perun_Lua_Wrapper/lua-5.1.5/`
  → gonfle le repo et l'audit. Le passer en sous-module / téléchargement au build.
- Build **manuel VS2017**, pas de CI, pas d'artefacts reproductibles.
- Contributions historiquement attendues sur la branche `dev` (README:135).
- Pas de `.gitignore` racine (un seul dans le wrapper).

**À faire**
- [ ] CI (build C# + `luacheck` + lint PHP + validation du schéma SQL).
- [ ] Migrations DB versionnées ; activer les FK progressivement.
- [ ] Sortir Lua amont du repo ; pipeline de build de la DLL.
- [ ] Releases taguées avec binaires (`perun.dll` + app) attachés.

---

## Feuille de route priorisée

**P0 — Sécurité bloquante (avant toute prod)**
1. Paramétrer **100 %** des requêtes SQL (S1).
2. Bind loopback par défaut + auth/allowlist sur le listener TCP (S2).

**P1 — Durcissement & conformité**
3. Échappement HTML de l'exemple PHP (S3).
4. Chiffrer le mot de passe MySQL — DPAPI (S4).
5. Rétention/anonymisation IP & UCID, doc RGPD (S5).
6. File thread-safe bornée à la place du buffer tableau (B1).

**P2 — Modernisation**
7. Découper les monolithes, DTO typés au lieu de `dynamic` (A1).
8. Connexion poolée + accès DB async + `ExecuteNonQuery` (B2).
9. `local` + `luacheck` sur le hook, version unifiée depuis git (L1).
10. Cibler .NET 8 / worker multiplateforme.
11. Tests + CI.

**P3 — Hygiène long terme**
12. Migrations DB, FK, mode SQL strict assumé (D2).
13. Désvendoriser Lua, build reproductible de la DLL (D1).
14. Releases taguées + artefacts.

## Plan de reprise suggéré

1. **Geler le comportement** : quelques tests de caractérisation sur le parsing de
   trames et la génération SQL, pour refactorer sans régresser.
2. **Sprint sécu (P0)** sur une branche `security/sql-and-tcp`, puis tag
   `v0.13.0-rc` testé en loopback.
3. **P1** en incréments livrables.
4. Décider de la cible app (garder WinForms vs worker .NET 8) **avant** d'attaquer
   P2 — ça oriente tout le refactor.

> Adapté à ton contexte : tu fais déjà tourner Gitea + CI sur le cluster ; un
> pipeline build/lint Perun s'y intègre directement, et la communauté Commus DCS
> est un terrain de test naturel pour les stats.