Files
Coltura/doc/audit-log-review-backlog.md
matthieu e6c8381b3c
Some checks failed
Auto Tag Develop / tag (push) Successful in 6s
Build & Push Docker Image / build (push) Failing after 9s
feat : audit log (table + writer + listener + API + admin UI + timeline) (#9)
## Résumé

Implémente le journal d'audit append-only couvrant les 5 tickets de `doc/audit-log.md` et embarque au passage plusieurs corrections périphériques (sidebar Admin/Mon compte, drawer RBAC, Swagger, schema_filter Doctrine) ainsi que l'initialisation de la suite e2e Playwright. Toutes les mutations Doctrine sur les entités portant `#[Auditable]` sont tracées dans une table PostgreSQL dédiée, exposée en lecture seule via API Platform et consultable par les admins dans une page dédiée.

## Ce qui change

### Audit log — cœur de la PR

**Backend**

- Migration : table `audit_log` (UUID v7 natif Postgres en PK, `jsonb changes`, 3 index pour tri chrono, par entité et par utilisateur).
- `AuditLogWriter` : service bas-niveau, écrit via une connexion DBAL dédiée `audit` (même DSN que `default`, service séparé) pour sortir de la transaction ORM en batch. Blacklist defense-in-depth `password`/`plainPassword`/`token`/`secret`.
- `RequestIdProvider` : UUID v4 généré au `kernel.request` principal, injecté dans chaque ligne d'audit de la requête.
- Attributs `#[Auditable]` / `#[AuditIgnore]` dans `src/Shared/Domain/Attribute/` (accessibles par tous les modules).
- `AuditListener` : capture `onFlush` / écriture `postFlush` avec pattern swap-and-clear contre les flushes ré-entrants. Erreurs loguées, jamais propagées. Entité `User` annotée (password / plainPassword ignorés).
- API Platform read-only `/api/audit-logs` (permission RBAC `core.audit_log.view`) : `GET` collection paginée + `GET` item, pas de POST/PUT/PATCH/DELETE. Filtres `entity_type`, `entity_id`, `action`, `performed_by`, `performed_at[after]`/`[before]`.
- `DbalPaginator` implémentant `PaginatorInterface` : `hydra:view` généré automatiquement par API Platform, pas de construction manuelle.
- Ressource `AuditLogEntityTypesResource` + provider dédié pour peupler le filtre par type d'entité côté UI (réponse cachée, pas de requête à chaque ouverture du drawer).
- Permission `core.audit_log.view` déclarée dans `CoreModule::permissions()`.
- `audit_log` exclu du `schema_filter` Doctrine : plus de faux diff sur `make migration-diff`.

**Frontend**

- Page admin `/admin/audit-log` : tableau paginé, filtres locaux (état dans le composant, non persistés dans l'URL — conforme règle CLAUDE.md « Tableaux : pas de persistance URL »), drawer de détail (diff + timeline complète de l'entité), badges colorés par action.
- Composable partagé `useAuditLog` avec `resetAuditLog()` auto-enregistré sur `onAuthSessionCleared` (règle CLAUDE.md composables singletons).
- Composant réutilisable `<AuditTimeline :entity-type :entity-id>` : garde permission (pas d'appel API sans le droit), lazy loading (10 items + bouton « Voir plus »), dates relatives FR via `Intl.RelativeTimeFormat`, skeleton loader.
- Entrée sidebar « Journal d'audit » gated sur `core.audit_log.view` + clés i18n imbriquées dans `fr.json`.

### Fixes embarqués

- **Review fixes audit-log** (commits `37eafd2`, `1505e84`, `99c77eb`) : précision des timestamps, `ESCAPE` sur les `LIKE`, plafond pagination, diverses remarques du 1er tour de review.
- **Sidebar** (`701a480`, `e2fbf51`) : nouvelle section « Administration » + groupe « Mon compte », gate de section sur permissions, « Tableau de bord » déplacé dans « Mon compte ». Convention admin documentée.
- **Drawer RBAC utilisateurs** (`617ee31`, `5f5afcc`) : corrige l'affichage des sites et l'écrasement via merge-patch (garde anti-écrasement + spec `GET /users/{id}/rbac` documentée).
- **Swagger UI** (`6db955f`) : réactivé en ajoutant `symfony/twig-bundle` aux deps (régression depuis l'arrivée d'API Platform 4.2).
- **`phpunit.dist.xml`** : `<env APP_ENV=dev>` forçait la suite à tourner sous `framework.test=false` (→ `test.service_container` introuvable) ; `JWT_PASSPHRASE` ne matchait pas les clés de dev. Corrigés pour débloquer la suite.

### E2E Playwright (nouveau, commit `4603ab2`)

- `playwright.config.ts` + structure `frontend/tests/e2e/` (personas, helpers `loginAs`, page objects `LoginPage` + `SidebarComponent`).
- Specs : `auth/login.spec.ts` + `permissions/sidebar-visibility.spec.ts` (vérifie la visibilité de la sidebar par rôle RBAC).
- Commande `SeedE2ECommand` pour préparer un jeu de données déterministe côté backend.
- `make e2e` ajouté au Makefile.

## Décisions techniques

- **UUID v7 natif Postgres** (16 octets vs 36 en varchar) : index `performed_at` ~40 % plus petit sur une table append-only à croissance infinie.
- **`entity_type` format `module.Entity`** (ex: `core.User`) : évite les collisions si deux modules ont des entités de même nom.
- **`performed_by` dénormalisé** (string, pas FK) : le nom persiste même après suppression de l'utilisateur.
- **Connexion DBAL dédiée `audit`** : évite l'entanglement transactionnel entre audit et ORM en batch.
- **`ManyToMany` non audité** : limitation connue (`getEntityChangeSet()` ne couvre pas les collections) ; extension future via `getScheduledCollectionUpdates()` si besoin.
- **Filtres locaux non persistés dans l'URL** : choix assumé (cf. CLAUDE.md) pour éviter le couplage table ↔ routeur.

## Test plan

- [x] `make test` : 218 tests passent (writer unitaires + listener intégration + API fonctionnels + UserRbacProcessor).
- [x] `npm run lint` + `npm run test` + `npm run build` (frontend).
- [x] Migration appliquée sur dev + test, `audit_log` ignoré par `schema_filter`.
- [x] Permissions synchronisées (`app:sync-permissions`).
- [x] Swagger `/api/docs` accessible de nouveau.
- [ ] Playwright : `make e2e` vert en local (login + sidebar-visibility).
- [ ] Vérifier en local : création/modif/suppression d'un user apparaît dans `/admin/audit-log`.
- [ ] Vérifier : user sans `core.audit_log.view` → 403 sur l'endpoint + item absent de la sidebar.
- [ ] Vérifier : expansion d'une ligne affiche la timeline de l'entité avec dates relatives FR.
- [ ] Vérifier : drawer RBAC utilisateur n'écrase plus la liste des sites au `PATCH`.

## Points d'attention pour le review

- `AuditListener` : pattern swap-and-clear sur `postFlush` — relire la gestion des flushes ré-entrants.
- `DbalPaginator` : vérifier que l'absence d'`Iterator` custom ne casse pas la normalisation API Platform sur collections vides.
- `UserRbacProcessor` : logique merge-patch + garde anti-écrasement des sites (régression corrigée dans `617ee31`).
- Playwright : nouvelle dépendance de dev, s'assurer que `make e2e` ne fait pas partie du pipeline CI par défaut (à brancher explicitement).

Co-authored-by: Matthieu <mtholot19@gmail.com>
Reviewed-on: #9
Co-authored-by: matthieu <matthieu@yuno.malio.fr>
Co-committed-by: matthieu <matthieu@yuno.malio.fr>
2026-05-13 08:29:30 +00:00

27 KiB
Raw Blame History

Backlog — Code review PR #9 (audit-log)

Findings du review multi-agent (security + architecture + codex sceptique) sur la PR #9 feat/audit-log, qui n'ont pas ete traites dans la PR et sont a ouvrir en tickets dedies.

Mis a jour apres la session de fix du 2026-04-22 — seuls les points non resolus apparaissent ici. Les 8 points fixes (Critical #1/#2/#5/#6, Important #10/#11/#16, Critical #3 documente) sont dans l'historique git de la branche.

Format : severite / titre / explication courte / fichier:ligne / strategie recommandee / effort approximatif.


🔴 Critical parke

C-4 — Blacklist password exact-match et non recursive

Fichier : src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:35,81-98

La blacklist ['password', 'plainPassword', 'token', 'secret'] est en match exact top-level. Trois trous :

  • Rate les variations de nommage (apiToken, accessToken, clientSecret, passwordHash, mfaSecret, webhookSecret, csrfToken, etc.)
  • Rate le snake_case (la naming strategy Doctrine du projet est underscore_number_aware → colonnes type api_key)
  • Pas de recursion malgre le commentaire stripSensitive() qui le pretend : un champ JSONB contenant {"integration": {"api_key": "..."}} fuite en clair

Risque aujourd'hui : nul — seule entite #[Auditable] actuelle est User, et les deux champs sensibles (password, plainPassword) sont correctement annotes #[AuditIgnore]. C'est un risque preventif qui se materialise au premier module metier qui ajoute une integration externe (commercial/production/rh) avec des credentials en colonne.

Strategie recommandee (Option B du brainstorm) :

  • Supprimer la blacklist (fausse securite)
  • Faire de #[AuditIgnore] la seule defense
  • Ajouter un test CI : parcourt toutes les entites #[Auditable] via reflection, liste les proprietes dont le nom matche /token|secret|password|key|salt|hash|passphrase/i, assert que chacune porte #[AuditIgnore]

Effort : 15-20 min.


🔴 Critical documente mais pas fixe code

C-3 — Savepoints + connexion audit dediee = lignes audit orphelines

Fichiers : src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:19-30, config/packages/doctrine.yaml:3-23

Le contrat est documente dans doc/audit-log.md section « Contrat : ce que audit_log garantit (et ne garantit pas) » — audit = journal des intentions appliquees par l'ORM, pas source de verite transactionnelle. Acceptable pour un CRM interne (rollbacks outermost rares).

Si un jour besoin d'une garantie « audit = reflet exact du commit final », deux options :

  • Option B : differer l'ecriture audit jusqu'au commit outermost (ecoute Events::transactionCommit, buffer cross-flush). Complexe, distinguer RELEASE SAVEPOINT d'un vrai COMMIT.
  • Option C : ecrire l'audit sur la meme connexion que le metier. Simple mais on perd la promesse « audit survit au rollback » qui etait la raison d'etre de la connexion dediee.

Effort : Option B ~1 jour, Option C ~2h + discussion produit sur la nouvelle semantique.


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

Fichier : src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:75-99

PostgreSQL (MVCC) doit reellement scanner pour COUNT(*) — pas O(1) comme MySQL MyISAM. Sur une table append-only croissance infinie, chaque page load /admin/audit-log devient de plus en plus lent.

Estimation : ~10k lignes/jour (50 users × 200 actions) → 3.65M lignes/an → page load de 20ms aujourd'hui, ~2-3s dans 2 ans. Pire avec un filtre ILIKE (wildcard leading = full scan).

Strategie recommandee (pragmatique) :

  • Pagination par curseur (keyset) basee sur (performed_at, id) decroissant
  • UI : remplacer le paginateur numerique par « precedent / suivant » + bouton explicite « voir le total »
  • Backend supporte keyset via API Platform 4 (hydra:next/previous)

Alternative rapide : estimation via pg_class.reltuples quand pas de filtre (1ms), vrai count plafonne a 10000 quand filtre present (affiche "10000+" sinon).

Effort : keyset complet ~1h30-2h, version pragmatique ~30 min.

Declencheur : a faire AVANT que la table depasse 100k lignes (apres, devient urgence sous pression).


I-8 — Pas de politique de retention / archival sur audit_log

Fichiers : migrations/Version20260420202749.php, doc/audit-log.md

La migration elle-meme decrit la table comme « croissance infinie ». Aucune TTL, archive job, ou partitioning documente. Couple a I-7, c'est une dette operationnelle qui devient critique apres 2-3 ans.

Options :

  • Retention simple : cron mensuel DELETE FROM audit_log WHERE performed_at < NOW() - INTERVAL '2 years' (requiert accord legal/compliance sur la duree)
  • Archival vers un bucket S3/cold storage : commande Symfony exportant en JSONL puis purge
  • Partitioning PostgreSQL par mois/trimestre : audit_log_2026_q1, audit_log_2026_q2, ... drop partition apres N mois

Effort : depend du choix. Retention simple ~2h. Archival ~1 jour. Partitioning ~1-2 jours + migration progressive.

Decision produit requise avant implementation.


I-9 — Echec DB audit silencieusement swallowed, pas d'alerting

Fichier : src/Module/Core/Infrastructure/Doctrine/AuditListener.php:151-169

Le try/catch swallow toute exception de AuditLogWriter::log(), log au niveau error dans Monolog, et continue. Si la connexion audit tombe (pool sature, disque plein, etc.), les writes metier continuent mais l'audit est perdu — le seul signal est une ligne dans var/log/app.log.

Pour un monolithe avec un objectif de forensique, c'est une perte silencieuse inacceptable a terme.

Strategie recommandee :

  • Compter les echecs via une metrique (Prometheus counter audit_write_failures_total)
  • Alerter si la metrique depasse un seuil (ex: > 5 echecs sur 10 min)
  • Option : table audit_log_failures locale qui stocke les payloads ratas pour retry manuel / forensique post-mortem

Effort : 20 min pour la metrique, +1h si dead-letter table.


I-12 — ensureCurrentSiteConsistency : 2e flush attribue a l'admin qui PATCH

Fichier : src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php:197-216

La methode declenche un 2eme flush dans la meme transaction pour auto-corriger currentSite si necessaire. Ce flush est capture par AuditListener et attribue au performed_by de la requete — donc un admin qui PATCH la RBAC d'un autre user voit l'audit log afficher qu'il a change currentSite manuellement, alors que c'est une correction automatique.

Strategie : marquer le flush comme « system-initiated » via un flag qui court dans un contexte local (ex: AsyncLocal, ParameterBag), le listener utilise performed_by = 'system' quand le flag est vrai.

Effort : 10-15 min.

Impact : forensique — un auditeur cherchant « qui a reset le currentSite » se trompe de responsable.


I-13 — AuditListener pas scope-pinne a l'EntityManager

Fichier : src/Module/Core/Infrastructure/Doctrine/AuditListener.php:65-66

Le listener utilise #[AsDoctrineListener(event: ...)] sans argument connection. Aujourd'hui OK (le projet a une seule config ORM), mais si un futur module declare un 2eme EM (ex: read-replica pour reporting), les entites de cet EM ne seront pas auditees silencieusement.

Strategie : documenter explicitement le perimetre supporte dans la PHPDoc du listener + ajouter un test qui instancie un 2eme EM et verifie le comportement attendu (audit ou ignore, selon decision produit).

Effort : 5 min doc + 30 min test si besoin.


I-14 — Pas de regression test direct pour "sites overwritten on PATCH omission"

Fichier : tests/Module/Core/Api/UserRbacSitesApiTest.php:142-169

Le test testRbacPatchWithoutSitesFieldDoesNotChangeCurrentSite verifie que currentSite n'est pas touche, mais n'assert pas que la collection sites elle-meme est preservee. Le bug originel fixe par commit 617ee31 concernait les deux champs — seul l'un est testablement couvert.

Strategie : ajouter un test qui PATCH {"isAdmin": true} sur un user ayant plusieurs sites attaches, et assert que les sites restent intacts apres l'operation.

Effort : 5 min.


I-15 — AuditLogProvider trop gras : extraire DbalAuditLogRepository

Fichier : src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php

Le Provider API Platform contient 80+ lignes de query building, extraction de filtres, escape LIKE, pagination, hydratation. Responsabilite mixte « orchestration API » et « requetes DBAL ». Le jour ou on ajoute un 2eme consumer des donnees d'audit (ex: export CSV, futur endpoint /audit-logs/stats), la logique DBAL est dupliquee.

Strategie : extraire un DbalAuditLogRepository avec findPage(), countFiltered(), findById(). Provider devient un thin adapter.

Effort : 20-30 min refacto.


I-18 — useAuditLog.fetchLogs exporte silencieusement la version cachee

Fichier : frontend/shared/composables/useAuditLog.ts:131-138

La fonction publique fetchLogs est en realite un alias vers fetchLogsCached qui ecrit dans le state module-level. Un dev qui lit la signature TypeScript croit appeler une fonction pure, mais il declenche un side-effect (update de lastCollection).

Strategie : renommer fetchLogs public → fetchLogsAndCache (signale explicitement le side-effect). Ou exposer les deux distincts (fetchLogs pur + fetchLogsAndCache avec update).

Effort : 5 min (ripple sur audit-log.vue a suivre).


I-19 — RequestIdProvider pas reset sur kernel.finish_request

Fichier : src/Module/Core/Infrastructure/Audit/RequestIdProvider.php:22-42

Le requestId est set en kernel.request mais jamais cleared. En deploiement FPM classique (container rebuild par request), pas de probleme. Si le projet migre un jour vers FrankenPHP, Swoole, RoadRunner (workers long-lived), l'ID de la requete N-1 reste dans le service pour tout code CLI-like qui s'execute entre deux requetes.

Strategie : ajouter un event listener sur kernel.finish_request qui reset $this->requestId = null si c'est la main request.

Effort : 5 min + test.

Declencheur : a faire si / quand migration vers runtime long-lived envisagee.


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-colturaphp-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

Fichier : config/packages/security.yaml:46

Swagger expose le schema complet a un acteur non-authentifie : noms des resources, expressions security:, schemas request/response. Pas une faille mais une surface d'info disclosure.

Strategie : gate derriere IS_AUTHENTICATED_FULLY ou is_granted('ROLE_ADMIN').

Effort : 2 min.


M-2 — Scope creep Playwright dans la PR audit-log

Fichiers : frontend/playwright.config.ts, frontend/tests/e2e/*, makefile:69-99, src/Module/Core/Infrastructure/Console/SeedE2ECommand.php

Deux reviewers ont signale que l'initialisation de la suite E2E Playwright (commit 4603ab2) ne fait pas partie du scope « audit log ». Ideal aurait ete une PR separee.

Strategie retrospective : a noter pour discipline future. Aucune action requise sur cette PR.


M-3 — GDPR : ip_address persiste sans retention documentee

Fichier : src/Module/Core/Application/DTO/AuditLogOutput.php:27

Les IP addresses de toutes les operations user sont persistees et exposees a tout user avec core.audit_log.view. En EU c'est de la donnee personnelle sous GDPR. Avec une table append-only sans retention (cf. I-8), on cumule les IP indefiniment.

Strategie :

  • Coupler avec I-8 (politique de retention generale)
  • Option : tronquer l'IP a /24 (IPv4) ou /48 (IPv6) pour les events non-security
  • Document legal a ecrire (mention dans politique de confidentialite interne Malio)

Effort : decision produit/legal + 15 min code.


M-5 — stripSensitive() commentaire mensonger

Fichier : src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:81-98

Docblock dit « recursivement » mais le code fait un unset() top-level uniquement. Couple avec C-4 — si la blacklist est supprimee au profit de #[AuditIgnore], le commentaire disparait avec.

Strategie : traiter via C-4, sinon corriger le commentaire en l'attendant.

Effort : 1 min si standalone.


M-6 — LIKE escape comment imprecis

Fichier : src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:175-177

Le commentaire dit que \ est le caractere d'echappement LIKE « par defaut en PostgreSQL », ce qui implique une dependance a standard_conforming_strings. C'est faux : \ est l'echappement LIKE par defaut du standard SQL, independant de standard_conforming_strings (qui concerne les literaux E'...').

Strategie : corriger le commentaire pour lever l'ambiguite.

Effort : 1 min.


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

Le spec dit « pas de DELETE », les tests font DELETE FROM audit_log en tearDown pour nettoyer leurs fixtures. Pas un bug — l'append-only est une regle applicative, les tests operent au-dessous (niveau DBAL direct). Juste a clarifier.

Strategie : ajouter une note dans doc/audit-log.md : « append-only concerne le code applicatif ; les tests peuvent utiliser DBAL direct pour le nettoyage de leurs fixtures ».

Effort : 2 min.


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 0 — securite bloquante avant prod : C-5 (voter cross-permission), I-20 (trusted_proxies), I-25 (rate limiter)

Bloc 0bis — DX bloquante (workflow dev quotidien) : I-26 (suite PHPUnit non-deterministe — bloque les commits sans --no-verify)

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 : C-3 reste a la discretion produit — le contrat actuel est documente et acceptable.