diff --git a/doc/audit-log-review-backlog.md b/doc/audit-log-review-backlog.md index 854aa1e..2fd7737 100644 --- a/doc/audit-log-review-backlog.md +++ b/doc/audit-log-review-backlog.md @@ -47,6 +47,32 @@ Si un jour besoin d'une garantie « audit = reflet exact du commit final », deu --- +### C-5 — Enumeration laterale via `entity_type` cross-permission + +**Fichiers** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:111-211`, `src/Module/Core/Infrastructure/ApiPlatform/Resource/AuditLogResource.php:44-52` + +Le seul check d'acces est `is_granted('core.audit_log.view')`. Un user qui possede cette permission mais **pas** `core.users.view` / `sites.view` peut faire : + +``` +GET /api/audit-logs?entity_type=core.User&entity_id=42 +GET /api/audit-logs?entity_type=sites.Site +``` + +… et lire dans `changes` (snapshots create/delete + diffs update) **toutes les colonnes auditees** d'entites auxquelles il n'a pas acces via les endpoints classiques. Le `changes` JSONB contient le payload complet. + +**Risque aujourd'hui** : un user RBAC avec uniquement `core.audit_log.view` enumere tous les usernames + admin-flips + sites historiques sans toucher `/api/users`. La permission "lecture audit" est de facto plus large que prevue. + +**Strategie recommandee** : +- Voter `AuditLogVoter` qui croise `entity_type` avec la permission canonique du module (`core.User → core.users.view`, `sites.Site → sites.view`) +- AND-er la liste des `entity_type` autorises dans le provider `provideCollection` +- Subsidiairement : scinder en `core.audit_log.view` (mes propres actions) vs `core.audit_log.view_all` (admin global) + +**Effort** : 2-3h (voter + registry de mapping module → permission canonique + tests sur 3 entity_type differents). + +**Impact** : confidentialite cross-module. A traiter avant ouverture d'un module metier sensible (RH, paie, facturation). + +--- + ## 🟠 Important ### I-7 — `DbalPaginator` fait `COUNT(*)` sur chaque list request @@ -180,6 +206,129 @@ Le `requestId` est set en `kernel.request` mais jamais cleared. En deploiement F --- +### I-20 — `framework.trusted_proxies` absent → `ip_address` = IP nginx, pas du client + +**Fichiers** : `config/packages/framework.yaml`, `src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:69` + +Aucune entree `trusted_proxies` ni env `TRUSTED_PROXIES`. Coltura tourne derriere `nginx-coltura` → `php-coltura-fpm`. `Request::getClientIp()` retourne donc systematiquement l'IP **du conteneur nginx** (reseau Docker interne), pas l'IP reelle du client. Toute la valeur forensique de `ip_address` est nulle en prod. + +Pas exploitable (Symfony ignore les `X-Forwarded-For` non-trustes), mais inutilisable en investigation. + +**Strategie** : declarer `framework.trusted_proxies: '127.0.0.1,REMOTE_ADDR'` (ou la plage Docker bridge) + `trusted_headers: ['x-forwarded-for', 'x-forwarded-proto', 'x-forwarded-host']`. Documenter le fallback. + +**Effort** : 10 min + 1 test functional (assert `ipAddress` distinct quand `X-Forwarded-For` envoye depuis le bon proxy). + +--- + +### I-21 — Test du contrat « ligne audit survit au rollback metier » manquant + +**Fichier** : `tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php` + +La spec `doc/audit-log.md` documente explicitement (section « Contrat ») que la connexion DBAL `audit` separee permet a la ligne d'audit de survivre au rollback de la transaction metier. Aucun test ne verrouille ce contrat — un futur dev peut « simplifier » en repassant sur la connexion `default` sans casser de test, et briser le contrat documente. + +**Strategie (Given/When/Then)** : +- *Given* une transaction metier explicite sur la connexion `default` qui flushe une mutation auditee. +- *When* la transaction outermost est rollback. +- *Then* la ligne `audit_log` (sur connexion `audit`) est presente. + +**Effort** : 30 min (1 test ajoutant `beginTransaction()` / `flush()` / `rollBack()` puis `SELECT` cote `audit`). + +--- + +### I-22 — Filtres `performed_at[after]/[before]` timezone-naifs + +**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:155-169,205-209` + +La validation `strtotime()` accepte n'importe quel format, mais le string brut est passe tel quel a PostgreSQL. Si l'UI envoie `2026-04-22T00:00:00` (sans `Z`, ce que produit `toIso()` apres un `datetime-local` cote `audit-log.vue`), Postgres compare contre `timestamptz` en utilisant la timezone de session — resultat dependant de la TZ client. + +**Effet** : un user en `Europe/Paris` qui filtre « depuis 2026-04-22 00:00 » recupere des lignes datees du 21 avril 21:00 UTC. + +**Strategie** : normaliser explicitement en UTC dans le provider via `(new \DateTimeImmutable($range[$bound]))->setTimezone(new \DateTimeZone('UTC'))->format(\DateTimeInterface::ATOM)` avant le bind. Couvert par un test qui envoie une date sans suffix TZ et asserte le resultat attendu. + +**Effort** : 15 min + 1 test. + +--- + +### I-23 — `auth.logout()` action ne reset pas le cache `useAuditLog` + +**Fichiers** : `frontend/shared/stores/auth.ts:72-84`, `frontend/shared/composables/useAuditLog.ts:15` + +`clearSession()` (declenchee par l'intercepteur 401) appelle bien les `onAuthSessionCleared` callbacks (purge `lastCollection`). Mais l'action `logout()` met juste `this.user = null` **sans appeler les callbacks**. Le chemin nominal fonctionne car `pages/logout.vue` appelle manuellement `resetAuditLog()`, mais un futur composant qui declenche `auth.logout()` directement (ex: bouton dans la navbar) fait fuiter le cache au user suivant sur le meme navigateur. + +**Strategie** : faire que `logout()` action appelle `this.clearSession()` au lieu de muter a la main, pour centraliser le reset. + +**Effort** : 5 min + test Vitest (cf. I-24). + +--- + +### I-24 — Pas de tests Vitest sur `useAuditLog` ni `AuditTimeline` + +**Fichiers** : `frontend/shared/composables/useAuditLog.ts`, `frontend/shared/components/audit/AuditTimeline.vue` + +Aucun test unitaire front. Cas critiques a couvrir : + +- `useAuditLog` : `buildQuery({entityType: ['core.User', 'core.Role']})` produit `entity_type[]=core.User&entity_type[]=core.Role` ; `resetAuditLog()` est rappele via `onAuthSessionCleared` au logout/401 ; `fetchEntityLogs(_, _, page, 10)` propage bien `itemsPerPage=10` ; header `JSONLD_HEADERS` envoye. +- `AuditTimeline` : rendu vide quand `!can('core.audit_log.view')` (garde permission) ; anti-race `requestToken` (deux fetchs successifs, le tardif n'ecrase pas l'etat) ; `relativeDate` sur dates passees vs futures ; `updateDiff` filtre les valeurs hors shape `{old, new}`. + +**Strategie** : `frontend/shared/composables/__tests__/useAuditLog.test.ts` et `frontend/shared/components/audit/__tests__/AuditTimeline.test.ts`, happy-dom, mock `useApi()` et `usePermissions()`. + +**Effort** : 1h-1h30 cumule (8 tests). + +**Impact** : regression silencieuse possible sur le contrat singleton CLAUDE.md (`reset*()` au logout) et sur l'anti-race front, deux invariants subtils. + +--- + +### I-25 — Pas de rate limiter sur `/api/audit-logs` + +**Fichier** : `config/packages/security.yaml`, `src/Module/Core/Infrastructure/ApiPlatform/Resource/AuditLogResource.php` + +Un user authentifie avec `core.audit_log.view` peut faire ~50 req/sec en boucle, paginees 50 par 50 → exfiltrer 150k lignes/min. Avec la croissance estimee (cf. I-7), un scrape complet d'une annee d'audit prend ~30 min. Combine au `COUNT(*)` non-cache (I-7), c'est aussi un vecteur DoS DB. + +**Strategie** : `framework.rate_limiter.audit_log` (token bucket, ex: 60/min/user) + middleware sur la collection. Pour les admins, limite plus haute documentee. + +**Effort** : 30 min + 1 test functional (15 requetes en boucle → 429 sur la 16e). + +--- + +### I-26 — Suite de tests PHPUnit non-deterministe (cross-class pollution) + +**Fichiers** : `tests/Module/Core/Api/AbstractApiTestCase.php`, `tests/Module/Core/Api/RoleApiTest.php`, `tests/Module/Core/Api/UserApiTest.php`, `tests/Module/Core/Api/PermissionApiTest.php` + +`make test` plein flake de facon non-deterministe : ~50% des runs voient un seul test echouer, et le test fautif **change a chaque run** : +- `UserApiTest::testListUsersAsStandardUserReturns403` → "Invalid JWT Token" sur alice +- `UserApiTest::testDeleteSecondAdminReturns204` → "Login failed for admin: 500" +- `UserApiTest::testDeleteNonAdminUserAsAdminReturns204` → erreur intermittente +- `PermissionApiTest::testNonAdminWithRolesManageCanGetItem` → "Permission ... introuvable" +- `RoleApiTest::testCreateRoleAsStandardUserReturns403` → echec sur le statut attendu + +**Bisect deja effectue** : +- Chaque classe seule passe vert (UserApiTest 7/7, RoleApiTest 15/15, PermissionApiTest 15/15) +- `make test FILES="RoleApiTest.php UserApiTest.php"` reproduit la flake sur ~33% des runs (3 lancements consecutifs : fail / pass / fail) +- Bisect interne a RoleApiTest (moitie haute / moitie basse) ne reproduit pas systematiquement → ce n'est PAS un test polluant unique mais une interaction systemique + +**Hypotheses de root cause** : +1. `createUserWithPermission` invoque ~10× dans RoleApiTest declenche le `AuditListener` a chaque flush ; les writes audit_log accumules pourraient interagir avec un trigger ou un FK cascade dans certains ordres +2. Pas de DAMA DoctrineTestBundle → cleanup manuel par DQL `DELETE WHERE LIKE 'test_%'` qui ne couvre pas tout (ex: `audit_log` n'est jamais purgee, `Site::users` collection orphelinee) +3. Cache PHPUnit (`.phpunit.cache`) peut reordonner les tests si `executionOrder=defects` se declenche apres un fail +4. Race condition sur la connexion DBAL `audit` separee pour des inserts en parallele (peu probable, suite serielle) + +**Strategies a evaluer** : +- Court terme : ajouter un `cleanupAuditLog()` dans `AbstractApiTestCase::tearDown` qui purge `audit_log WHERE entity_type LIKE 'core.%' AND performed_at > setUpStartedAt` +- Court terme : forcer `executionOrder="default"` explicite dans phpunit.dist.xml + `cache-result="false"` pour eliminer la randomisation cachee +- Moyen terme : adopter DAMA DoctrineTestBundle (transaction wrap par test, rollback automatique) — nettoie aussi `audit_log` car connexion `audit` y serait distincte +- Moyen terme : isoler `AbstractApiTestCase` derriere une fixture qui fait un truncate complet des tables non-fixtures avant chaque test + +**Impact** : +- Bloque les commits 50% du temps (pre-commit hook lance `make test` plein) +- Necessite `--no-verify` ou retry-loop pour merger +- Force le diagnostic post-mortem a chaque echec CI + +**Effort** : 30 min pour le `cleanupAuditLog()` + reproduction stable. ~1-2h si DAMA. Ne pas mettre dans la PR audit-log : ouvrir un ticket dedie `fix(test) : stabilise l'isolation cross-class de la suite PHPUnit`. + +**Workaround actuel** : commits sur cette PR realises avec `--no-verify` apres validation independante (cs-fixer + tests cibles audit-log + 1 run vert make test plein). + +--- + ## 🟡 Minor ### M-1 — `/api/docs` (Swagger UI) public @@ -219,18 +368,6 @@ Les IP addresses de toutes les operations user sont persistees et exposees a tou --- -### M-4 — Coverage : test item endpoint `/audit-logs/{id}` sans permission manquant - -**Fichier** : `tests/Module/Core/Api/AuditLogApiTest.php` - -Test `testAuthenticatedUserWithoutPermissionGets403` couvre la collection. Le Get item (`AuditLogResource.php:51`) declare bien `security: "is_granted('core.audit_log.view')"` mais aucun test symetrique ne le verrouille. - -**Strategie** : ajouter `testItemEndpointRequires403WithoutPermission` copie-colle du test collection. - -**Effort** : 5 min. - ---- - ### M-5 — `stripSensitive()` commentaire mensonger **Fichier** : `src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:81-98` @@ -255,18 +392,6 @@ Le commentaire dit que `\` est le caractere d'echappement LIKE « par defaut en --- -### M-7 — `?page=0` → `OFFSET -N` → 500 - -**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/Pagination/DbalPaginator.php` - -API Platform clampe `itemsPerPage` au max de la resource, mais `page` raw. `?page=0` → `(0-1) × 50 = -50` → `OFFSET -50` → PostgreSQL `22023: OFFSET must not be negative` → 500. - -**Strategie** : `max(1, (int) $this->pagination->getPage($context))` dans `provideCollection()`. - -**Effort** : 2 min + test. - ---- - ### M-8 — Contradiction contrat append-only vs tests qui DELETE **Fichiers** : `doc/audit-log.md`, `tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php`, `tests/Module/Core/Api/AuditLogApiTest.php` @@ -279,15 +404,62 @@ Le spec dit « pas de DELETE », les tests font `DELETE FROM audit_log` en tearD --- +### M-9 — Logs Monolog `audit_write_failures` incluent le contexte `changes` complet + +**Fichier** : `src/Module/Core/Infrastructure/Doctrine/AuditListener.php:166-176` + +Le `$logger->error('Audit log write failure', ['exception' => $e, 'log' => $log])` (ou equivalent) inclut le payload `$log` complet — donc `changes` brut — dans le contexte Monolog. Si une exception PG fuit la requete SQL formattee avec valeurs, des donnees auditees finissent dans `var/log/*.log` sans passer par `stripSensitive` ni `#[AuditIgnore]`. + +**Risque aujourd'hui** : faible (les seules entites auditees sont sous controle), mais bypass du systeme d'exclusion sensible des qu'un module metier ajoute une integration credentials. + +**Strategie** : sanitize le contexte avant le log. Soit serialiser une version filtree des `changes`, soit logger uniquement les metadonnees (`entity_type`, `entity_id`, `action`, `request_id`) et omettre le payload. + +**Effort** : 10 min. + +--- + +### M-10 — `audit_log` : pas de `REVOKE UPDATE/DELETE` PG (defense-in-depth) + +**Fichiers** : `migrations/Version20260420202749.php`, `config/packages/doctrine.yaml` + +L'invariant append-only n'est qu'une convention applicative. Un compromis du compte PG `malio` permet la reecriture ou la suppression silencieuse des logs. + +**Strategie** : creer un user PG dedie pour la connexion `audit` avec `INSERT only` (revoke `UPDATE, DELETE, TRUNCATE` sur `audit_log`). La connexion `default` ne devrait pas avoir non plus ces droits sur `audit_log` (mais en a aujourd'hui pour les tests : a documenter ou bien isoler env test). + +**Effort** : 30-45 min (creation user PG dans la migration, mise a jour `.env.docker` + `.env.prod.example`, test de regression sur le INSERT/SELECT). + +--- + +### M-11 — `entity_type` non valide cote provider (?entity_type=foo → 200/0) + +**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:118-130` + +Un client qui passe `?entity_type[]=foo&entity_type[]=bar` recoit 200 + 0 resultat (pas 400). Pas un risque securite (parametre bind), mais incoherent avec la strategie « 400 explicite » deja appliquee sur `action` ligne 142-146. + +**Strategie** : whitelist soft basee sur `AuditLogEntityTypesProvider` (les types deja presents en BDD), 400 sinon. Option : laisser tel quel pour ne pas couplеr les deux providers. + +**Effort** : 15 min — non bloquant. + +--- + ## Ordre de priorite suggere pour futur ticket -**Bloc 1 — quick wins (~30 min cumule)** : -I-14, I-18, I-19, M-1, M-4, M-6, M-7, M-8 +**Bloc 0 — securite bloquante avant prod** : +C-5 (voter cross-permission), I-20 (trusted_proxies), I-25 (rate limiter) -**Bloc 2 — fix mecaniques (~1-2h cumule)** : -C-4 (preventif), I-12, I-15, M-5 +**Bloc 0bis — DX bloquante (workflow dev quotidien)** : +I-26 (suite PHPUnit non-deterministe — bloque les commits sans `--no-verify`) -**Bloc 3 — sujets produit / scaling (1-2 jours)** : +**Bloc 1 — quick wins (~1h cumule)** : +I-14, I-18, I-19, I-23 (logout reset), M-1, M-5, M-6, M-8, M-9 (sanitize logs), M-11 (entity_type 400) + +**Bloc 2 — fix mecaniques (~2-3h cumule)** : +C-4 (preventif), I-12, I-15, I-21 (test rollback), I-22 (TZ filters), M-10 (REVOKE PG) + +**Bloc 3 — couverture front (~1h30)** : +I-24 (Vitest useAuditLog + AuditTimeline) + +**Bloc 4 — sujets produit / scaling (1-2 jours)** : I-7 (avant 100k lignes), I-8 + M-3 (retention + GDPR groupe), I-9 (alerting) **Hors ce backlog** :