Skip to content

Refactor egg & server icon uploading#2281

Open
Boy132 wants to merge 11 commits intomainfrom
boy132/clear-old-egg-images
Open

Refactor egg & server icon uploading#2281
Boy132 wants to merge 11 commits intomainfrom
boy132/clear-old-egg-images

Conversation

@Boy132
Copy link
Member

@Boy132 Boy132 commented Mar 23, 2026

Also closes #2269

@Boy132 Boy132 self-assigned this Mar 23, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 23, 2026

📝 Walkthrough

Walkthrough

Replaces image-based icon handling with icon-first workflows across models, Filament UIs, services, views, and translations; adds model writeIcon methods to normalize extensions and remove old files; introduces reusable UploadIcon/DeleteIcon actions; importer/exporter now use icon payloads.

Changes

Cohort / File(s) Summary
Egg model & services
app/Models/Egg.php, app/Services/Eggs/Sharing/EggExporterService.php, app/Services/Eggs/Sharing/EggImporterService.php
Renamed image→icon accessors/constants; added writeIcon() on Egg to normalize extensions, write to public/icons/egg/{uuid}.{ext}, and delete alternate-extension files; exporter/importer now produce/consume icon base64 and delegate persistence to model write methods.
Server model & persistence
app/Models/Server.php
Renamed IMAGE_FORMATS→ICON_FORMATS; added writeIcon() to normalize extensions, write to public/icons/server/{uuid}.{ext}, and remove previous-extension files; avatar fallback now prefers egg icon.
Filament actions (new)
app/Filament/Components/Actions/UploadIcon.php, app/Filament/Components/Actions/DeleteIcon.php
Added reusable UploadIcon (modal with URL/file tabs, URL validation, writes via model) and DeleteIcon (deletes all matching icon files, notifies and refreshes).
Filament admin pages
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php, app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php, app/Filament/Admin/Resources/Servers/Pages/EditServer.php, app/Filament/Server/Pages/Settings.php, app/Filament/App/Resources/Servers/Pages/ListServers.php
Replaced inline upload/import modals and per-page helpers with UploadIcon/DeleteIcon; switched previews/columns from imageicon and removed now-unused fields/imports.
Removed legacy action
app/Filament/Components/Actions/DeleteServerIcon.php
Deleted legacy per-page delete action; replaced by shared DeleteIcon component.
Views & Livewire
resources/views/livewire/server-entry.blade.php, resources/views/livewire/server-entry-placeholder.blade.php
Fallback background/placeholder now uses $server->egg->icon instead of $server->egg->image.
API & Transformer
app/Transformers/Api/Application/EggTransformer.php
Transformer output switched from imageicon (sourced from $model->icon).
Translations
lang/en/admin/egg.php, lang/en/admin/server.php
Renamed image-related translation keys/strings to icon equivalents (import_icon, delete_icon, no_icon, could_not_write, etc.).

Sequence Diagram(s)

sequenceDiagram
    participant UI as Filament UI
    participant Action as UploadIcon / DeleteIcon
    participant Model as Egg/Server Model
    participant Storage as Public Disk
    participant Notif as Notification

    UI->>Action: open modal (URL tab or File tab) / click delete
    alt Upload via URL
        Action->>Action: validate URL (scheme, format, host/IP)
        Action->>Model: fetch remote bytes, determine extension
        Action->>Model: writeIcon(extension, bytes)
    else Upload via File
        Action->>Model: read uploaded file bytes & extension
        Action->>Model: writeIcon(extension, bytes)
    else Delete
        Action->>Storage: delete icons/{type}/{uuid}.{eachAllowedExt}
        Action->>Notif: send icon_deleted
        Action->>UI: refresh record
    end
    Model->>Storage: write icons/{type}/{uuid}.{normalizedExt}
    Model->>Storage: delete other extensions for same uuid
    Model-->>Action: return path / throw on error
    Action->>Notif: show icon_updated or could_not_write
    Action->>UI: refresh record/state
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Refactor egg & server icon uploading' clearly and concisely summarizes the main change: refactoring the egg and server icon upload functionality from image-based to icon-based handling.
Description check ✅ Passed The description 'Also closes #2269' is brief but directly related to the changeset, referencing the linked issue about icon file cleanup during uploads.
Linked Issues check ✅ Passed The PR fully addresses issue #2269 by implementing automatic cleanup of existing icon files across all supported extensions before writing new files via the new writeIcon() methods in both Egg and Server models, eliminating the problem of orphaned old icon files.
Out of Scope Changes check ✅ Passed All changes are directly related to the refactoring objective: renaming image fields to icon fields, implementing unified icon upload/delete components (UploadIcon, DeleteIcon), updating models with writeIcon() methods, and updating all related UI/Blade templates and translations consistently.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)

195-208: ⚠️ Potential issue | 🟠 Major

The admin modal has the same stale-icon gap.

This file-upload branch never calls Server::writeServerIcon(), so it skips the extension-normalization and delete-other-files logic that the URL branch already uses. Replacing an existing icon with a different extension here will still leave the old file behind.

Also applies to: 215-240, 1205-1223

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 195 -
208, The FileUpload handling for the 'icon' field in EditServer.php currently
saves the uploaded file directly to disk and never invokes
Server::writeServerIcon(), so extension-normalization and cleanup of old icon
files (using Server::ICON_STORAGE_PATH and Server::ICON_FORMATS) are skipped;
update the FileUpload save flow to call Server::writeServerIcon($server,
$uploadedFile) (or the equivalent helper used by the URL branch) after upload
completes, passing the model instance and the uploaded file so the method can
normalize the extension and delete other legacy files; apply the same change to
the other FileUpload occurrences you noted (the other 'icon' FileUpload blocks)
to ensure consistent behavior across all branches.
app/Filament/Server/Pages/Settings.php (1)

163-176: ⚠️ Potential issue | 🟠 Major

The file-upload branch still skips writeServerIcon().

Only the URL path goes through saveIconFromUrl() and Server::writeServerIcon(). The uploaded-file path just reports success, so replacing uuid.png with uuid.jpg here never executes the helper that removes sibling extensions, and the stale icon can still be resolved first. A minimal fix is to read the stored upload from $data['icon'] and pass its bytes + extension into writeServerIcon() before notifying.

