Skip to content

move some hardcoded ratelimits for the api to env vars #2332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
O2theC wants to merge 3 commits into
base: main
Choose a base branch
from
+38 −14
Open

move some hardcoded ratelimits for the api to env vars #2332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1396ecc
move some hardcoded ratelimits for the api to env vars
O2theC May 13, 2026
c98f974
updated relevent comments for clarity
O2theC May 13, 2026
1800627
forgot to run pint - removed trailing whitespace in comment
O2theC May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions app/Enums/ResourceLimit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ public function middleware(): string
public function limit(): Limit
{
return match ($this) {
self::Websocket => Limit::perMinute(5),
self::BackupRestore => Limit::perMinutes(15, 3),
self::DatabaseCreate => Limit::perMinute(2),
self::SubuserCreate => Limit::perMinutes(15, 10),
self::FilePull => Limit::perMinutes(10, 5),
default => Limit::perMinute(2),
self::Websocket => Limit::perMinutes(config('http.rate_limit.websocket_period'), config('http.rate_limit.websocket')),
self::BackupRestore => Limit::perMinutes(config('http.rate_limit.backup_restore_period'), config('http.rate_limit.backup_restore')),
self::DatabaseCreate => Limit::perMinutes(config('http.rate_limit.database_create_period'), config('http.rate_limit.database_create')),
self::SubuserCreate => Limit::perMinutes(config('http.rate_limit.subuser_create_period'), config('http.rate_limit.subuser_create')),
self::FilePull => Limit::perMinutes(config('http.rate_limit.file_pull_period'), config('http.rate_limit.file_pull')),
default => Limit::perMinutes(config('http.rate_limit.default_period'), config('http.rate_limit.default')),
};
}

Expand Down
5 changes: 3 additions & 2 deletions app/Providers/RouteServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,13 @@ protected function configureRateLimiting(): void
// a limit of 10 requests per minute, for the forgot password endpoint apply a
// limit of two per minute for the requester so that there is less ability to
// trigger email spam.
// the ratelimits refrenced above are now configured by env vars (check config/http.php for more details)
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'));
});
Comment on lines 71 to 77
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 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 3

Repository: pelican-dev/panel

Length of output: 50373

🏁 Script executed:

# Find and examine the RouteServiceProvider.php file
find . -name "RouteServiceProvider.php" -type f | head -5

Repository: 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 -10

Repository: 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.php

Repository: 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 -50

Repository: 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 -100

Repository: 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 -80

Repository: 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 -20

Repository: 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 -20

Repository: 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.php

Repository: 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 php

Repository: 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 2

Repository: 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 -60

Repository: 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 5

Repository: 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 -la

Repository: 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.

Suggested change
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')).

Copy link
Copy Markdown
Author

@O2theC O2theC May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?


// Configure the throttles for both the application and client APIs below.
Expand Down
35 changes: 29 additions & 6 deletions config/http.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,39 @@
| 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 the rate limit for the number of requests that can be
| executed against the client and internal (application) APIs along with
| certain other endpoints over a defined period (1 minute for most)
*/
'rate_limit' => [
'client_period' => 1,
'client_period' => env('APP_API_CLIENT_RATELIMIT_PERIOD', 1),
'client' => env('APP_API_CLIENT_RATELIMIT', 256),

'application_period' => 1,
'application_period' => env('APP_API_APPLICATION_RATELIMIT_PERIOD', 1),
'application' => env('APP_API_APPLICATION_RATELIMIT', 256),

'auth_period' => env('APP_API_AUTH_RATELIMIT_PERIOD', 1),
'auth' => env('APP_API_AUTH_RATELIMIT', 10),

'password_reset_period' => env('APP_API_PASSWORD_RESET_RATELIMIT_PERIOD', 1),
'password_reset' => env('APP_API_PASSWORD_RESET_RATELIMIT', 2),

'websocket_period' => env('APP_API_WEBSOCKET_RATELIMIT_PERIOD', 1),
'websocket' => env('APP_API_WEBSOCKET_RATELIMIT', 5),

'backup_restore_period' => env('APP_API_BACKUP_RESTORE_RATELIMIT_PERIOD', 15),
'backup_restore' => env('APP_API_BACKUP_RESTORE_RATELIMIT', 3),

'database_create_period' => env('APP_API_DATABASE_CREATE_RATELIMIT_PERIOD', 1),
'database_create' => env('APP_API_DATABASE_CREATE_RATELIMIT', 2),

'subuser_create_period' => env('APP_API_SUBUSER_CREATE_RATELIMIT_PERIOD', 15),
'subuser_create' => env('APP_API_SUBUSER_CREATE_RATELIMIT', 10),

'file_pull_period' => env('APP_API_FILE_PULL_RATELIMIT_PERIOD', 10),
'file_pull' => env('APP_API_FILE_PULL_RATELIMIT', 5),

'default_period' => env('APP_API_DEFAULT_RATELIMIT_PERIOD', 1),
'default' => env('APP_API_DEFAULT_RATELIMIT', 2),
],
];
Loading