Переглянути джерело

fix: bound BlocklistCache with a 16-policy LRU (SEC_REVIEW F71)

`BlocklistCache::$cache` was a plain `array<int, …>` keyed by
`policy_id` with no bound. An admin can create unlimited policies,
and every policy ever requested via /api/v1/blocklist accumulated
in the cache for the worker's lifetime — Build cost stayed bounded
by the 30s TTL, but the resident-set memory grew monotonically.

Add an LRU bound. New `BlocklistCache::MAX_ENTRIES = 16` const +
matching `$maxEntries` constructor arg (defaults to the const).
Insertion goes through a private `store()` helper that

  - `unset()`s the key (a no-op if absent) then re-assigns,
    pushing the entry to the END of PHP's insertion-ordered
    array;
  - while `count() > $maxEntries`, drops `array_key_first()` —
    which is now the least-recently-used entry.

Reads through `getOrBuild` / `getRendered` mark the entry as
most-recently-used via a tiny `touch()` helper (same
unset-then-assign trick).

Worst-case memory is now bounded by 16 × the per-policy build
cost, regardless of how many policies the admin creates.

Two `@internal` accessors — `entryCount()` and
`cachedPolicyIds()` — let the regression test inspect cache state
without reaching into private fields via reflection. They're cheap
enough (count + array_keys) to leave on in production.

Regression test in
`api/tests/Integration/Public/BlocklistControllerTest.php`:

  - `testCacheIsLruBoundedAtSixteenEntries` rebuilds the cache
    binding with a 30s TTL (the test fixture default is 0s = no
    caching, which would skip the LRU path entirely), creates 18
    policies + one consumer per policy via direct DB inserts,
    hits /api/v1/blocklist for each, then asserts:
      - cache size ≤ MAX_ENTRIES (16),
      - the two oldest policy IDs are NOT in the cache,
      - the most-recent policy ID IS in the cache.

The autowired BlocklistController is rebuilt via `$c->make(...)`
after the cache rebind so it picks up the new cache reference —
mirroring the JobsAdminControllerTest pattern.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 3 днів тому
батько
коміт
1bcf7f312c

+ 81 - 3
api/src/Infrastructure/Reputation/BlocklistCache.php

@@ -27,6 +27,16 @@ use App\Domain\Time\Clock;
  */
 class BlocklistCache
 {
+    /**
+     * SEC_REVIEW F71: max distinct policies kept in the cache map.
+     * Admins can create unbounded numbers of policies; without this
+     * cap the cache grew monotonically over the worker's lifetime.
+     * 16 covers any realistic deployment (one policy per consumer
+     * tier is the typical pattern) while bounding worst-case memory
+     * to ~16 × the per-policy build cost.
+     */
+    public const MAX_ENTRIES = 16;
+
     /**
      * @var array<int, array{blocklist: Blocklist, expires_at: float, rendered: array<string, array{body: string, etag: string}>}>
      */
@@ -36,6 +46,7 @@ class BlocklistCache
         private readonly BlocklistBuilder $builder,
         private readonly Clock $clock,
         private readonly int $ttlSeconds = 30,
+        private readonly int $maxEntries = self::MAX_ENTRIES,
     ) {
     }
 
@@ -44,16 +55,20 @@ class BlocklistCache
         $now = (float) $this->clock->now()->getTimestamp();
         $entry = $this->cache[$policy->id] ?? null;
         if ($entry !== null && $now < $entry['expires_at']) {
+            // SEC_REVIEW F71: touch the entry on read so the LRU
+            // eviction sees it as recently used.
+            $this->touch($policy->id);
+
             return $entry['blocklist'];
         }
 
         $blocklist = $this->builder->build($policy);
         if ($this->ttlSeconds > 0) {
-            $this->cache[$policy->id] = [
+            $this->store($policy->id, [
                 'blocklist' => $blocklist,
                 'expires_at' => $now + $this->ttlSeconds,
                 'rendered' => [],
-            ];
+            ]);
         }
 
         return $blocklist;
@@ -102,7 +117,10 @@ class BlocklistCache
         }
 
         if ($this->ttlSeconds > 0) {
-            $this->cache[$policy->id] = $entry;
+            // SEC_REVIEW F71: write through `store()` so the LRU
+            // eviction marks this entry most-recently-used and drops
+            // the oldest entry past `maxEntries`.
+            $this->store($policy->id, $entry);
         }
 
         return [
@@ -121,4 +139,64 @@ class BlocklistCache
     {
         $this->cache = [];
     }
+
+    /**
+     * Number of policies currently held in the cache. Test-only:
+     * lets a regression spec assert the LRU bound is enforced
+     * without reaching into the private array via reflection.
+     *
+     * @internal
+     */
+    public function entryCount(): int
+    {
+        return count($this->cache);
+    }
+
+    /**
+     * Test-only: the policy IDs in the cache, in LRU-eviction order
+     * (oldest first, most-recently-used last).
+     *
+     * @return list<int>
+     * @internal
+     */
+    public function cachedPolicyIds(): array
+    {
+        return array_keys($this->cache);
+    }
+
+    /**
+     * SEC_REVIEW F71: write a cache entry and enforce the
+     * `maxEntries` LRU bound. PHP arrays preserve insertion order,
+     * so unsetting then assigning the key pushes it to the end of
+     * the iteration order — making `array_key_first()` the
+     * least-recently-used entry to evict.
+     *
+     * @param array{blocklist: Blocklist, expires_at: float, rendered: array<string, array{body: string, etag: string}>} $entry
+     */
+    private function store(int $policyId, array $entry): void
+    {
+        unset($this->cache[$policyId]);
+        $this->cache[$policyId] = $entry;
+        while (count($this->cache) > $this->maxEntries) {
+            $oldest = array_key_first($this->cache);
+            if ($oldest === null) {
+                break;
+            }
+            unset($this->cache[$oldest]);
+        }
+    }
+
+    /**
+     * Mark an existing entry as most-recently-used. No-op if the
+     * entry isn't in the cache.
+     */
+    private function touch(int $policyId): void
+    {
+        if (!array_key_exists($policyId, $this->cache)) {
+            return;
+        }
+        $entry = $this->cache[$policyId];
+        unset($this->cache[$policyId]);
+        $this->cache[$policyId] = $entry;
+    }
 }

