Cache only the last getAllowedMethods() lookup - #3468
Conversation
FastRouteDispatcher::getAllowedMethods() memoizes its result per requested URI in an instance property with no cap, TTL, or eviction. On dispatchers that outlive a single request (persistent-worker runtimes such as FrankenPHP worker mode, RoadRunner, Swoole, or any long-lived process reusing one App/Dispatcher instance, e.g. a queue worker or CLI daemon), an attacker can grow this cache without bound by requesting distinct, unmatched URIs, exhausting process memory. Cap the cache at 1000 entries with simple oldest-first eviction. This is a no-op for the traditional php-fpm-per-request model, where the dispatcher never survives past one request.
|
coverage: 99.456% (+0.001%) from 99.455% — kaii-k:fix/sec02-bound-allowed-methods-cache into slimphp:4.x |
|
Looking at this, I'm not sure that we actually need the array of allowedMethods at all. We only actually need one URI and one list of allowed methods. |
getAllowedMethods() only ever needs its most recent result: dispatch() calls it once per invocation for the current URI, and across calls the cache only pays off when the same URI repeats back-to-back. Replace the capped/evicting map with a single (uri, methods) pair, which is bounded by construction and removes the cap/eviction logic entirely.
|
Good point you're right, we don't need a history of URIs at all. Pushed a commit replacing the bounded map with a single |
akrabat
changed the title
3 participants
FastRouteDispatcher::getAllowedMethods()memoizes its result per requested URI in an instance property with no cap, TTL, or eviction. On dispatchers that outlive a single request — persistent-worker runtimes (FrankenPHP worker mode, RoadRunner, Swoole) or any long-lived process reusing oneApp/Dispatcherinstance (queue workers, CLI daemons, test/load harnesses) — an attacker can grow this cache without bound by requesting distinct, unmatched URIs, exhausting process memory over time.This caps the cache at 1000 entries with oldest-first eviction. It's a no-op for the standard php-fpm-per-request model, since the dispatcher never survives past one request there.
Reported privately via security@slimframework.com — this PR is the fix discussed there.
Test plan
testGetAllowedMethodsCacheIsBoundedreproducing unbounded growth against the cap.