Files
office_translator/docs/CODE_REVIEW.md
2026-03-07 11:42:58 +01:00

188 lines
10 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Revue de code complète — Office Translator
**Date** : Mars 2026
**Périmètre** : Backend FastAPI (Python) + Frontend Next.js (TypeScript/React)
**Méthode** : Exploration du codebase, recherche bonnes pratiques 2025, audits pip-audit / npm audit, analyse ciblée frontend.
---
## 1. Synthèse exécutive
| Priorité | Thème | Impact |
|----------|--------|--------|
| **Critique** | Path traversal sur `/download/{filename}` et `/cleanup/{filename}` | Lecture/suppression de fichiers hors répertoire |
| **Critique** | Vulnérabilités connues (pip + npm) | RCE, DoS, fuites (Next.js, Starlette, Pillow, etc.) |
| **Haute** | Pas de normalisation `session_id` / chemins dans legacy extract/reconstruct | Risque daccès à des fichiers non prévus |
| **Haute** | Token JWT en `localStorage` sans refresh | Expiration = déconnexion brute ; risque XSS |
| **Haute** | CORS `*` si `CORS_ORIGINS` vide | En prod, exposition à toutes origines |
| **Moyenne** | Config dispersée (`os.getenv` partout) | Erreurs de config, pas de validation au démarrage |
| **Moyenne** | Jobs de traduction en mémoire | Pas de partage entre workers (scale horizontal) |
| **Moyenne** | Duplication API_BASE / fetch+token côté frontend | Maintenance et incohérences |
---
## 2. Architecture (rapport agent)
- **Backend** : `main.py` → CORS, middlewares (rate limit, security, error, cleanup), router unique `/api/v1` (translate, auth, admin, api-keys, legacy, glossary, prompt).
- **Auth** : JWT (access 15 min, refresh 7 j), blocklist JTI (Redis/mémoire), admin par token opaque (Redis/mémoire), API Key (X-API-Key, Pro).
- **Traduction** : POST `/translate` → validation fichier/URL → job `tr_*``asyncio.to_thread` + excel/word/pptx_translator + provider (OpenRouter, etc.) ; clés/modèles depuis admin settings + env.
- **Fichiers** : Upload avec nom unique ; download v1 par `job_id` + ownership ; legacy `/download/{filename}` et `/cleanup/{filename}` **sans normalisation du chemin**.
---
## 3. Sécurité
### 3.1 Path traversal (critique)
**Fichiers** : `routes/legacy_routes.py`
- **`GET /api/v1/download/{filename}`** (L.299311) : `file_path = config.OUTPUT_DIR / filename`. Si `filename = "../../../etc/passwd"`, le chemin peut sortir de `OUTPUT_DIR`.
- **`DELETE /api/v1/cleanup/{filename}`** (L.314325) : même construction.
**Recommandation** :
- Normaliser : `safe_name = Path(filename).name` (pas de `..`).
- Résoudre : `resolved = (config.OUTPUT_DIR / safe_name).resolve()`.
- Vérifier : `resolved.is_relative_to(config.OUTPUT_DIR.resolve())` (Python 3.9+) ou équivalent.
- Sinon 400/404.
### 3.2 Session / extract-texts / reconstruct-document
- **`session_id`** (UUID généré côté serveur) : utilisé pour `session_{session_id}.json` et chemins. Vérifier que tout `session_id` reçu en form est bien un UUID strict.
- **`session_data["input_path"]`** : sassurer que le chemin résolu reste sous `UPLOAD_DIR` avant toute lecture.
### 3.3 Authentification
- **JWT** : si `JWT_SECRET_KEY` absent, clé éphémère au démarrage → tous les tokens invalidés au redémarrage (log CRITICAL). À corriger en prod.
- **Admin** : `ADMIN_PASSWORD` en clair à éviter ; utiliser uniquement `ADMIN_PASSWORD_HASH` (bcrypt) en production.
- **Frontend** : token et refresh en `localStorage` → exposés au XSS. Bonnes pratiques 2025 : cookies HTTP-only pour le token (ou au minimum documenter le risque et privilégier des pages à faible exposition XSS).
### 3.4 CORS
- **`main.py`** : si `CORS_ORIGINS` vide ou `*`, `allowed_origins = ["*"]` avec warning. En production, définir des origines explicites.
### 3.5 Stripe webhook
- **`services/payment_service.py`** : `stripe.Webhook.construct_event(payload, sig_header, STRIPE_WEBHOOK_SECRET)`**signature bien vérifiée**. Rien à changer côté vérification.
### 3.6 SSRF / Webhook URL
- **Translate** : `download_from_url` et validation webhook limitent schéma, DNS et IP privées/loopback. Cohérent avec les bonnes pratiques.
---
## 4. Vulnérabilités connues (dépendances)
### 4.1 Backend (pip-audit)
| Package | Version | Problème | Correction |
|---------|---------|----------|------------|
| deep-translator | 1.11.4 | PYSEC-2022-252 | Mettre à jour |
| fastapi | 0.109.0 | PYSEC-2024-38 | 0.109.1+ |
| pillow | 10.2.0 | CVE-2024-28219 | 10.3.0+ |
| python-multipart | 0.0.9 | CVE-2024-53981, CVE-2026-24486 | 0.0.18+ / 0.0.22+ |
| starlette | 0.35.1 | CVE-2024-47874, CVE-2025-54121 | 0.40.0+ / 0.47.2+ |
**Action** : Exécuter `pip-audit` puis mettre à jour les paquets (tests de non-régression après mise à jour).
### 4.2 Frontend (npm audit)
| Package | Problème | Action |
|---------|----------|--------|
| next | RCE (flight), DoS, exposition Server Actions, etc. | `npm audit fix` ou mise à jour ciblée (ex. next@16.1.6) |
| minimatch | ReDoS | `npm audit fix` |
| ajv | ReDoS ($data) | `npm audit fix` |
**Action** : `npm audit` puis `npm audit fix` (ou `--force` avec prudence et tests).
---
## 5. Fiabilité et bonnes pratiques
### 5.1 Gestion derreurs
- Middleware global + handlers dédiés (TranslationError, ValidationError, etc.) → correct.
- OpenRouter : retry 429 + levée dexception en cas déchec (plus de retour silencieux du texte original) → bon.
### 5.2 Configuration
- **Config** : tout en `os.getenv()` dans `config.py` et ailleurs. Risque : valeurs manquantes ou incohérentes au démarrage.
- **Recommandation** : centraliser dans un module type **Pydantic BaseSettings** (validation, typage, valeurs par défaut) et charger une seule fois au démarrage.
### 5.3 Jobs de traduction
- **`_translation_jobs`** en mémoire : en multi-workers (plusieurs processus uvicorn), les jobs ne sont pas partagés.
- **Recommandation** : pour un scale horizontal, stocker létat des jobs dans Redis (ou autre store partagé).
### 5.4 Legacy
- **`/translate-batch`**, **`/download/{filename}`**, **`/cleanup/{filename}`**, **`/extract-texts`**, **`/reconstruct-document`** : peu ou pas dauth, exposition de chemins/fichiers.
- **Recommandation** : renforcer lauth et la validation des paramètres, ou déprécier au profit des endpoints v1 (translate + download par `job_id`).
---
## 6. Frontend (rapport agent)
### 6.1 Types et erreurs
- **`any`** : `app/(app)/ollama-setup/page.tsx` (models, error), `app/(app)/settings/subscription/page.tsx` (PLAN_ICONS), `app/pricing/page.tsx` (PLAN_ICONS). Remplacer par des types précis.
- **Error boundary** : aucun au niveau racine (`app/layout.tsx`). Ajouter un ErrorBoundary pour éviter des écrans blancs.
### 6.2 Validation et timeout
- **Login / Admin login** : pas de validation explicite (email, mot de passe) avant submit.
- **Translate** : pas de validation centralisée de `file` / `config` (taille, type, champs requis) avant lappel API.
- **Fetch** : plusieurs appels sans timeout (pricing, subscription, admin verify, settings, translate, polling, download). Ajouter `AbortController` + timeout (ex. 830 s selon lendpoint).
### 6.3 Auth et stockage
- **Token** : `localStorage` ("token", "refresh_token", "user"). Risque XSS ; en production privilégier cookies HTTP-only si possible.
- **Refresh** : `refresh_token` stocké mais **jamais utilisé** ; aucun appel `/auth/refresh`. Quand laccess token expire, lutilisateur est déconnecté. Implémenter un refresh automatique (intercepteur ou hook) pour améliorer lUX.
### 6.4 Duplication et centralisation
- **API_BASE** : répété dans plusieurs fichiers (useTranslationSubmit, useTranslationConfig, TranslationComplete, settings/services, lib/api). Utiliser une seule source : `@/lib/config` ou `apiClient`.
- **Pattern fetch + token** : répété partout. Utiliser systématiquement **apiClient** (déjà en place pour auth, glossaries, api-keys) pour tous les appels authentifiés.
- **URL en dur** : `app/(app)/ollama-setup/page.tsx` L.85 `"http://localhost:8000/api/auth/settings"` → remplacer par `API_BASE + "/api/v1/..."` (et vérifier le chemin exact de lAPI).
---
## 7. Plan daction priorisé
### Immédiat (sécurité)
1. **Path traversal** : normaliser `filename` et vérifier `resolved.is_relative_to(OUTPUT_DIR)` dans `legacy_routes.py` pour `/download/{filename}` et `/cleanup/{filename}`.
2. **Dépendances** : mettre à jour FastAPI, Starlette, Pillow, python-multipart, deep-translator ; puis Next.js, minimatch, ajv (avec tests après chaque vague).
### Court terme (sécurité / robustesse)
3. **Session / reconstruct** : valider `session_id` (UUID) et sassurer que tout chemin dérivé reste sous `UPLOAD_DIR`.
4. **CORS** : en prod, définir `CORS_ORIGINS` explicitement (pas `*`).
5. **Admin** : en prod, nutiliser que `ADMIN_PASSWORD_HASH` (bcrypt).
6. **Frontend** : ajouter un ErrorBoundary racine et des timeouts sur les fetch critiques (translate, download, polling, auth verify).
### Moyen terme (qualité / maintenabilité)
7. **Config** : migrer vers Pydantic BaseSettings pour la config backend.
8. **Frontend** : centraliser API_BASE et utiliser apiClient partout ; implémenter le refresh token.
9. **Jobs** : si scale horizontal prévu, stocker létat des jobs dans Redis (ou équivalent).
10. **Legacy** : protéger ou déprécier les routes legacy (auth + validation des paramètres).
### Long terme (bonnes pratiques)
11. **Auth frontend** : étudier le passage à des cookies HTTP-only pour le token (avec adaptation du backend).
12. **Validation** : validation explicite login/admin et validation centralisée des paramètres de traduction avant appel API.
13. **Types** : supprimer les `any` et renforcer les types (discriminated unions, type predicates) côté TypeScript.
---
## 8. Références
- FastAPI & Next.js security (TurboStarter, Next.js Data Security, Mikul Gohil).
- Python code review (DeepSource, Kodus, Medium).
- TypeScript security & quality (Krython, Kodus).
- Exploration et revue frontend par agents (explore + generalPurpose) sur le dépôt.
---
*Document généré dans le cadre dune revue de code complète. Aucune modification na été appliquée automatiquement ; les changements sont à planifier et à tester.*