Also applies to: 183-207, 448-466

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Server/Pages/Settings.php` around lines 163 - 176, The
file-upload branch never invokes Server::writeServerIcon(), so uploaded icons
bypass the sibling-extension cleanup and stale images can still be served;
update the form handling that currently calls saveIconFromUrl() for URL uploads
to also handle uploaded files by reading the stored upload from $data['icon'],
extracting the file bytes and extension, and calling writeServerIcon($server,
$bytes, $extension) (the same helper used by saveIconFromUrl()) before sending
the success notification; ensure you reference the FileUpload field name 'icon',
Server::ICON_STORAGE_PATH and Server::ICON_FORMATS when locating the code to
modify and apply the same logic to the other similar blocks noted.
🧹 Nitpick comments (2)
app/Models/Server.php (2)

558-564: Translation keys reference "egg" but are used for server icons.

The error messages use trans('admin/egg.import.unknown_extension') and trans('admin/egg.import.could_not_write'), which are egg-specific translation keys. Consider using server-specific translations for clarity.

♻️ Proposed fix
         if (is_null($normalizedExtension)) {
             if ($throw) {
-                throw new Exception(trans('admin/egg.import.unknown_extension'));
+                throw new Exception(trans('admin/server.import.unknown_extension'));
             }

             return false;
         }
         } else {
             if ($throw) {
-                throw new Exception(trans('admin/egg.import.could_not_write'));
+                throw new Exception(trans('admin/server.import.could_not_write'));
             }
         }

Then add corresponding translation keys in the server language file.

Also applies to: 579-583

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Models/Server.php` around lines 558 - 564, The code in Server.php throws
Exceptions using egg-specific translation keys (e.g.,
trans('admin/egg.import.unknown_extension') and
trans('admin/egg.import.could_not_write')) even though this logic is for server
icon import; update those calls to use server-specific translation keys (for
example trans('admin/servers.import.unknown_extension') and
trans('admin/servers.import.could_not_write')) in the same places where
normalizedExtension is checked and where the Exception is thrown, and then add
matching entries to the server language file so the new keys resolve correctly.

148-156: Minor inconsistency: jpeg key exists here but not in Egg::ICON_FORMATS.

Server::ICON_FORMATS includes a jpeg key (line 153), while Egg::ICON_FORMATS omits it. Since writeServerIcon() normalizes jpegjpg anyway, the jpeg key here is redundant. However, keeping it doesn't cause functional issues—it just means getIconAttribute() will check for .jpeg files that will never be written by the new method.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Models/Server.php` around lines 148 - 156, Server::ICON_FORMATS contains
a redundant 'jpeg' key while Egg::ICON_FORMATS does not, causing
getIconAttribute() to check for .jpeg files that writeServerIcon() never writes
(it normalizes 'jpeg' to 'jpg'); remove the 'jpeg' entry from
Server::ICON_FORMATS (or add it to Egg::ICON_FORMATS if intentional) so formats
are consistent with writeServerIcon() and avoid checking for unreachable .jpeg
files in getIconAttribute().
🤖 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/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 122-125: The upload flow is rejecting or mishandling .jpeg because
validation checks against Egg::ICON_FORMATS before normalization and the
file-upload branch never calls Egg::writeEggIcon; update the validation and
upload handling so .jpeg is normalized to 'jpg' (or mapped) prior to the
array_key_exists check against Egg::ICON_FORMATS and ensure the file-upload
branch invokes Egg::writeEggIcon(...) after storing the uploaded file so that
sibling cleanup and extension normalization happen in one place; locate checks
in EditEgg.php (the URL/extension parsing and the file-upload handling) and
modify them to normalize 'jpeg'→'jpg' before validation and to call
Egg::writeEggIcon with the stored file path/filename.

In `@app/Filament/Components/Actions/DeleteServerIcon.php`:
- Line 26: The tooltip in app/Filament/Components/Actions/DeleteServerIcon.php
calls trans('admin/server.import_icon') which doesn't exist and is semantically
wrong for a delete action; either add the missing translation keys import_icon
and delete_icon to lang/en/admin/server.php (with appropriate English text) or
change the tooltip call in DeleteServerIcon.php to use an existing key such as
trans('admin/server.delete_image') (or another appropriate existing key) so the
tooltip resolves to a valid translation and matches the delete action.

In `@app/Transformers/Api/Application/EggTransformer.php`:
- Line 49: The API transformer changed the output key from image to icon, which
breaks existing consumers; in EggTransformer
(app/Transformers/Api/Application/EggTransformer.php) update the transform
payload to include both keys during a deprecation period by returning 'icon' =>
$model->icon and also 'image' => $model->icon (or vice versa depending on model
field), and update any transformer tests that assert on output to expect both
keys (or add a new test asserting the legacy 'image' key is present) so external
integrations continue to work until you remove the deprecated field or bump the
API version.

---

Outside diff comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 195-208: The FileUpload handling for the 'icon' field in
EditServer.php currently saves the uploaded file directly to disk and never
invokes Server::writeServerIcon(), so extension-normalization and cleanup of old
icon files (using Server::ICON_STORAGE_PATH and Server::ICON_FORMATS) are
skipped; update the FileUpload save flow to call
Server::writeServerIcon($server, $uploadedFile) (or the equivalent helper used
by the URL branch) after upload completes, passing the model instance and the
uploaded file so the method can normalize the extension and delete other legacy
files; apply the same change to the other FileUpload occurrences you noted (the
other 'icon' FileUpload blocks) to ensure consistent behavior across all
branches.

In `@app/Filament/Server/Pages/Settings.php`:
- Around line 163-176: The file-upload branch never invokes
Server::writeServerIcon(), so uploaded icons bypass the sibling-extension
cleanup and stale images can still be served; update the form handling that
currently calls saveIconFromUrl() for URL uploads to also handle uploaded files
by reading the stored upload from $data['icon'], extracting the file bytes and
extension, and calling writeServerIcon($server, $bytes, $extension) (the same
helper used by saveIconFromUrl()) before sending the success notification;
ensure you reference the FileUpload field name 'icon', Server::ICON_STORAGE_PATH
and Server::ICON_FORMATS when locating the code to modify and apply the same
logic to the other similar blocks noted.

