Merge pull request #1473 from wintercms/fix/security-improvements

Author: LukeTowers

modules/backend/ServiceProvider.php

@@ -303,7 +303,7 @@ protected function registerBackendSettings()
                     'description' => 'backend::lang.myaccount.menu_description',
                     'category'    => SettingsManager::CATEGORY_MYSETTINGS,
                     'icon'        => 'icon-user',
-                    'url'         => Backend::url('backend/users/myaccount'),
+                    'url'         => Backend::url('backend/myaccount'),
                     'order'       => 500,
                     'context'     => 'mysettings',
                     'keywords'    => 'backend::lang.myaccount.menu_keywords'

modules/backend/classes/Controller.php

@@ -319,11 +319,16 @@ public function run($action = null, $params = [])
          */
         elseif (
             ($handler = post('_handler')) &&
-            $this->verifyCsrfToken() &&
-            ($handlerResponse = $this->runAjaxHandler($handler)) &&
-            $handlerResponse !== true
+            $this->verifyCsrfToken()
         ) {
-            $result = $handlerResponse;
+            $this->validateHandlerName($handler);
+
+            if (
+                ($handlerResponse = $this->runAjaxHandler($handler)) &&
+                $handlerResponse !== true
+            ) {
+                $result = $handlerResponse;
+            }
         }
 
         /*
@@ -476,6 +481,18 @@ public function getAjaxHandler()
         return null;
     }
 
+    /**
+     * Validates the AJAX handler name follows the expected format.
+     *
+     * @throws \Winter\Storm\Exception\SystemException if the handler name is invalid
+     */
+    protected function validateHandlerName(string $handler): void
+    {
+        if (!preg_match('/^(?:\w+\:{2})?on[A-Z]{1}[\w+]*$/', $handler)) {
+            throw new SystemException(Lang::get('backend::lang.ajax_handler.invalid_name', ['name' => $handler]));
+        }
+    }
+
     /**
      * This method is used internally.
      * Invokes a controller event handler and loads the supplied partials.
@@ -487,9 +504,7 @@ protected function execAjaxHandlers()
                 /*
                  * Validate the handler name
                  */
-                if (!preg_match('/^(?:\w+\:{2})?on[A-Z]{1}[\w+]*$/', $handler)) {
-                    throw new SystemException(Lang::get('backend::lang.ajax_handler.invalid_name', ['name'=>$handler]));
-                }
+                $this->validateHandlerName($handler);
 
                 /*
                  * Validate the handler partial list

modules/backend/controllers/Index.php

@@ -75,7 +75,7 @@ protected function checkPermissionRedirect()
             if ($first = array_first(BackendMenu::listMainMenuItems())) {
                 return Redirect::intended($first->url);
             }
-            return Backend::redirect('backend/users/myaccount');
+            return Backend::redirect('backend/myaccount');
         }
     }
 }

modules/backend/controllers/MyAccount.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Backend\Controllers;
+
+use Backend\Behaviors\FormController;
+use Backend\Classes\Controller;
+use Backend\Facades\BackendAuth;
+use Backend\Facades\BackendMenu;
+use System\Classes\SettingsManager;
+
+/**
+ * My Account controller
+ *
+ * Allows any authenticated backend user to manage their own account settings.
+ * Isolated from the Users controller to prevent privilege escalation via
+ * handler dispatch on a controller with degraded permissions.
+ *
+ * @package winter\wn-backend-module
+ * @author Winter CMS
+ */
+class MyAccount extends Controller
+{
+    /**
+     * @var array Extensions implemented by this controller.
+     */
+    public $implement = [
+        FormController::class,
+    ];
+
+    /**
+     * @var array Permissions required to view this page.
+     *      Empty array — any logged-in user can access their own account.
+     */
+    public $requiredPermissions = [];
+
+    /**
+     * @var string HTML body tag class
+     */
+    public $bodyClass = 'compact-container';
+
+    public $formLayout = 'sidebar';
+
+    /**
+     * Constructor.
+     */
+    public function __construct()
+    {
+        parent::__construct();
+
+        BackendMenu::setContext('Winter.System', 'system', 'users');
+        SettingsManager::setContext('Winter.Backend', 'myaccount');
+    }
+
+    /**
+     * My Account page
+     */
+    public function index()
+    {
+        $this->pageTitle = 'backend::lang.myaccount.menu_label';
+        return $this->asExtension('FormController')->update($this->user->id, 'myaccount');
+    }
+
+    /**
+     * Save handler for the My Account form
+     */
+    public function index_onSave()
+    {
+        $result = $this->asExtension('FormController')->update_onSave($this->user->id, 'myaccount');
+
+        /*
+         * If the password or login name has been updated, reauthenticate the user
+         */
+        $loginChanged = $this->user->login != post('User[login]');
+        $passwordChanged = strlen(post('User[password]'));
+        if ($loginChanged || $passwordChanged) {
+            BackendAuth::login($this->user->reload(), true);
+        }
+
+        return $result;
+    }
+}

modules/backend/controllers/Users.php

@@ -55,10 +55,6 @@ public function __construct()
     {
         parent::__construct();
 
-        if ($this->action == 'myaccount') {
-            $this->requiredPermissions = null;
-        }
-
         BackendMenu::setContext('Winter.System', 'system', 'users');
         SettingsManager::setContext('Winter.System', 'administrators');
     }
