From c257270982533d012601f381ad4bef922240df4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matt=C3=A9o=20Dunoyer?= Date: Tue, 10 Mar 2026 15:14:25 +0000 Subject: [PATCH] Actualiser CODE_REVIEW.md --- CODE_REVIEW.md | 211 ------------------------------------------------- 1 file changed, 211 deletions(-) diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md index 33f8e74..8b13789 100644 --- a/CODE_REVIEW.md +++ b/CODE_REVIEW.md @@ -1,212 +1 @@ -# 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 *