move some hardcoded ratelimits for the api to env vars#2332
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRate limit values were moved to env-backed entries in ChangesRate Limit Configuration and Consumption
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/http.php (1)
4-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the configuration comment to reflect the expanded scope.
The comment still only mentions "client and internal (application) APIs" but the configuration now includes many additional rate limits (auth, password reset, websocket, backup restore, database create, subuser create, file pull, and default). Additionally, it should mention that these values are configurable via environment variables.
📝 Suggested comment update
/* |-------------------------------------------------------------------------- | API Rate Limits |-------------------------------------------------------------------------- | - | Defines the rate limit for the number of requests per minute that can be - | executed against both the client and internal (application) APIs over the - | defined period (by default, 1 minute). + | Defines rate limits for various API endpoints and resources. Each rate + | limit consists of a period (in minutes) and a maximum number of requests + | allowed within that period. All values can be configured via environment + | variables and include sensible defaults. | */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/http.php` around lines 4 - 13, The comment block above API rate limits in config/http.php is outdated—update the multi-line comment that documents "API Rate Limits" to list the full set of rate-limited categories (auth, password reset, websocket, backup restore, database create, subuser create, file pull, default, client/internal APIs) and note that each limit is configurable via environment variables; change the descriptive text in the existing comment block (the one wrapping the API Rate Limits section) to briefly describe the expanded scope and mention env var configurability so future readers can find and adjust limits for keys like auth, password_reset, websocket, backup_restore, database_create, subuser_create, file_pull and default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Providers/RouteServiceProvider.php`:
- Around line 70-76: The default authentication rate limiter defined in
RateLimiter::for('authentication') is missing the per-IP scoping present in the
forgot-password branch; update the default Limit returned from
Limit::perMinutes(config('http.rate_limit.auth_period'),
config('http.rate_limit.auth')) to include ->by($request->ip()) so it is keyed
per client IP (same pattern used when
$request->route()->named('auth.post.forgot-password')).
---
Outside diff comments:
In `@config/http.php`:
- Around line 4-13: The comment block above API rate limits in config/http.php
is outdated—update the multi-line comment that documents "API Rate Limits" to
list the full set of rate-limited categories (auth, password reset, websocket,
backup restore, database create, subuser create, file pull, default,
client/internal APIs) and note that each limit is configurable via environment
variables; change the descriptive text in the existing comment block (the one
wrapping the API Rate Limits section) to briefly describe the expanded scope and
mention env var configurability so future readers can find and adjust limits for
keys like auth, password_reset, websocket, backup_restore, database_create,
subuser_create, file_pull and default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8f096904-8b79-4ec7-aebe-cd52ca6cf04b
📒 Files selected for processing (3)
app/Enums/ResourceLimit.phpapp/Providers/RouteServiceProvider.phpconfig/http.php
| RateLimiter::for('authentication', function (Request $request) { | ||
| if ($request->route()->named('auth.post.forgot-password')) { | ||
| return Limit::perMinute(2)->by($request->ip()); | ||
| return Limit::perMinutes(config('http.rate_limit.password_reset_period'), config('http.rate_limit.password_reset'))->by($request->ip()); | ||
| } | ||
|
|
||
| return Limit::perMinute(10); | ||
| return Limit::perMinutes(config('http.rate_limit.auth_period'), config('http.rate_limit.auth')); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the old version of this rate limiter to see if IP-based keying was present
# Search for the old authentication rate limiter implementation
git log -p --all -S 'RateLimiter::for' -- app/Providers/RouteServiceProvider.php | grep -A 10 "authentication"
# Also check if there are any other references to authentication rate limiting
rg -n "authentication.*rate|throttle.*authentication" -C 3Repository: pelican-dev/panel
Length of output: 50373
🏁 Script executed:
# Find and examine the RouteServiceProvider.php file
find . -name "RouteServiceProvider.php" -type f | head -5Repository: pelican-dev/panel
Length of output: 102
🏁 Script executed:
# Read the specific lines from the file
sed -n '65,80p' ./app/Providers/RouteServiceProvider.php
# Also check git history for changes to this rate limiter
git log --oneline --all -S 'RateLimiter::for' -- ./app/Providers/RouteServiceProvider.php | head -10Repository: pelican-dev/panel
Length of output: 1194
🏁 Script executed:
# Check the full git history for this rate limiter to see if ->by() was removed
git log -p --all -- ./app/Providers/RouteServiceProvider.php | grep -A 15 "RateLimiter::for('authentication" | head -60
# Also check if there are any other rate limiters in the codebase that use ->by() to understand the pattern
rg -A 3 "RateLimiter::for" ./app/Providers/RouteServiceProvider.phpRepository: pelican-dev/panel
Length of output: 725
🏁 Script executed:
# Check Laravel's Limit class documentation and default behavior
# First, let's see if there are any vendor files or composer dependencies
find . -path ./vendor -prune -o -name "*.php" -type f | xargs grep -l "class Limit" 2>/dev/null | head -5
# Also check the config file being referenced
cat ./config/http.php 2>/dev/null | head -50Repository: pelican-dev/panel
Length of output: 1971
🏁 Script executed:
# Search for Laravel's RateLimiter or Limit class in vendor or any references
find . -type f -name "*.php" | xargs grep -l "Limit::perMinutes" 2>/dev/null | head -10
# Check the comments in the RouteServiceProvider to understand intent
sed -n '60,85p' ./app/Providers/RouteServiceProvider.php
# Let's also check the git commit that moved to standardized rate limiting
git show 22a8b2b3 --stat | head -20
</parameter>Repository: pelican-dev/panel
Length of output: 2077
🏁 Script executed:
# Check the full diff of that commit to see the original implementation
git show 22a8b2b3 -- app/Providers/RouteServiceProvider.php | head -100Repository: pelican-dev/panel
Length of output: 4340
🏁 Script executed:
# Get more of the diff to see the authentication rate limiter implementation
git show 22a8b2b3 -- app/Providers/RouteServiceProvider.php | tail -80Repository: pelican-dev/panel
Length of output: 3549
🏁 Script executed:
# Check the full git history to see if ->by() was ever added then removed
git log -p --all -- app/Providers/RouteServiceProvider.php | grep -B 5 -A 10 "authentication.*function" | grep -A 10 "return Limit" | head -60
# Also check Laravel's behavior when no ->by() is specified by looking at any tests or documentation
rg -i "rate.*limit|throttle" --type php ./tests/ 2>/dev/null | head -20Repository: pelican-dev/panel
Length of output: 638
🏁 Script executed:
# Let's check recent changes to the authentication rate limiter
git log -p --all -S "authentication.*function" -- app/Providers/RouteServiceProvider.php | head -150
# Check if there are any changes specifically to the default authentication limit
git log --all --oneline -- app/Providers/RouteServiceProvider.php | grep -i "auth\|rate" | head -20Repository: pelican-dev/panel
Length of output: 272
🏁 Script executed:
# Check the commit that moved rate limits to env vars
git show 1396ecc6 -- app/Providers/RouteServiceProvider.phpRepository: pelican-dev/panel
Length of output: 4793
🏁 Script executed:
# Let's check how Laravel's RateLimiter works by looking at comments and testing patterns
# Search for any Laravel/rate limiting documentation references in the codebase
rg -i "rate.limit.*by\(\)" ./app --type php -A 2 -B 2
# Check if there are any tests for the authentication rate limiter
find ./tests -name "*.php" -exec grep -l "authentication.*throttle\|RateLimiter.*authentication" {} \;Repository: pelican-dev/panel
Length of output: 43
🏁 Script executed:
# Let's check if there's any mention of default rate limiting behavior in the code
rg -i "perminute|perminutes|throttle" ./config -A 3 -B 1
# Also check Laravel middleware or any custom rate limiter implementations
find . -name "*.php" -path "*/app/*" -exec grep -l "class.*Throttle\|class.*RateLimit" {} \;
# Check the routes to see how the authentication limiter is actually applied
rg -A 5 -B 5 "throttle.*authentication|auth.*throttle" ./routes --type phpRepository: pelican-dev/panel
Length of output: 1517
🏁 Script executed:
# Search for where the authentication throttle is applied in routes
rg "throttle:authentication" ./routes --type php -B 3 -A 3
# Also check if there are any middleware applying the authentication throttle
rg "throttle.*authentication\|authentication.*throttle" ./app --type php -B 2 -A 2Repository: pelican-dev/panel
Length of output: 43
🏁 Script executed:
# Search for how RateLimiter middleware is being used in the app
rg "Throttle|throttle" ./app/Http/Middleware --type php -B 5 -A 10
# Check the routes to see if authentication throttle is applied
cat ./routes/auth.php | head -60Repository: pelican-dev/panel
Length of output: 498
🏁 Script executed:
# Search for all route definitions related to authentication
find ./routes -name "*.php" -exec cat {} \; | grep -B 5 -A 5 "auth\|login\|forgot" | head -80
# Also look for where the authentication middleware is applied
rg "auth.post.forgot-password" ./routes --type php -B 10 -A 5Repository: pelican-dev/panel
Length of output: 1187
🏁 Script executed:
# Search for the actual login and forgot-password route definitions
rg "forgot-password|checkpoint|login" ./routes --type php -B 3 -A 3
# Also check if there are Filament or other admin panel routes
find ./routes -name "*.php" -type f | xargs ls -laRepository: pelican-dev/panel
Length of output: 911
Add ->by($request->ip()) to the default authentication rate limiter.
Line 72 explicitly scopes the forgot-password rate limit by IP using ->by($request->ip()), but line 75 (the default case for login/checkpoint) lacks this key. Without a key, the rate limit may not be properly scoped per-IP, reducing protection against distributed brute-force attacks on authentication endpoints. This inconsistency should be fixed to match the pattern established for the password reset endpoint.
Suggested fix
- return Limit::perMinutes(config('http.rate_limit.auth_period'), config('http.rate_limit.auth'));
+ return Limit::perMinutes(config('http.rate_limit.auth_period'), config('http.rate_limit.auth'))->by($request->ip());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RateLimiter::for('authentication', function (Request $request) { | |
| if ($request->route()->named('auth.post.forgot-password')) { | |
| return Limit::perMinute(2)->by($request->ip()); | |
| return Limit::perMinutes(config('http.rate_limit.password_reset_period'), config('http.rate_limit.password_reset'))->by($request->ip()); | |
| } | |
| return Limit::perMinute(10); | |
| return Limit::perMinutes(config('http.rate_limit.auth_period'), config('http.rate_limit.auth')); | |
| }); | |
| RateLimiter::for('authentication', function (Request $request) { | |
| if ($request->route()->named('auth.post.forgot-password')) { | |
| return Limit::perMinutes(config('http.rate_limit.password_reset_period'), config('http.rate_limit.password_reset'))->by($request->ip()); | |
| } | |
| return Limit::perMinutes(config('http.rate_limit.auth_period'), config('http.rate_limit.auth'))->by($request->ip()); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Providers/RouteServiceProvider.php` around lines 70 - 76, The default
authentication rate limiter defined in RateLimiter::for('authentication') is
missing the per-IP scoping present in the forgot-password branch; update the
default Limit returned from
Limit::perMinutes(config('http.rate_limit.auth_period'),
config('http.rate_limit.auth')) to include ->by($request->ip()) so it is keyed
per client IP (same pattern used when
$request->route()->named('auth.post.forgot-password')).
There was a problem hiding this comment.
as the original code only added in ->by($request->ip()) for the password reset rate limit, i kept it as such. but as this pull request is targeted on just moving hard coded values to environment vars, i will not change the function/behavior of the code unless approved by others.
There was a problem hiding this comment.
@O2theC, that's a fair point — the original Limit::perMinute(10) had no ->by() key either, so you're correctly preserving the pre-existing behavior within the scope of this PR. The missing IP-scoping on the default auth limiter is a pre-existing concern rather than a regression introduced here.
Would you like me to open a follow-up issue to track adding ->by($request->ip()) to the default authentication rate limiter so it doesn't get lost?
|
adjusted some of the comments based on coderabbitai suggestions |
1 participant
while making an app using the api i ran into ratelimits that didn't make sense, turns out there are ratelimits outside of the client and application api rate limits, this moves the ones i found, including the auth and password reset ratelimits to using env vars for configuration .
ideally this would be in admin panel area where the client and application api ratelimits are but i know practically nothing about php, larvel , filament or the other things this project uses. i have programmed a decent amt and it turns combining that with some copy pasting and a linter/syntax checker ends up working out ok.
feel free to suggest better names for the config values, whether for naming conventions or something else
my main concern is that these ratelimits are able to be configured in a docker environment without needing to override the file via docker volume bind