@@ -123,9 +119,9 @@ public function formBeforeCreate($model)
      */
     public function update($recordId, $context = null)
     {
-        // Users cannot edit themselves, only use My Settings
+        // Users cannot edit themselves, only use My Account
         if ($context != 'myaccount' && $recordId == $this->user->id) {
-            return Backend::redirect('backend/users/myaccount');
+            return Backend::redirect('backend/myaccount');
         }
 
         return $this->asExtension('FormController')->update($recordId, $context);
@@ -158,7 +154,7 @@ public function update_onImpersonateUser($recordId)
 
         Flash::success(Lang::get('backend::lang.account.impersonate_success'));
 
-        return Backend::redirect('backend/users/myaccount');
+        return Backend::redirect('backend/myaccount');
     }
 
     /**
@@ -176,33 +172,11 @@ public function update_onUnsuspendUser($recordId)
     }
 
     /**
-     * My Settings controller
+     * Backward compatibility redirect to the new MyAccount controller.
      */
     public function myaccount()
     {
-        SettingsManager::setContext('Winter.Backend', 'myaccount');
-
-        $this->pageTitle = 'backend::lang.myaccount.menu_label';
-        return $this->update($this->user->id, 'myaccount');
-    }
-
-    /**
-     * Proxy update onSave event
-     */
-    public function myaccount_onSave()
-    {
-        $result = $this->asExtension('FormController')->update_onSave($this->user->id, 'myaccount');
-
-        /*
-         * If the password or login name has been updated, reauthenticate the user
-         */
-        $loginChanged = $this->user->login != post('User[login]');
-        $passwordChanged = strlen(post('User[password]'));
-        if ($loginChanged || $passwordChanged) {
-            BackendAuth::login($this->user->reload(), true);
-        }
-
-        return $result;
+        return Backend::redirect('backend/myaccount');
     }
 
     /**

modules/backend/controllers/myaccount/config_form.yaml

@@ -0,0 +1,12 @@
+# ===================================
+#  Form Behavior Config
+# ===================================
+
+name: backend::lang.user.name
+form: ~/modules/backend/models/user/fields.yaml
+modelClass: Backend\Models\User
+defaultRedirect: backend/myaccount
+
+update:
+    redirect: backend/myaccount
+    redirectClose: backend/myaccount

modules/backend/controllers/myaccount/index.php

@@ -0,0 +1,56 @@
+<?php if ($this->user->hasAccess('backend.manage_users')): ?>
+    <?php Block::put('breadcrumb') ?>
+        <ul>
+            <li><a href="<?= Backend::url('backend/users') ?>"><?= e(trans('backend::lang.user.menu_label')) ?></a></li>
+            <li><?= e(trans($this->pageTitle)) ?></li>
+        </ul>
+    <?php Block::endPut() ?>
+<?php endif ?>
+
+<?php if (!$this->fatalError): ?>
+
+    <?php Block::put('form-contents') ?>
+        <div class="layout">
+
+            <div class="layout-row">
+                <?= $this->formRenderOutsideFields() ?>
+                <?= $this->formRenderPrimaryTabs() ?>
+            </div>
+
+            <div class="form-buttons">
+                <div class="loading-indicator-container">
+                    <button
+                        type="submit"
+                        data-request="onSave"
+                        data-browser-validate
+                        data-request-data="redirect:0"
+                        data-hotkey="ctrl+s, cmd+s"
+                        data-load-indicator="<?= e(trans('backend::lang.form.saving')) ?>"
+                        class="btn btn-primary">
+                        <?= e(trans('backend::lang.form.save')) ?>
+                    </button>
+                </div>
+            </div>
+
+        </div>
+    <?php Block::endPut() ?>
+
+    <?php Block::put('form-sidebar') ?>
+        <div class="hide-tabs"><?= $this->formRenderSecondaryTabs() ?></div>
+    <?php Block::endPut() ?>
+
+    <?php Block::put('body') ?>
+        <?= Form::open(['class'=>'layout stretch']) ?>
+            <?= $this->makeLayout('form-with-sidebar') ?>
+        <?= Form::close() ?>
+    <?php Block::endPut() ?>
+
+<?php else: ?>
+    <div class="control-breadcrumb">
+        <?= Block::placeholder('breadcrumb') ?>
+    </div>
+    <div class="padded-container">
+        <p class="flash-message static error"><?= e(trans($this->fatalError)) ?></p>
+        <p><a href="<?= Backend::url('backend') ?>" class="btn btn-default"><?= e(trans('backend::lang.form.return_to_list')) ?></a></p>
+    </div>
+<?php endif ?>

modules/backend/lang/en/lang.php

@@ -168,8 +168,7 @@
         'deleted_at' => 'Deleted at',
         'show_deleted' => 'Show deleted',
         'self_escalation_denied' => 'You cannot modify your own role, permissions, or superuser status.',
-        'superuser_grant_denied' => 'Only superusers can grant superuser status or modify other superuser accounts.',
-        'manage_users_denied' => 'You do not have permission to manage other administrators.',
+        'cannot_manage_user' => 'You do not have permission to manage this administrator.',
         'throttle_tab' => 'Failed Logins',
         'throttle_tab_label' => 'Failed Login Records',
         'throttle_comment' => 'View failed login attempts for this user. These records are automatically generated when login attempts fail. Users are suspended after exceeding the attempt limit.',