Plugin system #1866
Plugin system #1866
Conversation
Boy132
commented
Nov 6, 2025
Boy132
commented
…gins Co-authored-by: pelican-vehikl <pelican.vehikl@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
composer.json (1)
83-85: Confirmp:plugin:composeris safe on fresh installs and when no plugins existHooking
php artisan p:plugin:composerintopost-install-cmdmeans a failure in that command will breakcomposer install(e.g., before migrations run or when thepluginstable is missing/empty). Please double‑check thatComposerPluginsCommandgracefully handles “no table / no plugins yet” and exits successfully in those cases so new installs don’t fail.app/Models/Plugin.php (1)
234-257: Use HTTP client instead offile_get_contentsfor update checks.
file_get_contents($this->update_url)has security and reliability issues: no timeout (can hang indefinitely), no HTTP error handling, and potential SSRF vulnerability. This was flagged in a previous review but remains unaddressed.app/Services/Helpers/PluginService.php (3)
387-402: Missing error handling for JSON operations insetMetaData.
File::json()andjson_encode()withJSON_THROW_ON_ERRORcan throwJsonException, but there's no try-catch. This was flagged in a previous review but remains unaddressed.
412-420: Load order updates lack atomicity.If a metadata write fails partway through the loop, some plugins will have updated load orders while others won't, leaving the system in an inconsistent state. This was flagged in a previous review.
351-370: SSRF protection is missing for URL downloads.While the HTTP client now has timeouts, there's no validation to prevent access to internal network resources. An attacker could use URLs like
http://localhost:6379or cloud metadata endpoints (http://169.254.169.254/...).Add URL validation before the HTTP request:
public function downloadPluginFromUrl(string $url, bool $cleanDownload = false): void { + // Validate URL scheme and prevent SSRF + $parsed = parse_url($url); + if (!$parsed || !in_array($parsed['scheme'] ?? '', ['http', 'https'], true)) { + throw new InvalidFileUploadException('Invalid URL scheme. Only http and https are allowed.'); + } + + $host = $parsed['host'] ?? ''; + if (filter_var($host, FILTER_VALIDATE_IP) && + !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + throw new InvalidFileUploadException('Access to private IP ranges is not allowed.'); + } + $info = pathinfo($url);
🧹 Nitpick comments (9)
config/panel.php (1)
72-75: Consider documenting units formax_import_sizeConfig structure is fine; to avoid ambiguity later, consider a short comment or doc note that
max_import_sizeis in bytes (defaulting to 100 MB), similar to howfiles.max_edit_sizeis expressed.app/Filament/Admin/Resources/Plugins/Pages/ListPlugins.php (1)
16-36: Tighten tab query handling and silence unused-parameter warningTwo small points here:
- Unused
$draggedRecordKeyparameter
Since the signature must match Filament’s, it’s fine not to use it, but PHPMD complains. You can suppress the warning explicitly to make that intent clear:- public function reorderTable(array $order, int|string|null $draggedRecordKey = null): void + /** @SuppressWarnings("UnusedFormalParameter") */ + public function reorderTable(array $order, int|string|null $draggedRecordKey = null): void
- Use the table query inside
modifyQueryUsinginstead of a captured builder
Capturing$query = Plugin::whereCategory(...)and then returning it frommodifyQueryUsing(fn () => $query)is a bit non‑idiomatic and can make it less obvious how this interacts with the base resource query. It’s clearer to filter the query that Filament passes in and compute the badge count separately:-use Filament\Resources\Pages\ListRecords; -use Filament\Schemas\Components\Tabs\Tab; +use Filament\Resources\Pages\ListRecords; +use Filament\Schemas\Components\Tabs\Tab; +use Illuminate\Database\Eloquent\Builder; @@ - foreach (PluginCategory::cases() as $category) { - $query = Plugin::whereCategory($category->value); - $tabs[$category->value] = Tab::make($category->value) - ->label($category->getLabel()) - ->icon($category->getIcon()) - ->badge($query->count()) - ->modifyQueryUsing(fn () => $query); - } + foreach (PluginCategory::cases() as $category) { + $tabs[$category->value] = Tab::make($category->value) + ->label($category->getLabel()) + ->icon($category->getIcon()) + ->badge(Plugin::whereCategory($category->value)->count()) + ->modifyQueryUsing( + fn (Builder $query) => $query->whereCategory($category->value) + ); + }This keeps the tab behavior aligned with Filament’s usual
getTabs()patterns and avoids reusing a shared builder instance.app/Console/Commands/Plugin/DisablePluginCommand.php (1)
15-36: Optional: handle the “no plugins to disable” case before promptingThe disable flow and
canDisable()check look good. For a slightly smoother UX, you might special‑case the situation wherePlugin::pluck('name', 'id')is empty (no plugins in the table) and print a friendly message instead of prompting with an empty list.app/Console/Commands/Plugin/UpdatePluginCommand.php (1)
15-36: Surface update success/failure fromPlugins::updatePluginRight now
Plugins::updatePlugin($plugin)is called and “Plugin updated.” is always printed. Given the service method itself wraps its work in atry/catchand forwards errors tohandlePluginException, this command may still show a success message even if the update actually failed or the plugin was marked as errored.Consider having
updatePlugin()either:
- return a boolean/enum status that you can check before printing the success message (and maybe adjust the exit code), or
- rethrow/propagate failures for CLI contexts so this command can show a clear error instead of unconditional success.
The same pattern/concern likely applies to the install/uninstall commands using the facade.
app/Console/Commands/Plugin/UninstallPluginCommand.php (1)
16-41: Align CLI message/exit code with actual uninstall outcomeThe uninstall flow and
PluginStatus::NotInstalledguard make sense. SincePlugins::uninstallPlugin()wraps its own logic in atry/catchand delegates errors tohandlePluginException, this command will still report “Plugin uninstalled …” even if something failed during uninstall.It may be worth having
uninstallPlugin()indicate success/failure (return value or throwable) so this command can:
- avoid printing a success message when the plugin ended up in an errored state, and
- optionally return a non‑zero exit code on failure (useful for automation).
This would keep behavior consistent with whatever you decide for the install/update commands.
app/Filament/Admin/Resources/Plugins/PluginResource.php (2)
113-129: Consider wrapping plugin operations in try-catch for user feedback.The
installaction callsPlugins::installPlugin()without catching exceptions. If installation fails (e.g., composer issues, migration failures), the user won't see a meaningful error notification—the exception will propagate up.Apply this pattern to provide user feedback on failure:
->action(function (Plugin $plugin, $livewire) { + try { Plugins::installPlugin($plugin, !$plugin->isTheme() || !Plugins::hasThemePluginEnabled()); redirect(ListPlugins::getUrl(['tab' => $livewire->activeTab])); Notification::make() ->success() ->title(trans('admin/plugin.notifications.installed')) ->send(); + } catch (Exception $exception) { + report($exception); + + Notification::make() + ->danger() + ->title(trans('admin/plugin.notifications.install_failed')) + ->body($exception->getMessage()) + ->send(); + } }),The same pattern should be applied to
update,enable,disable,delete, anduninstallactions.
264-268: URL validation withendsWith('.zip')is case-sensitive.The validation
->endsWith('.zip')won't match.ZIPor.Zip. Consider using a case-insensitive check.TextInput::make('url') ->required() ->url() - ->endsWith('.zip'), + ->rules(['regex:/\.zip$/i']),app/Models/Plugin.php (1)
367-376: Variable name$provideris misleading ingetCommands().The callback variable
$providershould be$commandor$filefor clarity, since this method discovers command classes, not providers.- return array_map(fn ($provider) => $this->namespace . '\\Console\\Commands\\' . str($provider->getRelativePathname())->remove('.php', false), File::allFiles($path)); + return array_map(fn ($file) => $this->namespace . '\\Console\\Commands\\' . str($file->getRelativePathname())->remove('.php', false), File::allFiles($path));app/Facades/Plugins.php (1)
11-31: PHPDoc annotations are missing optional parameters for some methods.The
downloadPluginFromFileanddownloadPluginFromUrlannotations don't include the$cleanDownloadparameter that exists in PluginService.- * @method static void downloadPluginFromFile(UploadedFile $file) - * @method static void downloadPluginFromUrl(string $url) + * @method static void downloadPluginFromFile(UploadedFile $file, bool $cleanDownload = false) + * @method static void downloadPluginFromUrl(string $url, bool $cleanDownload = false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Console/Commands/Plugin/ComposerPluginsCommand.php(1 hunks)app/Console/Commands/Plugin/DisablePluginCommand.php(1 hunks)app/Console/Commands/Plugin/InstallPluginCommand.php(1 hunks)app/Console/Commands/Plugin/MakePluginCommand.php(1 hunks)app/Console/Commands/Plugin/UninstallPluginCommand.php(1 hunks)app/Console/Commands/Plugin/UpdatePluginCommand.php(1 hunks)app/Facades/Plugins.php(1 hunks)app/Filament/Admin/Resources/Plugins/Pages/ListPlugins.php(1 hunks)app/Filament/Admin/Resources/Plugins/PluginResource.php(1 hunks)app/Models/Plugin.php(1 hunks)app/Models/Role.php(1 hunks)app/Providers/Filament/AdminPanelProvider.php(3 hunks)app/Providers/Filament/AppPanelProvider.php(3 hunks)app/Services/Helpers/PluginService.php(1 hunks)composer.json(2 hunks)config/panel.php(1 hunks)lang/en/admin/plugin.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/Models/Role.php
- lang/en/admin/plugin.php
- app/Console/Commands/Plugin/ComposerPluginsCommand.php
- app/Providers/Filament/AdminPanelProvider.php
- app/Console/Commands/Plugin/MakePluginCommand.php
🧰 Additional context used
🧬 Code graph analysis (7)
app/Console/Commands/Plugin/UninstallPluginCommand.php (3)
app/Facades/Plugins.php (1)
Plugins(32-38)app/Console/Commands/Plugin/InstallPluginCommand.php (1)
handle(16-37)app/Services/Helpers/PluginService.php (1)
uninstallPlugin(291-310)
app/Console/Commands/Plugin/InstallPluginCommand.php (3)
app/Models/Plugin.php (1)
Plugin(36-390)app/Facades/Plugins.php (1)
Plugins(32-38)app/Services/Helpers/PluginService.php (1)
installPlugin(257-276)
app/Console/Commands/Plugin/DisablePluginCommand.php (3)
app/Models/Plugin.php (2)
Plugin(36-390)canDisable(203-206)app/Facades/Plugins.php (1)
Plugins(32-38)app/Services/Helpers/PluginService.php (1)
disablePlugin(382-385)
app/Console/Commands/Plugin/UpdatePluginCommand.php (3)
app/Models/Plugin.php (2)
Plugin(36-390)isUpdateAvailable(259-279)app/Facades/Plugins.php (1)
Plugins(32-38)app/Services/Helpers/PluginService.php (1)
updatePlugin(278-289)
app/Providers/Filament/AppPanelProvider.php (5)
app/Facades/Plugins.php (1)
Plugins(32-38)app/Providers/Filament/PanelProvider.php (1)
panel(28-75)app/Providers/Filament/AdminPanelProvider.php (1)
panel(16-53)app/Providers/Filament/ServerPanelProvider.php (1)
panel(17-52)app/Services/Helpers/PluginService.php (1)
loadPanelPlugins(123-152)
app/Filament/Admin/Resources/Plugins/PluginResource.php (4)
app/Facades/Plugins.php (1)
Plugins(32-38)app/Filament/Admin/Resources/Plugins/Pages/ListPlugins.php (1)
ListPlugins(12-40)app/Models/Plugin.php (9)
Plugin(36-390)isUpdateAvailable(259-279)getReadme(378-389)hasSettings(303-314)getSettingsForm(319-332)saveSettings(337-348)isTheme(224-227)canEnable(198-201)canDisable(203-206)app/Services/Helpers/PluginService.php (6)
installPlugin(257-276)hasThemePluginEnabled(422-432)enablePlugin(377-380)disablePlugin(382-385)downloadPluginFromFile(312-349)downloadPluginFromUrl(351-370)
app/Filament/Admin/Resources/Plugins/Pages/ListPlugins.php (3)
app/Facades/Plugins.php (1)
Plugins(32-38)app/Models/Plugin.php (1)
Plugin(36-390)app/Services/Helpers/PluginService.php (1)
updateLoadOrder(413-420)
🪛 PHPMD (2.15.0)
app/Filament/Admin/Resources/Plugins/Pages/ListPlugins.php
16-16: Avoid unused parameters such as '$draggedRecordKey'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
app/Providers/Filament/AppPanelProvider.php (1)
5-36: Panel plugin loading wiring looks consistentImporting
Pluginsand delegating toPlugins::loadPanelPlugins($panel)after building the base panel matches the pattern in the other panel providers and keeps plugin wiring centralized inPluginService. This looks good.app/Console/Commands/Plugin/InstallPluginCommand.php (1)
16-37: Install flow and guards look reasonableThe command’s control flow—resolving the plugin, validating existence and
PluginStatus::NotInstalled, then delegating toPlugins::installPlugin()—is straightforward and consistent with the other lifecycle commands. From this file’s perspective the implementation looks solid.app/Filament/Admin/Resources/Plugins/PluginResource.php (3)
49-56: Table configuration and reordering look correct.The table is configured with proper reordering capability, authorization check for reorder permission, and default sorting by
load_order. The tooltip for the reorder trigger action provides good UX feedback.
57-82: Table columns are well-structured with appropriate visual indicators.Good use of icons and colors to indicate update availability status. The description truncation at 80 characters and conditional category visibility based on active tab are sensible UX decisions.
270-295: Good error handling for URL import action.The try-catch pattern with user notification for failures is correctly implemented. This matches the pattern used in the file import action.
app/Models/Plugin.php (3)
97-178: ThegetRows()method properly handles plugin discovery with fallback for errors.Good defensive coding: the method catches exceptions during JSON parsing, reports them, and creates an errored plugin entry with helpful debugging info. The dev mode check ensures developers see the full exception.
208-213: Version comparison logic is now correct.The
version_compare($currentPanelVersion, $this->panel_version, ...)order is correct—it checks if the current panel version meets the plugin's requirement.
353-362: Provider discovery uses consistent namespace building.The
getProviders()method correctly builds fully-qualified class names from the plugin's namespace and discovered provider files.app/Services/Helpers/PluginService.php (1)
28-121: TheloadPlugins()method is well-structured with proper isolation.Good design choices:
- Unit test bypass prevents test pollution
- PSR-4 autoload registration uses correct
getPrefixesPsr4()check- Config, translations, providers, commands, migrations, and views are loaded with proper path validation
- Exception handling per-plugin prevents one broken plugin from blocking others
app/Facades/Plugins.php (1)
32-38: Facade implementation is correct and follows Laravel conventions.The facade correctly maps to
PluginService::classand will resolve the service from the container.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/Services/Helpers/PluginService.php (4)
35-36: Add runtime type check for ClassLoader.
File::getRequire()returnsmixed. Ifvendor/autoload.phpis corrupted or returns unexpected value, this could cause a fatal error.- /** @var ClassLoader $classLoader */ - $classLoader = File::getRequire(base_path('vendor/autoload.php')); + $classLoader = File::getRequire(base_path('vendor/autoload.php')); + + if (!$classLoader instanceof ClassLoader) { + throw new Exception('Invalid autoloader: expected ClassLoader instance.'); + }
257-276:json_decodewithoutJSON_THROW_ON_ERRORmay cause silent failures.Line 260 calls
json_decode($plugin->composer_packages, true, 512)withoutJSON_THROW_ON_ERROR. Malformed JSON silently returnsnull, which is passed tomanageComposerPackages().- $this->manageComposerPackages(json_decode($plugin->composer_packages, true, 512)); + $composerPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : null; + $this->manageComposerPackages($composerPackages);
294-313: Samejson_decodeissue in uninstallPlugin.Line 297 has the same issue as
installPlugin-json_decodewithoutJSON_THROW_ON_ERROR.- $pluginPackages = json_decode($plugin->composer_packages, true, 512); + $pluginPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : null;
390-405: Missing error handling for JSON operations in setMetaData.
File::json()andjson_encode()withJSON_THROW_ON_ERRORcan throwJsonException, but there's no try-catch. Ifplugin.jsonis malformed, this propagates an unhandled exception.private function setMetaData(string|Plugin $plugin, array $data): void { $path = plugin_path($plugin instanceof Plugin ? $plugin->id : $plugin, 'plugin.json'); if (File::exists($path)) { + try { $pluginData = File::json($path, JSON_THROW_ON_ERROR); $metaData = array_merge($pluginData['meta'] ?? [], $data); $pluginData['meta'] = $metaData; File::put($path, json_encode($pluginData, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)); $plugin = $plugin instanceof Plugin ? $plugin : Plugin::findOrFail($plugin); $plugin->update($metaData); + } catch (\JsonException $e) { + report($e); + throw new Exception("Failed to update plugin metadata: {$e->getMessage()}"); + } } }app/Filament/Admin/Resources/Plugins/PluginResource.php (1)
239-243: Plugin ID validation relies on filename, not actual plugin.json content.The duplicate check at line 241 validates against the zip filename rather than the
idfield inplugin.jsoninside the archive. A zip namednew-plugin.zipcontaining aplugin.jsonwithid="existing-plugin"would bypass this check.Consider extracting and validating
plugin.jsonbefore full extraction, or perform this check inPluginService::downloadPluginFromFile()which has access to the zip contents.
🧹 Nitpick comments (2)
app/Filament/Admin/Resources/Plugins/PluginResource.php (1)
275-281: URL-based plugin name extraction is fragile.The plugin name extraction at line 277 assumes the URL path has the plugin name as the last segment before
.zip. URLs with query parameters (e.g.,example.com/download?file=plugin.zip) or different structures could yield unexpected results.Consider validating the extracted plugin name is non-empty and contains only valid characters.
try { - $pluginName = str($data['url'])->before('.zip')->explode('/')->last(); + $pluginName = str($data['url'])->before('.zip')->explode('/')->last(); + + if (empty($pluginName) || !preg_match('/^[a-zA-Z0-9_-]+$/', $pluginName)) { + throw new Exception(trans('admin/plugin.notifications.invalid_plugin_name')); + }app/Services/Helpers/PluginService.php (1)
354-373: Consider SSRF protection for URL downloads.The HTTP client usage with timeouts (line 360) is good, but there's no validation to prevent access to internal network resources. An attacker could potentially probe internal services via URLs like
http://localhost:6379or cloud metadata endpoints.Consider adding URL scheme and private IP validation:
public function downloadPluginFromUrl(string $url, bool $cleanDownload = false): void { + $parsed = parse_url($url); + if (!$parsed || !in_array($parsed['scheme'] ?? '', ['http', 'https'])) { + throw new InvalidFileUploadException('Invalid URL scheme.'); + } + + $host = $parsed['host'] ?? ''; + if (filter_var($host, FILTER_VALIDATE_IP) && + !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + throw new InvalidFileUploadException('Access to private IP ranges is not allowed.'); + } + $info = pathinfo($url);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Plugins/PluginResource.php(1 hunks)app/Models/Plugin.php(1 hunks)app/Services/Helpers/PluginService.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Models/Plugin.php (4)
app/Facades/Plugins.php (1)
Plugins(32-38)app/helpers.php (1)
plugin_path(91-94)app/Services/Helpers/PluginService.php (1)
isDevModeActive(454-457)app/Contracts/Plugins/HasPluginSettings.php (2)
getSettingsForm(12-12)saveSettings(17-17)
🔇 Additional comments (16)
app/Filament/Admin/Resources/Plugins/PluginResource.php (4)
1-49: LGTM - Navigation and label configuration.The navigation setup with localized labels, dynamic badge count, and proper icon configuration follows Filament conventions well.
50-83: LGTM - Table configuration is well-structured.The table setup with sortable/searchable columns, conditional visibility, authorization for reordering, and visual indicators for update availability is well-implemented.
114-216: Action group is well-organized with proper guards.The action handlers have appropriate authorization, visibility conditions, confirmation modals where needed, and consistent notification/redirect patterns. The theme plugin enabling logic at line 122 correctly handles the exclusive theme constraint.
307-312: LGTM - Pages configuration.The route configuration correctly maps the index to the ListPlugins page.
app/Models/Plugin.php (6)
98-183: LGTM - Plugin discovery with good validation.The
getRows()method properly validates that the plugin folder name matches theidinplugin.json(line 114-116), has appropriate error handling with dev-mode awareness, and correctly reads thepanel_versionfield (line 142).
213-218: LGTM - Version comparison logic is correct.The
isCompatible()method now correctly compares the current panel version against the plugin's required version (line 217). The strict mode (=) vs non-strict (>=) handling viaisPanelVersionStrict()is well-implemented.
239-262: LGTM - Update data fetching with proper HTTP handling.The
getUpdateData()method now uses Laravel's HTTP client with appropriate timeouts (lines 248), caching (line 246), and error handling. The support for multi-plugin update JSONs (lines 251-253) is a nice touch.
264-306: LGTM - Update availability and download URL methods.Both
isUpdateAvailable()andgetDownloadUrlForUpdate()correctly use the$panelVersionvariable for array key lookup (lines 275, 297) and include wildcard fallback support.
308-353: LGTM - Settings delegation pattern.The settings methods cleanly delegate to the loaded plugin instance via
filament($this->id), with appropriateHasPluginSettingsinterface checks and silent handling when plugins aren't loaded on the current panel.
383-394: LGTM - README caching.The
getReadme()method appropriately caches the README content to avoid repeated file reads.app/Services/Helpers/PluginService.php (6)
28-121: LGTM - Plugin loading with proper PSR-4 registration.The
loadPlugins()method now correctly usesgetPrefixesPsr4()for namespace checking (line 55), has proper compatibility filtering, and comprehensive loading of config, translations, providers, commands, migrations, and views with appropriate error handling.
154-206: LGTM - Composer package management with proper error handling.The method now uses
JSON_THROW_ON_ERROR(line 173) and sets appropriate timeouts (600s) for composer operations (lines 188, 201). The error handling and package aggregation logic is well-structured.
232-255: LGTM - Asset building with timeouts.The method now includes appropriate timeouts for
yarn install(300s) andyarn build(600s), addressing previous concerns about indefinite blocking.
278-292: LGTM - Update plugin with null URL guard.The
updatePlugin()method now properly checks if$downloadUrlis truthy before proceeding (line 282), addressing the previous concern about null URLs.
315-352: LGTM - Zip extraction with security validations.The method now includes file size validation (lines 318-321) and path traversal checks (lines 330-336), addressing previous security concerns. The extraction path logic (line 344) correctly handles both flat and nested zip structures.
425-468: LGTM - Helper methods.The
hasThemePluginEnabled(),getPluginLanguages(),isDevModeActive(), andhandlePluginException()methods are well-implemented with clear logic and appropriate patterns.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/Models/Plugin.php (1)
349-371: Fix FQCN building for nested Providers/Commands paths (slashes ingetRelativePathname()).
This still produces invalid namespaces if nested dirs exist (e.g.Sub/FooProvider).- return array_map(fn ($provider) => $this->namespace . '\\Providers\\' . str($provider->getRelativePathname())->remove('.php', false), File::allFiles($path)); + return array_map( + fn ($provider) => $this->namespace . '\\Providers\\' . + str($provider->getRelativePathname()) + ->remove('.php', false) + ->replace(['/', '\\'], '\\'), + File::allFiles($path) + ); ... - return array_map(fn ($provider) => $this->namespace . '\\Console\\Commands\\' . str($provider->getRelativePathname())->remove('.php', false), File::allFiles($path)); + return array_map( + fn ($provider) => $this->namespace . '\\Console\\Commands\\' . + str($provider->getRelativePathname()) + ->remove('.php', false) + ->replace(['/', '\\'], '\\'), + File::allFiles($path) + );
🧹 Nitpick comments (1)
database/Seeders/DatabaseSeeder.php (1)
21-33: Make plugin seeding more resilient (stable class naming + don’t let one plugin break all seeds).
Deriving the class from$plugin->nameis brittle; also consider skipping errored plugins for seeding and isolating failures so the core seed still completes.use App\Models\Plugin; use App\Models\Role; use Illuminate\Database\Seeder; use Illuminate\Support\Str; class DatabaseSeeder extends Seeder { public function run(): void { $this->call(EggSeeder::class); Role::firstOrCreate(['name' => Role::ROOT_ADMIN]); $plugins = Plugin::query()->orderBy('load_order')->get(); foreach ($plugins as $plugin) { if (!$plugin->shouldLoad()) { continue; } - $name = Str::studly($plugin->name); + // Prefer stable identifiers over display names for class conventions + $name = Str::studly($plugin->id); $seeder = "\\{$plugin->namespace}\Database\Seeders\\{$name}Seeder"; if (class_exists($seeder)) { - $this->call($seeder); + try { + $this->call($seeder); + } catch (\Throwable $e) { + report($e); + // continue seeding other plugins + } } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Models/Plugin.php(1 hunks)database/Seeders/DatabaseSeeder.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
database/Seeders/DatabaseSeeder.php (1)
app/Models/Plugin.php (2)
Plugin(37-385)shouldLoad(196-199)
app/Models/Plugin.php (4)
app/Facades/Plugins.php (1)
Plugins(32-38)app/helpers.php (1)
plugin_path(91-94)app/Services/Helpers/PluginService.php (1)
isDevModeActive(454-457)app/Contracts/Plugins/HasPluginSettings.php (2)
getSettingsForm(12-12)saveSettings(17-17)
🔇 Additional comments (1)
app/Models/Plugin.php (1)
37-52: The concern about Sushi model persistence is unfounded. All status, load_order, and status_message changes go throughPluginService::setMetaData()(lines 390–403), which explicitly writes toplugin.jsonviaFile::put()before calling$plugin->update(). No directupdate()calls exist elsewhere in the codebase—the only instance is within the controlled service method. Thefillablefields are appropriate and not misleading since callers do not interact with the model directly; they use service methods that handle persistence correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
app/Models/Plugin.php (5)
149-176: Undefined variable$datain catch block.If
File::json()throws before$datais assigned, the catch block's references to$data['id'],$data['name'], and$data['author']on lines 158-160 will cause undefined variable notices.Initialize
$data = []before the try block to ensure deterministic behavior.
206-209:canDisable()should not requireisCompatible().If a plugin becomes incompatible after a panel upgrade, administrators won't be able to disable it since
isCompatible()returns false. This traps users with incompatible plugins they cannot manage.Remove the
isCompatible()check fromcanDisable():public function canDisable(): bool { - return $this->status !== PluginStatus::Disabled && $this->status !== PluginStatus::NotInstalled && $this->isCompatible(); + return $this->status !== PluginStatus::Disabled && $this->status !== PluginStatus::NotInstalled; }
211-216: Caret constraints not properly handled byversion_compare().When
panel_versionis^1.0.0,isPanelVersionStrict()returns false, butisCompatible()still passes the raw^1.0.0string toversion_compare(). The caret character causes incorrect comparison results.Strip the caret before comparison:
public function isCompatible(): bool { $currentPanelVersion = config('app.version', 'canary'); - return !$this->panel_version || $currentPanelVersion === 'canary' || version_compare($currentPanelVersion, $this->panel_version, $this->isPanelVersionStrict() ? '=' : '>='); + if (!$this->panel_version || $currentPanelVersion === 'canary') { + return true; + } + + $required = ltrim($this->panel_version, '^'); + return version_compare($currentPanelVersion, $required, $this->isPanelVersionStrict() ? '=' : '>='); }
244-260: Add validation that HTTP response is an array.
Http::get()->json()can return non-array values (scalar, null) for malformed responses. Thearray_key_exists()calls on lines 249 and 253 will fail if$datais not an array.try { $data = Http::timeout(5)->connectTimeout(1)->get($this->update_url)->throw()->json(); + + if (!is_array($data)) { + return null; + } // Support update jsons that cover multiple plugins - if (array_key_exists($this->id, $data)) { + if (array_key_exists($this->id, $data) && is_array($data[$this->id])) { $data = $data[$this->id]; }
350-371: Nested directory paths produce invalid namespace separators.
getRelativePathname()returns paths with OS-specific directory separators (e.g.,Sub/Provider.php). The current implementation would produceNamespace\Providers\Sub/Providerwith a forward slash instead of a proper namespace separator.- return array_map(fn ($provider) => $this->namespace . '\\Providers\\' . str($provider->getRelativePathname())->remove('.php', false), File::allFiles($path)); + return array_map(fn ($provider) => $this->namespace . '\\Providers\\' . str($provider->getRelativePathname())->remove('.php', false)->replace('/', '\\'), File::allFiles($path));Apply the same fix to
getCommands():- return array_map(fn ($provider) => $this->namespace . '\\Console\\Commands\\' . str($provider->getRelativePathname())->remove('.php', false), File::allFiles($path)); + return array_map(fn ($command) => $this->namespace . '\\Console\\Commands\\' . str($command->getRelativePathname())->remove('.php', false)->replace('/', '\\'), File::allFiles($path));app/Services/Helpers/PluginService.php (5)
35-36: Add runtime type validation for ClassLoader.
File::getRequire()returnsmixed. Ifvendor/autoload.phpis corrupted or returns an unexpected value, subsequent method calls will cause fatal errors.- /** @var ClassLoader $classLoader */ - $classLoader = File::getRequire(base_path('vendor/autoload.php')); + $classLoader = File::getRequire(base_path('vendor/autoload.php')); + + if (!$classLoader instanceof ClassLoader) { + throw new Exception('Invalid autoloader: expected ClassLoader instance.'); + }
270-291:json_decodesilently returnsnullfor malformed JSON.Line 273 calls
json_decode()withoutJSON_THROW_ON_ERROR. Ifcomposer_packagescontains malformed JSON,nullis passed tomanageComposerPackages()and the error is silently ignored.public function installPlugin(Plugin $plugin, bool $enable = true): void { try { - $this->manageComposerPackages(json_decode($plugin->composer_packages, true, 512)); + $composerPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : null; + $this->manageComposerPackages($composerPackages);
369-388: SSRF vulnerability - add URL validation.The method accepts arbitrary URLs without validation. Attackers could access internal resources via
http://localhost:6379, cloud metadata endpoints, or use non-HTTP schemes likefile://.Add URL validation before the HTTP request:
public function downloadPluginFromUrl(string $url, bool $cleanDownload = false): void { + // Validate URL scheme + $parsed = parse_url($url); + if (!$parsed || !in_array($parsed['scheme'] ?? '', ['http', 'https'], true)) { + throw new InvalidFileUploadException('Invalid URL scheme. Only http and https are allowed.'); + } + + // Prevent access to private networks + $host = $parsed['host'] ?? ''; + if (filter_var($host, FILTER_VALIDATE_IP)) { + if (!filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + throw new InvalidFileUploadException('Access to private IP ranges is not allowed.'); + } + } + $info = pathinfo($url);
405-420: Add error handling for JSON operations.
File::json()andjson_encode()withJSON_THROW_ON_ERRORcan throwJsonException. Ifplugin.jsonis malformed, this propagates an unhandled exception.private function setMetaData(string|Plugin $plugin, array $data): void { $path = plugin_path($plugin instanceof Plugin ? $plugin->id : $plugin, 'plugin.json'); if (File::exists($path)) { + try { $pluginData = File::json($path, JSON_THROW_ON_ERROR); $metaData = array_merge($pluginData['meta'] ?? [], $data); $pluginData['meta'] = $metaData; File::put($path, json_encode($pluginData, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)); + } catch (\JsonException $e) { + report($e); + throw new Exception("Failed to update plugin metadata: {$e->getMessage()}"); + } $plugin = $plugin instanceof Plugin ? $plugin : Plugin::findOrFail($plugin); $plugin->update($metaData); } }
353-359: Validate plugin name extracted from filename.The plugin name is extracted directly from the uploaded filename without sanitization. A malicious filename like
../../etc.zipwould result in$pluginName = "../../etc", which could cause path traversal when used inplugin_path()orFile::deleteDirectory().$pluginName = str($file->getClientOriginalName())->before('.zip')->toString(); + + // Validate plugin name to prevent directory traversal + if (preg_match('/[^a-zA-Z0-9_-]/', $pluginName)) { + throw new Exception('Invalid plugin name. Only alphanumeric characters, dashes, and underscores are allowed.'); + } if ($cleanDownload) {
🧹 Nitpick comments (1)
app/Services/Helpers/PluginService.php (1)
293-307: Consider providing feedback when no update URL is available.The null check at line 297 prevents the crash, but when
getDownloadUrlForUpdate()returns null (e.g., for canary builds), the method silently does nothing. Consider logging a warning or throwing an exception so users know why the update didn't proceed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Facades/Plugins.php(1 hunks)app/Models/Plugin.php(1 hunks)app/Services/Helpers/PluginService.php(1 hunks)database/Seeders/DatabaseSeeder.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- database/Seeders/DatabaseSeeder.php
🔇 Additional comments (4)
app/Facades/Plugins.php (1)
1-40: LGTM - Well-structured Laravel facade.The facade follows Laravel conventions correctly. The PHPDoc annotations provide good IDE support for the proxied methods.
app/Services/Helpers/PluginService.php (3)
159-207: LGTM - Good timeout and error handling for Composer operations.The method properly handles JSON parsing with
JSON_THROW_ON_ERROR, includes 600-second timeouts on Process calls, and has appropriate error handling.
245-268: LGTM - Proper timeouts added for asset building.Both
yarn install(5 min) andyarn build(10 min) now have appropriate timeouts, addressing the previous hanging concern.
469-484: LGTM - Well-structured helper methods and centralized exception handling.The
handlePluginException()method provides consistent dev-mode-aware error handling. The query helper methods (hasThemePluginEnabled,getPluginLanguages,isDevModeActive) are clean and straightforward.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/Services/Helpers/PluginService.php (5)
35-37: Validate autoloader type before using it
File::getRequire()returnsmixed; ifvendor/autoload.phpreturns something unexpected, later calls can fatally error./** @var ClassLoader $classLoader */ - $classLoader = File::getRequire(base_path('vendor/autoload.php')); + $classLoader = File::getRequire(base_path('vendor/autoload.php')); + if (!$classLoader instanceof ClassLoader) { + throw new Exception('Invalid Composer autoloader: expected ' . ClassLoader::class); + }
272-293: Don’t silently swallow malformed composer_packages JSON
json_decode(..., 512)returnsnullon malformed JSON; that can lead to missing package installs/removals with no clear error.public function installPlugin(Plugin $plugin, bool $enable = true): void { try { - $this->manageComposerPackages(json_decode($plugin->composer_packages, true, 512)); + $composerPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : []; + $this->manageComposerPackages($composerPackages); @@ } catch (Exception $exception) { $this->handlePluginException($plugin, $exception); } } @@ public function uninstallPlugin(Plugin $plugin, bool $deleteFiles = false): void { try { - $pluginPackages = json_decode($plugin->composer_packages, true, 512); + $pluginPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : []; @@ $this->manageComposerPackages(oldPackages: $pluginPackages); } catch (Exception $exception) { $this->handlePluginException($plugin, $exception); } }Also applies to: 311-330
332-369: Fix max_import_size units + address remaining ZipSlip/SSRF risks
UploadedFile::getSize()andstrlen($content)are bytes, but the error message says “MiB”; if config is MiB, current comparisons will wrongly reject almost all uploads (likely matching the reported unzip/import failure).- URL downloads still allow SSRF (no scheme/host/IP allowlist, redirect policy, or streaming to file).
- Zip validation should also reject backslashes / drive letters and ensure extracted paths stay within the intended directory.
public function downloadPluginFromFile(UploadedFile $file, bool $cleanDownload = false): void { // Validate file size to prevent zip bombs - $maxSize = config('panel.plugin.max_import_size'); - if ($file->getSize() > $maxSize) { - throw new Exception("Zip file too large. ($maxSize MiB)"); + $maxMiB = (int) config('panel.plugin.max_import_size'); + $maxBytes = $maxMiB * 1024 * 1024; + if ($file->getSize() !== null && $file->getSize() > $maxBytes) { + throw new Exception("Zip file too large. ({$maxMiB} MiB)"); } @@ for ($i = 0; $i < $zip->numFiles; $i++) { $filename = $zip->getNameIndex($i); - if (Str::contains($filename, '..') || Str::startsWith($filename, '/')) { + if ( + $filename === false || + Str::contains($filename, '..') || + Str::contains($filename, '\\') || // windows path traversal + Str::startsWith($filename, '/') || // absolute *nix + preg_match('/^[a-zA-Z]:\//', $filename) // absolute windows drive + ) { $zip->close(); throw new Exception('Zip file contains invalid path traversal sequences.'); } } @@ public function downloadPluginFromUrl(string $url, bool $cleanDownload = false): void { + // TODO: Enforce SSRF policy (scheme allowlist, block private IP ranges, limit redirects). $info = pathinfo($url); $tmpDir = TemporaryDirectory::make()->deleteWhenDestroyed(); $tmpPath = $tmpDir->path($info['basename']); - $content = Http::timeout(60)->connectTimeout(5)->throw()->get($url)->body(); + $content = Http::timeout(60)->connectTimeout(5)->throw()->get($url)->body(); @@ - $maxSize = config('panel.plugin.max_import_size'); - if (strlen($content) > $maxSize) { - throw new InvalidFileUploadException("Zip file too large. ($maxSize MiB)"); + $maxMiB = (int) config('panel.plugin.max_import_size'); + $maxBytes = $maxMiB * 1024 * 1024; + if (strlen($content) > $maxBytes) { + throw new InvalidFileUploadException("Zip file too large. ({$maxMiB} MiB)"); }If you want to fully close the SSRF + large-file memory issues, the next step is: validate URL (scheme + resolved IP not private/reserved), set
max_redirects, and stream to$tmpPathinstead of loading into memory.Also applies to: 371-390
408-422: Harden plugin.json read/modify/write (exceptions + atomicity + locking)
File::json(..., JSON_THROW_ON_ERROR)/json_encode(..., JSON_THROW_ON_ERROR)can throw, and current writes are non-atomic (risking corruption / partial updates, especially with concurrent actions like load-order updates).private function setMetaData(string|Plugin $plugin, array $data): void { $path = plugin_path($plugin instanceof Plugin ? $plugin->id : $plugin, 'plugin.json'); if (File::exists($path)) { - $pluginData = File::json($path, JSON_THROW_ON_ERROR); - $metaData = array_merge($pluginData['meta'] ?? [], $data); - $pluginData['meta'] = $metaData; - - File::put($path, json_encode($pluginData, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)); - - $plugin = $plugin instanceof Plugin ? $plugin : Plugin::findOrFail($plugin); - $plugin->update($metaData); + try { + $pluginData = File::json($path, JSON_THROW_ON_ERROR); + $metaData = array_merge($pluginData['meta'] ?? [], $data); + $pluginData['meta'] = $metaData; + + $payload = json_encode($pluginData, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + File::put($path, $payload, true); // consider atomic temp+rename if needed + + $plugin = $plugin instanceof Plugin ? $plugin : Plugin::findOrFail($plugin); + $plugin->update($metaData); + } catch (\Throwable $e) { + report($e); + throw new Exception("Failed to update plugin metadata: {$e->getMessage()}"); + } } }Also consider wrapping
updateLoadOrder()in a DB transaction and/or batching writes so partial failures don’t leave mixed load orders.Also applies to: 432-440
355-362: Sanitize$pluginNamebefore using it in filesystem paths
$pluginNamecomes from the client filename and is used inplugin_path(...)/ deletion; enforce a strict allowlist to prevent weird paths and collisions.$pluginName = str($file->getClientOriginalName())->before('.zip')->toString(); + if (!preg_match('/^[A-Za-z0-9_-]+$/', $pluginName)) { + $zip->close(); + throw new Exception('Invalid plugin name.'); + }
🧹 Nitpick comments (2)
app/Services/Helpers/PluginService.php (2)
126-154: Instantiate panel plugin via container (avoid hard ctor requirement)
new $pluginClass()forces a zero-arg constructor; using the container is more robust and consistent with Laravel conventions.- $panel->plugin(new $pluginClass()); + $panel->plugin($this->app->make($pluginClass));
28-124: CatchThrowable(not onlyException) in plugin lifecycle flows
TypeErrors / Errors from plugin code won’t be caught today, so status won’t be set toErroredoutside dev mode.- } catch (Exception $exception) { + } catch (\Throwable $exception) { $this->handlePluginException($plugin, $exception); }…and update the handler signature:
- private function handlePluginException(string|Plugin $plugin, Exception $exception): void + private function handlePluginException(string|Plugin $plugin, \Throwable $exception): voidAlso applies to: 126-155, 161-209, 247-270, 272-293, 295-309, 311-330, 476-485
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
app/Services/Helpers/PluginService.php (6)
35-37: Validatevendor/autoload.phpreturn type before using it
File::getRequire()ismixed; callinggetPrefixesPsr4()will fatal if it’s not aClassLoader./** @var ClassLoader $classLoader */ - $classLoader = File::getRequire(base_path('vendor/autoload.php')); + $classLoader = File::getRequire(base_path('vendor/autoload.php')); + if (!$classLoader instanceof ClassLoader) { + throw new Exception('Invalid Composer autoloader instance.'); + }
272-293: Don’t silently accept malformedcomposer_packagesJSON during install/uninstall
json_decode(..., 512)can returnnullon malformed JSON, causing confusing downstream behavior.public function installPlugin(Plugin $plugin, bool $enable = true): void { try { - $this->manageComposerPackages(json_decode($plugin->composer_packages, true, 512)); + $pluginPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : null; + $this->manageComposerPackages($pluginPackages); @@ } catch (Exception $exception) { $this->handlePluginException($plugin, $exception); } } @@ public function uninstallPlugin(Plugin $plugin, bool $deleteFiles = false): void { try { - $pluginPackages = json_decode($plugin->composer_packages, true, 512); + $pluginPackages = $plugin->composer_packages + ? json_decode($plugin->composer_packages, true, 512, JSON_THROW_ON_ERROR) + : null;Also applies to: 311-330
355-363: Sanitize$pluginNamebefore using it in paths / deletions
$pluginNamecomes from client filename and is used inplugin_path()andFile::deleteDirectory(). Reject unexpected characters to avoid traversal/foot-guns.$pluginName = str($file->getClientOriginalName())->before('.zip')->toString(); + if (!preg_match('/^[a-zA-Z0-9_-]+$/', $pluginName)) { + $zip->close(); + throw new Exception('Invalid plugin archive name.'); + }
346-369: Zip extraction hardening is still incomplete (path traversal / zip bomb / symlinks)
Current checks ('..'and leading/) are not sufficient; also no max entry count / max uncompressed size, and extraction happens directly into the target directory.Minimum improvements:
- Validate each entry’s normalized path (including backslashes / Windows drive letters).
- Enforce: max entries, max per-file uncompressed size, max total uncompressed size.
- Extract to a temp dir first, validate expected layout (must contain
plugin.json), then atomically move into place.
371-390: SSRF + memory risk indownloadPluginFromUrl()(needs allowlist/denylist + streaming + redirect control)
This fetches arbitrary URLs with default redirect behavior and loads the full body into memory before size checks.Concrete fixes to consider:
- Parse/validate scheme (
http/httpsonly), block localhost/private/reserved IPs (incl. DNS rebinding considerations).- Disable redirects (or strictly limit and re-validate the final host).
- Stream to disk (Guzzle
sink) and enforce a hard max download size.
408-422: Add try/catch + atomic write/locking aroundplugin.jsonupdates
File::json(..., JSON_THROW_ON_ERROR)/json_encode(..., JSON_THROW_ON_ERROR)/ filesystem writes can throw; current code can crash flows and also has read-modify-write race potential.At minimum:
- catch
JsonException+Throwable- write via temp file + rename (or
flock) to avoid corrupted/partial writes- consider not calling
$plugin->update($metaData)if the file write failed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Models/Plugin.php(1 hunks)app/Services/Helpers/PluginService.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Models/Plugin.php
notAreYouScared
left a comment
notAreYouScared
left a comment
There was a problem hiding this comment.
Good to go, once the update script backs up the new folders/files that will be created by plugins!
We wouldn't want people to lose plugins after every update!
lancepioch
left a comment
lancepioch
left a comment
There was a problem hiding this comment.
Two main things:
- Use dependency injection instead of facade for Plugins
- Do not use Sushi
