From a3fa72cf868eae4e504adccbe9bb5a79b1557616 Mon Sep 17 00:00:00 2001 From: Imugiii Date: Tue, 16 Jun 2026 12:15:21 +0000 Subject: [PATCH] fix(admin): chemin d'echec PIN atomique (pin.failed + throttle dans 1 transaction) Sur un PIN invalide, ProductController ecrivait la trace audit pin.failed (autocommit) PUIS appelait PinThrottle::recordFailure (qui ouvrait SA propre transaction) : deux ecritures non atomiques, un crash entre les deux laissait un etat partiel, en tension avec le claim RG-T08 (audit dans la meme transaction que l'effet). - PinThrottle : extraction de recordFailureWithin(db, ...) (memes effets, SANS transaction propre) ; recordFailure() reste l'API autonome (l'enveloppe). - ProductController (update + destroy) : les chemins d'echec PIN enveloppent logFailedPin(, ...) + recordFailureWithin(, ...) dans UNE transaction. - logFailedPin prend desormais le de la transaction. Aucun changement de test necessaire (les assertions audit/throttle tiennent dans la transaction). 188 verts, PinThrottleDbTest 2/2 contre la vraie DB, PHPStan L6. --- src/app/Auth/PinThrottle.php | 86 ++++++++++++++--------- src/app/Controllers/ProductController.php | 23 ++++-- 2 files changed, 70 insertions(+), 39 deletions(-) diff --git a/src/app/Auth/PinThrottle.php b/src/app/Auth/PinThrottle.php index 0066cba..8542681 100644 --- a/src/app/Auth/PinThrottle.php +++ b/src/app/Auth/PinThrottle.php @@ -75,47 +75,67 @@ final class PinThrottle return; } + // Variante autonome : ouvre sa propre transaction. Le controleur, lui, + // prefere recordFailureWithin() pour ecrire la trace pin.failed et cet + // increment dans UNE SEULE transaction (RG-T08). + $this->db->transaction(function (DatabaseInterface $db) use ($actorUserId, $now): void { + $this->recordFailureWithin($db, $actorUserId, $now); + }); + } + + /** + * Variante SANS transaction propre : suppose que l'appelant a deja ouvert une + * transaction (le controleur enveloppe la trace audit pin.failed (RG-T14) et + * cet increment dans la meme, RG-T08 : pas d'etat partiel si crash entre les + * deux). Memes effets que recordFailure : upsert atomique sous verrou de ligne, + * fenetre glissante reinitialisee en SQL, backoff degressif. Ne touche jamais + * user ni login_throttle (RG-T22). + */ + public function recordFailureWithin(DatabaseInterface $db, int $actorUserId, ?int $now = null): void + { + if ($actorUserId <= 0) { + return; + } + $now ??= time(); $nowDt = date('Y-m-d H:i:s', $now); $windowSeconds = $this->config->int('PIN_THROTTLE_WINDOW_SECONDS', 900); $windowCutoff = date('Y-m-d H:i:s', $now - $windowSeconds); $policy = ThrottlePolicy::fromConfig($this->config, 'pin'); - $this->db->transaction(function (DatabaseInterface $db) use ($actorUserId, $nowDt, $windowCutoff, $policy, $now): void { - // Increment ATOMIQUE cote SQL sous le verrou de ligne pris par l'upsert - // (anti lost-update sous POSTs concurrents). Placeholders distincts : en - // prepare reelle (EMULATE_PREPARES = false) un meme nom ne peut etre lie - // qu'une fois. Meme forme que AuthService (dimension IP). - $db->execute( - 'INSERT INTO pin_throttle (actor_user_id, failed_attempts, window_started_at, last_attempt_at) ' - . 'VALUES (:uid, 1, :now_i, :now_li) ' - . 'ON DUPLICATE KEY UPDATE ' - . 'failed_attempts = IF(window_started_at < :cutoff, 1, failed_attempts + 1), ' - . 'window_started_at = IF(window_started_at < :cutoff2, :now_w, window_started_at), ' - . 'last_attempt_at = :now_lu', - [ - 'uid' => $actorUserId, - 'now_i' => $nowDt, - 'now_li' => $nowDt, - 'cutoff' => $windowCutoff, - 'cutoff2' => $windowCutoff, - 'now_w' => $nowDt, - 'now_lu' => $nowDt, - ], - ); + // Increment ATOMIQUE cote SQL sous le verrou de ligne pris par l'upsert + // (anti lost-update sous POSTs concurrents). Placeholders distincts : en + // prepare reelle (EMULATE_PREPARES = false) un meme nom ne peut etre lie + // qu'une fois. Meme forme que AuthService (dimension IP). + $db->execute( + 'INSERT INTO pin_throttle (actor_user_id, failed_attempts, window_started_at, last_attempt_at) ' + . 'VALUES (:uid, 1, :now_i, :now_li) ' + . 'ON DUPLICATE KEY UPDATE ' + . 'failed_attempts = IF(window_started_at < :cutoff, 1, failed_attempts + 1), ' + . 'window_started_at = IF(window_started_at < :cutoff2, :now_w, window_started_at), ' + . 'last_attempt_at = :now_lu', + [ + 'uid' => $actorUserId, + 'now_i' => $nowDt, + 'now_li' => $nowDt, + 'cutoff' => $windowCutoff, + 'cutoff2' => $windowCutoff, + 'now_w' => $nowDt, + 'now_lu' => $nowDt, + ], + ); - // Relit le compteur autoritaire (ligne deja verrouillee par cette tx) - // pour calculer le backoff en PHP, puis pose le verrou. - $row = $db->fetch('SELECT failed_attempts FROM pin_throttle WHERE actor_user_id = :uid', ['uid' => $actorUserId]); - $attempts = (int) ($row['failed_attempts'] ?? 1); - $lockSeconds = $policy->lockoutSeconds($attempts); - $lockUntil = $lockSeconds > 0 ? date('Y-m-d H:i:s', $now + $lockSeconds) : null; + // Relit le compteur autoritaire (ligne deja verrouillee par cette tx) + // pour calculer le backoff en PHP, puis pose le verrou. + $row = $db->fetch('SELECT failed_attempts FROM pin_throttle WHERE actor_user_id = :uid', ['uid' => $actorUserId]); + $attempts = (int) ($row['failed_attempts'] ?? 1); + $lockSeconds = $policy->lockoutSeconds($attempts); + $lockUntil = $lockSeconds > 0 ? date('Y-m-d H:i:s', $now + $lockSeconds) : null; - $db->execute( - 'UPDATE pin_throttle SET lockout_until = :lock WHERE actor_user_id = :uid', - ['lock' => $lockUntil, 'uid' => $actorUserId], - ); - }); + $db->execute( + 'UPDATE pin_throttle SET lockout_until = :lock WHERE actor_user_id = :uid', + ['lock' => $lockUntil, 'uid' => $actorUserId], + ); } /** diff --git a/src/app/Controllers/ProductController.php b/src/app/Controllers/ProductController.php index 6a487f4..54ec6f0 100644 --- a/src/app/Controllers/ProductController.php +++ b/src/app/Controllers/ProductController.php @@ -157,8 +157,14 @@ class ProductController extends AdminController $actor = $this->pinVerifier()->resolveActingUser(trim($form['pin_email'] ?? ''), $form['pin'] ?? ''); if ($actor === null) { - $this->logFailedPin(trim($form['pin_email'] ?? ''), $id); - $this->pinThrottle()->recordFailure($actorId); + // RG-T08 : la trace pin.failed (RG-T14) et l'increment du throttle + // (RG-T22) sont ecrits dans UNE meme transaction (pas d'etat partiel + // si crash entre les deux ecritures). + $email = trim($form['pin_email'] ?? ''); + $this->db()->transaction(function (DatabaseInterface $db) use ($email, $id, $actorId): void { + $this->logFailedPin($db, $email, $id); + $this->pinThrottle()->recordFailureWithin($db, $actorId); + }); return $this->renderForm($guard, $id, $form, ['pin' => 'Email ou PIN invalide (requis pour modifier prix/TVA).'], 422); } @@ -232,8 +238,13 @@ class ProductController extends AdminController $actor = $this->pinVerifier()->resolveActingUser(trim($form['pin_email'] ?? ''), $form['pin'] ?? ''); if ($actor === null) { - $this->logFailedPin(trim($form['pin_email'] ?? ''), $id); - $this->pinThrottle()->recordFailure($actorId); + // RG-T08 : trace pin.failed (RG-T14) + increment throttle (RG-T22) dans + // UNE meme transaction (pas d'etat partiel si crash entre les deux). + $email = trim($form['pin_email'] ?? ''); + $this->db()->transaction(function (DatabaseInterface $db) use ($email, $id, $actorId): void { + $this->logFailedPin($db, $email, $id); + $this->pinThrottle()->recordFailureWithin($db, $actorId); + }); return $this->renderDelete($guard, $id, $product, 'Email ou PIN invalide (requis pour supprimer).'); } @@ -376,9 +387,9 @@ class ProductController extends AdminController * echecs ayant arme le verrou sont deja audites), ce qui borne l'amplification * de l'audit append-only (RG-T14). */ - private function logFailedPin(string $email, int $productId): void + private function logFailedPin(DatabaseInterface $db, string $email, int $productId): void { - $this->db()->execute( + $db->execute( 'INSERT INTO audit_log (actor_user_id, actor_role_id, action_code, entity_type, entity_id, summary) ' . 'VALUES (:uid, :rid, :code, :etype, :eid, :summary)', [