New. Security. Lost-password interface is under rate limit.

inc/spbc-auth.php

@@ -22,7 +22,8 @@
     die('Not allowed!');
 }
 
-add_filter('authenticate', 'spbc_authenticate', 20, 2); // Hooks for authentificate
+add_filter('authenticate', 'spbc_authenticate', 20, 2); // Hooks for authenticate
+add_action('lostpassword_post', 'spbc_lostpassword_rate_limit', 20, 2); // Hooks for lost password request checking
 
 // Hook for token-based logins (plugins like "Temporary Login Without Password")
 add_action('set_logged_in_cookie', 'spbc_detect_token_login', 10, 6);
@@ -266,6 +267,30 @@ function spbc_authenticate($user, $username)
     return $user;
 }
 
+function spbc_lostpassword_rate_limit($error)
+{
+    global $spbc;
+    $limit = !empty($spbc->settings['bfp__allowed_wrong_auths'])
+            ? (int) $spbc->settings['bfp__allowed_wrong_auths']
+            : 5;
+    $period = !empty($spbc->settings['bfp__block_period__5_fails'])
+        ? (int) $spbc->settings['bfp__block_period__5_fails']
+        : 3600;
+    $config = new RateLimiterConfig('lostpassword_rate_limit', $limit, $period);
+    $rate_limiter = new SpbcRateLimiter($config);
+    $rate_limiter_check = $rate_limiter->checkPassed();
+    if ( ! $rate_limiter_check ) {
+        $limit_message = esc_html__('Current route access denied. (Security by CleanTalk)', 'security-malware-firewall');
+        add_filter('lostpassword_errors', function ($_errors, $_user) use ($limit_message) {
+            return new WP_Error(403, $limit_message);
+        }, 20, 2);
+        if (function_exists('wc_add_notice')) {
+            wp_die($limit_message, '', ['response' => 403]);
+        }
+    }
+    return $error;
+}
+
 /**
  * Detecting new device
  *

js/src/react/components/TabContent/TabSettingsGeneral/Settings/BfpEnabled.js

@@ -77,6 +77,8 @@ export default function BfpEnabled(props) {
                         countIntervalMinutes,
                         blockPeriodHuman,
                     )}
+                    <br />
+                    {sprintf('The `lost password` interface is under protection too.', 'security-malware-firewall')}
                 </span>
             </div>
         </>

lib/CleantalkSP/Common/RateLimit/RateLimitDTO.php

@@ -15,6 +15,7 @@ class RateLimitDTO extends DTO
      * Current request counter for the UID
      *
      * @var int
+     * @psalm-suppress PossiblyUnusedProperty
      */
     public $counter = 0;
 
@@ -46,6 +47,16 @@ class RateLimitDTO extends DTO
      */
     public $ua = '';
 
