Repository navigation
Conversation
|
847fd9f to
d5a1730
Compare
tomaj
left a comment
There was a problem hiding this comment.
Automated code review (Claude Code), 10 findings. The inline comments are numbered from most to least severe.
How these were checked: #1 and #2 were reproduced by compiling a real container, with the PSR clock interface stubbed because the local vendor/ did not match this branch. #3 to #10 come from reading the code and were not run.
| $this->endpointService($endpoint->authorization, $config->defaults->authorization, 'authorization', $endpoint) | ||
| ?? new Statement(NoAuthorization::class), | ||
| $this->endpointService($endpoint->rateLimit, $config->defaults->rateLimit, 'rateLimit', $endpoint), | ||
| $this->endpointRateLimit($endpoint, $config->defaults->rateLimit), |
There was a problem hiding this comment.
#1: A Redis outage takes down the whole API instead of failing open (reproduced)
Every limiter, its storage and the Redis client are built eagerly inside ApiDecider's addApi setups.
Use the config from docs/guide/rate-limit.md (services: redis: {factory: Redis, setup: [connect(...)]}, rateLimiting: {storage: redis, redis: @redis}) and stop Redis. Creating ApiDecider throws RedisException from the container, outside StorageRateLimiter's try/catch, so every request returns 500, including endpoints with rateLimit: false. This contradicts "A rate limit must not take the API down".
With a fake client whose connect() throws, getByType(ApiDecider::class) raised Connection refused. Every request also pays for a Redis connection, even on endpoints with no limit.
There was a problem hiding this comment.
Fixed. Storage is now created lazily on the first rate limited request (LazyStorage). If creating it fails, the request passes and it retries after 1 s. A config error (LogicException) still fails loudly. Tested with a Redis client that is down.
| if (!is_string($item)) { | ||
| throw new InvalidConfigurationException(sprintf('Item %d of the rate limit list in %s is %s, but it must be a name from api.rateLimits, @service or a class, for example rateLimit: [perIp, perUser] or #[RateLimit([\'perIp\', \'perUser\'])].', $index, $where, get_debug_type($item))); | ||
| } | ||
| $limiters[] = $this->service($item, $this->namedServices('rateLimit'), sprintf('%s[%d]', $where, $index), RateLimiterInterface::class, 'rateLimit'); |
There was a problem hiding this comment.
#2: A legacy limiter in a rate limit list causes a TypeError on all endpoints (reproduced)
A deprecated RateLimitInterface class or @service in a list passes checkType() but is not wrapped in LegacyRateLimitAdapter, so RateLimiterChain gets the wrong type.
rateLimit: [perIp, App\LegacyLimit] compiles, because checkType returns early for legacy classes at line 1210. Creating ApiDecider then throws TypeError: RateLimiterChain::__construct(): Argument #2 must be of type RateLimiterInterface, so all API endpoints return 500, not only that one. Only named api.rateLimits entries and single values reaching addApi() get wrapped, yet the error message advertises @service or a class as valid list items.
Suggested fix: wrap each list item with LegacyRateLimitAdapter::wrap, or reject legacy classes at compile time.
There was a problem hiding this comment.
Fixed. A legacy limiter in a list is now wrapped with LegacyRateLimitAdapter too. Added a test.
| private static function wait($handle, float $timeout): bool | ||
| { | ||
| $deadline = microtime(true) + $timeout; | ||
| while (!flock($handle, LOCK_EX | LOCK_NB)) { |
There was a problem hiding this comment.
#3: wait() treats any flock() failure as contention
On a filesystem without flock support, this spins for the full 3 seconds on every request and then throws.
With %tempDir% on NFS without lockd, or another filesystem where flock returns false with ENOLCK/ENOTSUP and $wouldBlock = 0, the LOCK_NB call never succeeds. Every rate-limited request sleeps 3 seconds per policy, then RateLimitStorageException lets it pass unlimited. The result is 3 seconds or more of added latency and no rate limit at all. The docblock on lock() promises the opposite: "null when the file cannot be used or locked (the counter is then updated without the lock)".
Suggested fix: pass flock's third $wouldBlock argument and return null when it is 0.
There was a problem hiding this comment.
Fixed. If the file system has no locks (NFS), we no longer wait: wait() returns null and the counter goes on without the lock. Added a test.
|
|
||
| $authorization = $api->getAuthorization(); | ||
| $rateLimit = $api->getRateLimit(); | ||
| $ip = $rateLimit instanceof NoRateLimit ? null : $this->context->getByType(IpDetectorInterface::class, false)?->getRequestIp(); |
There was a problem hiding this comment.
#4: Without an IpDetectorInterface service, every IP-keyed limit is silently skipped
The guide's "Without the DI extension" section adds SlidingWindowLimiter with new IpKey() and NoAuthorization via addApi(). That setup does not require an IpDetector service, so getByType(..., false) returns null and IpKey::key() returns null. StorageRateLimiter then returns [] and the endpoint has no limit, no RateLimit headers, no log entry and no error.
Suggested fix: throw or log when a limiter other than NoRateLimit is in use and no detector exists.
There was a problem hiding this comment.
Not changed yet. This needs a decision: fall back to a default IpDetector (header spoofing risk), throw a LogicException (loud, but breaks custom limiters that don't use the IP), or log a warning once.
There was a problem hiding this comment.
Fixed. Without an IpDetectorInterface service, IpKey now throws a LogicException that names the endpoint and says to register Tomaj\NetteApi\Misc\IpDetector or your own detector. It throws at runtime because the DI extension always registers a detector, so this can only happen without the extension and can't be checked at compile time. Only the ip key throws, so limiters that don't use the IP keep working without a detector. Added tests and docs.
| */ | ||
| private function customAlgorithm(mixed $algorithm, string $where): Statement|Reference | ||
| { | ||
| if ($algorithm instanceof Statement || (is_string($algorithm) && str_starts_with($algorithm, '@'))) { |
There was a problem hiding this comment.
#5: The check that a custom algorithm can run in Redis has gaps
The check is skipped for @service and Statement algorithms, and for storages that are RedisStorage under another name, so the failure appears at runtime as an uncaught LogicException.
algorithm: @myAlgo or App\MyAlgo(1) with storage: redis, or algorithm: App\PhpOnlyAlgo with storage: @myRedisStorage, passes compilation. RedisStorage::consume() line 49 throws \LogicException, which is not a RateLimitStorageException and is outside the try block in run(), so every request on that endpoint returns 500. The compile-time guard only runs when the value is a class-name string and storage === 'redis'.
There was a problem hiding this comment.
Fixed. The Redis Lua check now covers algorithms given as a statement or @service, and storage given as an own RedisStorage class or statement. Added tests.
| * | ||
| * @param array<mixed> $definition | ||
| */ | ||
| private function limiter(array $definition, string $policy, string $where): Statement |
There was a problem hiding this comment.
#6: An identity-keyed policy on an endpoint with no identity compiles and enforces nothing
Take {limit: 5, interval: 1 minute, key: identity} on an endpoint with authorization: false or NoAuthorization, or with a 3.x custom ApiAuthorizationInterface that lacks AuthorizationIdentityInterface. getIdentity is never available, so IdentityKey returns null and StorageRateLimiter returns [] on every request, with no 429, header or warning.
The guide mentions the skip, but this config mistake is detectable when the authorization class is known at compile time, as with the other typo checks in this extension. An upgrading app gets an API with no limit and no signal.
There was a problem hiding this comment.
Not changed yet. My proposal is a compile error when an endpoint's own rateLimit uses the identity key and its authorization doesn't implement AuthorizationIdentityInterface, with defaults exempt. The other options are a warning only, or leaving it as is.
There was a problem hiding this comment.
Fixed. An endpoint's own rateLimit keyed by identity (inline, named or in a list) now fails the compilation when its authorization is a known class without AuthorizationIdentityInterface, e.g. authorization: false or NoAuthorization. The message says to use key: ip or an authorization with an identity. defaults.rateLimit is exempt, and so is a service typed only by an interface. Added tests and docs.
| { | ||
| return <<<'LUA' | ||
| local limit, window, cost, now = tonumber(ARGV[1]), tonumber(ARGV[2]), tonumber(ARGV[3]), tonumber(ARGV[4]) | ||
| local start = math.floor(now / window) * window |
There was a problem hiding this comment.
#7: The Lua scripts lack the "window start never moves back" logic that step() has
Clock skew therefore behaves differently in Redis than in PHP storages.
Say server A's clock is 2s behind server B's. At a window edge, B writes to key ...:<new start> while A keeps using ...:<old start>, because start comes only from ARGV now. A client alternating between servers gets the old window's limit plus the new window's limit during the skew. SlidingWindow.php line 64 has the same gap, reading the wrong current and previous keys. With PHP storages, max($start, state start) prevents this.
This breaks the interface contract "The Lua script with the same logic as step()" and the PR's claim that the window start never moves back. Redis is the storage recommended for multi-server use, which is where skew occurs.
There was a problem hiding this comment.
Fixed. The window start now uses its own :start key, and the docs list the window keys.
| #[\Override] | ||
| public function consume(RateLimitAlgorithmInterface $algorithm, string $key, int $limit, int $interval, int $cost, float $now): StorageResult | ||
| { | ||
| $step = $algorithm->step($this->load($key, $now), $limit, $interval, $cost, $now); |
There was a problem hiding this comment.
#8: Fail-open depends on each KeyValueStorage subclass wrapping its own exceptions
The try/catch lives in NetteCacheStorage rather than in the base class.
A custom Memcached or PDO storage extending KeyValueStorage, as the guide suggests, throws PDOException from load() during a database outage. StorageRateLimiter catches only RateLimitStorageException, and checkRateLimit runs outside the presenter's handler try block, so every request fails instead of passing with a warning.
Suggested fix: move the load/save try/catch from NetteCacheStorage lines 152-167 into KeyValueStorage::consume(), so every subclass fails open and the duplicate code goes away.
There was a problem hiding this comment.
Fixed. Errors from load/save in KeyValueStorage are wrapped into RateLimitStorageException, so a broken custom storage lets the request pass. Added a test.
| if (!is_dir($directory)) { | ||
| @mkdir($directory, $this->private ? 0700 : 0777, true); // @ - another process may create it at the same time | ||
| } | ||
| $handle = @fopen($file, 'c'); // @ - a missing lock only means the old race |
There was a problem hiding this comment.
#9: With umask 022, a second user silently runs without any lock
php-fpm (www-data) creates %tempDir%/nette-api-rate-limit as 0755 and N.lock as 0644. A cron or worker running as another user gets EACCES from fopen($file, 'c'), which @ suppresses, so open() returns null and consume() runs unlocked with no log entry. The two users' read-modify-write cycles then overlap and the limit can be exceeded.
The docs say the umask permissions let "a web server and a cron of another user share them", which only holds with a umask like 002 and a shared group.
There was a problem hiding this comment.
Fixed. If the lock file belongs to another user and can't be opened for writing, it is opened read-only and still locks. Added a test.
| // a server with a clock behind the others must not reset the newer window of the others | ||
| $start = max($start, intdiv($state[0], $interval) * $interval); | ||
| } | ||
| $count = is_array($state) && ($state[0] ?? null) === $start && is_int($state[1] ?? null) ? $state[1] : 0; |
There was a problem hiding this comment.
#10: FixedWindow reads a SlidingWindow state as its own
RateLimitAlgorithmInterface warns that state may come from another algorithm after a config change ("check its shape, it may come from another algorithm"), but this only checks $state[0] and $state[1].
A policy changes from slidingWindow to fixedWindow while state [start, previous, current] is live in NetteCacheStorage or InMemoryStorage. If the window start matches, $state[1] is the previous window's count, not the current one. A client that used 90 of 100 last window and 5 this window is counted as 90 instead of 5, and a client at 0 last window and 95 this window is counted as 0 and gets a fresh quota until the next window starts.
Suggested fix: add count($state) === 2.
There was a problem hiding this comment.
Fixed together with the window key change, with tests for both algorithms.
6dbbace to
edaa1c1
Compare
Built-in rate limits behind interfaces: three algorithms, keys by IP, identity, endpoint or a combination, and storages in memory, in the Nette cache or atomically in Redis (ext-redis or Predis) via one Lua script per algorithm. ApiPresenter checks the limits before and after authorization and sends the IETF RateLimit and RateLimit-Policy headers plus Retry-After. A list of policies stops at the first rejection. A failing storage lets the request pass and logs a warning; a LogicException or an Error is not hidden. The DI extension adds named policies, an app-wide default with per-endpoint override or disable, inline limits, #[RateLimit] and rateLimiting.logger, with compile-time checks of identity limits, Redis clients and keys. RateLimitInterface keeps working through LegacyRateLimitAdapter and is deprecated, to be removed in 5.0. IpDetector now takes the address from IRequest::getRemoteAddress(), so forwarded headers are trusted only from proxies in http: proxy:, and the ip key counts an IPv6 client per /64. Closes #195
edaa1c1 to
7616aba
Compare
Summary
Built-in rate limiting for API endpoints (#195):
InMemoryStorage,NetteCacheStorage(atomic on one server through a file lock, documented) andRedisStorage, which is atomic through Lua and works with ext-redis (\Redis,\RedisCluster) or Predis. Both clients are listed insuggest.RateLimitKeyInterface; IPv6 counts per /64.RateLimitandRateLimit-Policy, plusRetry-Afteron 429.#[RateLimit]. Time comes from psr/clock.RateLimitInterfacekeeps working throughLegacyRateLimitAdapter. It is deprecated and will be removed in 5.0.examples/rate-limitwith e2e.Closes #195
Closes #234
Short names for policies are also covered, which resolves #229. I've left that issue open on purpose.
Decisions
@infection-ignore-allinSlidingWindow: themax(1 → 2)mutant is equivalent. The numerator is only positive when the previous window count is at least 2, so both versions of themaxreturn the same value.Nette Cache storage is atomic only on one server: it locks one of 64 striped files in
%tempDir%/nette-api-rate-limitaround read and write (lockDirectory: falseturns it off). Servers sharing one cache do not share the lock, so the docs point them to Redis.The lock waits at most 3 seconds (
lockTimeout): then the storage throws and the request passes with a warning, like any storage failure. The application directory keeps umask permissions so a web server and a cron of different users share it; only the default directory in the system temp is per user (0700/0600) and ignores files of other users.@infection-ignore-allonNetteCacheStorage::wait(): the poll interval and the deadline comparison only shift the timing by a millisecond, so those mutants are equivalent; the timeout itself is tested.Storage failures let requests through: a broken storage logs a warning instead of returning 500 or 429.
The legacy deprecation is silenced: it uses an
@-silencedE_USER_DEPRECATED, so it doesn't break apps that turn notices into exceptions.New requirements: nette/caching and psr/clock are now required.
IpDetectortrusts forwarded headers only from configured proxies (IpDetector: trust forwarded headers only from configured proxies #234): it returnsNette\Http\IRequest::getRemoteAddress(), soClient-IPandX-Forwarded-Forfrom a client are ignored. This is a breaking change for token IP restrictions, logs andkey: ip: behind a reverse proxy withouthttp: proxy:every client has the address of the proxy, documented first inUPGRADE-4.0.md; in debug mode withouthttp: proxy:the default detector logs one warning with the fix on the first request with anX-Forwarded-FororForwardedheader (production stays quiet, a client can send the header too). An ownapi.ipDetectoris used as it is. Values longer than 45 characters are hashed.An inline
namekeeps the counters per endpoint: the extension addsendpointto the key, sonamechanges only the policy in the headers. Two named policies with the samenamefail the compilation, because they would share their counters.Token bucket with skewed clocks: the stored time never moves back (
max(now, stored), in PHP and in Lua), so a server whose clock is behind cannot refill the bucket twice. The window start never moves back either;Retry-Afterand TTLs use the clock of the answering server, and the docs ask for NTP.Storage is created lazily:
api.rateLimiting.storageis aLazyStoragethat creates the real storage on the first rate limited request, so a Redis that is down doesn't break the container or requests without a limit. A failed creation lets the request pass and is retried after 1 s; aLogicException(a config error) still fails loudly.A lock file of another user is opened read-only:
flockworks on it too, so the lock is still shared.No locks on the file system (NFS) means no waiting: the counter goes on without the lock instead of waiting 3 s on every request.
A missing IP detector fails at runtime, only for the
ipkey: a null IP means the container has no detector,IpKeythen throws aLogicException, which goes to the error handler as a500; limiters without the IP keep working.A list of policies stops at the first rejection: rejected requests must not use up a quota shared with others, so the limits of one client go before the shared ones. This holds within one stage: a policy before the authorization is counted before a policy keyed by
identitycan reject the request, which the guide says.The client IP is read only when a limiter needs it (
RateLimitContext::ip()): an error of the IP detector does not fail limiters without the IP. A failing detector in a list can still leave a count in the limits before it; the request answers500and the error is logged.An own IP detector is always found: a service with
autowired: false,selfor a list of its class gets an autowired alias, likeapi.ipDetector: @service.A broken logger keeps the response of the handler: the handler has run, so the error only goes to the Tracy log (
error_logwhen Tracy cannot log), not to the error handler, whose response would be thrown away.A clock far ahead rejects rather than bypasses: the newest window or time always wins, so the limit holds with any difference of the clocks, and a server far ahead rejects clients on the others until its time passes. A bound on this was tried and reverted: with clocks more than one interval apart it let 100 of 100 requests through a limit of 10.
An autowired
RateLimitInterfaceof 3.x: Nette DI does not autowire the new union type, soaddApi()without the 4th argument looks the service up itself;nullmeans no limit. The default is an internalAutowiredRateLimitmarker instead offunc_num_args(); with more such servicesaddApi()throws aLogicExceptionthat names them and says how to fix it, instead of silently using none.Fail-open covers outages, not bugs: any exception of a storage except a
\LogicExceptionlets the request pass with a warning; a\LogicExceptionor an\Errorgoes to the error handler.An identity limit without identity fails the compilation only for the endpoint's own
rateLimit:defaults.rateLimitis shared by endpoints with and without an authorization, so it is exempt; an authorization whose class is not known (a service typed by an interface) is not checked.The missing-identity warning is logged once per process and policy: it is the same on every request, so it must not flood the log.
@infection-ignore-allonAutowiredRateLimit::consume(): the marker never counts, the$costdefault only repeats the interface.@infection-ignore-allonLazyStorage::waitsForRetry():<and<=on a float time are equivalent.Review log
Rounds 1 to N, on code, tests and docs:
Review round 1, a deep review of the whole PR:
InMemoryStoragegrew without a bound in long-running workers; expired counters are now removed as it grows;IpKey: a forged longX-Forwarded-Formade long storage keys; values longer than an IP are hashed;namemade every endpoint share one counter, which contradicted the docs; fixed;nameshared their counters silently; this now fails the compilation, with a fix in the message;namebehaviour, the duplicate-name error, the IP spoofing warning, NTP for more servers, memory pruning and theFileStoragecleanup; CHANGELOG updated.Review round 2: clean. Every option, default, neon key, attribute form, header and example was checked against the code, along with EN/SK parity, the custom algorithm guide, CHANGELOG and UPGRADE.
Review round 3, an adversarial full re-review:
99999999999999999999 daysfail with a clear message instead of an integer overflow;error_log;prefix+ key instead of the real keyprefix{key};namedid not addendpointto the key, so two endpoints whose names map to one policy name (users.listvsusers+list) shared counters; inline limits now always count per endpoint,EndpointKeyencodes its parts, and an inlinenameequal to a named policy fails the compilation;KEYS[1] .. ':gap', so switching a policy to the token bucket (a hash onKEYS[1]) cannot hitWRONGTYPE;Retry-Afteralways passes), the IETF header format, CORS preflight, logging of 429, Problem Details, attribute discovery and UPGRADE.Review round 4: clean. Re-read the round 3 changes and the EN/SK docs against the code.
Review round 5, property and concurrency tests on every storage, vendor API audit, threat model, DI matrix,
php -Sload tests and a sentence-by-sentence docs audit:flock, DI puts the locks in%tempDir%;%.17g;|could collide (a|b+cvsa+b|c); parts are escaped now;Retry-Afteris sent as0;%tempDir%;\wregexes depended on the locale and one had noDmodifier; names are ASCII now;exceptions: falsereported an EVAL error as an unexpected result; it now says the EVAL failed;MemoryStorage/SQLiteStorage/DevNullStoragecaveats, clock skew, limit range, key and header escaping, own Redis client andKeyValueStoragelocking, SK comments translated; CHANGELOG updated;php -Swith a limit of 50, for cache, ext-redis and Predis and all three algorithms: always exactly 50 × 200 and 150 × 429, valid headers, no duplicatervalues.Review round 6: a lighter re-check of the round 5 changes; not counted as a clean full round.
Review round 7, new lock tests (contention, timeout, crash while locked, deleted file, foreign files, 64-lock spread), threat model of the round 5 changes, flock and Nette cache semantics, mixed load tests:
0700, files0600) and files of other users are not used;q/r/theaders; all are sent as0;perIpbefore andperKeyafter the authorization, both active, every algorithm combination, on cache, ext-redis and Predis: exactly 25 × 200 per key, exactly 100 requests passedperIp, no duplicater.Review round 8:
0700directory, owner checks) also applied to%tempDir%, so with a web server and a cron of different users the second one silently lost the lock; it now applies only to the default system temp directory;sys_temp_dir.Review round 9: clean. Re-read the lock code, the docs (EN and SK) and CHANGELOG, re-ran the mixed load tests.
Review round 10:
LegacyRateLimitAdapter;lockTimeout;RedisStorage;:startkey now;KeyValueStoragewere not turned intoRateLimitStorageException;IpDetectorInterfaceservice everyiplimit was skipped silently;IpKeynow throws aLogicExceptionnaming the endpoint and the fix;identitylimit with an authorization without identity compiled and never applied; it now fails the compilation.Review round 11, three review passes over the whole PR and its fixes:
key: ipcould be bypassed by sending a newX-Forwarded-Forwith every request;IpDetectorreturns the address from Nette now (IpDetector: trust forwarded headers only from configured proxies #234), and an IPv6 client counts per /64;addApi()without the 4th argument silently lost an autowiredRateLimitInterface;storageorstorageFactorybroke the container;getIdentity()escaped the error handler;@servicein named policies;Tracy\ILoggerservices failed the compilation;rateLimiting.loggeris explicit now;twas shorter than the real wait, in PHP and Lua;#[RateLimit('name', key: ...)]ignored the options, a 3.x limiter sentt=0, an emptyREMOTE_ADDRskipped the limit, a key class with constructor arguments failed with a raw error;getIdentity()in UPGRADE, every neon example of the guide compiled.Review round 12, the whole PR after IpDetector: trust forwarded headers only from configured proxies #234, and a verification of its fixes:
http: proxy:; a prominent warning in UPGRADE and the authorization guide;autowired: false,self), andapi.ipDetector: @servicewith an autowired service, broke at runtime; a broken logger lost the response of the handler;key: []became one quota for all, the wrong hint for a named policy withoutlimit, the Tracy claim about the deprecation, the Redis timeouts per limiter andread_write_timeoutof Predis;addApi()with 3 arguments (PHP cannot tell a neon call from a PHP call),EVALinstead ofEVALSHA, token bucket writes on rejection, IPv6 rotation beyond a /64, keys not declared inKEYSfor Redis-compatible servers that enforce it (untested),RedisTestCase::tearDown()without ext-redis.Review round 13, fixes of the review of the whole PR:
http: proxy:nothing warned; theIpDetectorchange is the first item of UPGRADE, and in debug mode the first forwarded header logs a warning with the fix;addApi()without the 4th argument and more autowiredRateLimitInterfaceservices used no limit silently; it throws aLogicExceptionnaming them,func_num_args()is replaced by a default marker;ApiDecider, "is not a @service nor a class", a docblock on whyRateLimitContext::ip()is a method andidentitya property;http: proxy:only before the http extension normalized it, so a configured proxy still warned; it reads both forms now.Verification