+ 75 - 0
api/tests/Integration/Public/BlocklistControllerTest.php

@@ -245,6 +245,81 @@ final class BlocklistControllerTest extends AppTestCase
         self::assertSame($r1->getHeaderLine('ETag'), $r2->getHeaderLine('ETag'));
     }
 
+    public function testCacheIsLruBoundedAtSixteenEntries(): void
+    {
+        // SEC_REVIEW F71: BlocklistCache is per-process and grew
+        // monotonically with no LRU. With unbounded admin policy
+        // creation, memory usage was effectively unbounded over the
+        // worker's lifetime. Cache now caps at MAX_ENTRIES (16) and
+        // evicts the least-recently-used entry when full.
+        //
+        // The default test fixture sets `blocklist_cache_ttl_seconds=0`
+        // (no caching) for deterministic per-test builds. Rebuild
+        // the cache with a real TTL so we exercise the LRU path.
+        if (method_exists($this->container, 'set')) {
+            /** @var \DI\Container $c */
+            $c = $this->container;
+            $c->set(
+                \App\Infrastructure\Reputation\BlocklistCache::class,
+                new \App\Infrastructure\Reputation\BlocklistCache(
+                    builder: $c->get(\App\Domain\Reputation\BlocklistBuilder::class),
+                    clock: $c->get(\App\Domain\Time\Clock::class),
+                    ttlSeconds: 30,
+                ),
+            );
+            // BlocklistController is autowired and singleton-scoped, so
+            // it captured the OLD cache reference at first resolution.
+            // Rebuild it so it picks up the new cache binding.
+            $c->set(
+                \App\Application\Public\BlocklistController::class,
+                $c->make(\App\Application\Public\BlocklistController::class),
+            );
+            $this->app = \App\App\AppFactory::build($this->container);
+        }
+        /** @var \App\Infrastructure\Reputation\BlocklistCache $cache */
+        $cache = $this->container->get(\App\Infrastructure\Reputation\BlocklistCache::class);
+
+        // Create 18 policies and a consumer-token per policy. Hit
+        // /blocklist for each so each policy ends up in the cache.
+        // The LRU should evict the first 2 by the time we reach the
+        // last (cache cap is 16).
+        $policyIds = [];
+        for ($i = 1; $i <= 18; $i++) {
+            $this->db->insert('policies', [
+                'name' => 'lru-policy-' . $i,
+                'description' => null,
+                'include_manual_blocks' => 1,
+            ]);
+            $policyId = (int) $this->db->lastInsertId();
+            $policyIds[] = $policyId;
+            $this->db->insert('consumers', [
+                'name' => 'lru-consumer-' . $i,
+                'policy_id' => $policyId,
+                'is_active' => 1,
+            ]);
+            $consumerId = (int) $this->db->lastInsertId();
+            $token = $this->createToken(TokenKind::Consumer, consumerId: $consumerId);
+
+            $resp = $this->request('GET', '/api/v1/blocklist', [
+                'Authorization' => 'Bearer ' . $token,
+            ]);
+            self::assertSame(200, $resp->getStatusCode(), "consumer {$i} should succeed");
+        }
+
+        // Cache must never exceed the MAX_ENTRIES bound.
+        self::assertLessThanOrEqual(
+            \App\Infrastructure\Reputation\BlocklistCache::MAX_ENTRIES,
+            $cache->entryCount(),
+            'cache must not grow past MAX_ENTRIES policies',
+        );
+        // The two oldest policies (first two created) must have been
+        // evicted; the most recently used must still be present.
+        $remaining = $cache->cachedPolicyIds();
+        self::assertNotContains($policyIds[0], $remaining, 'oldest policy must have been evicted');
+        self::assertNotContains($policyIds[1], $remaining, 'second-oldest policy must have been evicted');
+        self::assertContains(end($policyIds), $remaining, 'most recent policy must still be cached');
+    }
+
     public function testTextAndJsonRendersBothCachedIndependently(): void
     {
         // SEC_REVIEW F70: switching format must not invalidate the