Conversation
…ckups/transfers Co-authored-by: DaneEveritt <dane@daneeveritt.com>
… deletion Co-authored-by: DaneEveritt <dane@daneeveritt.com>
Co-authored-by: DaneEveritt <dane@daneeveritt.com>
Co-authored-by: DaneEveritt <dane@daneeveritt.com>
Co-authored-by: danny6167 <danielb@purpleflaghosting.com>
Co-authored-by: MrSoulPenguin <28676680+MrSoulPenguin@users.noreply.github.com>
Co-authored-by: DaneEveritt <dane@daneeveritt.com>
Co-authored-by: DaneEveritt <dane@daneeveritt.com>
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR introduces asynchronous SFTP access revocation triggered by user deletion or password changes via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as Application
participant Event as Event System
participant Listener as RevocationListener
participant Queue as Job Queue
participant Daemon as Daemon
User->>App: Delete User / Change Password
App->>App: Save Model Changes
App->>Event: Dispatch Deleting/PasswordChanged Event
Event->>Listener: Call Handle
Listener->>Listener: Query User's Accessible Nodes
loop For Each Node (chunked)
Listener->>Queue: Dispatch RevokeSftpAccessJob<br/>(user uuid, target node)
end
Queue-->>Daemon: Async: Execute deauthorize<br/>(retry on connection error)
Daemon->>Daemon: Revoke SFTP Access
Possibly Related PRs
🚥 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. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Services/Databases/DatabaseManagementService.php (1)
145-155:⚠️ Potential issue | 🔴 CriticalCritical:
sharedLock()without WHERE clause updates all database records.Calling
$database->sharedLock()->update([...])on an Eloquent Model delegates via__call()to a fresh query Builder with no WHERE clause. This is equivalent toDatabase::sharedLock()->update([...]), which updates ALL database records in the table to the same password instead of just the intended record.Use:
Database::where('id', $database->id)->sharedLock()->update([ 'password' => $password, ]);Or simply
$database->update([...])since you're already in a transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Databases/DatabaseManagementService.php` around lines 145 - 155, Inside the transaction in DatabaseManagementService (the closure using $database and $password), replace the unsafe call to $database->sharedLock()->update([...]) with a per-model update — for example call $database->update([...]) — so only the intended record has its password changed; keep the rest of the chain ($database->dropUser()->createUser()->assignUserToDatabase()->flushPrivileges()) intact.app/Services/Subusers/SubuserDeletionService.php (1)
20-26:⚠️ Potential issue | 🟠 MajorJob dispatch inside database transaction may cause inconsistency on rollback.
If the transaction fails and rolls back after
RevokeSftpAccessJob::dispatch()is called, the job will still be queued and executed, potentially revoking SFTP access for a subuser that wasn't actually deleted. Consider dispatching the job after the transaction commits.Proposed fix using afterCommit
$log->transaction(function ($instance) use ($server, $subuser) { $subuser->delete(); event(new SubUserRemoved($subuser->server, $subuser->user)); - - RevokeSftpAccessJob::dispatch($subuser->user->uuid, $server); }); + + RevokeSftpAccessJob::dispatch($subuser->user->uuid, $server);Alternatively, you can use Laravel's
afterCommit()method on the job:RevokeSftpAccessJob::dispatch($subuser->user->uuid, $server)->afterCommit();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Subusers/SubuserDeletionService.php` around lines 20 - 26, The job dispatch is happening inside the DB transaction closure ($log->transaction) which can lead to SFTP revocation running even if the transaction rolls back; update SubuserDeletionService to either move the RevokeSftpAccessJob dispatch call out of the $log->transaction closure to run after the transaction completes, or call ->afterCommit() on the job (RevokeSftpAccessJob::dispatch(...)->afterCommit()) so the job only queues once the deletion (and event dispatch of SubUserRemoved) is committed.app/Services/Allocations/FindAssignableAllocationService.php (1)
58-79:⚠️ Potential issue | 🔴 Critical
lockForUpdate()without a transaction leaves the allocation assignment vulnerable to race conditions.The pessimistic lock is released immediately after
->first()completes at line 67. The subsequent->update()call at line 77 executes outside the lock, allowing concurrent requests to select the same unassigned allocation and both attempt assignment. This pattern exists in both the initial query (lines 58–67) and increateNewAllocation()(lines 128–133).Wrap the entire find-or-create-and-assign sequence in a
DB::transaction()block. Within the transaction, movelockForUpdate()immediately before the->first()call, and replace theupdate()method with direct property assignment followed bysave(). This ensures the lock remains active through the assignment operation and aligns with the allocation update pattern used inServerCreationService.🔒 Suggested shape
+use Illuminate\Support\Facades\DB; + public function handle(Server $server): Allocation { if (!config('panel.client_features.allocations.enabled')) { throw new AutoAllocationNotEnabledException(); } @@ - /** `@var` Allocation|null $allocation */ - $allocation = Allocation::withoutGlobalScopes() - ->lockForUpdate() - ->where('node_id', $server->node_id) - ->when($server->allocation, function ($query) use ($server) { - $query->where('ip', $server->allocation->ip); - }) - ->whereBetween('port', [$start, $end]) - ->whereNull('server_id') - ->inRandomOrder() - ->first(); - - if (!$createNew && !$allocation) { - throw new NoAutoAllocationSpaceAvailableException(); - } - - $allocation ??= $this->createNewAllocation($server, $start, $end); - - $allocation->update(['server_id' => $server->id]); - - return $allocation->refresh(); + return DB::transaction(function () use ($server, $createNew, $start, $end) { + /** `@var` Allocation|null $allocation */ + $allocation = Allocation::withoutGlobalScopes() + ->where('node_id', $server->node_id) + ->when($server->allocation, function ($query) use ($server) { + $query->where('ip', $server->allocation->ip); + }) + ->whereBetween('port', [$start, $end]) + ->whereNull('server_id') + ->lockForUpdate() + ->inRandomOrder() + ->first(); + + if (!$createNew && !$allocation) { + throw new NoAutoAllocationSpaceAvailableException(); + } + + $allocation ??= $this->createNewAllocation($server, $start, $end); + $allocation->server_id = $server->id; + $allocation->save(); + + return $allocation->refresh(); + }); }Also applies to: lines 128–133 in
createNewAllocation().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Allocations/FindAssignableAllocationService.php` around lines 58 - 79, Wrap the entire find-or-create-and-assign flow in a DB::transaction() inside FindAssignableAllocationService so the lock persists through assignment: perform the Allocation::withoutGlobalScopes()->lockForUpdate() query and call ->first() inside the transaction, and if none found either call createNewAllocation($server, $start, $end) from inside the transaction (also wrapped with lockForUpdate() there) or create the allocation record inside the same transaction; replace the post-query $allocation->update([...]) call with direct property assignment ($allocation->server_id = $server->id) followed by $allocation->save() so the assignment happens while the row lock is held. Ensure createNewAllocation()’s allocation creation/assignment (the code around lines where it currently uses update()) is moved into the same DB::transaction() or also wrapped similarly.
🧹 Nitpick comments (6)
app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php (1)
100-103: Prefer directnode_idcomparison inrestore()for consistency and simpler authorization.Current logic works, but matching
index()style avoids an extra relation dereference and keeps the check uniform.♻️ Suggested refactor
- $node = $request->attributes->get('node'); - if (!$model->server->node->is($node)) { + /** `@var` Node $node */ + $node = $request->attributes->get('node'); + if ($model->server->node_id !== $node->id) { throw new HttpForbiddenException('Requesting node does not have permission to access this server.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php` around lines 100 - 103, The authorization in restore() currently compares model->server->node via is(), add consistency with index() by comparing node IDs directly: retrieve $node from $request->attributes, compare $model->server->node_id !== $node->id (or cast both to int) and throw the same HttpForbiddenException when they differ; update the check in the restore() method to use this direct node_id comparison instead of ->is().app/Http/Middleware/SetSecurityHeaders.php (1)
20-25: Use a class constant for immutable security headers.This map is static configuration and is never mutated, so a constant is cleaner and safer than a mutable static property.
♻️ Proposed refactor
- protected static array $headers = [ + private const HEADERS = [ 'X-Frame-Options' => 'DENY', 'X-Content-Type-Options' => 'nosniff', 'X-XSS-Protection' => '1; mode=block', 'Referrer-Policy' => 'no-referrer-when-downgrade', ]; @@ - foreach (static::$headers as $key => $value) { + foreach (self::HEADERS as $key => $value) { if (!$response->headers->has($key)) { $response->headers->set($key, $value); } }Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Middleware/SetSecurityHeaders.php` around lines 20 - 25, Replace the mutable protected static array $headers in class SetSecurityHeaders with a class constant (e.g. protected const HEADERS = [...]) and update all usages (self::$headers or static::$headers) to reference the constant (self::HEADERS or static::HEADERS); do the same refactor for the other static array referenced around the same file (the one at the other occurrence) so both immutable header maps become class constants to reflect immutability.config/http.php (1)
14-20: Rate limits significantly increased.The client rate limit doubled (120→256) and application rate limit increased (240→256). Per the PR objectives, this is a temporary bump for problematic endpoints. Consider adding a comment noting this is temporary, or tracking with a TODO/issue so it can be revisited once the underlying issues are addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/http.php` around lines 14 - 20, The rate limits in the 'rate_limit' config (keys 'client' and 'application' / env vars APP_API_CLIENT_RATELIMIT and APP_API_APPLICATION_RATELIMIT) were temporarily bumped; add a short inline comment or TODO next to these entries explaining this is a temporary increase and reference an issue/ticket ID (or create one) so the change is revisited when the underlying endpoint problems are fixed; ensure the comment mentions which env vars control the values and the original limits for context.app/Events/User/PasswordChanged.php (1)
8-12: Consider addingSerializesModelstrait for consistency.The sibling
Deletingevent usesSerializesModels, but this event does not. WhileDispatchablehandles event dispatching,SerializesModelsensures proper model serialization when the event is queued. If this event is handled by a queued listener, the User model should serialize correctly.♻️ Suggested change for consistency
use App\Models\User; use Illuminate\Foundation\Events\Dispatchable; +use Illuminate\Queue\SerializesModels; final class PasswordChanged { use Dispatchable; + use SerializesModels; public function __construct(public readonly User $user) {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Events/User/PasswordChanged.php` around lines 8 - 12, Add the SerializesModels trait to the PasswordChanged event so the User model serializes correctly when queued; update the class to use both Dispatchable and SerializesModels (i.e., add SerializesModels to the use clause in class PasswordChanged) and ensure the SerializesModels symbol is imported so the existing constructor public readonly User $user continues to work with queued listeners.app/Jobs/RevokeSftpAccessJob.php (1)
23-25: Clarify the interaction between$tries,$maxExceptions, and manualrelease().With
$tries = 3and$maxExceptions = 1:
$maxExceptions = 1means the job fails permanently after 1 uncaught exception- Since
ConnectionExceptionis caught and the job is released, it won't count toward$maxExceptions- However, each
release()still counts toward$triesAfter 3 connection failures (releases), the job will be marked as failed without ever logging an exception. Consider whether this silent failure behavior is intentional, or if you want to log/alert when max retries are exhausted.
Optional: Add a failed() method to handle exhausted retries
+ public function failed(?\Throwable $exception): void + { + // Log or alert when SFTP revocation ultimately fails + \Log::warning("Failed to revoke SFTP access for user {$this->user} on {$this->target->uuid} after {$this->tries} attempts"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/RevokeSftpAccessJob.php` around lines 23 - 25, The job's current configuration (public int $tries = 3; public int $maxExceptions = 1;) plus the use of release() on ConnectionException means releases consume $tries and can silently exhaust retries without any exception being logged; in RevokeSftpAccessJob, either adjust $tries/$maxExceptions to reflect desired behavior or implement a public function failed(Throwable $exception = null) that logs/alerts when retries are exhausted (include context like job id and ConnectionException details) so you get visibility when release()-based retries hit the $tries limit rather than silently failing; reference the class RevokeSftpAccessJob, the release() calls for ConnectionException, and add/implement failed() to surface the exhausted-retries condition.tests/Integration/Api/Remote/ServerTransferControllerTest.php (1)
37-102: Consider extracting bearer token header creation into a helper.The repeated
"Bearer {$node->daemon_token_id}.{$node->daemon_token}"logic appears in multiple tests; a private helper would reduce duplication and future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Api/Remote/ServerTransferControllerTest.php` around lines 37 - 102, Tests in ServerTransferControllerTest duplicate the Authorization header string construction ("Bearer {$node->daemon_token_id}.{$node->daemon_token}") across multiple withHeader calls; add a private helper on the test class (e.g. authHeaderFor(Node $node): array or authHeaderValue(Node $node): string) that returns the properly formatted header and update all usages of withHeader('Authorization', "Bearer ...") to call that helper (refer to withHeader calls in test_success_status_update_cannot_be_sent_from_old_node, test_success_status_update_cannot_be_sent_from_unauthorized_node, test_failure_status_update_cannot_be_sent_from_unauthorized_node, and other tests using $oldNode/$newNode/$node and their daemon_token_id/daemon_token).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Listeners/AuthenticationListener.php`:
- Line 20: The AuthenticationListener's method name "login" isn't wired into
Laravel's event system, so update event registration: add explicit mappings in
EventServiceProvider::$listen that map Illuminate\Auth\Events\Login and
Illuminate\Auth\Events\Failed to App\Listeners\AuthenticationListener@login (or
implement Illuminate\Contracts\Events\DispatcherSubscriber and define
subscribe() in AuthenticationListener, or rename the listener method to handle()
and register the listener class for the events) so the AuthenticationListener
methods are actually invoked when auth events fire.
In `@app/Services/Servers/DetailsModificationService.php`:
- Around line 48-50: The code revokes SFTP for the new owner because after
DetailsModificationService::forceFill() updates owner_id the $server->user
relation points to the new owner; capture the old owner before mutating and
dispatch RevokeSftpAccessJob with that old owner's identifier instead.
Concretely, read and store the previous owner id or uuid (e.g. $oldOwnerId =
$server->owner_id or $oldUserUuid = $server->user->uuid) before calling
forceFill(), then after detecting the change (the existing if ($server->owner_id
!== $owner) logic) call RevokeSftpAccessJob::dispatch(...) with the stored
$oldUserUuid/$oldOwnerId so the previous owner's SFTP is revoked rather than the
new owner's.
In `@app/Services/Subusers/SubuserUpdateService.php`:
- Around line 39-45: The RevokeSftpAccessJob is being dispatched inside the
$log->transaction closure in SubuserUpdateService which risks revoking SFTP
access if the transaction later rolls back; move the job dispatch so it runs
only after a successful commit (either call
RevokeSftpAccessJob::dispatch(...)->afterCommit() from inside the transaction or
defer the dispatch to after the $log->transaction call), ensuring the
$subuser->update(['permissions' => $cleanedPermissions]) is committed before
revocation is triggered.
In `@database/Factories/ServerTransferFactory.php`:
- Around line 22-23: The ServerTransferFactory::definition() method is missing
the explicit array return type; update the method signature for definition() in
class ServerTransferFactory to declare a return type of array (i.e., change the
signature to include : array) so it matches the other factories and enforces the
same typing consistency across the codebase.
In `@tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php`:
- Around line 48-50: The test is using Bus::assertDispatchedTimes() with a
closure (incorrect API) and risks false positives because the same job is
dispatched twice; replace the Bus::assertDispatchedTimes(...) checks with
Bus::assertDispatched(RevokeSftpAccessJob::class, fn($job) => $job->user ===
$subuser->uuid && $job->target->is($server)) and reset the bus fake between the
two DELETE operations (e.g., call Bus::fake() after the first DELETE or before
the second assertion) so the second assertion only sees the second dispatch;
update both the assertion around RevokeSftpAccessJob and the duplicate at the
later location accordingly.
In `@tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php`:
- Around line 62-64: Replace the incorrect Bus::assertDispatchedTimes(...) call
with Bus::assertDispatched(...) using a closure to filter by the job payload:
locate the assertion referencing RevokeSftpAccessJob and change it to
Bus::assertDispatched(function (RevokeSftpAccessJob $job) use ($server,
$subuser) { return $job->user === $subuser->user->uuid &&
$job->target->is($server); }); Make the same replacement for the second
occurrence around the RevokeSftpAccessJob assertion at the later lines
(105-107).
In `@tests/Integration/Jobs/RevokeSftpAccessJobTest` .php:
- Line 1: The test file name contains an extra space before the .php extension
which breaks PHPUnit discovery; rename the file from "RevokeSftpAccessJobTest
.php" to "RevokeSftpAccessJobTest.php" and update any references/imports or
test-suite entries if present so the class RevokeSftpAccessJobTest is located by
PHPUnit's *Test.php pattern.
In `@tests/Integration/Services/Users/UserDeletionServiceTest.php`:
- Around line 25-32: The test uses
$this->expectException(DisplayException::class) which aborts execution when the
exception is thrown, making the subsequent assertions
($this->assertModelExists($server->user) and
Bus::assertNotDispatched(RevokeSftpAccessJob::class)) unreachable; replace the
expectException pattern with an explicit try/catch around the
$server->user->delete() call (or manually fail if no exception) so you can catch
DisplayException and then run the post-exception assertions inside the catch
block, ensuring DisplayException is asserted and that assertModelExists and
Bus::assertNotDispatched still execute.
---
Outside diff comments:
In `@app/Services/Allocations/FindAssignableAllocationService.php`:
- Around line 58-79: Wrap the entire find-or-create-and-assign flow in a
DB::transaction() inside FindAssignableAllocationService so the lock persists
through assignment: perform the
Allocation::withoutGlobalScopes()->lockForUpdate() query and call ->first()
inside the transaction, and if none found either call
createNewAllocation($server, $start, $end) from inside the transaction (also
wrapped with lockForUpdate() there) or create the allocation record inside the
same transaction; replace the post-query $allocation->update([...]) call with
direct property assignment ($allocation->server_id = $server->id) followed by
$allocation->save() so the assignment happens while the row lock is held. Ensure
createNewAllocation()’s allocation creation/assignment (the code around lines
where it currently uses update()) is moved into the same DB::transaction() or
also wrapped similarly.
In `@app/Services/Databases/DatabaseManagementService.php`:
- Around line 145-155: Inside the transaction in DatabaseManagementService (the
closure using $database and $password), replace the unsafe call to
$database->sharedLock()->update([...]) with a per-model update — for example
call $database->update([...]) — so only the intended record has its password
changed; keep the rest of the chain
($database->dropUser()->createUser()->assignUserToDatabase()->flushPrivileges())
intact.
In `@app/Services/Subusers/SubuserDeletionService.php`:
- Around line 20-26: The job dispatch is happening inside the DB transaction
closure ($log->transaction) which can lead to SFTP revocation running even if
the transaction rolls back; update SubuserDeletionService to either move the
RevokeSftpAccessJob dispatch call out of the $log->transaction closure to run
after the transaction completes, or call ->afterCommit() on the job
(RevokeSftpAccessJob::dispatch(...)->afterCommit()) so the job only queues once
the deletion (and event dispatch of SubUserRemoved) is committed.
---
Nitpick comments:
In `@app/Events/User/PasswordChanged.php`:
- Around line 8-12: Add the SerializesModels trait to the PasswordChanged event
so the User model serializes correctly when queued; update the class to use both
Dispatchable and SerializesModels (i.e., add SerializesModels to the use clause
in class PasswordChanged) and ensure the SerializesModels symbol is imported so
the existing constructor public readonly User $user continues to work with
queued listeners.
In `@app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php`:
- Around line 100-103: The authorization in restore() currently compares
model->server->node via is(), add consistency with index() by comparing node IDs
directly: retrieve $node from $request->attributes, compare
$model->server->node_id !== $node->id (or cast both to int) and throw the same
HttpForbiddenException when they differ; update the check in the restore()
method to use this direct node_id comparison instead of ->is().
In `@app/Http/Middleware/SetSecurityHeaders.php`:
- Around line 20-25: Replace the mutable protected static array $headers in
class SetSecurityHeaders with a class constant (e.g. protected const HEADERS =
[...]) and update all usages (self::$headers or static::$headers) to reference
the constant (self::HEADERS or static::HEADERS); do the same refactor for the
other static array referenced around the same file (the one at the other
occurrence) so both immutable header maps become class constants to reflect
immutability.
In `@app/Jobs/RevokeSftpAccessJob.php`:
- Around line 23-25: The job's current configuration (public int $tries = 3;
public int $maxExceptions = 1;) plus the use of release() on ConnectionException
means releases consume $tries and can silently exhaust retries without any
exception being logged; in RevokeSftpAccessJob, either adjust
$tries/$maxExceptions to reflect desired behavior or implement a public function
failed(Throwable $exception = null) that logs/alerts when retries are exhausted
(include context like job id and ConnectionException details) so you get
visibility when release()-based retries hit the $tries limit rather than
silently failing; reference the class RevokeSftpAccessJob, the release() calls
for ConnectionException, and add/implement failed() to surface the
exhausted-retries condition.
In `@config/http.php`:
- Around line 14-20: The rate limits in the 'rate_limit' config (keys 'client'
and 'application' / env vars APP_API_CLIENT_RATELIMIT and
APP_API_APPLICATION_RATELIMIT) were temporarily bumped; add a short inline
comment or TODO next to these entries explaining this is a temporary increase
and reference an issue/ticket ID (or create one) so the change is revisited when
the underlying endpoint problems are fixed; ensure the comment mentions which
env vars control the values and the original limits for context.
In `@tests/Integration/Api/Remote/ServerTransferControllerTest.php`:
- Around line 37-102: Tests in ServerTransferControllerTest duplicate the
Authorization header string construction ("Bearer
{$node->daemon_token_id}.{$node->daemon_token}") across multiple withHeader
calls; add a private helper on the test class (e.g. authHeaderFor(Node $node):
array or authHeaderValue(Node $node): string) that returns the properly
formatted header and update all usages of withHeader('Authorization', "Bearer
...") to call that helper (refer to withHeader calls in
test_success_status_update_cannot_be_sent_from_old_node,
test_success_status_update_cannot_be_sent_from_unauthorized_node,
test_failure_status_update_cannot_be_sent_from_unauthorized_node, and other
tests using $oldNode/$newNode/$node and their daemon_token_id/daemon_token).
🪄 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: 773f00d5-d95e-4be9-8e42-742872db96af
📒 Files selected for processing (38)
app/Events/User/Deleting.phpapp/Events/User/PasswordChanged.phpapp/Http/Controllers/Api/Client/AccountController.phpapp/Http/Controllers/Api/Client/Servers/BackupController.phpapp/Http/Controllers/Api/Client/Servers/DatabaseController.phpapp/Http/Controllers/Api/Client/Servers/StartupController.phpapp/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.phpapp/Http/Controllers/Api/Remote/Backups/BackupStatusController.phpapp/Http/Middleware/SetSecurityHeaders.phpapp/Jobs/Job.phpapp/Jobs/RevokeSftpAccessJob.phpapp/Jobs/Schedule/RunTaskJob.phpapp/Listeners/Auth/PasswordResetListener.phpapp/Listeners/AuthenticationListener.phpapp/Listeners/RevocationListener.phpapp/Listeners/TwoFactorListener.phpapp/Models/ServerTransfer.phpapp/Models/User.phpapp/Providers/Filament/PanelProvider.phpapp/Repositories/Daemon/DaemonServerRepository.phpapp/Services/Allocations/FindAssignableAllocationService.phpapp/Services/Databases/DatabaseManagementService.phpapp/Services/Servers/DetailsModificationService.phpapp/Services/Subusers/SubuserDeletionService.phpapp/Services/Subusers/SubuserUpdateService.phpapp/Services/Users/UserUpdateService.phpbootstrap/app.phpconfig/http.phpdatabase/Factories/ServerTransferFactory.phproutes/api-client.phproutes/api-remote.phptests/Integration/Api/Client/AccountControllerTest.phptests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.phptests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.phptests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.phptests/Integration/Api/Remote/ServerTransferControllerTest.phptests/Integration/Jobs/RevokeSftpAccessJobTest .phptests/Integration/Services/Users/UserDeletionServiceTest.php
💤 Files with no reviewable changes (3)
- app/Jobs/Job.php
- app/Listeners/Auth/PasswordResetListener.php
- routes/api-remote.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Requests/Api/Remote/InstallationDataRequest.php (1)
5-13:⚠️ Potential issue | 🟠 Major
rules()will not run when extendingIlluminate\Http\RequestOn Line 7, this class extends
Illuminate\Http\Request, but therules()method on Line 12 is auto-applied only throughFormRequestresolution. This allowssuccessful/reinstallto bypass validation inServerInstallController::store()(line 49, 53).Proposed fix
-use Illuminate\Http\Request; +use Illuminate\Foundation\Http\FormRequest; -class InstallationDataRequest extends Request +class InstallationDataRequest extends FormRequest { + public function authorize(): bool + { + return true; + } + /** * `@return` array<string, string|string[]> */ public function rules(): array🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Requests/Api/Remote/InstallationDataRequest.php` around lines 5 - 13, The InstallationDataRequest class currently extends Illuminate\Http\Request so its rules() aren't applied; change the base class to Illuminate\Foundation\Http\FormRequest (or import and extend FormRequest) so the rules() method is used by Laravel's validation pipeline; update the class declaration in InstallationDataRequest and ensure any custom authorize() or validation logic is preserved/added to the FormRequest implementation so ServerInstallController::store()'s successful/reinstall flows run validation.
🧹 Nitpick comments (2)
app/Http/Controllers/Api/Remote/Servers/ServerInstallController.php (1)
21-34: Authorization check should precede relationship loading.The
$egg = $server->eggassignment on line 23 triggers a database query before the authorization check on lines 25-27. If the request is unauthorized, this query is wasted.♻️ Proposed reordering
public function index(Request $request, Server $server): JsonResponse { - $egg = $server->egg; - if (!$server->node->is($request->attributes->get('node'))) { throw new HttpForbiddenException('Requesting node does not have permission to access this server.'); } + $egg = $server->egg; + return new JsonResponse([ 'container_image' => $egg->copy_script_container, 'entrypoint' => $egg->copy_script_entry, 'script' => $egg->copy_script_install, ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/Remote/Servers/ServerInstallController.php` around lines 21 - 34, Move the authorization check to before loading related models to avoid unnecessary queries: in the ServerInstallController::index method, perform the node permission check (the if (! $server->node->is($request->attributes->get('node'))) throw HttpForbiddenException) before accessing $server->egg, then only assign $egg = $server->egg and return the JsonResponse if the check passes.app/Http/Controllers/Api/Remote/Servers/ServerTransferController.php (1)
42-49: Slight inconsistency with other controllers usingAssert::isInstanceOf.This controller uses
Assert::isInstanceOffor node validation whileServerContainersControllerandServerInstallControllerdirectly use the node from attributes without assertion. WhileAssert::isInstanceOfprovides defense-in-depth, it throwsInvalidArgumentExceptionrather than an HTTP exception if the middleware somehow fails to set the node attribute.Given that
DaemonAuthenticatemiddleware guarantees the node attribute is set (context snippet 1), this is a minor inconsistency rather than a bug. The authorization logic itself is correct—allowing either node to report transfer failure is appropriate since both nodes are aware of the transfer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/Remote/Servers/ServerTransferController.php` around lines 42 - 49, The code in ServerTransferController uses Assert::isInstanceOf($node = $request->attributes->get('node'), Node::class) which is inconsistent with ServerContainersController and ServerInstallController; remove the Assert::isInstanceOf check and directly retrieve the node from $request->attributes->get('node') (trusting DaemonAuthenticate middleware) and keep the subsequent authorization logic that compares $node->is($transfer->newNode) and $node->is($transfer->oldNode); this aligns behavior with other controllers and avoids throwing an InvalidArgumentException from Assert::isInstanceOf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/Http/Requests/Api/Remote/InstallationDataRequest.php`:
- Around line 5-13: The InstallationDataRequest class currently extends
Illuminate\Http\Request so its rules() aren't applied; change the base class to
Illuminate\Foundation\Http\FormRequest (or import and extend FormRequest) so the
rules() method is used by Laravel's validation pipeline; update the class
declaration in InstallationDataRequest and ensure any custom authorize() or
validation logic is preserved/added to the FormRequest implementation so
ServerInstallController::store()'s successful/reinstall flows run validation.
---
Nitpick comments:
In `@app/Http/Controllers/Api/Remote/Servers/ServerInstallController.php`:
- Around line 21-34: Move the authorization check to before loading related
models to avoid unnecessary queries: in the ServerInstallController::index
method, perform the node permission check (the if (!
$server->node->is($request->attributes->get('node'))) throw
HttpForbiddenException) before accessing $server->egg, then only assign $egg =
$server->egg and return the JsonResponse if the check passes.
In `@app/Http/Controllers/Api/Remote/Servers/ServerTransferController.php`:
- Around line 42-49: The code in ServerTransferController uses
Assert::isInstanceOf($node = $request->attributes->get('node'), Node::class)
which is inconsistent with ServerContainersController and
ServerInstallController; remove the Assert::isInstanceOf check and directly
retrieve the node from $request->attributes->get('node') (trusting
DaemonAuthenticate middleware) and keep the subsequent authorization logic that
compares $node->is($transfer->newNode) and $node->is($transfer->oldNode); this
aligns behavior with other controllers and avoids throwing an
InvalidArgumentException from Assert::isInstanceOf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d2a128e2-78da-4203-8c3f-28547690f3e4
📒 Files selected for processing (8)
app/Http/Controllers/Api/Remote/Servers/ServerContainersController.phpapp/Http/Controllers/Api/Remote/Servers/ServerDetailsController.phpapp/Http/Controllers/Api/Remote/Servers/ServerInstallController.phpapp/Http/Controllers/Api/Remote/Servers/ServerTransferController.phpapp/Http/Requests/Api/Remote/InstallationDataRequest.phpapp/Http/Requests/Api/Remote/ServerRequest.phpapp/Listeners/Auth/PasswordResetListener.phpapp/Listeners/RevocationListener.php
💤 Files with no reviewable changes (1)
- app/Http/Requests/Api/Remote/ServerRequest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Listeners/Auth/PasswordResetListener.php
- app/Listeners/RevocationListener.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Http/Controllers/Api/Remote/Servers/ServerInstallController.php (1)
23-25: Consider extracting node authorization into a private helper.The same guard logic is duplicated in two methods; centralizing it will reduce future drift.
♻️ Optional refactor
class ServerInstallController extends Controller { + private function ensureNodeCanAccessServer(Request $request, Server $server): void + { + if (!$server->node->is($request->attributes->get('node'))) { + throw new HttpForbiddenException('Requesting node does not have permission to access this server.'); + } + } + public function index(Request $request, Server $server): JsonResponse { - if (!$server->node->is($request->attributes->get('node'))) { - throw new HttpForbiddenException('Requesting node does not have permission to access this server.'); - } + $this->ensureNodeCanAccessServer($request, $server); $egg = $server->egg; @@ public function store(InstallationDataRequest $request, Server $server): JsonResponse { $status = null; - if (!$server->node->is($request->attributes->get('node'))) { - throw new HttpForbiddenException('Requesting node does not have permission to access this server.'); - } + $this->ensureNodeCanAccessServer($request, $server);Also applies to: 45-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/Remote/Servers/ServerInstallController.php` around lines 23 - 25, Extract the duplicated node-authorization guard into a private helper on ServerInstallController (e.g., private function assertNodeAuthorized(Server $server, Request $request) or ensureNodeMatches), move the existing check (if (!$server->node->is($request->attributes->get('node'))) throw new HttpForbiddenException(...)) into that helper, and replace the two inline guards in the controller methods with a call to this helper; ensure the helper uses the same exception message and types so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Http/Controllers/Api/Remote/Servers/ServerInstallController.php`:
- Around line 23-25: Extract the duplicated node-authorization guard into a
private helper on ServerInstallController (e.g., private function
assertNodeAuthorized(Server $server, Request $request) or ensureNodeMatches),
move the existing check (if
(!$server->node->is($request->attributes->get('node'))) throw new
HttpForbiddenException(...)) into that helper, and replace the two inline guards
in the controller methods with a call to this helper; ensure the helper uses the
same exception message and types so behavior remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da17e9ba-8d26-47bc-b4fb-c8b84ab27d74
📒 Files selected for processing (3)
app/Http/Controllers/Api/Remote/Servers/ServerInstallController.phpapp/Http/Requests/Api/Remote/InstallationDataRequest.phptests/Integration/Jobs/RevokeSftpAccessJobTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Http/Requests/Api/Remote/InstallationDataRequest.php
- tests/Integration/Jobs/RevokeSftpAccessJobTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pterodactyl/panel@7c9c56b
pterodactyl/panel@0e74f3a
pterodactyl/panel@14185a9
pterodactyl/panel@6c60596
pterodactyl/panel@51bbd10
pterodactyl/panel@33695c6
pterodactyl/panel@56fe10f
pterodactyl/panel@ec7231b