diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000..c701728 --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,730 @@ +# Review PR `feat/audit-log` — 4e passe + +> Audit complet de la PR audit-log (89 commits, 233 fichiers, +52k/-876 lignes) apres les 3 passes de review deja mergees. +> Objectif : faire sortir ce qui reste avant merge dans `main`. +> Genere le 2026-04-23. + +**Branche** : `feat/audit-log` +**Base** : `main` +**Revues anterieures** (deja appliquees dans la branche) : +- `bb6a4c3 fix(review) : blockers review PR #9` +- `25cd6a1 fix(review) : regression drawers RBAC + race snapshot + stale-data admin` +- `b1255bb fix(review) : 3e passe review (HIGH frontend + MEDIUMs)` +- `7117744 docs(claude) : refactor CLAUDE.md` + +La branche est globalement solide : les trois miroirs RBAC sont synchronises, le pattern swap-and-clear de l'audit est correctement implemente, la connexion DBAL dediee est bien configuree. Les findings ci-dessous sont incrementaux et ne remettent pas en cause la feature. + +--- + +## Table des matieres + +1. [Securite](#1-securite) +2. [Bugs silencieux](#2-bugs-silencieux) +3. [Violations des regles projet](#3-violations-des-regles-projet) +4. [Incoherences de patterns](#4-incoherences-de-patterns) +5. [Documentation et configuration](#5-documentation-et-configuration) +6. [Frontend et UX](#6-frontend-et-ux) +7. [Bonnes pratiques a retenir](#7-bonnes-pratiques-a-retenir) +8. [Resume par priorite](#8-resume-par-priorite) + +--- + +## 1. Securite + +### 1.1 CRITIQUE — `/api/docs` public en production + +**Fichier** : `config/packages/security.yaml:46` + +```yaml +- { path: ^/api/docs, roles: PUBLIC_ACCESS } +``` + +La documentation Swagger/OpenAPI d'API Platform est accessible sans authentification, quel que soit l'environnement — y compris en production sur `coltura.malio-dev.fr`. Elle expose : + +- la liste complete des endpoints (`/api/audit-logs`, `/api/users/{id}/rbac`, `/api/sites`, etc.) +- les schemas de securite (`is_granted('core.audit_log.view')`) +- les filtres acceptes par chaque provider (y compris `performed_at[after]`) +- la structure des DTOs (`AuditLogOutput`, `UserOutput`...) +- les patterns UUID/IDs + +**Pourquoi c'est grave** : un attaquant a une cartographie gratuite de la surface d'attaque. Pour un CRM interne sur DNS public, c'est une fuite d'information inutile. API Platform genere cette doc automatiquement mais rien n'oblige a la rendre publique. + +**Correction** : fermer en prod. + +```yaml +# config/packages/security.yaml +- { path: ^/login_check, roles: PUBLIC_ACCESS } +- { path: ^/api/version, roles: PUBLIC_ACCESS, methods: [GET] } +- { path: ^/api/modules, roles: PUBLIC_ACCESS, methods: [GET] } +- { path: ^/api/sidebar, roles: PUBLIC_ACCESS, methods: [GET] } +- { path: ^/api, roles: IS_AUTHENTICATED_FULLY } +# supprimer la ligne "- { path: ^/api/docs, roles: PUBLIC_ACCESS }" +``` + +Ou conditionner l'acces au debug mode : + +```yaml +when@prod: + security: + access_control: + - { path: ^/api/docs, roles: IS_AUTHENTICATED_FULLY } +``` + +--- + +### 1.2 CRITIQUE — Aucun en-tete de securite HTTP en production + +**Fichier** : `infra/prod/nginx.conf` et `infra/prod/nginx-proxy.conf` (aucune directive `add_header`) + +Le Nginx de prod n'emet aucun des en-tetes de securite standards : + +| En-tete | Role | Present ? | +|---------|------|-----------| +| `X-Frame-Options: DENY` | anti-clickjacking (pas d'embed iframe) | non | +| `X-Content-Type-Options: nosniff` | anti MIME-sniffing | non | +| `Referrer-Policy` | limite les fuites dans le Referer | non | +| `Content-Security-Policy` | anti-XSS | non | +| `Strict-Transport-Security` | force HTTPS | non | + +Le reverse proxy ecoute uniquement sur le port 80 (HTTP), sans redirection 301 vers HTTPS. Combine avec `JWT_COOKIE_SECURE=1` (defaut dans `.env.prod.example`), le cookie ne serait meme pas envoye en HTTP — donc un premier acces HTTP casse le login silencieusement, l'utilisateur croira que l'auth est buggee. + +**Correction minimale dans `nginx-proxy.conf`** (niveau proxy public) : + +```nginx +server { + listen 80; + listen [::]:80; + server_name coltura.malio-dev.fr; + + # Redirection HTTPS obligatoire (ajouter un server block HTTPS par ailleurs). + # Tant que le TLS n'est pas en place, au minimum poser les en-tetes suivants. + add_header X-Frame-Options "DENY" always; + add_header X-Content-Type-Options "nosniff" always; + add_header Referrer-Policy "strict-origin-when-cross-origin" always; + + # ... reste de la config +} +``` + +Quand TLS est en place, ajouter : + +```nginx +add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always; +``` + +--- + +### 1.3 CRITIQUE — `robots.txt` autorise toute l'indexation + +**Fichier** : `frontend/public/robots.txt` + +``` +User-Agent: * +Disallow: +``` + +La valeur `Disallow:` (vide) signifie "rien n'est interdit" — tous les crawlers peuvent indexer la totalite du site. Pour un outil CRM interne accessible sur un DNS public (`coltura.malio-dev.fr`), c'est un leak inutile : la page de login, les URLs `/admin/*`, les URLs des fiches clients peuvent remonter dans Google. + +**Correction** : + +``` +User-Agent: * +Disallow: / +``` + +--- + +### 1.4 IMPORTANT — `performed_at[after|before]` sans typage DBAL → crash 500 sur date malformee + +**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:182-186` + +```php +if (isset($filters['performed_at_after'])) { + $qb->andWhere('performed_at >= :performed_at_after') + ->setParameter('performed_at_after', $filters['performed_at_after']); +} +``` + +La valeur est passee comme chaine brute a DBAL. La colonne `performed_at` est un `timestamptz`. Si un client envoie `?performed_at[after]=not-a-date`, PostgreSQL leve une erreur de cast et l'API retourne une 500. Pas d'injection SQL (le parametre est bien binde), mais : + +- erreur 500 loguee pour chaque mauvaise entree (pollution des logs + bruit pour l'oncall) +- DoS tres bas effort : un utilisateur avec `core.audit_log.view` peut envoyer des requetes mal formees en boucle +- mauvaise UX : le front recoit une erreur generique au lieu d'un 400 explicite + +**Correction** : + +```php +use Doctrine\DBAL\Types\Types; +use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; + +if (isset($filters['performed_at_after'])) { + try { + $after = new \DateTimeImmutable($filters['performed_at_after']); + } catch (\Throwable) { + throw new BadRequestHttpException('performed_at[after] doit etre une date ISO 8601 valide.'); + } + $qb->andWhere('performed_at >= :performed_at_after') + ->setParameter('performed_at_after', $after, Types::DATETIMETZ_IMMUTABLE); +} +// idem pour performed_at_before +``` + +Cette correction donne en prime un 400 propre avec un message clair. + +--- + +### 1.5 IMPORTANT — Clause `ESCAPE` absente du `ILIKE` (filtre `performed_by`) + +**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:177-180` + +```php +$escaped = str_replace(['\\', '%', '_'], ['\\\\', '\%', '\_'], $filters['performed_by']); +$qb->andWhere('performed_by ILIKE :performed_by') + ->setParameter('performed_by', '%'.$escaped.'%'); +``` + +Le commentaire dit : *"`\` est deja le caractere d'echappement LIKE par defaut en PostgreSQL"*. C'est **inexact**. En SQL-standard PostgreSQL, il n'y a pas de caractere d'echappement par defaut pour LIKE/ILIKE : pour echapper `%` ou `_`, il faut soit `LIKE pattern ESCAPE '\'`, soit utiliser un autre caractere (ex: `ESCAPE '|'`). + +En pratique, sur PostgreSQL avec `standard_conforming_strings=on` (defaut depuis 9.1), `\` n'est PAS interprete par LIKE. Donc `'%\_%'` matche la chaine `%\_%` — pas ce qu'on veut. Le filtre est silencieusement casse pour tout nom contenant `_` ou `%`. + +**Test a faire en psql pour confirmer** : +```sql +SELECT 'admin_backup' ILIKE '%admin\_backup%'; -- t sur PG moderne ? non : f +SELECT 'admin_backup' ILIKE '%admin\_backup%' ESCAPE '\'; -- t +``` + +**Correction** : ajouter explicitement la clause `ESCAPE`. + +```php +$qb->andWhere("performed_by ILIKE :performed_by ESCAPE '\\\\'") + ->setParameter('performed_by', '%'.$escaped.'%'); +``` + +(Les quatre `\` en PHP donnent deux `\` dans le SQL, soit un `\` litteral une fois parse par PostgreSQL.) + +Alternative plus sure : utiliser `position()` au lieu de LIKE. + +```php +$qb->andWhere('position(lower(:performed_by) IN lower(performed_by)) > 0') + ->setParameter('performed_by', $filters['performed_by']); +``` + +--- + +### 1.6 IMPORTANT — `SiteAwareInjectionProcessor` : bypass silencieux si l'appelant n'est pas une instance de `User` + +**Fichier** : `src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessor.php:64-75` + +```php +if (!$this->security->isGranted('sites.bypass_scope')) { + $user = $this->security->getUser(); + $explicitSite = $data->getSite(); + if ($user instanceof User && $explicitSite instanceof Site && !$user->hasSite($explicitSite)) { + throw new AccessDeniedHttpException(...); + } +} +``` + +Si `$user` n'est pas exactement une instance de `App\Module\Core\Domain\Entity\User` (ex: futur provider d'auth tiers, token systeme), la condition `instanceof User` est fausse et la garde cross-site write est **silencieusement sautee**. L'utilisateur peut alors specifier n'importe quel `site` dans le payload sans verification. + +Aujourd'hui le risque est faible (un seul `app_user_provider` configure). Mais le pattern est fragile : une absence de type doit lever une erreur, pas passer. + +**Correction** : transformer le cas "pas un User" en refus explicite. + +```php +if (!$this->security->isGranted('sites.bypass_scope')) { + $user = $this->security->getUser(); + if (!$user instanceof User) { + throw new AccessDeniedHttpException('Utilisateur non reconnu pour la validation de site.'); + } + $explicitSite = $data->getSite(); + if ($explicitSite instanceof Site && !$user->hasSite($explicitSite)) { + throw new AccessDeniedHttpException(...); + } +} +``` + +--- + +### 1.7 IMPORTANT — `isHandlingUnauthorized` sans `try/finally` : flag bloque si `navigateTo` throw + +**Fichier** : `frontend/shared/composables/useApi.ts:25,125-130` + +```typescript +let isHandlingUnauthorized = false // module-level singleton + +// ... +if (!isLoginCheck && !isLogout) { + if (!isHandlingUnauthorized) { + isHandlingUnauthorized = true + auth.clearSession() + await navigateTo('/login') + isHandlingUnauthorized = false // <-- jamais atteint si navigateTo throw + } +} +``` + +Si `navigateTo('/login')` echoue (middleware qui throw, plugin qui throw dans un hook, navigation cancelee par `abortNavigation`), le flag reste `true` **indefiniment**. Toutes les 401 futures sont silencieusement ignorees, l'utilisateur reste sur la page courante avec l'impression que les requetes ne font rien. Le seul remede est un hard-reload. + +**Correction** : `try/finally`. + +```typescript +if (!isLoginCheck && !isLogout) { + if (!isHandlingUnauthorized) { + isHandlingUnauthorized = true + try { + auth.clearSession() + await navigateTo('/login') + } finally { + isHandlingUnauthorized = false + } + } +} +``` + +--- + +### 1.8 IMPORTANT — Pagination maximale absente sur `Permission`, `Role`, `Site` (itemsPerPage 999 cote front) + +**Fichiers** : +- `frontend/modules/core/components/UserRbacDrawer.vue:235,236` +- `frontend/modules/core/components/RoleDrawer.vue:149` +- `frontend/modules/sites/pages/admin/sites.vue:117` + +```typescript +api.get<{ member: Permission[] }>('/permissions', { orphan: false, itemsPerPage: 999 }, ...) +api.get<{ member: Site[] }>('/sites', { itemsPerPage: 999 }, ...) +``` + +Deux problemes cumules : + +1. **`paginationClientItemsPerPage` n'est pas active** sur les resources `Permission`, `Role`, `Site` (seul `AuditLogResource` l'active). API Platform ignore donc `itemsPerPage=999` et retourne 30 elements par defaut. **Le `999` est un no-op**. Aujourd'hui ca marche parce que ces catalogues comptent <30 entrees, mais quand les modules grandiront, les drawers vont silencieusement tronquer. + +2. **Aucun `paginationMaximumItemsPerPage`** n'est pose sur ces ressources. Si un dev decide d'activer `paginationClientItemsPerPage: true` plus tard, `?itemsPerPage=99999` deviendra une requete valide qui pourra faire suer la DB. + +**Correction** : deux options selon l'intention. + +*Option A — Desactiver la pagination pour ces catalogues* (ils sont small + exhaustifs par nature) : + +```php +// Permission.php — GetCollection +new GetCollection( + normalizationContext: ['groups' => ['permission:read']], + security: "...", + paginationEnabled: false, +), +``` + +Cote front, retirer `itemsPerPage: 999` (devient inutile). + +*Option B — Garder la pagination avec un plafond explicite* : + +```php +new GetCollection( + paginationClientItemsPerPage: true, + paginationMaximumItemsPerPage: 200, + // ... +), +``` + +--- + +## 2. Bugs silencieux + +### 2.1 IMPORTANT — `AuditLogDetail.vue` : `JSON.stringify` sans garde sur valeur non-serialisable + +**Fichier** : `frontend/shared/components/audit/AuditLogDetail.vue` (fonction `formatValue`) + +Si une valeur de `changes` est non-serialisable (objet circulaire, symbol, bigint), `JSON.stringify` throw et casse tout le rendu du drawer. Ce cas est theoriquement impossible avec les donnees ecrites par `AuditListener` aujourd'hui, mais un futur enrichissement (ex: serialisation d'un objet metier complexe) peut introduire ce risque. + +**Correction** : wrapper defensif. + +```typescript +function formatValue(value: unknown): string { + if (value === null || value === undefined) return 'vide' + if (typeof value === 'boolean') return value ? t('common.yes') : t('common.no') + if (typeof value === 'object') { + try { return JSON.stringify(value) } catch { return '[valeur non serialisable]' } + } + return String(value) +} +``` + +--- + +### 2.2 MOYEN — `UserRbacProcessor` : payload JSON invalide = regression silencieuse des collections + +**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php:241-248` + +Le processor parse `$request->getContent()` via `json_decode()` pour savoir quelles cles sont absentes du payload et restaurer les collections qu'API Platform aurait ecrasees. Si le body est un JSON invalide (rare mais possible : content-type incorrect, body vide suite a un intercepteur buggue), `json_decode` retourne `null` et la restauration est `return` sans aucun log. + +Consequence : les collections `rbacRoles`, `directPermissions`, `sites` peuvent etre ecrasees par des tableaux vides sans trace. Bug quasi-impossible a diagnostiquer en prod. + +**Correction** : logger `warning` dans ce cas. + +```php +$payload = json_decode($request->getContent(), true); +if (!is_array($payload)) { + $this->logger->warning('UserRbacProcessor : body JSON invalide, skip de restoreAbsentCollections', [ + 'user_id' => $data->getId(), + ]); + return; +} +``` + +--- + +## 3. Violations des regles projet + +### 3.1 MOYEN — ` +``` + +**Regle violee** : `.claude/rules/frontend.md` — *"Tout champ de formulaire / filtre / bouton doit utiliser les composants Malio*"*. + +C'est le seul bouton HTML brut dans la PR. Aucun commentaire `TODO` ne documente une exception. + +**Correction** : utiliser `MalioButton` avec un variant secondaire/link. + +```html + +``` + +Si `MalioButton` ne propose pas de variant "link" adapte, commenter l'exception : + +```html + + +``` + +```html + + +``` + +Si `MalioButton` n'a pas de variant adapte au rendu "lien inline" actuel, garder le `