---

Nitpick comments:
In `@app/Models/Server.php`:
- Around line 558-564: The code in Server.php throws Exceptions using
egg-specific translation keys (e.g., trans('admin/egg.import.unknown_extension')
and trans('admin/egg.import.could_not_write')) even though this logic is for
server icon import; update those calls to use server-specific translation keys
(for example trans('admin/servers.import.unknown_extension') and
trans('admin/servers.import.could_not_write')) in the same places where
normalizedExtension is checked and where the Exception is thrown, and then add
matching entries to the server language file so the new keys resolve correctly.
- Around line 148-156: Server::ICON_FORMATS contains a redundant 'jpeg' key
while Egg::ICON_FORMATS does not, causing getIconAttribute() to check for .jpeg
files that writeServerIcon() never writes (it normalizes 'jpeg' to 'jpg');
remove the 'jpeg' entry from Server::ICON_FORMATS (or add it to
Egg::ICON_FORMATS if intentional) so formats are consistent with
writeServerIcon() and avoid checking for unreachable .jpeg files in
getIconAttribute().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2413ac5c-1986-400f-a40d-49220351483f

📥 Commits

Reviewing files that changed from the base of the PR and between b693d0e and 9dcacbb.

📒 Files selected for processing (14)
  • app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php
  • app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php
  • app/Filament/Admin/Resources/Servers/Pages/EditServer.php
  • app/Filament/App/Resources/Servers/Pages/ListServers.php
  • app/Filament/Components/Actions/DeleteServerIcon.php
  • app/Filament/Server/Pages/Settings.php
  • app/Models/Egg.php
  • app/Models/Server.php
  • app/Services/Eggs/Sharing/EggExporterService.php
  • app/Services/Eggs/Sharing/EggImporterService.php
  • app/Transformers/Api/Application/EggTransformer.php
  • lang/en/admin/egg.php
  • resources/views/livewire/server-entry-placeholder.blade.php
  • resources/views/livewire/server-entry.blade.php

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (2)

101-145: ⚠️ Potential issue | 🟠 Major

Separate the raw URL input from the validated source state.

icon_url is currently both the text box state and the hidden “ready to save” value, so the validation hook clears the user’s input when it nulls that field. Because the file tab never clears those URL fields, a previously validated URL also still wins at Lines 177-178 after a file upload.

🧭 Suggested fix
                                                         Hidden::make('icon_url'),
                                                         Hidden::make('icon_extension'),
-                                                        TextInput::make('icon_url')
+                                                        TextInput::make('icon_url_input')
                                                             ->label(trans('admin/egg.import.icon_url'))
                                                             ->reactive()
                                                             ->autocomplete(false)
                                                             ->debounce(500)
                                                             ->afterStateUpdated(function ($state, Set $set) {
                                                                 if (!$state) {
                                                                     $set('icon_url_error', null);
                                                                     $set('icon_url', null);
                                                                     $set('icon_extension', null);

                                                                     return;
                                                                 }

                                                                 // ...
                                                                 $set('icon_url', $state);
                                                                 $set('icon_extension', $extension);
                                                                 $set('icon_url_error', null);
                                                             }),
@@
                                                         FileUpload::make('icon')
+                                                            ->afterStateUpdated(function ($state, Set $set) {
+                                                                if (!empty($state)) {
+                                                                    $set('icon_url_input', null);
+                                                                    $set('icon_url', null);
+                                                                    $set('icon_extension', null);
+                                                                    $set('icon_url_error', null);
+                                                                }
+                                                            })

Also applies to: 158-178


117-135: ⚠️ Potential issue | 🟠 Major

Allowlist HTTP(S) before dereferencing the URL.

