fix: harden local-login throttle against XFF spoof and IP rotation

Closes SEC_REVIEW F1 and F2. The local-admin throttle now ignores
X-Forwarded-For (Caddy is the trust boundary; PHP reads REMOTE_ADDR
only) and adds a per-username bucket alongside the per-(user, ip) one
so a residential-proxy spray hits a global lockout at 25/50/100
failures.

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

ui/src/Auth/LocalLoginController.php

@@ -109,20 +109,17 @@ final class LocalLoginController
     }
 
     /**
-     * Best-effort source IP. Honours X-Forwarded-For when present (Caddy in
-     * front sets it for trusted proxies), else falls back to the request's
-     * server param. We never trust the header for routing decisions; this
-     * is for bucketing brute-force attempts only.
+     * Source IP for the throttle bucket. We deliberately do **not** read
+     * `X-Forwarded-For` here: an attacker hitting Caddy directly can spoof
+     * the header and rotate it per request to mint a fresh per-IP bucket on
+     * every attempt, defeating the lockout (SEC_REVIEW F1). Caddy is the
+     * trust boundary: its `trusted_proxies static private_ranges` directive
+     * already rewrites `REMOTE_ADDR` to the real client when an upstream
+     * proxy is in RFC1918, and leaves the public peer IP otherwise. PHP
+     * just consumes whatever Caddy decided.
      */
     private static function extractSourceIp(ServerRequestInterface $request): string
     {
-        $xff = $request->getHeaderLine('X-Forwarded-For');
-        if ($xff !== '') {
-            $first = trim(explode(',', $xff)[0]);
-            if ($first !== '') {
-                return $first;
-            }
-        }
         $server = $request->getServerParams();
         $remote = $server['REMOTE_ADDR'] ?? '';
 

ui/src/Auth/LoginThrottle.php

@@ -9,24 +9,32 @@ use Psr\Log\LoggerInterface;
 /**
  * Brute-force lockout for the local-admin sign-in (SPEC §M14.2).
  *
- * Attempts are bucketed by `(username, source_ip)` so an attacker who is
- * spraying one username from one IP can't lock out a legitimate admin
- * coming in from a different IP. Storage is a per-process in-memory array;
- * the singleton is constructed once per FrankenPHP worker and lives for
- * the worker's lifetime. A container restart clears all entries — this is
- * intentional and documented as the operator's "unlock the admin" path.
+ * Two parallel buckets are evaluated on every attempt:
  *
- * Failure progression (clamped — the brief's "1/5/30" steps):
- *   1–4 failures      fast retry, no lockout
- *   5  failures       lock for 60 s
- *   10 failures       lock for 300 s
- *   15+ failures      lock for 1800 s
+ *   - per-(username, ip): catches single-source spray. Looser triggers so a
+ *     legit admin retrying from one IP isn't locked out by a few typos.
+ *     Failure ladder: 5 / 10 / 15 → 60 s / 300 s / 1800 s.
  *
- * Successful login clears the bucket.
+ *   - per-(username): catches a distributed spray (residential proxy pool,
+ *     botnet) where each new IP would otherwise be a fresh per-IP bucket.
+ *     Tighter ladder so the global bucket trips before the attacker exhausts
+ *     a meaningful Argon2id budget, but loose enough that an attacker can't
+ *     trivially hold the legit admin out indefinitely.
+ *     Failure ladder: 25 / 50 / 100 → 60 s / 300 s / 1800 s.
  *
- * Multi-replica caveat: each replica has its own counter. A determined
+ * `isLocked()` returns true if either bucket is locked; `lockoutSeconds…`
+ * returns the larger of the two remaining windows so the user-facing
+ * message reflects the worst-case wait. `recordFailure()` increments both,
+ * `clear()` (called on success) resets both.
+ *
+ * Storage is a per-process in-memory array; the singleton is constructed
+ * once per FrankenPHP worker and lives for the worker's lifetime. A
+ * container restart clears all entries — this is intentional and
+ * documented as the operator's "unlock the admin" path.
+ *
+ * Multi-replica caveat: each replica has its own counters. A determined
  * attacker who can reach multiple replicas through a non-sticky LB would
- * see the 5-fail step at 5*N total attempts. Single-replica is the
+ * see the per-username step at 25*N total attempts. Single-replica is the
  * documented topology for the ui container; if you scale, put the LB in
  * sticky mode.
  */
@@ -37,6 +45,11 @@ final class LoginThrottle
      */
     private array $entries = [];
 
+    /**
+     * @var array<string, array{count: int, lockedUntil: int}>
+     */
+    private array $usernameEntries = [];
+
     /** @var \Closure(): int */
     private \Closure $timeFn;
 
@@ -54,47 +67,61 @@ final class LoginThrottle
 
     public function lockoutSecondsRemaining(string $username, string $ip): int
     {
-        $key = self::key($username, $ip);
-        $entry = $this->entries[$key] ?? null;
-        if ($entry === null) {
-            return 0;
-        }
         $now = ($this->timeFn)();
-        if ($entry['lockedUntil'] <= $now) {
-            return 0;
-        }
+        $perIp = $this->remaining($this->entries, self::ipKey($username, $ip), $now);
+        $perUser = $this->remaining($this->usernameEntries, self::userKey($username), $now);
 
-        return $entry['lockedUntil'] - $now;
+        return max($perIp, $perUser);
     }
 
     public function recordFailure(string $username, string $ip): void
     {
-        $key = self::key($username, $ip);
         $now = ($this->timeFn)();
-        $entry = $this->entries[$key] ?? ['count' => 0, 'lockedUntil' => 0];
-        $entry['count']++;
-        $lockSeconds = self::lockoutSecondsForAttempt($entry['count']);
-        if ($lockSeconds > 0) {
-            $entry['lockedUntil'] = $now + $lockSeconds;
+
+        $ipKey = self::ipKey($username, $ip);
+        $ipEntry = $this->entries[$ipKey] ?? ['count' => 0, 'lockedUntil' => 0];
+        $ipEntry['count']++;
+        $ipLock = self::ipLockoutSecondsForAttempt($ipEntry['count']);
+        if ($ipLock > 0) {
+            $ipEntry['lockedUntil'] = $now + $ipLock;
             $this->logger->error('local login lockout triggered', [
+                'bucket' => 'ip',
                 'username' => $username,
                 'source_ip' => $ip,
-                'failure_count' => $entry['count'],
-                'lock_seconds' => $lockSeconds,
+                'failure_count' => $ipEntry['count'],
+                'lock_seconds' => $ipLock,
             ]);
         } else {
             $this->logger->warning('local login failure', [
+                'bucket' => 'ip',
                 'username' => $username,
                 'source_ip' => $ip,
-                'failure_count' => $entry['count'],
+                'failure_count' => $ipEntry['count'],
             ]);
         }
-        $this->entries[$key] = $entry;
+        $this->entries[$ipKey] = $ipEntry;
+
+        $userKey = self::userKey($username);
+        $userEntry = $this->usernameEntries[$userKey] ?? ['count' => 0, 'lockedUntil' => 0];
+        $userEntry['count']++;
+        $userLock = self::usernameLockoutSecondsForAttempt($userEntry['count']);
+        if ($userLock > 0) {
+            $userEntry['lockedUntil'] = $now + $userLock;
+            $this->logger->error('local login lockout triggered', [
+                'bucket' => 'username',
+                'username' => $username,
+                'source_ip' => $ip,
+                'failure_count' => $userEntry['count'],
+                'lock_seconds' => $userLock,
+            ]);
+        }
+        $this->usernameEntries[$userKey] = $userEntry;
     }
 
     public function clear(string $username, string $ip): void
     {
-        unset($this->entries[self::key($username, $ip)]);
+        unset($this->entries[self::ipKey($username, $ip)]);
+        unset($this->usernameEntries[self::userKey($username)]);
     }
 
     /**
@@ -104,16 +131,26 @@ final class LoginThrottle
     public function reset(): void
     {
         $this->entries = [];
+        $this->usernameEntries = [];
     }
 
     /**
-     * Lockout duration in seconds for the given failure count, or 0 if
-     * no lockout should fire yet. The early-returns descend from highest
-     * threshold to lowest so each step kicks in at exactly the documented
-     * count; PHPStan flags `&& $count < 15` as redundant after the first
-     * check, so we rely on the early-return ordering instead.
+     * @param array<string, array{count: int, lockedUntil: int}> $bucket
      */
-    private static function lockoutSecondsForAttempt(int $count): int
+    private function remaining(array $bucket, string $key, int $now): int
+    {
+        $entry = $bucket[$key] ?? null;
+        if ($entry === null || $entry['lockedUntil'] <= $now) {
+            return 0;
+        }
+
+        return $entry['lockedUntil'] - $now;
+    }
+
+    /**
+     * Per-IP lockout duration; 0 means no lockout at this count yet.
+     */
+    private static function ipLockoutSecondsForAttempt(int $count): int
     {
         if ($count >= 15) {
             return 1800;
@@ -128,11 +165,36 @@ final class LoginThrottle
         return 0;
     }
 
-    private static function key(string $username, string $ip): string
+    /**
+     * Per-username (cross-IP) lockout duration; thresholds are higher than
+     * per-IP because this bucket is global and a too-tight ladder would let
+     * an attacker DoS the legit admin trivially.
+     */
+    private static function usernameLockoutSecondsForAttempt(int $count): int
+    {
+        if ($count >= 100) {
+            return 1800;
+        }
+        if ($count >= 50) {
+            return 300;
+        }
+        if ($count >= 25) {
+            return 60;
+        }
+
+        return 0;
+    }
+
+    private static function ipKey(string $username, string $ip): string
     {
         // Lower-case the username so casing doesn't multiply buckets, but
         // keep the IP byte-for-byte (v6 is case-sensitive in canonical form
         // already).
         return strtolower($username) . '|' . $ip;
     }
+
+    private static function userKey(string $username): string
+    {
+        return strtolower($username);
+    }
 }

ui/tests/Integration/Auth/LocalLoginTest.php

@@ -122,6 +122,91 @@ final class LocalLoginTest extends AppTestCase
         self::assertStringContainsStringIgnoringCase('too many', $flash[0]['message']);
     }
 
+    public function testRotatingXForwardedForDoesNotEvadePerIpLockout(): void
+    {
+        // SEC_REVIEW F1: an attacker spoofing X-Forwarded-For per request
+        // must NOT mint a fresh throttle bucket. The per-IP bucket is keyed
+        // on REMOTE_ADDR, which is constant here — so 5 failures should
+        // still trip the lockout regardless of XFF rotation.
+        $this->request('GET', '/login');
+        $token = (string) ($_SESSION[CsrfMiddleware::SESSION_KEY] ?? '');
+
+        $bad = http_build_query(['csrf_token' => $token, 'username' => 'admin', 'password' => 'WRONG']);
+        for ($i = 0; $i < 5; ++$i) {
+            $this->request(
+                'POST',
+                '/login/local',
+                ['X-Forwarded-For' => '203.0.113.' . $i],
+                $bad,
+                'application/x-www-form-urlencoded',
+                ['REMOTE_ADDR' => '198.51.100.7'],
+            );
+        }
+
+        // 6th attempt with correct credentials but a fresh XFF still gets the
+        // lockout flash because the per-(user, REMOTE_ADDR) bucket is full.
+        $this->enqueueApiResponse(200, [
+            'user_id' => 1, 'role' => 'admin', 'email' => null, 'display_name' => 'Local Admin', 'is_local' => true,
+        ]);
+        $good = http_build_query(['csrf_token' => $token, 'username' => 'admin', 'password' => 'test1234']);
+        $response = $this->request(
+            'POST',
+            '/login/local',
+            ['X-Forwarded-For' => '203.0.113.99'],
+            $good,
+            'application/x-www-form-urlencoded',
+            ['REMOTE_ADDR' => '198.51.100.7'],
+        );
+
+        self::assertSame(303, $response->getStatusCode());
+        self::assertSame('/login', $response->getHeaderLine('Location'));
+        $flash = $_SESSION['_flash'] ?? [];
+        self::assertNotEmpty($flash);
+        self::assertStringContainsStringIgnoringCase('too many', $flash[0]['message']);
+    }
+
+    public function testRotatingRemoteAddrEventuallyHitsPerUsernameLockout(): void
+    {
+        // SEC_REVIEW F2: even with proper REMOTE_ADDR, an attacker on a
+        // residential proxy pool gets a fresh per-IP bucket per request.
+        // The per-username (cross-IP) bucket must catch this at 25 failures.
+        $this->request('GET', '/login');
+        $token = (string) ($_SESSION[CsrfMiddleware::SESSION_KEY] ?? '');
+
+        $bad = http_build_query(['csrf_token' => $token, 'username' => 'admin', 'password' => 'WRONG']);
+        for ($i = 0; $i < 25; ++$i) {
+            $this->request(
+                'POST',
+                '/login/local',
+                [],
+                $bad,
+                'application/x-www-form-urlencoded',
+                ['REMOTE_ADDR' => '198.51.100.' . $i],
+            );
+        }
+
+        // Next attempt from yet another IP — even with correct credentials —
+        // gets the lockout flash because the per-username bucket is full.
+        $this->enqueueApiResponse(200, [
+            'user_id' => 1, 'role' => 'admin', 'email' => null, 'display_name' => 'Local Admin', 'is_local' => true,
+        ]);
+        $good = http_build_query(['csrf_token' => $token, 'username' => 'admin', 'password' => 'test1234']);
+        $response = $this->request(
+            'POST',
+            '/login/local',
+            [],
+            $good,
+            'application/x-www-form-urlencoded',
+            ['REMOTE_ADDR' => '198.51.100.250'],
+        );
+
+        self::assertSame(303, $response->getStatusCode());
+        self::assertSame('/login', $response->getHeaderLine('Location'));
+        $flash = $_SESSION['_flash'] ?? [];
+        self::assertNotEmpty($flash);
+        self::assertStringContainsStringIgnoringCase('too many', $flash[0]['message']);
+    }
+
     public function testApiDownDuringUpsertFlashesError(): void
     {
         $this->enqueueApiException(new \GuzzleHttp\Exception\ConnectException(

ui/tests/Integration/Support/AppTestCase.php

@@ -110,6 +110,7 @@ abstract class AppTestCase extends TestCase
 
     /**
      * @param array<string, string> $headers
+     * @param array<string, mixed>  $serverParams
      */
     protected function request(
         string $method,
@@ -117,9 +118,10 @@ abstract class AppTestCase extends TestCase
         array $headers = [],
         ?string $body = null,
         ?string $contentType = null,
+        array $serverParams = [],
     ): \Psr\Http\Message\ResponseInterface {
         $factory = new ServerRequestFactory();
-        $request = $factory->createServerRequest($method, $path);
+        $request = $factory->createServerRequest($method, $path, $serverParams);
         foreach ($headers as $name => $value) {
             $request = $request->withHeader($name, $value);
         }

ui/tests/Unit/Auth/LoginThrottleTest.php

@@ -104,6 +104,78 @@ final class LoginThrottleTest extends TestCase
         self::assertTrue($t->isLocked('Admin', '10.0.0.1'));
     }
 
+    public function testPerUsernameBucketLocksOutAcrossDistinctIps(): void
+    {
+        $t = $this->throttle();
+        // 24 failures from 24 distinct IPs — each is a fresh per-IP bucket
+        // (only 1 attempt apiece), but the per-username counter accumulates.
+        for ($i = 0; $i < 24; ++$i) {
+            $t->recordFailure('admin', '10.0.0.' . $i);
+        }
+        self::assertFalse($t->isLocked('admin', '10.0.0.99'));
+        // 25th attempt from yet another IP trips the per-username lockout.
+        $t->recordFailure('admin', '10.0.0.99');
+        self::assertTrue($t->isLocked('admin', '10.0.1.1'));
+    }
+
+    public function testPerUsernameLadderProgresses(): void
+    {
+        $now = 1000000;
+        $t = $this->throttle($now);
+        for ($i = 0; $i < 25; ++$i) {
+            $t->recordFailure('admin', '10.0.0.' . $i);
+        }
+        self::assertSame(60, $t->lockoutSecondsRemaining('admin', '10.99.0.1'));
+        for ($i = 25; $i < 50; ++$i) {
+            $t->recordFailure('admin', '10.0.0.' . $i);
+        }
+        self::assertSame(300, $t->lockoutSecondsRemaining('admin', '10.99.0.1'));
+        for ($i = 50; $i < 100; ++$i) {
+            $t->recordFailure('admin', '10.0.0.' . $i);
+        }
+        self::assertSame(1800, $t->lockoutSecondsRemaining('admin', '10.99.0.1'));
+    }
+
+    public function testLockoutSecondsRemainingReturnsLargerOfBuckets(): void
+    {
+        $now = 1000000;
+        $t = $this->throttle($now);
+        // 5 failures from one IP → per-IP locked for 60s.
+        for ($i = 0; $i < 5; ++$i) {
+            $t->recordFailure('admin', '10.0.0.1');
+        }
+        // Plus 45 more from distinct IPs → per-username at count=50 → 300s.
+        for ($i = 0; $i < 45; ++$i) {
+            $t->recordFailure('admin', '10.1.0.' . $i);
+        }
+        // Asked from yet-another-IP, only the per-username lock applies — 300s.
+        self::assertSame(300, $t->lockoutSecondsRemaining('admin', '10.99.0.1'));
+        // Asked from the per-IP bucket's IP, 300s still wins (max of 60/300).
+        self::assertSame(300, $t->lockoutSecondsRemaining('admin', '10.0.0.1'));
+    }
+
+    public function testClearResetsBothBuckets(): void
+    {
+        $t = $this->throttle();
+        // Build per-username pressure.
+        for ($i = 0; $i < 25; ++$i) {
+            $t->recordFailure('admin', '10.0.0.' . $i);
+        }
+        self::assertTrue($t->isLocked('admin', '10.99.0.1'));
+        $t->clear('admin', '10.0.0.0');
+        self::assertFalse($t->isLocked('admin', '10.99.0.1'));
+        self::assertSame(0, $t->lockoutSecondsRemaining('admin', '10.99.0.1'));
+    }
+
+    public function testEmptyUsernameStillBucketsPerUsername(): void
+    {
+        $t = $this->throttle();
+        for ($i = 0; $i < 25; ++$i) {
+            $t->recordFailure('', '10.0.0.' . $i);
+        }
+        self::assertTrue($t->isLocked('', '10.99.0.1'));
+    }
+
     private function throttle(?int $fixedTime = null): LoginThrottle
     {
         if ($fixedTime === null) {