From 49d5c0aed3a03e4d99acff68bf35bf3fc863fef8 Mon Sep 17 00:00:00 2001
From: boban <boban@git.bave.dev>
Date: Sat, 20 Jun 2026 17:39:01 +0200
Subject: [PATCH] harden(auth): BruteforceGuard review fixes (hit count, mask
 bounds, once-guard, mapped loopback)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 app/Services/BruteforceGuard.php | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/app/Services/BruteforceGuard.php b/app/Services/BruteforceGuard.php
index 6dfd9d8..54f175b 100644
--- a/app/Services/BruteforceGuard.php
+++ b/app/Services/BruteforceGuard.php
@@ -16,11 +16,12 @@ use Illuminate\Support\Facades\RateLimiter;
 class BruteforceGuard
 {
     /** Always exempt — hard, non-configurable (prevents loopback/health self-lockout). */
-    private const LOOPBACK = ['127.0.0.0/8', '::1', '::ffff:127.0.0.1'];
+    private const LOOPBACK = ['127.0.0.0/8', '::1', '::ffff:127.0.0.0/104'];
 
     /** Default operator whitelist: private ranges (covers LAN/VPN + the Docker/proxy net). */
     private const DEFAULT_WHITELIST = "10.0.0.0/8\n172.16.0.0/12\n192.168.0.0/16";
 
+    /** Ban-check cache TTL (seconds). Stale window on natural expiry; keep <= bantime()/2. */
     private const CHECK_TTL = 30;
 
     public function enabled(): bool
@@ -76,23 +77,29 @@ class BruteforceGuard
 
         $canonical = $this->canonical($ip);
         $key = 'bruteforce:'.md5($canonical);
-        RateLimiter::hit($key, $this->findtime() * 60); // minutes → seconds
-        $hits = RateLimiter::attempts($key);
+        $hits = RateLimiter::hit($key, $this->findtime() * 60); // minutes → seconds; hit() returns the count
 
         if ($hits < $this->maxretry()) {
             return;
         }
 
-        // Atomic upsert: two concurrent requests at the threshold must not collide on the
-        // unique ip index. Eloquent upsert does not auto-manage timestamps for bulk ops.
+        // Ban + audit at most once per IP per window even if two requests cross the threshold
+        // concurrently (the upsert is idempotent; this also de-dupes the audit row).
+        if (! Cache::add('bruteforce:banning:'.$canonical, true, 60)) {
+            return;
+        }
+
+        // Atomic upsert keyed on the unique ip index. Eloquent upsert needs explicit timestamps
+        // for the bulk path; a single $now keeps created_at/updated_at identical.
+        $now = now();
         BannedIp::upsert(
             [[
                 'ip' => $canonical,
-                'banned_until' => now()->addMinutes($this->bantime()),
+                'banned_until' => $now->copy()->addMinutes($this->bantime()),
                 'reason' => $reason,
                 'attempts' => $hits,
-                'created_at' => now(),
-                'updated_at' => now(),
+                'created_at' => $now,
+                'updated_at' => $now,
             ]],
             ['ip'],
             ['banned_until', 'reason', 'attempts', 'updated_at'],
@@ -110,6 +117,11 @@ class BruteforceGuard
         ]);
     }
 
+    /**
+     * Whether $ip has an active ban. Does NOT check enabled() — the BlockBannedIp middleware
+     * gates on enabled() before calling this, so disabling the feature stops enforcement while
+     * existing rows persist and resume if it is re-enabled (spec §7).
+     */
     public function isBanned(string $ip): bool
     {
         if ($this->isExempt($ip)) {
@@ -149,6 +161,9 @@ class BruteforceGuard
         }
 
         $mask = (int) $maskStr;
+        if ($mask > strlen($pip) * 8) {
+            return false; // mask wider than the address family (e.g. IPv4 /33)
+        }
         $bytes = intdiv($mask, 8);
         $rem = $mask % 8;
         if ($bytes > 0 && substr($pip, 0, $bytes) !== substr($psub, 0, $bytes)) {