Преглед на файлове

fix: bound RateLimiter bucket map with LRU eviction (SEC_REVIEW F28)

The `$buckets` array on the `RateLimiter` singleton was never evicted.
Long-lived FrankenPHP workers therefore accumulated one entry per
distinct bucket key forever — every rotated/revoked token, every
unauthenticated source IP that ever hit a 401, every dead bucket
sitting at full refill. Memory grew without bound and an attacker
could plausibly cause noticeable RSS growth by churning IPs against
pre-auth paths (the `ip:*` namespace now covered by F27).

`RateLimiter` now caps the map at a configurable `maxBuckets`
(default 10 000). Every `tryConsume()` re-inserts the touched key at
the end of the PHP insertion-ordered array, so the front of the array
is the least-recently-touched entry; on overflow the oldest entries
are dropped in batches of 256 so eviction amortises across requests
instead of running on every call once we're at steady state.

Behavioural invariant: a dropped bucket comes back as a fresh
full-capacity bucket on next access — equivalent to the bucket having
idled long enough to refill (`capacity / refillPerSecond` seconds), so
eviction never grants more tokens than the configured rate already
permits. Regression tests pin all three properties: the cap is
enforced, an evicted bucket returns at full capacity, and a hot key
is never evicted in favour of cold churn keys.

