Aller au contenu

PD-276 — Revue de Code

Résumé

Critère Statut
Patterns NestJS ⅘
Qualité code ⅘
Gestion erreurs ⅘
Maintenabilité ⅗

Verdict : RÉSERVES

Points positifs

  • Argon2Service respecte bien le contrat “accept/reject only” : validateParams() retourne { valid: true } ou lève une exception, sans dérivation de clé.
  • deriveKey() est bien un alias public de validateParams() et ne calcule aucune clé.
  • Les minima OWASP/RFC sont centralisés dans ARGON2_CONFIG (pas de littéraux inline dans le service) et la validation est atomique (au moindre écart, rejet global).
  • MetadataBindingService applique correctement HKDF avec contexte dédié, HMAC-SHA256, concaténation canonique avec séparateurs NUL, comparaison via timingSafeEqual, et zeroization de kBinding en finally.
  • CryptoModule expose bien Argon2Service et MetadataBindingService dans providers + exports, et inclut Argon2Controller dans controllers.
  • Migration en 2 phases cohérente pour rollout progressif (nullable puis NOT NULL + CHECK length=32).

Points à améliorer

ID Description Fichier Gravité
RV-276-01 ARGON2_CONFIG est “immutable” au niveau TypeScript (as const) mais pas figé runtime (Object.freeze). Le contrat parle d’un singleton immuable “fort”. argon2.constants.ts Moyenne
RV-276-02 getConfig() retourne la référence directe du singleton : un consommateur malveillant/négligent pourrait la muter via cast any. argon2.service.ts Moyenne
RV-276-03 Le DTO autorise des minima à @Min(1) alors que la règle métier est beaucoup plus stricte (65536/¾/2/32). Ce n’est pas bloquant car le service filtre, mais c’est moins défensif. validate-argon2-params.dto.ts Faible
RV-276-04 Pas de tests E2E/migration dans ce lot : acceptable “hors scope”, mais laisse un risque de non-régression sur l’intégration DB + service. Lot global Moyenne

Détail par fichier

  • argon2.constants.ts
  • Conforme sur le fond (bornes OWASP, contexte HKDF, taille tag).
  • Réserve sur l’immutabilité runtime : pour coller strictement à INV-276-04, figer l’objet (deep freeze simple sur ce niveau).
  • validate-argon2-params.dto.ts
  • Correct pour typage/Swagger/Nest.
  • Les contraintes class-validator ne reflètent pas les invariants de sécurité (elles pourraient être alignées pour fail-fast HTTP avant le service).
  • argon2.service.ts
  • Très bon respect C1 (alias, reject global, pas de crypto native dérivée).
  • BadRequestException structurée avec liste d’erreurs : bon pour opérabilité.
  • Réserve sur getConfig() qui expose la référence brute.
  • argon2.controller.ts
  • Pattern NestJS propre (DI constructeur, endpoint clair, Swagger).
  • RAS contractuel.
  • metadata-binding.service.ts
  • Très bonne implémentation C3 : HKDF contexte constant, tag 32 bytes, concat canonique, comparaison timing-safe, zeroization.
  • Logging ne divulgue pas de secret clé (bon point).
  • key-envelope.entity.ts
  • Ajout de colonne cohérent avec le besoin de binding.
  • Aligné avec la migration progressive.
  • crypto.module.ts
  • C7 respecté sur les éléments fournis (providers/exports/controllers).
  • Pas d’anti-pattern NestJS visible.
  • Migrations
  • Stratégie 2 phases saine pour éviter les blocages de déploiement.
  • La phase 2 apporte bien la contrainte de longueur 32, conforme au contrat.

En synthèse, l’implémentation est globalement solide et sécurisée, mais je garde RÉSERVES tant que l’immutabilité runtime de la config Argon2 n’est pas verrouillée explicitement et que la défense en profondeur (DTO/tests d’intégration) n’est pas renforcée.