Skip to content

Harden per-item permission checks in the Settings controller - #1531

Merged
LukeTowers merged 1 commit into
developfrom
fix/settings-controller-item-permissions
Aug 26, 2026
Merged

Harden per-item permission checks in the Settings controller#1531
LukeTowers merged 1 commit into
developfrom
fix/settings-controller-item-permissions

Conversation

@LukeTowers

Copy link
Copy Markdown
Member

System\Controllers\Settings declares no $requiredPermissions, so a setting item's own permissions are the only thing gating it. Those are enforced solely by SettingsManager::loadItems(), which filters the shared item cache that findSettingItem() reads — so the gate does hold on every path that ships today.

It fails open in two situations:

  • filterItemPermissions() returns the list untouched when $user is null, so anything that warms the item cache before authentication caches the unfiltered set for the rest of the request.
  • registerSettingItems() sets $this->items = [] when it is falsy, so calling it directly on the manager rather than inside registerCallback() leaves $this->items non-null and loadItems() never runs — nothing is filtered at all.

Neither is reachable from core or from any first-party plugin I checked, so this is hardening rather than a fix for a live issue.

It came out of triaging GHSA-8ffc-66jg-44wc, which reported the Settings write path as writable by any backend user. That report did not reproduce — loadItems() reassigns $this->items with the filtered result, and findSettingItem() reads that same cache — but the controller shouldn't depend on the manager having taken that path.

Changes

findSettingItem() now checks the item's own permissions, returning false for a forbidden item exactly as the manager already does for a missing one, so no new error shape is introduced. It covers update(), update_onSave(), update_onTest(), update_onResetDefault() and formGetWidget(), which all resolve their item through it.

Tests

modules/system/tests/controllers/SettingsSecurityTest.php:

  • a directly-registered item declaring a permission the user lacks is refused — fails without this change, where the item resolves and saves with HTTP 200
  • the same item is still served to a user who does hold the permission
  • core Mail Settings still save on the normal registerCallback() path for a permitted user
  • core Mail Settings are still refused for an unprivileged user

The pre-existing ViteCompileTest, MixCompileTest and ViewMakerTest failures in modules/system are unrelated — they fail identically with this change reverted.

🤖 Generated with Claude Code

The Settings controller declares no $requiredPermissions, so a setting item's own
permissions are the only thing gating it, and those are enforced solely by
SettingsManager::loadItems() filtering the shared item cache that findSettingItem()
reads. That filtering fails open when no user is authenticated at the time the cache
is warmed, and is skipped entirely when items are registered directly on the manager
rather than through registerCallback(), since loadItems() then never runs. Neither is
reachable from code that ships today.

findSettingItem() now checks the item's own permissions as well, returning false for a
forbidden item exactly as the manager already does for a missing one, so no new error
shape is introduced. This covers update(), update_onSave(), update_onTest(),
update_onResetDefault() and formGetWidget(), which all resolve their item through it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@LukeTowers
LukeTowers merged commit 1b97ad6 into develop Aug 26, 2026
2 checks passed
@LukeTowers
LukeTowers deleted the fix/settings-controller-item-permissions branch August 26, 2026 23:11
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.

1 participant