+    /**
+     * Timestamp when the UID got locked (counter exceeded the limit).
+     * 0 means "not locked". Kept separate from created_at so the block
+     * duration is counted from the moment the limit was exceeded, not
+     * from the first request of the counting window.
+     *
+     * @var int
+     */
+    public $locked_at = 0;
+
     /**
      * Unique identifier for the rate limit record
      *

lib/CleantalkSP/Common/RateLimit/RateLimiter.php

@@ -75,7 +75,7 @@ public function __construct(RateLimiterConfig $config)
      */
     protected function setUID(): void
     {
-        $this->uid = md5($this->ip . $this->ua . $this->config->type);
+        $this->uid = md5($this->ip . $this->config->type);
     }
 
     /**
@@ -115,6 +115,7 @@ public function checkPassed(): bool
                         'created_at' => $this->current_ts,
                         'ip' => $this->ip,
                         'ua' => $this->ua,
+                        'locked_at' => 0,
                     )
                 );
                 if (!$uid_data->data_ok) {
@@ -136,12 +137,19 @@ public function checkPassed(): bool
     /**
      * Checks if the current UID has exceeded the rate limit
      *
+     * The lock is driven by `locked_at`, a timestamp set (atomically, at
+     * storage level) the moment the counter crosses the limit. The block
+     * therefore lasts `period` seconds starting from the moment the limit
+     * was exceeded, regardless of when the counting window itself started.
+     *
      * @param RateLimitDTO $uid_data
      * @return bool True if rate limited, false otherwise
      */
     protected function isLocked(RateLimitDTO $uid_data): bool
     {
-        return $uid_data->data_ok && ($uid_data->counter > $this->config->limit);
+        return $uid_data->data_ok
+            && $uid_data->locked_at > 0
+            && ($this->current_ts - $uid_data->locked_at) < $this->config->period;
     }
 
     /**

lib/CleantalkSP/SpbctWP/DB/SQLSchema.php

@@ -350,6 +350,7 @@ class SQLSchema extends \CleantalkSP\Common\DB\SQLSchema
                 array('field' => 'counter', 'type' => 'int unsigned', 'null' => 'no', 'default' => '1'),
                 array('field' => 'last_call', 'type' => 'int unsigned', 'null' => 'no',),
                 array('field' => 'created_at', 'type' => 'int unsigned', 'null' => 'no',),
+                array('field' => 'locked_at', 'type' => 'int unsigned', 'null' => 'no', 'default' => '0'),
             ),
             'indexes' => array(
                 array('type' => 'PRIMARY', 'name' => 'KEY', 'body' => '(`uid`)'),

lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcRateLimiter.php

@@ -99,7 +99,7 @@ public function selectUIDData()
         global $wpdb;
         $sql = $wpdb->prepare(
             '
-                SELECT uid, type, ip, ua, counter, last_call, created_at FROM ' . static::$table_name . '
+                SELECT uid, type, ip, ua, counter, last_call, created_at, locked_at FROM ' . static::$table_name . '
                 WHERE uid = %s LIMIT 1;
             ',
             $this->uid
@@ -109,72 +109,176 @@ public function selectUIDData()
         return !empty($result) ? new RateLimitDTO($result) : false;
     }
 
+    /**
+     * Re-reads the authoritative row state from the database into the given
+     * DTO after an atomic insert/update, so callers (isLocked) always decide
+     * based on what is actually persisted, not on PHP-side guesses that may
+     * have been invalidated by a concurrent request.
+     *
+     * @param RateLimitDTO $uid_data
+     * @return void
+     */
+    private function refreshUidData($uid_data): void
+    {
+        global $wpdb;
+        $sql = $wpdb->prepare(
+            '
+                SELECT counter, last_call, created_at, locked_at FROM ' . static::$table_name . '
+                WHERE uid = %s LIMIT 1;
+            ',
+            $uid_data->uid
+        );
+        $result = $wpdb->get_row($sql, ARRAY_A);
+        if (!empty($result)) {
+            $uid_data->counter = (int)$result['counter'];
+            $uid_data->last_call = (int)$result['last_call'];
+            $uid_data->created_at = (int)$result['created_at'];
+            $uid_data->locked_at = (int)$result['locked_at'];
+        }
+    }
+
     /**
      * Inserts a new rate limit record for the current UID in database.
+     * Uses an atomic UPSERT: if a concurrent request created the row first
+     * (race on INSERT), the ON DUPLICATE KEY branch applies the same
+     * atomic counter/lock logic as increment() instead of silently
+     * overwriting or losing the other request's increment.
+     *
+     * The whole counter/window/lock recalculation is expressed with plain
+     * CASE expressions (no `:=` user-variable assignment inside the
+     * statement) on purpose: assigning user variables in a statement other
+     * than SET has been deprecated by MySQL since 8.0.13 and is not
+     * guaranteed to keep working on every MySQL/MariaDB/Percona version
+     * this plugin may run on.
+     *
      * @param RateLimitDTO $uid_data
      * @return bool True on successful insert, false on failure
      */
     protected function insert($uid_data): bool
     {
         global $wpdb;
 
+        $window_threshold = $this->current_ts - $this->config->period;
+        $should_reset = '(created_at < %d OR (locked_at != 0 AND locked_at < %d))';
+        $new_counter = '(CASE WHEN ' . $should_reset . ' THEN 1 ELSE counter + 1 END)';
+
         $sql = $wpdb->prepare(
             '
             INSERT INTO ' . static::$table_name . '
-                (uid, type, ip, ua, counter, last_call, created_at)
-            VALUES (%s, %s, %s, %s, 1, %d, %d)
-            ON DUPLICATE KEY UPDATE last_call = %s, counter = counter + 1;
+                (uid, type, ip, ua, counter, last_call, created_at, locked_at)
+            VALUES (%s, %s, %s, %s, 1, %d, %d, 0)
+            ON DUPLICATE KEY UPDATE
+                counter = CASE WHEN ' . $should_reset . ' THEN 1 ELSE counter + 1 END,
+                created_at = CASE WHEN ' . $should_reset . ' THEN %d ELSE created_at END,
+                locked_at = CASE
+                    WHEN ' . $should_reset . ' THEN 0
+                    WHEN locked_at = 0 AND ' . $new_counter . ' > %d THEN %d
+                    ELSE locked_at
+                END,
+                last_call = %d;
             ',
             $uid_data->uid,
             $uid_data->type,
             $uid_data->ip,
             $uid_data->ua,
             $uid_data->last_call,
             $uid_data->created_at,
-            $uid_data->last_call
+            $window_threshold,
+            $window_threshold,
+            $window_threshold,
+            $window_threshold,
+            $this->current_ts,
+            $window_threshold,
+            $window_threshold,
+            $window_threshold,
+            $window_threshold,
+            $this->config->limit,
+            $this->current_ts,
+            $this->current_ts
         );
 
         $result = $wpdb->query($sql);
 
+        if (false !== $result) {
+            $this->refreshUidData($uid_data);
+        }
+
         return false !== $result;
     }
 
     /**
-     * Increments the counter for an existing rate limit record
-     * Resets counter if the period has expired
+     * Increments the counter for an existing rate limit record.
+     *
+     * The counter/window/lock state is computed and written in a single
+     * atomic UPDATE (row-level locked by InnoDB for the duration of the
+     * statement), so concurrent requests for the same UID can never read
+     * a stale counter and overwrite each other's increment (no more lost
+     * updates under concurrency). The recalculation is expressed with
+     * plain CASE expressions only - see the note in insert() about why
+     * `:=` user-variable assignment is deliberately avoided here.
+     *
+     * The lock itself is anchored to `locked_at`, which is set once, the
+     * moment the counter first exceeds the limit, and only cleared once
+     * `period` seconds have passed since that moment - not since the first
+     * request of the counting window. This way a ban always lasts a full
+     * `period`, regardless of when inside the window the limit was hit.
+     *
      * @param RateLimitDTO $uid_data
      * @return bool True on successful update, false on failure
      */
     protected function increment($uid_data): bool
     {
         global $wpdb;
 
-        $is_expired = ($this->current_ts - $uid_data->created_at) > $this->config->period;
-
-        $uid_data->counter = $is_expired ? 1 : $uid_data->counter + 1;
-        $uid_data->created_at = $is_expired ? $this->current_ts : $uid_data->created_at;
-        $uid_data->last_call = $this->current_ts;
+        $threshold = $this->current_ts - $this->config->period;
+        $should_reset = '(created_at < %d OR (locked_at != 0 AND locked_at < %d))';
+        $new_counter = '(CASE WHEN ' . $should_reset . ' THEN 1 ELSE counter + 1 END)';
 
         $sql = $wpdb->prepare(
             '
             UPDATE ' . static::$table_name . ' SET
-                counter = %d,
-                last_call = %d,
-                created_at = %d
+                counter = CASE WHEN ' . $should_reset . ' THEN 1 ELSE counter + 1 END,
+                created_at = CASE WHEN ' . $should_reset . ' THEN %d ELSE created_at END,
+                locked_at = CASE
+                    WHEN ' . $should_reset . ' THEN 0
+                    WHEN locked_at = 0 AND ' . $new_counter . ' > %d THEN %d
+                    ELSE locked_at
+                END,
+                last_call = %d
             WHERE uid = %s
         ',
-            $uid_data->counter,
-            $uid_data->last_call,
-            $uid_data->created_at,
+            $threshold,
+            $threshold,
+            $threshold,
+            $threshold,
+            $this->current_ts,
+            $threshold,
+            $threshold,
+            $threshold,
+            $threshold,
+            $this->config->limit,
+            $this->current_ts,
+            $this->current_ts,
             $uid_data->uid
         );
 
-        return false !== $wpdb->query($sql);
+        $result = $wpdb->query($sql);
+
+        if (false !== $result) {
+            $this->refreshUidData($uid_data);
+        }
+
+        return false !== $result;
     }
 
     /**
      * Removes expired rate limit records from the database
-     * Records older than (period + 10 seconds) are deleted
+     * Records are deleted only when both the counting window (created_at)
+     * AND any active lock (locked_at) are past (period + 10 seconds) old.
+     * Checking created_at alone would let cleanup delete a row whose lock
+     * is still active (a lock set near the end of the window can outlive
+     * the window by almost another full period), effectively un-banning
+     * a client early.
      *
      * @return bool True on successful cleanup, false on failure
      */
@@ -185,9 +289,15 @@ protected function cleanUp(): bool
         $threshold = $this->current_ts - ($this->config->period + 10);
 
         $sql = $wpdb->prepare(
-            'DELETE FROM ' . static::$table_name . ' WHERE created_at < %d AND type = %s;',
+            '
+            DELETE FROM ' . static::$table_name . '
+            WHERE type = %s
+                AND created_at < %d
+                AND (locked_at = 0 OR locked_at < %d);
+            ',
+            $this->config->type,
             $threshold,
-            $this->config->type
+            $threshold
         );
 
         $result = $wpdb->query($sql);