diff --git a/modules/system/controllers/Settings.php b/modules/system/controllers/Settings.php index d6d4b649f9..be4351ebbc 100644 --- a/modules/system/controllers/Settings.php +++ b/modules/system/controllers/Settings.php @@ -238,6 +238,17 @@ protected function findSettingItem($author = null, $plugin = null, $code = null) $item = $manager->findSettingItem($pluginOwner, $pluginCode); } + /* + * The manager drops items the user lacks permission for as it loads them, but that + * filtering is skipped when items are registered outside registerCallback() or when + * the item cache is warmed before authentication. This controller carries no + * $requiredPermissions of its own, so check the item here as well. Missing items and + * forbidden items are reported the same way, as the manager already does. + */ + if ($item && !empty($item->permissions) && !$this->user?->hasAnyAccess((array) $item->permissions)) { + return false; + } + return $item; } diff --git a/modules/system/tests/controllers/SettingsSecurityTest.php b/modules/system/tests/controllers/SettingsSecurityTest.php new file mode 100644 index 0000000000..fd8c16a29e --- /dev/null +++ b/modules/system/tests/controllers/SettingsSecurityTest.php @@ -0,0 +1,138 @@ + $login, + 'code' => $login, + 'permissions' => $permissions, + ]); + $user = User::create([ + 'first_name' => ucfirst($login), + 'last_name' => 'User', + 'login' => $login, + 'email' => "{$login}@test.test", + 'password' => 'TestPassword1', + 'password_confirmation' => 'TestPassword1', + 'is_activated' => true, + 'role_id' => $role->id, + ]); + Model::reguard(); + + return $user; + } + + /** registerBackendSettings() is gated behind runningInBackend(), which is false here. */ + protected function registerCoreSettingItems(): void + { + $provider = new \System\ServiceProvider($this->app); + $method = new \ReflectionMethod($provider, 'registerBackendSettings'); + $method->setAccessible(true); + $method->invoke($provider); + } + + /** Registered directly on the instance, so loadItems() -- and its filtering -- never runs. */ + protected function registerUnfilteredSettingItem(): void + { + SettingsManager::instance()->registerSettingItems('Acme.Test', [ + 'protected' => [ + 'label' => 'Protected', + 'class' => MailSetting::class, + 'permissions' => ['system.manage_mail_settings'], + ], + ]); + } + + protected function save(string $uri, string $address) + { + return $this->post('backend/system/settings/update/' . $uri, [ + 'MailSetting' => ['send_mode' => 'smtp', 'smtp_address' => $address], + ], [ + 'X-WINTER-REQUEST-HANDLER' => 'onSave', + 'X-Requested-With' => 'XMLHttpRequest', + ]); + } + + public function testAnUnfilteredItemIsRefusedWithoutItsPermission(): void + { + $this->registerUnfilteredSettingItem(); + $this->actingAs($this->makeUser('peon', ['cms.manage_pages' => 1])); + + $this->save('acme/test/protected', 'attacker.example.com'); + + MailSetting::clearInternalCache(); + $this->assertNotEquals('attacker.example.com', MailSetting::instance()->smtp_address); + } + + public function testAnUnfilteredItemIsServedToAUserHoldingItsPermission(): void + { + $this->registerUnfilteredSettingItem(); + $this->actingAs($this->makeUser('mailadmin', ['system.manage_mail_settings' => 1])); + + $response = $this->save('acme/test/protected', 'legit.example.com'); + + $this->assertEquals(200, $response->getStatusCode()); + MailSetting::clearInternalCache(); + $this->assertEquals('legit.example.com', MailSetting::instance()->smtp_address); + } + + /** Nothing legitimate regressed on the normal registerCallback() path. */ + public function testCoreMailSettingsStillSaveForAPermittedUser(): void + { + $this->registerCoreSettingItems(); + $this->actingAs($this->makeUser('mailadmin2', ['system.manage_mail_settings' => 1])); + + $response = $this->save('winter/system/mail_settings', 'legit.example.com'); + + $this->assertEquals(200, $response->getStatusCode()); + MailSetting::clearInternalCache(); + $this->assertEquals('legit.example.com', MailSetting::instance()->smtp_address); + } + + public function testCoreMailSettingsAreRefusedForAnUnprivilegedUser(): void + { + $this->registerCoreSettingItems(); + $this->actingAs($this->makeUser('peon2', ['cms.manage_pages' => 1])); + + $this->save('winter/system/mail_settings', 'attacker.example.com'); + + MailSetting::clearInternalCache(); + $this->assertNotEquals('attacker.example.com', MailSetting::instance()->smtp_address); + } +}