Files

matthieu fcc7a2e3d4 docs(audit-log) : ajoute backlog des findings review non-traites dans la PR

Resume structure des 9 Important + 8 Minor + C-4 parke + C-3 documente.

Chaque entree contient : severite, fichier:ligne, strategie recommandee,
effort estime, declencheur (quand / pourquoi faire). Trois blocs
priorises pour faciliter le picking en tickets (quick wins, mecaniques,
scaling produit).

Permet de ne pas perdre la vue d'ensemble du review et d'ouvrir les
tickets dedies avec contexte complet sans re-analyser le diff.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

2026-05-02 14:21:49 +02:00

15 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.

🟠 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.

15 KiB Raw Blame History Unescape Escape

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

🔴 Critical parke

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

🔴 Critical documente mais pas fixe code

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

🟠 Important

I-7 — DbalPaginator fait COUNT(*) sur chaque list request

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

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

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

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