fix(roles): gate the sidenav pin list and #add-item on the admin role - #1603
Merged
Merged
Conversation
In reverse-proxy roles mode the dashboard filters tiles by the caller's roles, but the chrome around it does not. The sidenav pin list is built from all_apps, which is not role-filtered, and it is rendered for every visitor: a non-admin is served the title of every item in the database plus an items.pintoggle link for each one, while the dashboard above it correctly shows only their own. #add-item, which opens that list, is rendered unconditionally too, even though #config-buttons beside it is already gated on the admin role. Pinning is global item state in roles mode rather than a per-visitor preference, so the pin list belongs behind the same admin check as the config buttons. Gate both. Role-filtering all_apps instead was the other option and is not enough on its own: it would leave a non-admin able to unpin their own tiles for everyone, and it takes an admin's ability to unpin an item outside their own roles. There is a test covering that.
KodeStar
approved these changes
Sep 9, 2026
KodeStar
left a comment
Member
There was a problem hiding this comment.
Reviewed and verified locally.
- Agree with gating on the admin role rather than role-filtering all_apps: pinning is global item state, and filtering would silently drop an admin's ability to unpin items outside their own roles.
- Removing the nav is safe for the front end: the #add-item / .close-sidenav handlers are delegated from #app, and $('#pinlist').on(...) is a no-op when the element is absent.
- Outside roles mode enable_auth_admin_controls is unconditionally true, so the default install is unchanged; the roles-disabled test covers that.
- Ran the full suite on the branch: 159 passed, 1 skipped, 392 assertions, phpcs unchanged.
- Confirmed the new tests catch the leak: with the 2.x resources/views tree and only the test file applied, 4 of 7 fail; all 7 pass with the fix.
- Merged together with #1605 locally: 163 passed, no conflicts.
One note for a follow-up rather than this PR: the pin/unpin/pintoggle routes and the item CRUD routes have no role check server-side in roles mode (CheckAllowed never looks at the roles header), so a non-admin who knows the URL can still toggle a pin directly. This PR closes the leak in the rendered page, which is what #1602 describes; route-level enforcement should be its own change.
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
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.
Fixes #1602.
Problem
In roles mode the dashboard filters tiles by the caller's roles, but the sidenav pin list beside it is rendered for every visitor from
all_apps, which carries no role filter. A non-admin visitor receives the title and id of every item in the database plus anitems.pintogglelink for each.#add-item, which opens that list, is ungated as well, while#config-buttonstwelve lines below it in the same template is already gated on$enable_auth_admin_controls.Change
Two
@if($enable_auth_admin_controls)gates, one around the.sidenavblock and one around#add-item, plus tests.Pinning is global item state in roles mode rather than a per-visitor preference, so the pin list belongs behind the same check as the other controls that write global state.
Outside roles mode this is a no-op:
AppServiceProvidersetsenable_auth_admin_controlstotrueunconditionally whenauth_roles_enableis off, so the default single-user install renders exactly as before. There is a test for that.I considered role-filtering
all_appsinstead and did not, for two reasons. It would still leave a non-admin able to unpin their own tiles for everyone, and it takes away an admin's ability to unpin an item outside their own roles, since the pin list is the only place that can be done.test_admin_keeps_the_pin_control_for_items_of_other_rolespins that down. I am happy to add the filter on top if you would rather have both.Test
tests/Feature/DashRolesTest.php, 7 cases: the leak, the ungated control, a multi-role visitor, two admin control cases, and roles mode disabled.On
2.xwithout the change, 4 of the 7 fail and the two admin control cases and the roles-disabled case pass:With the change, all 7 pass.
Suite and linter,
php artisan testand./vendor/bin/phpcs --report=summary:2.xThe 7 added tests are the whole difference. The phpcs findings are all pre-existing and unchanged.
Live verification
Built as an overlay on the stock
linuxserver/heimdallimage we run and A/B'd against a copy of our live config, same environment, same database, both containers up at once:items.pintogglelinks#add-itemitems.pintogglelinks#add-item#config-buttonsThe non-admin's own tiles are untouched and every admin value is identical.