Add user_mountable functionality for Mounts (#2077)#2181
Open
lancepioch wants to merge 8 commits intomainfrom
Open
Add user_mountable functionality for Mounts (#2077)#2181lancepioch wants to merge 8 commits intomainfrom
lancepioch wants to merge 8 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds admin control for a mount's user_mountable flag, two subuser permissions ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MountsPage as MountsPage
participant Auth as Authorization
participant DB as Database
participant Activity as ActivityLog
participant UI as Notification
User->>MountsPage: open Mounts page
MountsPage->>Auth: authorize(MountRead)
Auth-->>MountsPage: allowed / denied
MountsPage->>DB: query mounts (user_mountable && server-compatible)
DB-->>MountsPage: mounts list
MountsPage-->>User: render CheckboxList (pre-filled)
User->>MountsPage: update selection / save
MountsPage->>Auth: authorize(MountUpdate)
Auth-->>MountsPage: allowed / denied
MountsPage->>DB: sync server↔mount relations
DB-->>MountsPage: success / error
MountsPage->>Activity: record mounts.update
Activity-->>MountsPage: logged
MountsPage->>UI: show success or failure notification
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
|
@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: 1
🧹 Nitpick comments (1)
app/Filament/Server/Pages/Mounts.php (1)
71-73: Drop unused callback parameter.
$stateis never read in this callback.♻️ Proposed cleanup
- ->afterStateUpdated(function ($state) { + ->afterStateUpdated(function () { $this->save(); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Pages/Mounts.php` around lines 71 - 73, The closure passed to afterStateUpdated in the Mounts page is declaring an unused parameter ($state); remove the unused $state parameter from the anonymous function so it becomes function () { $this->save(); } (i.e., update the afterStateUpdated callback where ->afterStateUpdated(function ($state) { $this->save(); }) is defined to drop the $state parameter).
🤖 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/Server/Pages/Mounts.php`:
- Around line 65-67: The descriptions closure builds an HtmlString from mount
fields without escaping, creating XSS risk; update the descriptions callback
(the ->descriptions(fn () => ... ) closure that maps Mount $mount) to escape
$mount->source, $mount->target and the stripped $mount->description using e()
(or equivalent HTML-encoding) before concatenating and wrapping in HtmlString so
all dynamic values are HTML-escaped while still allowing the intended line
break; keep the existing stripTags() for description but pass its result through
e() as well.
---
Nitpick comments:
In `@app/Filament/Server/Pages/Mounts.php`:
- Around line 71-73: The closure passed to afterStateUpdated in the Mounts page
is declaring an unused parameter ($state); remove the unused $state parameter
from the anonymous function so it becomes function () { $this->save(); } (i.e.,
update the afterStateUpdated callback where ->afterStateUpdated(function
($state) { $this->save(); }) is defined to drop the $state parameter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b35254d1-7174-41d3-981e-eacbddbcd982
📒 Files selected for processing (9)
app/Enums/SubuserPermission.phpapp/Filament/Admin/Resources/Mounts/MountResource.phpapp/Filament/Admin/Resources/Mounts/Pages/CreateMount.phpapp/Filament/Server/Pages/Mounts.phpapp/Filament/Server/Pages/Settings.phpapp/Filament/Server/Pages/Startup.phplang/en/admin/mount.phplang/en/server/mount.phplang/en/server/user.php
💤 Files with no reviewable changes (1)
- app/Filament/Admin/Resources/Mounts/Pages/CreateMount.php
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/Filament/Server/Pages/Mounts.php (2)
98-103: Consider sanitizing exception messages shown to users.Displaying raw exception messages via
$exception->getMessage()could potentially expose internal implementation details (e.g., SQL errors, file paths, class names) to users. While this is a server management panel, it's good practice to show generic error messages and log the details server-side.Proposed fix
} catch (Exception $exception) { + report($exception); + Notification::make() ->title(trans('server/mount.notification_failed')) - ->body($exception->getMessage()) + ->body(trans('server/mount.notification_failed_body')) ->danger() ->send(); }This requires adding a new translation key
server/mount.notification_failed_bodywith a generic error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Pages/Mounts.php` around lines 98 - 103, Replace the direct usage of $exception->getMessage() in the catch block that builds the Notification (the Notification::make() call in Mounts.php) with a generic translated message key (server/mount.notification_failed_body), and ensure the full exception is logged on the server (e.g., via Log::error or the existing logger) so internal details are recorded but not shown to users; also add the new translation key server/mount.notification_failed_body with a generic user-facing message.
71-73: Remove unused$stateparameter.The
$stateparameter in theafterStateUpdatedcallback is not used. Consider removing it to avoid confusion.Proposed fix
->live() - ->afterStateUpdated(function ($state) { + ->afterStateUpdated(function () { $this->save(); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Pages/Mounts.php` around lines 71 - 73, In the afterStateUpdated callback on the Mounts form, remove the unused $state parameter to avoid confusion; update the closure from function ($state) { $this->save(); } to a parameterless closure (function () { $this->save(); }) so the call to $this->save() remains but the unused $state argument is eliminated in the afterStateUpdated usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Filament/Server/Pages/Mounts.php`:
- Around line 98-103: Replace the direct usage of $exception->getMessage() in
the catch block that builds the Notification (the Notification::make() call in
Mounts.php) with a generic translated message key
(server/mount.notification_failed_body), and ensure the full exception is logged
on the server (e.g., via Log::error or the existing logger) so internal details
are recorded but not shown to users; also add the new translation key
server/mount.notification_failed_body with a generic user-facing message.
- Around line 71-73: In the afterStateUpdated callback on the Mounts form,
remove the unused $state parameter to avoid confusion; update the closure from
function ($state) { $this->save(); } to a parameterless closure (function () {
$this->save(); }) so the call to $this->save() remains but the unused $state
argument is eliminated in the afterStateUpdated usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa500256-3c29-4691-872c-8122f3ce0d70
📒 Files selected for processing (1)
app/Filament/Server/Pages/Mounts.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Filament/Server/Pages/Mounts.php (1)
65-66:⚠️ Potential issue | 🟠 MajorEscape mount fields before wrapping in
HtmlString.The
stripTags()method only removes HTML tags but does not escape HTML entities. An attacker with admin access to create mounts could use entity encoding (e.g.,<script>) to inject XSS payloads that bypassstripTags().Use
e()to escape all dynamic values:Proposed fix
- ->descriptions(fn () => $allowedMounts->mapWithKeys(fn (Mount $mount) => [$mount->id => new HtmlString(str("$mount->source -> $mount->target")->stripTags() . ($mount->description ? '<br>' . str($mount->description)->stripTags() : ''))])) + ->descriptions(fn () => $allowedMounts->mapWithKeys(function (Mount $mount) { + $path = e($mount->source) . ' -> ' . e($mount->target); + $description = $mount->description ? '<br>' . e(strip_tags($mount->description)) : ''; + + return [$mount->id => new HtmlString($path . $description)]; + }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Pages/Mounts.php` around lines 65 - 66, The descriptions callback is currently building an HtmlString from unescaped mount fields (in the ->descriptions(...) lambda using $allowedMounts->mapWithKeys and new HtmlString(...)), which allows entity-encoded XSS to bypass stripTags(); update that callback to escape each dynamic value (use e($mount->source), e($mount->target), and e($mount->description)) before concatenating and then construct the HtmlString only from the escaped pieces (you can still insert a literal '<br>' between escaped parts if you need a line break), ensuring all $mount properties are passed through e() prior to wrapping in HtmlString.
🧹 Nitpick comments (1)
app/Filament/Server/Pages/Mounts.php (1)
71-73: Remove unused$stateparameter.The
$stateparameter is not used in the callback. Consider simplifying to an arrow function or removing the parameter.Proposed fix
- ->afterStateUpdated(function ($state) { - $this->save(); - }) + ->afterStateUpdated(fn () => $this->save())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Pages/Mounts.php` around lines 71 - 73, The anonymous callback passed to afterStateUpdated includes an unused $state parameter; remove the parameter and simplify the callback to avoid unused variable warnings by changing the handler to a parameterless function that calls $this->save() (i.e., update the afterStateUpdated(...) callback where ->afterStateUpdated(function ($state) { $this->save(); }) is defined to a parameterless version or an arrow function that invokes $this->save()).
🤖 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/Server/Pages/Mounts.php`:
- Around line 98-105: Replace the
Notification::make()->body($exception->getMessage()) usage so the UI shows a
generic message instead of the raw exception; keep report($exception) to log
details, change the body call to use the translation key
trans('server/mount.notification_failed_body'), and add that translation entry
with a generic message like "An error occurred while updating mounts. Please try
again." This avoids exposing $exception->getMessage() in Mounts.php while
preserving server-side reporting via report($exception).
---
Duplicate comments:
In `@app/Filament/Server/Pages/Mounts.php`:
- Around line 65-66: The descriptions callback is currently building an
HtmlString from unescaped mount fields (in the ->descriptions(...) lambda using
$allowedMounts->mapWithKeys and new HtmlString(...)), which allows
entity-encoded XSS to bypass stripTags(); update that callback to escape each
dynamic value (use e($mount->source), e($mount->target), and
e($mount->description)) before concatenating and then construct the HtmlString
only from the escaped pieces (you can still insert a literal '<br>' between
escaped parts if you need a line break), ensuring all $mount properties are
passed through e() prior to wrapping in HtmlString.
---
Nitpick comments:
In `@app/Filament/Server/Pages/Mounts.php`:
- Around line 71-73: The anonymous callback passed to afterStateUpdated includes
an unused $state parameter; remove the parameter and simplify the callback to
avoid unused variable warnings by changing the handler to a parameterless
function that calls $this->save() (i.e., update the afterStateUpdated(...)
callback where ->afterStateUpdated(function ($state) { $this->save(); }) is
defined to a parameterless version or an arrow function that invokes
$this->save()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 040e838f-4531-4ab1-8a5b-507f622741fe
📒 Files selected for processing (1)
app/Filament/Server/Pages/Mounts.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Filament/Server/Pages/Mounts.php (1)
97-100:⚠️ Potential issue | 🟠 MajorDo not expose raw exception messages in this user-facing page.
At Line 99,
$exception->getMessage()can leak internal details to subusers. Keepreport($exception), but show a generic translated failure body in the notification instead.Proposed fix
Notification::make() ->title(trans('server/mount.notification_failed')) - ->body($exception->getMessage()) + ->body(trans('server/mount.notification_failed_body')) ->danger() ->send();Also add
server/mount.notification_failed_bodyto translations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Pages/Mounts.php` around lines 97 - 100, Replace the user-facing raw exception message in Notification::make() — keep the existing report($exception) call to log the error, but change ->body($exception->getMessage()) to use a generic translated message like ->body(trans('server/mount.notification_failed_body')); also add the key server/mount.notification_failed_body to your locale files with an appropriate generic message so users see a safe, localized failure description while internal details remain logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Filament/Server/Pages/Mounts.php`:
- Around line 97-100: Replace the user-facing raw exception message in
Notification::make() — keep the existing report($exception) call to log the
error, but change ->body($exception->getMessage()) to use a generic translated
message like ->body(trans('server/mount.notification_failed_body')); also add
the key server/mount.notification_failed_body to your locale files with an
appropriate generic message so users see a safe, localized failure description
while internal details remain logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c196bbe-d546-4405-9028-39341814644d
📒 Files selected for processing (2)
app/Filament/Server/Pages/Mounts.phplang/en/activity.php
✅ Files skipped from review due to trivial changes (1)
- lang/en/activity.php
Boy132
approved these changes
Mar 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #2077