The multi-replica half of F28 ("a fresh bucket per replica behind a
non-sticky LB") is a SPEC §10 topology constraint already documented
("token-bucket; in-process per replica — acceptable for single-replica
deployments"); horizontal `api` scaling requires sticky LB and a
shared-backend limiter is tracked as future scaling work.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa преди 3 дни
родител
ревизия
e09964b4ad
променени са 2 файла, в които са добавени 198 реда и са изтрити 4 реда
  1. 84 4
      api/src/Infrastructure/Http/RateLimiter.php
  2. 114 0
      api/tests/Unit/Http/RateLimiterTest.php

+ 84 - 4
api/src/Infrastructure/Http/RateLimiter.php

@@ -19,37 +19,90 @@ use App\Domain\Time\Clock;
  * Refill is computed lazily on each `tryConsume()` call, so an idle bucket
  * jumps from empty to full once `2/refill` seconds elapse.
  *
- * Holds state in a PHP array on a singleton instance — fine for a single
- * api replica. PROGRESS.md flags multi-replica deployments as needing a
- * shared store.
+ * **Memory bound (SEC_REVIEW F28):** the bucket map is capped at
+ * `maxBuckets` entries (default {@see self::DEFAULT_MAX_BUCKETS}). Every
+ * `tryConsume()` re-inserts the touched key at the end of the PHP array,
+ * so insertion order tracks LRU; on overflow the oldest entries are
+ * dropped in batches of {@see self::EVICTION_BATCH} so the eviction cost
+ * amortises across requests instead of running on every call once we're
+ * at steady state. A dropped bucket comes back as a fresh full-capacity
+ * bucket on next access — equivalent to the bucket having idled long
+ * enough to refill, so eviction never grants an attacker more tokens
+ * than the configured rate would already permit. The cap protects
+ * long-lived FrankenPHP workers from token churn (rotated tokens, IP
+ * fan-out from unauthenticated traffic) that would otherwise grow
+ * `$buckets` without bound.
+ *
+ * **Multi-replica:** state is per-process and per-replica. SPEC §10
+ * documents single-replica as the supported topology for this limiter
+ * ("token-bucket; in-process per replica — acceptable for single-replica
+ * deployments"). Horizontal `api` scaling requires sticky-LB so a client
+ * consistently lands on the same replica's bucket; replacing the
+ * limiter with a shared backend (Redis / DB) is tracked separately as
+ * future scaling work.
  */
 final class RateLimiter
 {
+    /**
+     * Default cap on distinct keys retained in memory. Two orders of
+     * magnitude above the realistic concurrent working set (a handful of
+     * authenticated tokens plus per-IP unauthenticated buckets) — well
+     * below a memory-pressure threshold even at one bucket per row.
+     */
+    private const DEFAULT_MAX_BUCKETS = 10_000;
+
+    /**
+     * When the cap is exceeded, drop this many entries past the cap so
+     * eviction amortises across many calls. Must be smaller than
+     * `maxBuckets` so we never evict the entry we just inserted.
+     */
+    private const EVICTION_BATCH = 256;
+
     /** @var array<string, array{tokens: float, updated: float}> */
     private array $buckets = [];
 
+    private readonly int $maxBuckets;
+
     public function __construct(
         private readonly Clock $clock,
         private readonly float $refillPerSecond,
         private readonly float $capacity,
+        ?int $maxBuckets = null,
     ) {
+        $resolved = $maxBuckets ?? self::DEFAULT_MAX_BUCKETS;
+        if ($resolved <= self::EVICTION_BATCH) {
+            throw new \InvalidArgumentException(
+                'maxBuckets must be greater than EVICTION_BATCH (' . self::EVICTION_BATCH . ')'
+            );
+        }
+        $this->maxBuckets = $resolved;
     }
 
     public function tryConsume(string $key): bool
     {
         $now = (float) $this->clock->now()->getTimestamp() + ((float) $this->clock->now()->format('u') / 1_000_000.0);
 
-        $state = $this->buckets[$key] ?? ['tokens' => $this->capacity, 'updated' => $now];
+        // Re-insert at end of array so PHP's insertion-order semantics
+        // give us LRU for free: oldest entry is at the front.
+        if (isset($this->buckets[$key])) {
+            $state = $this->buckets[$key];
+            unset($this->buckets[$key]);
+        } else {
+            $state = ['tokens' => $this->capacity, 'updated' => $now];
+        }
+
         $elapsed = max(0.0, $now - $state['updated']);
         $tokens = min($this->capacity, $state['tokens'] + ($elapsed * $this->refillPerSecond));
 
         if ($tokens < 1.0) {
             $this->buckets[$key] = ['tokens' => $tokens, 'updated' => $now];
+            $this->enforceCapacity();
 
             return false;
         }
 
         $this->buckets[$key] = ['tokens' => $tokens - 1.0, 'updated' => $now];
+        $this->enforceCapacity();
 
         return true;
     }
@@ -59,4 +112,31 @@ final class RateLimiter
     {
         $this->buckets = [];
     }
+
+    /** Test hook: how many keys are currently tracked. */
+    public function bucketCount(): int
+    {
+        return count($this->buckets);
+    }
+
+    private function enforceCapacity(): void
+    {
+        $size = count($this->buckets);
+        if ($size <= $this->maxBuckets) {
+            return;
+        }
+
+        // Drop the oldest `(size - maxBuckets) + EVICTION_BATCH` entries.
+        // Adding the batch keeps us below the cap for the next batch of
+        // unique-key arrivals so we don't pay an eviction on every call.
+        $toDrop = ($size - $this->maxBuckets) + self::EVICTION_BATCH;
+        $i = 0;
+        foreach ($this->buckets as $k => $_) {
+            if ($i >= $toDrop) {
+                break;
+            }
+            unset($this->buckets[$k]);
+            $i++;
+        }
+    }
 }

+ 114 - 0
api/tests/Unit/Http/RateLimiterTest.php

@@ -80,4 +80,118 @@ final class RateLimiterTest extends TestCase
         self::assertTrue($limiter->tryConsume('ip:42'));
         self::assertFalse($limiter->tryConsume('ip:42'));
     }
+
+    /**
+     * SEC_REVIEW F28: the bucket map must not grow without bound. When
+     * the cap is exceeded, the oldest entries (LRU front of the PHP
+     * insertion-ordered array) get dropped in a batch.
+     */
+    public function testBucketMapIsBoundedByMaxBucketsCap(): void
+    {
+        $clock = FixedClock::at('2026-04-29T00:00:00Z');
+        // Cap at 300 (must exceed the 256-entry eviction batch).
+        $limiter = new RateLimiter(
+            $clock,
+            refillPerSecond: 1.0,
+            capacity: 2.0,
+            maxBuckets: 300,
+        );
+
+        // Touch 600 distinct keys — well past the cap.
+        for ($i = 0; $i < 600; $i++) {
+            $limiter->tryConsume("ip:10.0.0.{$i}");
+        }
+
+        self::assertLessThanOrEqual(
+            300,
+            $limiter->bucketCount(),
+            'bucket count must stay within the configured cap'
+        );
+    }
+
+    /**
+     * SEC_REVIEW F28: an evicted bucket comes back as a fresh full-capacity
+     * bucket on its next consume — equivalent to the bucket having idled
+     * long enough to refill ($capacity / $refill seconds), so eviction
+     * never gives an attacker more tokens than the rate would already
+     * permit. This test asserts the behaviour explicitly so a future
+     * "make eviction smarter" change cannot regress it into something
+     * stricter (and break the documented refill semantics) or looser
+     * (and grant tokens beyond the documented refill).
+     */
+    public function testEvictedBucketReturnsAtFullCapacityOnReuse(): void
+    {
+        $clock = FixedClock::at('2026-04-29T00:00:00Z');
+        $limiter = new RateLimiter(
+            $clock,
+            refillPerSecond: 1.0,
+            capacity: 2.0,
+            maxBuckets: 300,
+        );
+
+        // Drain key=A.
+        self::assertTrue($limiter->tryConsume('ip:A'));
+        self::assertTrue($limiter->tryConsume('ip:A'));
+        self::assertFalse($limiter->tryConsume('ip:A'));
+
+        // Force eviction by inserting many fresh keys past the cap.
+        for ($i = 0; $i < 600; $i++) {
+            $limiter->tryConsume("ip:fan-{$i}");
+        }
+
+        // Same wall-clock time — without eviction, A would still be drained.
+        // With eviction, A is gone and a re-touch starts a fresh full bucket.
+        self::assertTrue($limiter->tryConsume('ip:A'));
+        self::assertTrue($limiter->tryConsume('ip:A'));
+        self::assertFalse($limiter->tryConsume('ip:A'));
+    }
+
+    /**
+     * SEC_REVIEW F28: re-touching a key moves it to the end of the PHP
+     * insertion-ordered array, so a hot key is the *last* candidate for
+     * eviction even under heavy churn from other keys. Asserting this
+     * keeps a regression that broke the LRU ordering from silently
+     * letting an attacker reset their own bucket by smashing many
+     * unrelated keys.
+     */
+    public function testRecentlyTouchedKeyIsNotEvicted(): void
+    {
+        $clock = FixedClock::at('2026-04-29T00:00:00Z');
+        $limiter = new RateLimiter(
+            $clock,
+            refillPerSecond: 1.0,
+            capacity: 2.0,
+            maxBuckets: 300,
+        );
+
+        // Drain ip:hot.
+        self::assertTrue($limiter->tryConsume('ip:hot'));
+        self::assertTrue($limiter->tryConsume('ip:hot'));
+        self::assertFalse($limiter->tryConsume('ip:hot'));
+
+        // Heavy fan-out from other keys, but keep re-touching hot so it
+        // stays at the LRU tail.
+        for ($i = 0; $i < 600; $i++) {
+            $limiter->tryConsume("ip:churn-{$i}");
+            $limiter->tryConsume('ip:hot'); // returns false; bucket stays drained
+        }
+
+        // hot still drained — eviction picked the cold churn keys, not hot.
+        self::assertFalse($limiter->tryConsume('ip:hot'));
+    }
+
+    public function testMaxBucketsBelowEvictionBatchIsRejected(): void
+    {
+        // A pathologically small cap would evict the entry we just
+        // inserted; refuse to construct.
+        $clock = FixedClock::at('2026-04-29T00:00:00Z');
+
+        $this->expectException(\InvalidArgumentException::class);
+        new RateLimiter(
+            $clock,
+            refillPerSecond: 1.0,
+            capacity: 2.0,
+            maxBuckets: 10,
+        );
+    }
 }