# 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