diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..33f8e74 --- /dev/null +++ b/CODE_REVIEW.md @@ -0,0 +1,212 @@ +# Code Review - Scripts Serveur (MALIO) + +**Date** : 2026-03-09 +**Reviewer** : Claude (Opus 4.6) +**Scope** : Revue complète de tout le code du dépôt + +--- + +**Note** : Le fichier `CheckStorage/.env` contient un webhook Discord en local mais n'est **pas commité** dans le dépôt (correctement ignoré par le `.gitignore`). Pas de fuite de secret. + +--- + +## 1. BackupVaultWarden/backup-vaultwarden.sh + +### Qualite globale : Bonne + +Le script est bien structuré, utilise `set -euo pipefail`, et gère correctement les erreurs. + +### Problemes + +| Severite | Ligne | Description | +|----------|-------|-------------| +| **CRITIQUE** | 8 | Chemin `.env` en dur (`/home/matt/vaultwarden/scripts/...`) au lieu d'utiliser `$SCRIPT_DIR`. Ce script ne fonctionnera **que sur la machine de matt**. | +| **HAUTE** | 80-83 | **Injection de code** dans le message Discord. La variable `$msg` est injectée directement dans du code Python via un heredoc. Si le contenu du backup contient des guillemets triples `"""` ou du code Python, il sera exécuté. | +| **MOYENNE** | 7 | `SCRIPT_DIR` est calculé mais jamais utilisé (variable morte). | +| **BASSE** | 36 | `WEBHOOK_URL` utilise `:=` (valeur par défaut vide) au lieu de `:-`. Ce n'est pas un bug mais c'est incohérent avec les autres variables qui utilisent `:?`. | + +### Suggestions + +- **Ligne 8** : Remplacer par `ENV_FILE="${SCRIPT_DIR}/.env"` comme dans les autres scripts. +- **Ligne 80-83** : Utiliser `jq` ou `python3 -c` avec passage par argument au lieu d'interpoler dans un heredoc Python : + ```bash + printf '%s' "$msg" | python3 -c 'import sys,json; print(json.dumps({"content": sys.stdin.read()}))' | curl ... + ``` +- Ajouter une rotation des backups distants (pas de purge des anciens backups actuellement). + +--- + +## 2. CheckStorage/check-storage.sh + +### Qualite globale : Faible + +Ce script est le moins mature du dépôt. Pas de `set -euo pipefail`, pas de gestion d'erreurs, et des pratiques fragiles. + +### Problemes + +| Severite | Ligne | Description | +|----------|-------|-------------| +| **HAUTE** | 1 | Pas de `set -euo pipefail`. Le script continue silencieusement en cas d'erreur. | +| **HAUTE** | 10 | Lecture du `.env` avec `grep | cut` au lieu de `source`. Fragile : ne supporte pas les valeurs avec `=`, les espaces, ou les guillemets. | +| **HAUTE** | 39-45 | **Injection JSON** : le message Discord est construit par interpolation directe dans du JSON. Si une variable contient `"` ou `\`, le JSON sera invalide ou pire. | +| **MOYENNE** | 10 | Chemin `.env` relatif sans `cd` vers le répertoire du script. Le script cassera s'il est exécuté depuis un autre répertoire (ex: via cron). | +| **MOYENNE** | 37 | Pas de notification quand tout va bien (aucun message si usage < limite). Impossible de savoir si le cron fonctionne. | +| **BASSE** | - | Pas de shebang `#!/usr/bin/env bash`, utilise `#!/bin/bash` directement (moins portable). | +| **BASSE** | - | Pas de logging dans un fichier, seulement `echo` sur stdout. | + +### Suggestions + +- Aligner sur le pattern des scripts RecetteScripts : `set -euo pipefail`, `source .env`, `SCRIPT_DIR`, gestion d'erreurs. +- Utiliser `jq` ou Python pour construire le JSON Discord de maniere sure. +- Ajouter un chemin absolu vers le `.env` base sur `$SCRIPT_DIR`. +- Ajouter un mode `--verbose` ou un log file. + +--- + +## 3. RecetteScripts/backup-bdd-recette.sh + +### Qualite globale : Tres bonne + +C'est le script le plus mature du dépôt. Bien structure, bonne gestion d'erreurs, verrou d'execution, messages Discord granulaires. + +### Problemes + +| Severite | Ligne | Description | +|----------|-------|-------------| +| **HAUTE** | 92 | `export PGPASSWORD` : le mot de passe PostgreSQL est expose dans l'environnement. Tout process fils peut le lire (visible dans `/proc/*/environ`). Preferer un fichier `.pgpass` avec permissions 600. | +| **HAUTE** | 106-107 | **Injection JSON** dans `discord_send()` : la variable `$msg` est interpolee directement dans le JSON curl. Meme probleme que les autres scripts. | +| **MOYENNE** | 223 | Les noms de dossiers distants (`ferme`, `sirh`, `inventory`, `user`) sont en dur au lieu d'etre derives de `$DBS`. Si on ajoute une base dans `.env`, le dossier distant ne sera pas cree. | +| **MOYENNE** | 237-239 | L'export des "roles" fait en realite un simple `SELECT rolname` : ca ne sauvegarde que les **noms** des roles, pas leurs privileges, mots de passe, ou attributs. Utiliser `pg_dumpall --roles-only` pour un vrai backup des roles. | +| **BASSE** | 336 | `exit 2` en fin de script en cas d'erreur partielle : c'est bien, mais pas documente dans le README. | +| **BASSE** | 85 | Le `TMP_DIR` est sous `/tmp` : sur un systeme multi-utilisateur, un autre user pourrait pre-creer le dossier (race condition / symlink attack). Utiliser `mktemp -d`. | + +### Suggestions + +- Remplacer `export PGPASSWORD` par un fichier `.pgpass`. +- Deriver les dossiers distants de `$DBS_ARRAY` au lieu de les hardcoder. +- Utiliser `pg_dumpall --roles-only` pour un vrai export des roles. +- Utiliser `mktemp -d` pour le repertoire temporaire. + +--- + +## 4. RecetteScripts/check-statut-recette.sh + +### Qualite globale : Bonne + +Script bien ecrit, bonne separation des responsabilites, gestion propre des erreurs curl. + +### Problemes + +| Severite | Ligne | Description | +|----------|-------|-------------| +| **HAUTE** | 92-94 | **Injection JSON** dans Discord (meme pattern que partout). | +| **MOYENNE** | 1 | `set -u` sans `set -e`. Les erreurs non gerees ne stopperont pas le script. C'est voulu (le script doit continuer pour checker tous les sites), mais `set -o pipefail` manque. | +| **MOYENNE** | 52 | Schema HTTP en dur (`http`). Pas de support HTTPS. Si les apps passent en HTTPS, le script retournera des redirections 301/302 au lieu de verifier le vrai endpoint. | +| **BASSE** | 134 | Le `mktemp` pour stderr n'est pas nettoye en cas d'interruption (pas de `trap`). Fuite mineure de fichiers temp. | + +### Suggestions + +- Ajouter le support HTTPS (configurable par URL dans le `.env`, ou suivre les redirections avec `-L`). +- Ajouter `set -o pipefail`. +- Nettoyer le fichier `stderr` temporaire dans un `trap`. + +--- + +## 5. Problemes transversaux + +### 5.1 Injection JSON dans les notifications Discord + +**Tous les scripts** construisent le payload JSON Discord par interpolation de chaines. C'est le probleme le plus repandu. + +**Pattern actuel (dangereux)** : +```bash +curl -d "{\"content\":\"$msg\"}" "$WEBHOOK_URL" +``` + +**Pattern corrige** : +```bash +jq -n --arg msg "$msg" '{content: $msg}' | curl -d @- ... +``` + +Ou si `jq` n'est pas disponible : +```bash +python3 -c "import sys,json; print(json.dumps({'content': sys.argv[1]}))" "$msg" | curl -d @- ... +``` + +### 5.2 Pas de rotation des sauvegardes + +Aucun script ne gere la purge des anciens backups sur le serveur distant. L'espace disque distant finira par se remplir. + +**Suggestion** : ajouter une commande SSH pour supprimer les backups de plus de N jours : +```bash +ssh "$REMOTE" "find '$REMOTE_DIR' -name '*.dump' -mtime +30 -delete" +``` + +### 5.3 Incoherence de style entre les scripts + +| Aspect | check-storage | backup-vaultwarden | backup-bdd-recette | check-statut-recette | +|--------|--------------|--------------------|--------------------|---------------------| +| `set -euo pipefail` | Non | Oui | Oui | Partiel (`set -u`) | +| Chargement .env | `grep\|cut` | `source` (chemin dur) | `source` (SCRIPT_DIR) | `source` (SCRIPT_DIR) | +| Logging | `echo` | `tee` + fichier | `tee` + fichier | `tee` + fichier | +| Gestion erreurs | Aucune | `fail()` | Granulaire | `log_line()` | +| Shebang | `#!/bin/bash` | `#!/usr/bin/env bash` | `#!/usr/bin/env bash` | `#!/usr/bin/env bash` | + +`check-storage.sh` est clairement en retard sur les conventions adoptees dans les autres scripts. + +### 5.4 README du BackupVaultWarden desynchronise + +Le README de BackupVaultWarden decrit l'ancien code (lecture `.env` avec `grep | cut`) alors que le script actuel utilise `source`. La documentation ne correspond plus au code. + +--- + +## 6. .gitignore + +### Problemes + +Le `.gitignore` est **trop complexe** et redondant. Les regles se contredisent : + +```gitignore +.env +.env.* +!.env.example +!.env.exemple +RecetteScripts/.env # redondant avec .env +CheckStorage/.env # redondant avec .env +``` + +Les regles fonctionnent correctement (les fichiers `.env` ne sont pas commites), mais la redondance rend le fichier difficile a maintenir. + +**Suggestion** : simplifier le `.gitignore` : +```gitignore +# Secrets +.env +!.env.exemple +!.env.example +``` + +--- + +## 7. Resume et priorites + +### Actions immediates (a faire maintenant) + +1. Corriger le chemin `.env` en dur dans `backup-vaultwarden.sh` (ligne 8) +2. Corriger l'injection JSON dans **tous** les scripts (utiliser `jq`) + +### Actions a court terme + +3. Remonter `check-storage.sh` au niveau des autres scripts (set -euo, source .env, SCRIPT_DIR, logging) +4. Remplacer `export PGPASSWORD` par `.pgpass` dans `backup-bdd-recette.sh` +5. Utiliser `pg_dumpall --roles-only` au lieu du simple `SELECT rolname` + +### Actions a moyen terme + +8. Ajouter la rotation des backups distants +9. Ajouter le support HTTPS dans `check-statut-recette.sh` +10. Mettre a jour le README de BackupVaultWarden +11. Simplifier le `.gitignore` + +--- + +*Revue generee par Claude (Opus 4.6) - Co-Authored-By: Claude Opus 4.6 *