FILTER_VALIDATE_URL is only syntactic and accepts non-HTTP schemes (e.g., ftp://, ssh://). Gate the scheme explicitly to ensure only HTTP(S) is allowed.

Additionally, the host/IP validation in afterStateUpdated (lines 128-135) occurs before saveIconFromUrl (lines 470-482) fetches the URL. Since file_get_contents() with the HTTP stream context follows redirects by default (max_redirects=20), an attacker can bypass the private IP check by providing a URL to an external host that redirects to a private IP.

🛡️ Suggested fix
                                                                    if (!filter_var($state, FILTER_VALIDATE_URL)) {
                                                                        throw new Exception(trans('admin/egg.import.invalid_url'));
                                                                    }
+
+                                                                   $scheme = strtolower(parse_url($state, PHP_URL_SCHEME) ?? '');
+                                                                   if (!in_array($scheme, ['http', 'https'], true)) {
+                                                                       throw new Exception(trans('admin/egg.import.invalid_url'));
+                                                                   }

                                                                    $extension = strtolower(pathinfo(parse_url($state, PHP_URL_PATH), PATHINFO_EXTENSION));

For the redirect bypass: disable redirect following in saveIconFromUrl by setting 'follow_location' => false in the stream context, or validate the final destination after fetch.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php` around lines 117 - 135,
The URL check in afterStateUpdated only validates syntax and allows non-HTTP
schemes and the host/IP check can be bypassed via redirects; update
afterStateUpdated to explicitly validate the scheme is 'http' or 'https' (e.g.,
reject when parse_url($state, PHP_URL_SCHEME) is not 'http' or 'https') before
dereferencing and keep the existing host/ip validation using parse_url and
gethostbyname; then modify saveIconFromUrl to prevent redirect-based bypasses by
disabling automatic redirects in the HTTP stream context (set follow_location =>
false) or by resolving and validating the final redirected destination IP after
fetch so the private-IP check applies to the actual resource location.
♻️ Duplicate comments (1)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)

122-125: ⚠️ Potential issue | 🟠 Major

Normalize .jpeg before checking Egg::ICON_FORMATS.

Egg::writeEggIcon() already accepts jpeg and normalizes it to jpg, but this branch rejects .jpeg URLs before that helper runs. The URL importer still misses a valid JPEG path here.

🔧 Suggested fix
                                                                 $extension = strtolower(pathinfo(parse_url($state, PHP_URL_PATH), PATHINFO_EXTENSION));
+                                                                if ($extension === 'jpeg') {
+                                                                    $extension = 'jpg';
+                                                                }

                                                                 if (!array_key_exists($extension, Egg::ICON_FORMATS)) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php` around lines 122 - 125,
The extension check in EditEgg (variable $extension) rejects ".jpeg" URLs even
though Egg::writeEggIcon() accepts and normalizes "jpeg" to "jpg"; update the
logic that sets/uses $extension (before the array_key_exists check against
Egg::ICON_FORMATS) to normalize "jpeg" to "jpg" (e.g., lowercase then map 'jpeg'
=> 'jpg') so the importer accepts JPEG URLs; keep the rest of the flow and still
throw the same Exception if the normalized extension is not in
Egg::ICON_FORMATS.
🤖 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/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 117-135: The URL check in afterStateUpdated only validates syntax
and allows non-HTTP schemes and the host/IP check can be bypassed via redirects;
update afterStateUpdated to explicitly validate the scheme is 'http' or 'https'
(e.g., reject when parse_url($state, PHP_URL_SCHEME) is not 'http' or 'https')
before dereferencing and keep the existing host/ip validation using parse_url
and gethostbyname; then modify saveIconFromUrl to prevent redirect-based
bypasses by disabling automatic redirects in the HTTP stream context (set
follow_location => false) or by resolving and validating the final redirected
destination IP after fetch so the private-IP check applies to the actual
resource location.

---

Duplicate comments:
In `@app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 122-125: The extension check in EditEgg (variable $extension)
rejects ".jpeg" URLs even though Egg::writeEggIcon() accepts and normalizes
"jpeg" to "jpg"; update the logic that sets/uses $extension (before the
array_key_exists check against Egg::ICON_FORMATS) to normalize "jpeg" to "jpg"
(e.g., lowercase then map 'jpeg' => 'jpg') so the importer accepts JPEG URLs;
keep the rest of the flow and still throw the same Exception if the normalized
extension is not in Egg::ICON_FORMATS.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a0c2244e-fe3f-4a14-8259-95c31256aec1

📥 Commits

Reviewing files that changed from the base of the PR and between aca043e and 4d35306.

📒 Files selected for processing (4)
  • app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php
  • app/Filament/Admin/Resources/Servers/Pages/EditServer.php
  • app/Filament/Server/Pages/Settings.php
  • app/Models/Server.php
🚧 Files skipped from review as they are similar to previous changes (3)
  • app/Models/Server.php
  • app/Filament/Server/Pages/Settings.php
  • app/Filament/Admin/Resources/Servers/Pages/EditServer.php

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)

209-229: ⚠️ Potential issue | 🟡 Minor

Reject invalid URL submissions explicitly.

When icon_url is filled but validation has already failed, this handler neither saves nor notifies. That leaves a quiet no-op path; surfacing icon_url_error on submit would make the failure obvious.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 209 -
229, The submit action closure handling icon uploads currently ignores cases
where icon_url is provided but validation failed (icon_url_error is set),
creating a silent no-op; update the action (the anonymous function passed to
->action) to explicitly detect when !empty($data['icon_url']) && !not
empty($data['icon_url_error']) (i.e. icon_url is present but icon_url_error
exists) and surface the failure by returning a validation/notification response
instead of doing nothing—for example call
Notification::make()->title(trans('server/setting.server_info.icon.error'))->danger()->body($data['icon_url_error'])->send()
or throw a ValidationException with the icon_url_error message; ensure
saveIconFromUrl($data['icon_url'], $data['icon_extension'], $record) is only
called when icon_url_error is empty.

163-170: ⚠️ Potential issue | 🔴 Critical

Use a HTTP client that disables redirects and validates the destination IP at request time.

The validation on lines 163-170 only checks the hostname's DNS resolution once. The subsequent file_get_contents() call in saveIconFromUrl() receives the original URL and follows redirects by default, allowing an attacker to bypass the public IP check via HTTP redirects or DNS rebinding attacks—redirecting to private/reserved ranges or metadata endpoints like 169.254.169.254. The stream context configuration does not disable redirects.

Replace file_get_contents() with a HTTP client (e.g., cURL via curl_exec() with CURLOPT_FOLLOWLOCATION disabled, or Guzzle with redirect checking) that validates the resolved destination IP matches the originally validated public IP before connecting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 163 -
170, The current hostname/IP check using parse_url and gethostbyname before
calling saveIconFromUrl is insufficient because file_get_contents follows
redirects and can be abused; modify saveIconFromUrl to use a HTTP client (e.g.,
cURL or Guzzle) with redirects disabled (CURLOPT_FOLLOWLOCATION = false or
Guzzle's allow_redirects = false) and perform IP validation at request time by
resolving the request URL's host and comparing its resolved IP(s) against the
existing filter_var check (reuse parse_url and the validation logic) before
issuing the request, and if a redirect response is returned validate the new
Location host/IP the same way (or reject redirects entirely) to ensure the final
destination IP is not private/reserved; update any call sites that used
file_get_contents to use this new safe fetch path (functions/methods:
saveIconFromUrl, parse_url, gethostbyname).
🤖 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/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 172-174: The server currently trusts client-provided fields
(icon_extension, icon_url_error) in the action callback and calls
saveIconFromUrl($data['icon_url'], $data['icon_extension'], $record) directly;
move all URL parsing/validation and extension normalization into
saveIconFromUrl() so the server re-derives the extension, enforces allowed
schemes/hosts, checks for private/internal IPs, and only then performs the
download (e.g., file_get_contents). Update saveIconFromUrl() to ignore the
passed icon_extension and icon_url_error, compute and validate the extension
from the URL or fetched headers, verify scheme and hostname against your
allow-list, perform public-IP checks, and throw or return an error before any
network fetch if validation fails; apply the same change to the other call site
noted in the review (lines ~466-483).
- Line 168: The saveUploadedFileUsing callback currently calls Egg::writeEggIcon
(via saveUploadedFileUsing(fn (TemporaryUploadedFile $file, Egg $record) =>
$record->writeEggIcon(...))) but writeEggIcon returns bool, while Filament
expects the callback to return the stored file path string; update the callback
passed to saveUploadedFileUsing so it calls $record->writeEggIcon(...), captures
its resulting stored path (or obtains the path from the write operation), and
returns that path string (ensure the callback signature using
TemporaryUploadedFile and Egg remains the same and handle failure by returning
an empty string or null if required by Filament).

In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 157-160: The code extracts the raw URL extension and validates it
against Server::ICON_FORMATS without normalizing common aliases (e.g., "jpeg" ->
"jpg"), causing valid JPEG URLs to be rejected; update the logic in EditServer
(the block setting $extension from pathinfo/parse_url) to normalize aliases the
same way as Server::writeServerIcon() before the array_key_exists check (e.g.,
strtolower the extension then map known aliases like "jpeg" => "jpg" or call a
shared Server::normalizeIconExtension() if available), and apply the same
normalization to the second identical occurrence mentioned so both checks use
the normalized extension when validating against Server::ICON_FORMATS.

---

Outside diff comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 209-229: The submit action closure handling icon uploads currently
ignores cases where icon_url is provided but validation failed (icon_url_error
is set), creating a silent no-op; update the action (the anonymous function
passed to ->action) to explicitly detect when !empty($data['icon_url']) && !not
empty($data['icon_url_error']) (i.e. icon_url is present but icon_url_error
exists) and surface the failure by returning a validation/notification response
instead of doing nothing—for example call
Notification::make()->title(trans('server/setting.server_info.icon.error'))->danger()->body($data['icon_url_error'])->send()
or throw a ValidationException with the icon_url_error message; ensure
saveIconFromUrl($data['icon_url'], $data['icon_extension'], $record) is only
called when icon_url_error is empty.
- Around line 163-170: The current hostname/IP check using parse_url and
gethostbyname before calling saveIconFromUrl is insufficient because
file_get_contents follows redirects and can be abused; modify saveIconFromUrl to
use a HTTP client (e.g., cURL or Guzzle) with redirects disabled
(CURLOPT_FOLLOWLOCATION = false or Guzzle's allow_redirects = false) and perform
IP validation at request time by resolving the request URL's host and comparing
its resolved IP(s) against the existing filter_var check (reuse parse_url and
the validation logic) before issuing the request, and if a redirect response is
returned validate the new Location host/IP the same way (or reject redirects
entirely) to ensure the final destination IP is not private/reserved; update any
call sites that used file_get_contents to use this new safe fetch path
(functions/methods: saveIconFromUrl, parse_url, gethostbyname).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e7ef1d52-c82e-4851-9d79-7593a700ae7d

📥 Commits

Reviewing files that changed from the base of the PR and between 4d35306 and 70f523c.

📒 Files selected for processing (3)
  • app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php
  • app/Filament/Admin/Resources/Servers/Pages/EditServer.php
  • app/Filament/Server/Pages/Settings.php
✅ Files skipped from review due to trivial changes (1)
  • app/Filament/Server/Pages/Settings.php

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)

457-477: ⚠️ Potential issue | 🔴 Critical

Server-side URL validation is missing — SSRF vulnerability remains.

The reactive callback (lines 114-127) validates scheme, URL format, and private IP ranges, but saveIconFromUrl() does not re-validate before calling file_get_contents(). A crafted Livewire payload can bypass the UI callback entirely and supply any URL, enabling requests to internal services.

🔒 Suggested fix: Re-validate inside saveIconFromUrl
 private function saveIconFromUrl(string $icon_url, Egg $egg): void
 {
+    // Re-validate server-side to prevent SSRF via crafted Livewire payloads
+    if (!in_array(parse_url($icon_url, PHP_URL_SCHEME), ['http', 'https'], true)) {
+        throw new Exception(trans('admin/egg.import.invalid_url'));
+    }
+
+    if (!filter_var($icon_url, FILTER_VALIDATE_URL)) {
+        throw new Exception(trans('admin/egg.import.invalid_url'));
+    }
+
+    $host = parse_url($icon_url, PHP_URL_HOST);
+    $ip = gethostbyname($host);
+
+    if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) === false) {
+        throw new Exception(trans('admin/egg.import.no_local_ip'));
+    }
+
     $context = stream_context_create([
         'http' => ['timeout' => 3],
         'https' => [
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php` around lines 457 - 477,
saveIconFromUrl lacks server-side URL validation allowing SSRF; before calling
file_get_contents in saveIconFromUrl(string $icon_url, Egg $egg) re-validate the
input URL (parse_url) to ensure scheme is http/https, the host is present and
not a local/loopback/private IP (resolve host via DNS and/or
gethostbyname/getaddrinfo and check with FILTER_FLAG_NO_PRIV_RANGE |
FILTER_FLAG_NO_RES_RANGE), reject invalid/malformed URLs and throw the same
Exception(trans('admin/egg.import.invalid_url')) on failure, and only then
proceed to fetch and call Egg::writeEggIcon; keep existing timeout context and
preserve the extension handling.
app/Filament/Server/Pages/Settings.php (1)

108-134: ⚠️ Potential issue | 🔴 Critical

Re-validate icon_url on the submission path.

Here too, the modal-side afterStateUpdated() check is not authoritative. The action still downloads any non-empty icon_url it receives, so a crafted submit can skip the scheme/public-IP guard and trigger server-side fetches to arbitrary/private hosts. Please enforce the same validation inside saveIconFromUrl() before the download.

Also applies to: 165-167, 430-449

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Server/Pages/Settings.php` around lines 108 - 134, The
client-side afterStateUpdated() URL checks are not enforced server-side, so add
the same validation inside saveIconFromUrl(): validate parse_url($url,
PHP_URL_SCHEME) is 'http' or 'https', ensure filter_var($url,
FILTER_VALIDATE_URL) succeeds, resolve host via parse_url(..., PHP_URL_HOST) and
gethostbyname(), and reject any resolved IPs that are in private/reserved ranges
using FILTER_VALIDATE_IP with FILTER_FLAG_NO_PRIV_RANGE |
FILTER_FLAG_NO_RES_RANGE; if validation fails, throw or return an error (set the
same error message used in the afterStateUpdated block) before performing any
download. Ensure this change covers all entry points that call saveIconFromUrl()
so server-side fetches cannot be triggered with crafted submit data.
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)

139-165: ⚠️ Potential issue | 🔴 Critical

Move the icon URL validation into saveIconFromUrl().

The http/https and public-IP checks only run in the reactive field hook. The submit path still forwards any non-empty icon_url to saveIconFromUrl(), so a tampered request can bypass that precheck and make the panel fetch arbitrary/private targets. Please enforce the same validation inside the download method before file_get_contents().

Also applies to: 196-198, 1187-1206

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 139 -
165, The icon URL validation must be enforced in saveIconFromUrl(): before
calling file_get_contents() (and any downstream download/stream logic) replicate
the reactive-field checks — verify parse_url($url, PHP_URL_SCHEME) is 'http' or
'https', validate the URL with filter_var(..., FILTER_VALIDATE_URL), resolve the
host via parse_url($url, PHP_URL_HOST) and gethostbyname(), and ensure the
resolved IP passes FILTER_VALIDATE_IP with FILTER_FLAG_NO_PRIV_RANGE |
FILTER_FLAG_NO_RES_RANGE; if any check fails, throw or return the same
translated errors used in the form (trans('admin/egg.import.invalid_url') and
trans('admin/egg.import.no_local_ip')) so tampered submit requests cannot fetch
private or non-http(s) targets.
🤖 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/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 474-476: The extension extraction using
strtolower(pathinfo(parse_url($icon_url, PHP_URL_PATH), PATHINFO_EXTENSION)) can
return an empty string for URLs without a path extension; update the EditEgg
logic before calling $egg->writeEggIcon(...) to handle that case by fetching the
resource headers/body to determine MIME type (e.g., via get_headers or a
Guzzle/cURL request), map the Content-Type to a file extension, and only then
call $egg->writeEggIcon($extension, $data); if content-type is unavailable or
unmappable, return a clear, translatable user-facing error instead of passing an
empty extension to writeEggIcon.

---

Outside diff comments:
In `@app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 457-477: saveIconFromUrl lacks server-side URL validation allowing
SSRF; before calling file_get_contents in saveIconFromUrl(string $icon_url, Egg
$egg) re-validate the input URL (parse_url) to ensure scheme is http/https, the
host is present and not a local/loopback/private IP (resolve host via DNS and/or
gethostbyname/getaddrinfo and check with FILTER_FLAG_NO_PRIV_RANGE |
FILTER_FLAG_NO_RES_RANGE), reject invalid/malformed URLs and throw the same
Exception(trans('admin/egg.import.invalid_url')) on failure, and only then
proceed to fetch and call Egg::writeEggIcon; keep existing timeout context and
preserve the extension handling.

In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 139-165: The icon URL validation must be enforced in
saveIconFromUrl(): before calling file_get_contents() (and any downstream
download/stream logic) replicate the reactive-field checks — verify
parse_url($url, PHP_URL_SCHEME) is 'http' or 'https', validate the URL with
filter_var(..., FILTER_VALIDATE_URL), resolve the host via parse_url($url,
PHP_URL_HOST) and gethostbyname(), and ensure the resolved IP passes
FILTER_VALIDATE_IP with FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE; if
any check fails, throw or return the same translated errors used in the form
(trans('admin/egg.import.invalid_url') and
trans('admin/egg.import.no_local_ip')) so tampered submit requests cannot fetch
private or non-http(s) targets.

In `@app/Filament/Server/Pages/Settings.php`:
- Around line 108-134: The client-side afterStateUpdated() URL checks are not
enforced server-side, so add the same validation inside saveIconFromUrl():
validate parse_url($url, PHP_URL_SCHEME) is 'http' or 'https', ensure
filter_var($url, FILTER_VALIDATE_URL) succeeds, resolve host via parse_url(...,
PHP_URL_HOST) and gethostbyname(), and reject any resolved IPs that are in
private/reserved ranges using FILTER_VALIDATE_IP with FILTER_FLAG_NO_PRIV_RANGE
| FILTER_FLAG_NO_RES_RANGE; if validation fails, throw or return an error (set
the same error message used in the afterStateUpdated block) before performing
any download. Ensure this change covers all entry points that call
saveIconFromUrl() so server-side fetches cannot be triggered with crafted submit
data.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 69a1a84a-59ca-4456-a25b-063e8e91535b

📥 Commits

Reviewing files that changed from the base of the PR and between 70f523c and 149c636.

📒 Files selected for processing (5)
  • app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php
  • app/Filament/Admin/Resources/Servers/Pages/EditServer.php
  • app/Filament/Server/Pages/Settings.php
  • app/Models/Egg.php
  • app/Models/Server.php
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/Models/Egg.php
  • app/Models/Server.php

@Boy132
Copy link
Member Author

Boy132 commented Mar 23, 2026

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Mar 23, 2026

✅ Actions performed

Full review triggered.

@Boy132 Boy132 changed the title Rename egg image to icon and delete old icons when uploading new one Refactor egg & server icon uploading Mar 23, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/Services/Eggs/Sharing/EggImporterService.php (1)

265-275: ⚠️ Potential issue | 🔴 Critical

Call the renamed model method here.

Line 275 still invokes writeEggIcon(), but App\Models\Egg now exposes writeIcon(). Any import that includes embedded icon data will fail at runtime before the egg can be saved.

🔧 Suggested fix
-            $egg->writeEggIcon($extension, $data);
+            $egg->writeIcon($extension, $data);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Services/Eggs/Sharing/EggImporterService.php` around lines 265 - 275, The
code in saveEggIconFromBase64 still calls the old Egg model method writeEggIcon
which was renamed to writeIcon; update the call inside saveEggIconFromBase64 to
invoke Egg::writeIcon($extension, $data) instead of writeEggIcon so embedded
icon imports succeed at runtime.
♻️ Duplicate comments (1)
app/Transformers/Api/Application/EggTransformer.php (1)

49-49: ⚠️ Potential issue | 🟠 Major

Preserve backward compatibility for API consumers.

Line 49 removes the legacy image field from the transformer payload. That is a breaking API change for existing clients expecting image. Please keep both fields during a deprecation window (or explicitly version/break the API).

🔁 Compatibility-safe payload adjustment
-            'icon' => $model->icon,
+            'icon' => $model->icon,
+            'image' => $model->icon, // deprecated: remove in next API version
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Transformers/Api/Application/EggTransformer.php` at line 49, The
transformer removed the legacy "image" field causing a breaking change; in
EggTransformer keep backward compatibility by returning both fields, i.e.,
preserve the existing 'image' payload while also returning the new 'icon'
payload (use the same value $model->icon for 'image' => $model->icon), so locate
EggTransformer (the array with 'icon' => $model->icon) and add an 'image' =>
$model->icon entry alongside it to support old clients during the deprecation
window.
🧹 Nitpick comments (2)
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (1)

52-54: Avoid per-row file I/O for the default icon.

Lines 52–54 call file_get_contents() inside getStateUsing, which runs per record. Precompute the fallback once in table() and capture it in the closure.

♻️ Proposed refactor
 public function table(Table $table): Table
 {
+    $defaultEggIcon = 'data:image/svg+xml;base64,' . base64_encode(file_get_contents(public_path('pelican.svg')));
+
     return $table
         ->searchable(true)
@@
                 ImageColumn::make('icon')
@@
-                    ->getStateUsing(fn ($record) => $record->icon
-                        ? $record->icon
-                        : 'data:image/svg+xml;base64,' . base64_encode(file_get_contents(public_path('pelican.svg')))),
+                    ->getStateUsing(fn ($record) => $record->icon ?: $defaultEggIcon),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php` around lines 52 - 54,
The getStateUsing closure in ListEggs::table() currently calls
file_get_contents(public_path('pelican.svg')) per record; precompute the
fallback data URL once above where table() builds the column (e.g., $defaultIcon
=
'data:image/svg+xml;base64,'.base64_encode(file_get_contents(public_path('pelican.svg'))));
then change the closure used in ->getStateUsing(fn($record) => $record->icon ?
$record->icon : $defaultIcon) so the closure captures the single precomputed
$defaultIcon instead of doing per-row file I/O.
app/Models/Egg.php (1)

381-425: Extract the shared icon lifecycle into one helper.

getIconAttribute() and writeIcon() now mirror App\Models\Server almost exactly. Keeping format normalization, scan order, and sibling cleanup in two models is easy to desync the next time one path changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Models/Egg.php` around lines 381 - 425, Extract the duplicated icon
lifecycle logic from Egg::getIconAttribute and Egg::writeIcon (which mirror
App\Models\Server) into a single reusable helper (e.g., an IconTrait or
IconService) that implements the normalization mapping, storage scan for
existing formats, write-with-fallback, and sibling-file cleanup; keep the
constants ICON_FORMATS and ICON_STORAGE_PATH as the source of truth, move the
normalization/matching logic and Storage::disk('public') calls into the helper,
throw the same translated exceptions
(trans('admin/egg.import.unknown_extension') and
trans('admin/egg.import.could_not_write')) from the helper, and then update
Egg::getIconAttribute and Egg::writeIcon to delegate to the new helper so both
Egg and Server use the shared implementation.
🤖 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/Filament/Components/Actions/UploadIcon.php`:
- Around line 99-111: The remote icon fetch in UploadIcon's action closure
bypasses host validation and lacks response and size checks; update the HTTP
fetch (the Http::timeout(...)->connectTimeout(...)->get(...) call inside the
action(function (array $data, $record) { ... })) to disable redirects (or re-run
validateIconUrl against the final URL), verify the response status is 200 before
reading the body, and enforce the same 256 KB maximum (reject if response body
exceeds 256*1024 bytes) prior to calling $record->writeIcon($extension,
$content); ensure validateIconUrl is applied to the final resolved URL if you
choose to allow redirects.

In `@lang/en/admin/egg.php`:
- Line 18: Rename the translation key 'image_too_large' to 'icon_too_large'
(value can remain "Icon too large. Limit is 1024KB") so it matches the
surrounding 'icon_*' keys; update this key in the English file and mirror the
same rename across all other language files to keep key naming consistent
throughout the codebase and ensure any code that references the old
'image_too_large' key is updated to use 'icon_too_large'.

---

Outside diff comments:
In `@app/Services/Eggs/Sharing/EggImporterService.php`:
- Around line 265-275: The code in saveEggIconFromBase64 still calls the old Egg
model method writeEggIcon which was renamed to writeIcon; update the call inside
saveEggIconFromBase64 to invoke Egg::writeIcon($extension, $data) instead of
writeEggIcon so embedded icon imports succeed at runtime.

---

Duplicate comments:
In `@app/Transformers/Api/Application/EggTransformer.php`:
- Line 49: The transformer removed the legacy "image" field causing a breaking
change; in EggTransformer keep backward compatibility by returning both fields,
i.e., preserve the existing 'image' payload while also returning the new 'icon'
payload (use the same value $model->icon for 'image' => $model->icon), so locate
EggTransformer (the array with 'icon' => $model->icon) and add an 'image' =>
$model->icon entry alongside it to support old clients during the deprecation
window.

---

Nitpick comments:
In `@app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php`:
- Around line 52-54: The getStateUsing closure in ListEggs::table() currently
calls file_get_contents(public_path('pelican.svg')) per record; precompute the
fallback data URL once above where table() builds the column (e.g., $defaultIcon
=
'data:image/svg+xml;base64,'.base64_encode(file_get_contents(public_path('pelican.svg'))));
then change the closure used in ->getStateUsing(fn($record) => $record->icon ?
$record->icon : $defaultIcon) so the closure captures the single precomputed
$defaultIcon instead of doing per-row file I/O.

In `@app/Models/Egg.php`:
- Around line 381-425: Extract the duplicated icon lifecycle logic from
Egg::getIconAttribute and Egg::writeIcon (which mirror App\Models\Server) into a
single reusable helper (e.g., an IconTrait or IconService) that implements the
normalization mapping, storage scan for existing formats, write-with-fallback,
and sibling-file cleanup; keep the constants ICON_FORMATS and ICON_STORAGE_PATH
as the source of truth, move the normalization/matching logic and
Storage::disk('public') calls into the helper, throw the same translated
exceptions (trans('admin/egg.import.unknown_extension') and
trans('admin/egg.import.could_not_write')) from the helper, and then update
Egg::getIconAttribute and Egg::writeIcon to delegate to the new helper so both
Egg and Server use the shared implementation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a55a418e-1642-461e-9641-70617d1ee813

📥 Commits

Reviewing files that changed from the base of the PR and between b693d0e and c6c6e10.

📒 Files selected for processing (17)
  • app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php
  • app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php
  • app/Filament/Admin/Resources/Servers/Pages/EditServer.php
  • app/Filament/App/Resources/Servers/Pages/ListServers.php
  • app/Filament/Components/Actions/DeleteIcon.php
  • app/Filament/Components/Actions/DeleteServerIcon.php
  • app/Filament/Components/Actions/UploadIcon.php
  • app/Filament/Server/Pages/Settings.php
  • app/Models/Egg.php
  • app/Models/Server.php
  • app/Services/Eggs/Sharing/EggExporterService.php
  • app/Services/Eggs/Sharing/EggImporterService.php
  • app/Transformers/Api/Application/EggTransformer.php
  • lang/en/admin/egg.php
  • lang/en/admin/server.php
  • resources/views/livewire/server-entry-placeholder.blade.php
  • resources/views/livewire/server-entry.blade.php
💤 Files with no reviewable changes (1)
  • app/Filament/Components/Actions/DeleteServerIcon.php

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
app/Filament/Components/Actions/UploadIcon.php (1)

104-112: ⚠️ Potential issue | 🟠 Major

Harden remote fetch: validate HTTP status and enforce the 256 KB ceiling before persist.

Line 104 currently reads the body directly and only checks emptiness. Non-200 responses and oversized payloads can still flow into writeIcon(), which breaks parity with Line 88 upload safeguards.

Suggested patch
-                $content = Http::timeout(5)->connectTimeout(1)->withoutRedirecting()->get($data['icon_url'])->body();
-
-                if (empty($content)) {
+                $response = Http::timeout(5)
+                    ->connectTimeout(1)
+                    ->withoutRedirecting()
+                    ->get($data['icon_url']);
+
+                if (!$response->ok()) {
+                    throw new Exception(trans('admin/egg.import.invalid_url'));
+                }
+
+                $contentLength = (int) $response->header('Content-Length', 0);
+                if ($contentLength > 256 * 1024) {
+                    throw new Exception(trans('admin/egg.import.invalid_url'));
+                }
+
+                $content = $response->body();
+                if ($content === '' || strlen($content) > 256 * 1024) {
                     throw new Exception(trans('admin/egg.import.invalid_url'));
                 }
#!/bin/bash
set -euo pipefail

# Verify current hardening at the remote fetch call site.
cat -n app/Filament/Components/Actions/UploadIcon.php | sed -n '100,122p'
rg -n "withoutRedirecting|->ok\\(|->successful\\(|Content-Length|strlen\\(" app/Filament/Components/Actions/UploadIcon.php
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/Filament/Components/Actions/UploadIcon.php` around lines 104 - 112, The
remote fetch in UploadIcon.php reads the response body directly and only checks
for emptiness; update the fetch logic (the Http::...->get(...) call and
subsequent handling before $record->writeIcon) to first assert a successful 2xx
response (use the response's ok()/successful()/status() check) and then enforce
a 256 KB ceiling by checking Content-Length header if present and falling back
to strlen($body) after retrieving the body; if the response is non-2xx or the
size exceeds 262144 bytes, throw the same
Exception(trans('admin/egg.import.invalid_url')) to mirror the upload safeguards
before calling $record->writeIcon($extension, $content).
🤖 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/Filament/Components/Actions/UploadIcon.php`:
- Around line 142-146: The current check in UploadIcon.php uses
gethostbyname($host) which returns only a single IPv4 and misses IPv6 or
multiple-record cases; replace the single-resolution logic in the UploadIcon
component so you perform DNS A and AAAA lookups (e.g. dns_get_record($host,
DNS_A | DNS_AAAA)) and iterate all returned addresses, validating each with
filter_var(..., FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE |
FILTER_FLAG_NO_RES_RANGE); if no records are returned or any resolved address
passes as private/reserved then throw the same
Exception(trans('admin/egg.import.no_local_ip')) — update references around
$host and the old $ip usage accordingly and ensure IPv6 records are correctly
validated.

---

Duplicate comments:
In `@app/Filament/Components/Actions/UploadIcon.php`:
- Around line 104-112: The remote fetch in UploadIcon.php reads the response
body directly and only checks for emptiness; update the fetch logic (the
Http::...->get(...) call and subsequent handling before $record->writeIcon) to
first assert a successful 2xx response (use the response's
ok()/successful()/status() check) and then enforce a 256 KB ceiling by checking
Content-Length header if present and falling back to strlen($body) after
retrieving the body; if the response is non-2xx or the size exceeds 262144
bytes, throw the same Exception(trans('admin/egg.import.invalid_url')) to mirror
the upload safeguards before calling $record->writeIcon($extension, $content).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4ac51c16-99db-46a8-a19a-cc83efa2705b

📥 Commits

Reviewing files that changed from the base of the PR and between c6c6e10 and ae911ff.

📒 Files selected for processing (5)
  • app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php
  • app/Filament/Components/Actions/DeleteIcon.php
  • app/Filament/Components/Actions/UploadIcon.php
  • app/Services/Eggs/Sharing/EggImporterService.php
  • lang/en/admin/egg.php
✅ Files skipped from review due to trivial changes (1)
  • app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php
🚧 Files skipped from review as they are similar to previous changes (3)
  • app/Services/Eggs/Sharing/EggImporterService.php
  • lang/en/admin/egg.php
  • app/Filament/Components/Actions/DeleteIcon.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Importing egg icon with different file type does not remove old icon file

1 participant