Add changes from upstream #2076
Conversation
Co-authored-by: DaneEveritt <[email protected]>
Co-authored-by: DaneEveritt <[email protected]>
Co-authored-by: DaneEveritt <[email protected]>
📝 WalkthroughWalkthroughAdds a new ResourceLimit enum registering per-server resource rate limiters and applies them to client API routes; lowers client API default rate limit; replaces daemon revokeUserJTI calls with deauthorize; consolidates several creation flows into DB transactions with lockForUpdate and transactional logging; relaxes node upload_size validation; adds migration to clear orphan allocation notes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Middleware as ResourceLimitMiddleware
participant RateLimiter
participant DB as ServerModel
participant Controller
Client->>Router: HTTP request to protected client route
Router->>Middleware: apply ResourceLimit::X->middleware()
Middleware->>RateLimiter: compute throttle key (resource + server UUID)
RateLimiter->>DB: resolve Server from request (validate/lookup)
DB-->>RateLimiter: return server UUID
RateLimiter->>RateLimiter: check/apply per-server RateLimit
alt allowed
Middleware->>Controller: forward request
Controller->>DB: perform action (may use lockForUpdate inside transaction)
DB-->>Controller: result
Controller-->>Client: success response
else throttled
Middleware-->>Client: 429 Too Many Requests
end
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)config/backups.php (1)
🔇 Additional comments (6)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Node.php (1)
83-91: Remove duplicate 'description' entry.The
'description'field appears twice in the$fillablearray (lines 84 and 90), which is redundant.🔎 Proposed fix
protected $fillable = [ - 'public', 'name', 'description', + 'public', 'name', 'fqdn', 'scheme', 'behind_proxy', 'memory', 'memory_overallocate', 'disk', 'disk_overallocate', 'cpu', 'cpu_overallocate', 'upload_size', 'daemon_base', 'daemon_sftp', 'daemon_sftp_alias', 'daemon_listen', 'daemon_connect', 'description', 'maintenance_mode', 'tags', ];
🤖 Fix all issues with AI Agents
In @app/Http/Controllers/Api/Client/Servers/DatabaseController.php:
- Around line 62-70: The lockForUpdate() call on the Eloquent relation in
DatabaseController (the $server->databases()->lockForUpdate() inside the
Activity::event(...) transaction) does not execute a query and therefore does
not acquire the row lock; change that call to execute the query (for example
replace it with $server->databases()->lockForUpdate()->get()) so the lock is
actually acquired before calling deployDatabaseService->handle; apply the same
fix in BackupController where a similar $server->...->lockForUpdate() is used
(use ->get() or another terminating method like ->count()).
In @app/Repositories/Daemon/DaemonServerRepository.php:
- Around line 154-162: The deauthorize method is sending the payload wrapped
under a 'json' key (Guzzle style) which results in {"json": {...}} being posted;
update the deauthorize method (the post call on getHttpClient()) to send the
body array directly (match how create() and revokeUserJTI() pass data) so the
request payload is {"user": "...", "servers": [...]} rather than wrapped,
preserving the same keys ('user' and 'servers') and keeping the server UUID from
$this->server->uuid.
In @tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php:
- Line 45: The test mock for the server deauthorization call is expecting an
integer ID but the implementation calls deauthorize with the subuser's UUID
string; update the two expectations on the mock for setServer->deauthorize
(currently using $subuser->id) to use $subuser->uuid instead so they match the
method signature public function deauthorize(string $user): void and correctly
validate UUID usage.
In @tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php:
- Line 40: The test is calling deauthorize with the wrong type: update the mock
expectation so deauthorize() receives the UUID string rather than the integer
id; replace the argument $internal->id with $internal->uuid in the mock
expectation (the line using $mock->expects('setServer->deauthorize')->with(...))
so it matches the method signature deauthorize(string $user) and production
usage.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/Enums/ResourceLimit.phpapp/Extensions/OAuth/Schemas/GithubSchema.phpapp/Filament/Admin/Resources/Nodes/Pages/CreateNode.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.phpapp/Http/Controllers/Api/Client/Servers/BackupController.phpapp/Http/Controllers/Api/Client/Servers/DatabaseController.phpapp/Http/Controllers/Api/Client/Servers/NetworkAllocationController.phpapp/Models/Node.phpapp/Providers/RouteServiceProvider.phpapp/Repositories/Daemon/DaemonServerRepository.phpapp/Services/Servers/DetailsModificationService.phpapp/Services/Servers/ServerDeletionService.phpapp/Services/Subusers/SubuserDeletionService.phpapp/Services/Subusers/SubuserUpdateService.phpconfig/http.phpcontributing.mdroutes/api-client.phpsecurity.mdtests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.phptests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php
💤 Files with no reviewable changes (1)
- app/Filament/Admin/Resources/Nodes/Pages/CreateNode.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Services/Servers/ServerDeletionService.phpapp/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php
📚 Learning: 2025-11-05T22:21:31.863Z
Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1865
File: app/Filament/Admin/Resources/Nodes/Pages/EditNode.php:682-682
Timestamp: 2025-11-05T22:21:31.863Z
Learning: In app/Filament/Admin/Resources/Nodes/Pages/EditNode.php, the diagnostics tab's upload action intentionally does not use the iconButton() modifier, while the pull action does. This UI difference is intentional.
Applied to files:
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php
🧬 Code graph analysis (8)
app/Services/Servers/ServerDeletionService.php (1)
app/Models/Node.php (1)
allocations(267-270)
app/Services/Servers/DetailsModificationService.php (2)
app/Repositories/Daemon/DaemonRepository.php (1)
setServer(24-31)app/Repositories/Daemon/DaemonServerRepository.php (1)
deauthorize(154-162)
app/Providers/RouteServiceProvider.php (1)
app/Enums/ResourceLimit.php (1)
boot(55-64)
app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php (5)
app/Models/Server.php (2)
allocation(296-299)allocations(306-309)app/Services/Activity/ActivityLogService.php (3)
transaction(173-182)log(136-155)property(109-117)app/Models/Allocation.php (1)
server(129-132)app/Models/ServerTransfer.php (1)
server(76-79)app/Exceptions/DisplayException.php (1)
DisplayException(19-99)
app/Http/Controllers/Api/Client/Servers/DatabaseController.php (3)
app/Services/Activity/ActivityLogService.php (4)
event(48-53)transaction(173-182)log(136-155)property(109-117)app/Models/Database.php (1)
server(84-87)app/Models/Server.php (1)
databases(378-381)
app/Services/Subusers/SubuserDeletionService.php (2)
app/Repositories/Daemon/DaemonRepository.php (1)
setServer(24-31)app/Repositories/Daemon/DaemonServerRepository.php (1)
deauthorize(154-162)
app/Services/Subusers/SubuserUpdateService.php (2)
app/Repositories/Daemon/DaemonRepository.php (1)
setServer(24-31)app/Repositories/Daemon/DaemonServerRepository.php (1)
deauthorize(154-162)
routes/api-client.php (1)
app/Enums/ResourceLimit.php (1)
middleware(38-41)
🔇 Additional comments (20)
app/Services/Servers/ServerDeletionService.php (1)
81-84: LGTM! Proper cleanup of allocation notes during server deletion.The addition of clearing the
notesfield alongsideserver_idensures that allocations are fully reset when a server is deleted, making them ready for reuse without residual data.The mass update approach within the transaction is appropriate for this cleanup scenario and provides good performance.
app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php (1)
110-120: Excellent implementation of allocation creation with proper concurrency control.The transactional approach with
lockForUpdate()correctly prevents race conditions when enforcing the allocation limit. By locking existing allocations before counting, this ensures that concurrent requests for the same server are serialized—preventing multiple requests from simultaneously seeing "under limit" and creating allocations that exceed the configured maximum.The pattern aligns with best practices: acquiring locks, validating constraints, creating resources, and logging activity all within a single atomic transaction.
Based on learnings, the
lockForUpdate()pattern is the preferred approach for allocation operations to prevent race conditions.app/Providers/RouteServiceProvider.php (1)
102-103: LGTM! Resource-based rate limiting properly bootstrapped.The placement of
ResourceLimit::boot()at the end ofconfigureRateLimiting()is appropriate, ensuring that per-resource rate limiters are registered after the base API rate limiters.app/Extensions/OAuth/Schemas/GithubSchema.php (2)
21-21: LGTM! Correct branding applied.The step label has been updated to use the correct "GitHub" capitalization.
25-25: LGTM! Correct branding applied.The HTML link text has been updated to use the correct "GitHub" capitalization.
contributing.md (1)
11-11: LGTM! Correct branding applied.The documentation has been updated to use the correct "GitHub" capitalization.
config/http.php (1)
16-16: The claim about reducing the rate limit from 720 to 120 cannot be verified.The
config/http.phpfile is new to this codebase—it wasn't modified from a previous value. Additionally, theResourceLimitenum provides per-resource rate limiting that is already in place and adequately restrictive:
- Creation operations: 2–10 requests per 10–15 minutes (per server)
- Websocket: 5 requests per minute
- Backup restore: 3 requests per 15 minutes
These per-resource limits are applied per-server (not per-user) and complement the global 120 requests/minute limit. The rate limiting strategy is properly implemented without evidence of any 83% reduction occurring.
Likely an incorrect or invalid review comment.
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (1)
323-326: The upload_size field validation is properly configured with only a minimum constraint.The field in EditNode.php (lines 323-326) and CreateNode.php correctly enforce
minValue(1)with no maximum limit, aligning with the PR objective to accept any positive integer. This is consistent with the Node model validation rule ('upload_size' => ['int', 'min:1']). The upload_size value is properly passed to the daemon asupload_limitin the Node model configuration. No changes are needed here.app/Models/Node.php (1)
113-113: LGTM: Upload size validation relaxed.The validation correctly removes the upper bound constraint, allowing any positive integer as specified in the PR objectives. This provides more flexibility for node configurations with large upload requirements.
app/Http/Controllers/Api/Client/Servers/BackupController.php (2)
86-86: LGTM: Proper boolean input handling.Using
$request->boolean('is_locked')is more explicit and type-safe compared to the genericinput()method.
89-100: Verify lockForUpdate() usage - lock may not be acquired.Similar to the DatabaseController,
$server->backups()->lockForUpdate()on line 90 does not execute a query and therefore doesn't acquire a lock. ThelockForUpdate()method requires a terminating call likeget(),first(), orcount()to execute.This pattern appears in multiple controllers in this PR, suggesting it may be an upstream pattern. Please verify the intended locking behavior.
routes/api-client.php (1)
3-3: LGTM: Resource-based rate limiting properly applied.The addition of
ResourceLimitmiddleware to resource creation endpoints follows a consistent pattern and aligns with the PR objective to add throttling. The middleware is appropriately applied to operations that could be resource-intensive or subject to abuse:
- WebSocket connections
- Database creation
- File pulling
- Schedule creation
- Allocation creation
- Subuser creation
- Backup restoration
Also applies to: 54-56, 65-66, 83-84, 90-91, 104-105, 113-114, 127-128
app/Services/Servers/DetailsModificationService.php (1)
51-51: LGTM: Correctly migrated to UUID-based deauthorization.The change from
revokeUserJTI($owner)todeauthorize($server->user->uuid)correctly uses the owner's UUID for the new deauthorization method. The error handling remains appropriate for daemon connectivity issues.app/Services/Subusers/SubuserUpdateService.php (1)
49-56: LGTM: Correct UUID usage with improved error handling.The migration to
deauthorize($subuser->user->uuid)correctly uses the UUID string. The enhanced error handling with contextual logging is a good improvement over the previous silent catch.app/Services/Subusers/SubuserDeletionService.php (1)
31-37: LGTM: Correct UUID usage with appropriate error handling.The change to
deauthorize($subuser->user->uuid)correctly uses the UUID string. The error handling with contextual logging and therevokedflag update ensures graceful degradation when the daemon is unreachable.security.md (1)
11-13: LGTM: Improved formatting for vulnerability reporting.The markdown formatting improvements enhance readability and provide proper linking for the security advisory submission.
app/Repositories/Daemon/DaemonServerRepository.php (1)
136-147: LGTM!The deprecation annotation with
@seereference to the new method is appropriate and provides clear migration guidance.app/Enums/ResourceLimit.php (3)
18-31: LGTM!The enum cases and
throttleKey()method are well-structured. Using$this->nameensures unique keys per resource type.
38-53: LGTM!The middleware integration and rate limit definitions are appropriate. The default fallback in
limit()handles cases not explicitly listed (AllocationCreate, ScheduleCreate).
55-64: Assertion failure would result in 500 error if routes misconfigured.
Assert::isInstanceOf()throwsInvalidArgumentExceptionif theserverroute parameter is missing or not aServerinstance, yielding an opaque 500 error. However, this is mitigated by design: all routes usingResourceLimit::*->middleware()are nested under the/servers/{server:uuid}prefix inroutes/api-client.php, which guarantees model binding of the server parameter. The middleware is sufficiently tightly coupled to this routing structure that misconfiguration is unlikely without breaking other routes as well.
tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php
Outdated
Show resolved
Hide resolved
tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php:
- Around line 110-122: The row lock call
($server->allocations()->lockForUpdate()) is not executed, so the subsequent
check ($server->allocations->count()) can read an unlocked or cached collection
and cause a race; execute the locked query and use its result for the count
inside the Activity::event(...)->transaction(...) closure (e.g. call a terminal
operation on $server->allocations()->lockForUpdate() and store that result in a
local variable), then perform the allocation_limit check against that
locked/counted result and pass the locked allocation or fresh data into
assignableAllocationService->handle to prevent concurrent requests from
bypassing the limit.
In @lang/en/admin/node.php:
- Line 64: The 'upload_limit_help' translation in lang/en/admin/node.php was
changed to a single string but corresponding keys in other locales (e.g., zh-CN,
hu-HU, fr-FR) remain arrays, causing trans() to return different types per
locale; update each locale file to define 'upload_limit_help' as the same single
string value (not an array) so the key's type is consistent across translations,
ensuring functions like trans('admin.node.upload_limit_help') always return a
string.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Filament/Admin/Resources/Nodes/Pages/CreateNode.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.phpapp/Http/Controllers/Api/Client/Servers/NetworkAllocationController.phplang/en/admin/node.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Filament/Admin/Resources/Nodes/Pages/EditNode.php
- app/Filament/Admin/Resources/Nodes/Pages/CreateNode.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php
🔇 Additional comments (1)
app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php (1)
18-18: LGTM! Scramble API documentation grouping added.The addition of the
#[Group('Server - Allocation')]attribute properly groups these allocation endpoints for API documentation.Also applies to: 21-21
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @database/migrations/2026_01_09_134116_clear_unused_allocation_notes.php:
- Around line 11-16: Wrap the bulk update in a DB::transaction() inside the
migration's up() method to ensure atomicity; instead of a single update call,
select rows whereNull('server_id') and process them in chunks (e.g., chunkById)
while applying lockForUpdate() on the query, then update each chunk's notes to
null, so the operation is performed safely and prevents partial commits and race
conditions.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
database/migrations/2026_01_09_134116_clear_unused_allocation_notes.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
database/migrations/2026_01_09_134116_clear_unused_allocation_notes.php
🔇 Additional comments (1)
database/migrations/2026_01_09_134116_clear_unused_allocation_notes.php (1)
21-24: LGTM!The no-op
down()method is appropriate for a data cleanup migration, since restoring cleared notes is not meaningful.
Requires pelican-dev/wings#156
pterodactyl/panel@512cfad
pterodactyl/panel@2bd9d8b
pterodactyl/panel@3dd206c
pterodactyl/panel@a215f6d
pterodactyl/panel@09caa0d
pterodactyl/panel@d458007