Merge remote-tracking branch 'origin/fix/code-review' into fix/code-review
# Conflicts: # CODE_REVIEW.md
This commit is contained in:
AkiNoKure
212
CODE_REVIEW.md
Normal file
212
CODE_REVIEW.md
Normal file
@@ -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 <noreply@anthropic.com>*
|
||||
Reference in New Issue
Block a user