feat: added rate limiter for API #3962
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a DB-backed API rate limiter with a new RateLimiter class, schema and migration for a Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant RL as RateLimiter
participant Conf as Configuration
participant DB as Database
Client->>RL: check(key, limit, interval)
RL->>Conf: getDb()
RL->>DB: INSERT window row (attempt)
DB-->>RL: success / duplicate / fail
alt duplicate
RL->>DB: UPDATE increment
DB-->>RL: success / fail
end
RL->>DB: SELECT currentRequests
DB-->>RL: currentRequests / fail
alt DB failure or unreadable count
RL->>Client: deny (false) + headers (Limit, Remaining=0, Reset, Retry-After)
else if currentRequests > limit
RL->>Client: deny (false) + headers (Limit, Remaining=0, Reset, Retry-After)
else
RL->>Client: allow (true) + headers (Limit, Remaining, Reset)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Http/RateLimiter.php`:
- Around line 43-108: The check method in RateLimiter has a TOCTOU race between
the SELECT and subsequent INSERT/UPDATE allowing over‑limit requests; fix by
making the increment atomic: either perform an upsert that increments requests
in one statement (e.g. INSERT ... ON DUPLICATE KEY UPDATE requests = requests +
1) against the faqrate_limits table and then read the resulting requests count,
or wrap the SELECT + UPDATE in a DB transaction with a row lock (SELECT ... FOR
UPDATE) before mutating; ensure you update the same logic in RateLimiter::check
to compute remaining/headers from the post‑increment requests value (and handle
insert conflicts/errors) so concurrent requests cannot exceed the limit.
- Around line 78-86: The INSERT in RateLimiter when $currentRequests === 0 can
silently fail on duplicate-key race; change the logic after building
$insertQuery and calling $db->query($insertQuery) to check the return value and,
on failure, perform a defensive fallback UPDATE that increments requests for the
same rate_key/window_start (or re-query the current count and proceed
accordingly). Specifically, in the RateLimiter method that uses $currentRequests
and $insertQuery, detect a failed $db->query() (or a duplicate-key error) and
run an UPDATE ... SET requests = requests + 1 (or reselect and update) for the
same $escapedKey/$windowStart so the second concurrent writer does not get
silently ignored; ensure the method then uses the updated request count to
compute $remaining.
In `@tests/phpMyFAQ/Http/RateLimiterTest.php`:
- Around line 14-65: The RateLimiter::check method has a TOCTOU race between
SELECT and separate INSERT/UPDATE and doesn't verify write success; fix by
making the increment atomic and checking DB results: change RateLimiter::check
to use an atomic upsert (e.g. INSERT ... ON DUPLICATE KEY UPDATE / INSERT ... ON
CONFLICT) or wrap the read+increment in a transaction with SELECT ... FOR UPDATE
to lock the row, then perform the increment/update, and ensure the query/execute
return values are validated (throw or treat as denied on failure). Update
references in RateLimiter::check and any helper DB calls so
remaining/limit/reset headers are computed from the authoritative DB result
only.
🧹 Nitpick comments (3)
phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php (1)
492-501: Consider adding a cleanup strategy for expired rate-limit rows.Every time window produces a new row per key, and nothing ever deletes old ones. Over time, this table will grow unboundedly. Consider adding a periodic purge (e.g., a cron job or a migration that adds a TTL-based cleanup) to remove rows where
window_start + interval < now(). An index onwindow_startwould also help efficient bulk deletes.phpmyfaq/src/phpMyFAQ/Http/RateLimiter.php (1)
110-116:getHeaders()is redundant with the public$headersproperty.Both
getHeaders()and$headersexpose the same data. Consider keeping only one access path to reduce API surface area. If the property hook is the preferred approach,getHeaders()could be removed (or vice versa).tests/phpMyFAQ/Http/RateLimiterTest.php (1)
29-50: Test correctly covers the "first request in window" path.The mock setup simulates no prior requests, and assertions verify the expected headers. Consider also asserting that the INSERT query was issued (e.g., using
$this->db->expects($this->once())for the insert call) to confirm state is persisted.
# Conflicts: # phpmyfaq/src/phpMyFAQ/Instance/Database.php # phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php # tests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.php # tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php # tests/phpMyFAQ/Setup/Migration/Versions/Migration420AlphaTest.php
thorsten
force-pushed
the
feat/rate-limiter
branch
from
a099633 to
6607bd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/translations/language_en.php`:
- Around line 1675-1677: Update the hint for
$LANG_CONF['api.rateLimit.interval'] to match the actual default of 3600
seconds; change the hint text from "Standard: 60 seconds" to "Standard: 3600
seconds (1 hour)" so it aligns with the defaults set in DefaultDataSeeder and
Migration420Alpha.
🧹 Nitpick comments (4)
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php (1)
525-572: Migration forfaqrate_limitsis correct across all backends.The table schema and config entries are properly defined. One operational concern:
No cleanup mechanism for expired rate-limit windows. The
faqrate_limitstable will accumulate rows indefinitely since old windows are never pruned. Consider adding a scheduled cleanup (e.g.,DELETE FROM faqrate_limits WHERE window_start < :cutoff) or documenting that administrators should periodically purge stale rows.phpmyfaq/src/phpMyFAQ/Http/RateLimiter.php (1)
49-50:$keyis interpolated into SQL viasprintf— relies on$db->escape()for safety.The
$escapedKeyis produced by$db->escape($key)and then string-interpolated into SQL with single quotes. While this follows the existing codebase pattern (e.g., other repositories using the same DB abstraction), it's inherently fragile — if a future caller passes a key containing unescaped characters that the DB driver doesn't handle, it could lead to SQL injection. Consider documenting that$keymust be a safe, predictable value (e.g., IP address or API token hash) rather than arbitrary user input.tests/phpMyFAQ/Http/RateLimiterTest.php (2)
57-82: Consider adding a boundary test whererequests == limit.The implementation uses
$currentRequests > $limit(strict greater-than), meaning the limit-th request is allowed withremaining = 0. There is no test covering this exact boundary (requests == limit), which is where off-by-one bugs typically hide. A test with e.g.limit = 3, requests = 3assertingcheck()returnstruewithremaining = 0would lock in that behavior.
31-32: Optional: Extract shared mock setup to reduce repetition.Every test method re-configures
escapeandnowidentically. Consider moving those two stubs intosetUp()to reduce boilerplate — the per-testquery/fetchObjectstubs can remain inline since they vary.♻️ Suggested refactor
protected function setUp(): void { parent::setUp(); Database::setTablePrefix(''); $this->configuration = $this->createMock(Configuration::class); $this->db = $this->createMock(DatabaseDriver::class); $this->configuration->method('getDb')->willReturn($this->db); + + $this->db->method('escape')->willReturnCallback(static fn (string $value): string => $value); + $this->db->method('now')->willReturn('CURRENT_TIMESTAMP'); }Then remove the duplicate
escape/nowstubs from each test method.Also applies to: 59-60, 86-87, 113-114, 135-136
1 participant
Summary by CodeRabbit
New Features
Database
Configuration
Services
Chore
Tests