Add Laravel/Filament Log Viewer #1778
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRegisters the achyutn/filament-log-viewer plugin in both Admin and App Filament panel providers, adds the package to composer require, and adds a new special permission Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminFP as Admin Panel Provider
participant AppFP as App Panel Provider
participant PL as FilamentLogViewer
participant Auth as Auth (user())
rect rgb(248,249,251)
Note over AdminFP,PL: Admin panel registers plugin with runtime authorization check
AdminFP->>PL: register FilamentLogViewer::make()
PL->>Auth: user()->can('view panelLog')?
alt allowed
Auth-->>PL: true
PL-->>AdminFP: expose navigation (group/icon)
else denied
Auth-->>PL: false
PL-->>AdminFP: do not expose navigation
end
end
rect rgb(249,255,250)
Note over AppFP,PL: App panel registers plugin with authorize(false)
AppFP->>PL: register FilamentLogViewer::make()->authorize(false)
PL-->>AppFP: always integrate/expose
end
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/Providers/Filament/AdminPanelProvider.php (2)
39-39: Simplify redundant authorization check.Since this plugin is registered within the AdminPanel itself, users accessing it have already passed the
canAccessPanel('admin')gate. The authorization check is redundant. Consider simplifying to just check authentication or removing it entirely if the panel-level authentication is sufficient.Apply this diff to simplify:
FilamentLogViewer::make() - ->authorize(fn () => auth()->user()->canAccessPanel(Filament::getPanel('admin'))) + ->authorize(fn () => auth()->check()) ->navigationUrl('/logs')Alternatively, if panel-level authentication is sufficient, remove the authorize call entirely.
37-43: Consider additional security restrictions for log viewer access.Given that the PR description mentions this feature was previously removed due to security concerns, consider implementing stricter access controls:
- Restrict to root admins only: Log files can contain highly sensitive information (credentials, PII, stack traces). Consider limiting access to root admins only.
- Audit logging: Add audit logs when the log viewer is accessed.
- Rate limiting: Implement rate limits to prevent abuse.
- Log sanitization: Ensure sensitive data is redacted or consider excluding certain log files from the viewer.
If you choose to restrict to root admins only, apply this diff:
FilamentLogViewer::make() - ->authorize(fn () => auth()->user()->canAccessPanel(Filament::getPanel('admin'))) + ->authorize(fn () => auth()->check() && auth()->user()->isRootAdmin()) ->navigationUrl('/logs')Based on the User.php snippet showing
isRootAdmin()andisAdmin()methods are available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
app/Providers/Filament/AdminPanelProvider.php(2 hunks)app/Providers/Filament/AppPanelProvider.php(2 hunks)composer.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Providers/Filament/AppPanelProvider.php (1)
app/Models/User.php (1)
canAccessPanel(389-400)
app/Providers/Filament/AdminPanelProvider.php (1)
app/Models/User.php (1)
canAccessPanel(389-400)
🔇 Additional comments (3)
app/Providers/Filament/AppPanelProvider.php (1)
29-29: Cross-panel access is intentional. AppPanel’s log viewer requiringcanAccessPanel('admin')aligns with existingto_admin/exit_adminactions across panels.app/Providers/Filament/AdminPanelProvider.php (1)
40-40: No conflicting /logs route found. Confirmed no existing/logsor other log-related routes in the route definitions.composer.json (1)
12-12: Verify package existence and security status. Automated checks didn’t locateachyutn/filament-log-vieweron Packagist or GitHub. Confirm the package name is correct, ensure version ^1.4 is the latest stable release on Packagist, and review its repository (or vendor) for any security advisories or vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Providers/Filament/AppPanelProvider.php (1)
29-29: Add null safety to authorization check.The authorization closure will throw a null pointer exception if
auth()->user()returnsnull. This is the same concern previously flagged.Apply this diff to add null safety:
FilamentLogViewer::make() - ->authorize(fn () => auth()->user()->canAccessPanel(Filament::getPanel('admin'))), + ->authorize(fn () => auth()->check() && auth()->user()->canAccessPanel(Filament::getPanel('admin'))),
🧹 Nitpick comments (1)
app/Providers/Filament/AppPanelProvider.php (1)
27-30: Remove FilamentLogViewer registration from AppPanelProvider
The App panel has navigation disabled and no URL for logs, making this plugin inaccessible—consolidate the log viewer to AdminPanelProvider only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Providers/Filament/AppPanelProvider.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Providers/Filament/AppPanelProvider.php (1)
app/Models/User.php (1)
canAccessPanel(389-400)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Providers/Filament/AdminPanelProvider.php (1)
39-39: Critical: Null pointer exception risk in authorization closure.The
user()helper returns?App\Models\User(nullable). If called when no user is authenticated, this will throw a null pointer exception. While Filament panels typically require authentication, defensive programming is essential for security-sensitive features like log viewers.Apply this diff to add null safety:
- ->authorize(fn () => user()->can('view panellog')) + ->authorize(fn () => user()?->can('view panellog') ?? false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Providers/Filament/AdminPanelProvider.php(2 hunks)app/Providers/Filament/AppPanelProvider.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Providers/Filament/AppPanelProvider.php
🧰 Additional context used
🧬 Code graph analysis (1)
app/Providers/Filament/AdminPanelProvider.php (2)
app/helpers.php (1)
user(127-130)app/Models/User.php (1)
can(357-370)
🔇 Additional comments (1)
app/Providers/Filament/AdminPanelProvider.php (1)
37-42: FilamentLogViewer access properly restricted and disabled
AdminPanelProvider uses->authorize(fn()=>user()->can('view panellog'))and AppPanelProvider explicitly disables the viewer with->authorize(false). No code‐level bypass or sensitive data patterns were found. Confirm theview panellogpermission is scoped correctly and review log contents for PII before enabling in production.
Co-authored-by: Boy132 <Boy132@users.noreply.github.com>
We had this a long time ago, but it had some security concerns.
New and improved!