diff --git a/doc/audit-log-review-backlog.md b/doc/audit-log-review-backlog.md new file mode 100644 index 0000000..854aa1e --- /dev/null +++ b/doc/audit-log-review-backlog.md @@ -0,0 +1,294 @@ +# 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. + +--- + +## 🟠 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. + +--- + +## 🟡 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-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` + +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-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` + +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. + +--- + +## 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 2 — fix mecaniques (~1-2h cumule)** : +C-4 (preventif), I-12, I-15, M-5 + +**Bloc 3 — 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.