diff --git a/modules/backend/ServiceProvider.php b/modules/backend/ServiceProvider.php index 9086f3f63b..04dbe5153e 100644 --- a/modules/backend/ServiceProvider.php +++ b/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' diff --git a/modules/backend/classes/Controller.php b/modules/backend/classes/Controller.php index a4d8a9b151..2259e16509 100644 --- a/modules/backend/classes/Controller.php +++ b/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 diff --git a/modules/backend/controllers/Index.php b/modules/backend/controllers/Index.php index 57937b147a..6856a9b729 100644 --- a/modules/backend/controllers/Index.php +++ b/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'); } } } diff --git a/modules/backend/controllers/MyAccount.php b/modules/backend/controllers/MyAccount.php new file mode 100644 index 0000000000..75a7cd4a41 --- /dev/null +++ b/modules/backend/controllers/MyAccount.php @@ -0,0 +1,81 @@ +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; + } +} diff --git a/modules/backend/controllers/Users.php b/modules/backend/controllers/Users.php index 2042afc75f..ef0813febd 100644 --- a/modules/backend/controllers/Users.php +++ b/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'); } /** diff --git a/modules/backend/controllers/myaccount/config_form.yaml b/modules/backend/controllers/myaccount/config_form.yaml new file mode 100644 index 0000000000..060251d0db --- /dev/null +++ b/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 diff --git a/modules/backend/controllers/myaccount/index.php b/modules/backend/controllers/myaccount/index.php new file mode 100644 index 0000000000..a9e49f535a --- /dev/null +++ b/modules/backend/controllers/myaccount/index.php @@ -0,0 +1,56 @@ +user->hasAccess('backend.manage_users')): ?> + + + + + +fatalError): ?> + + +
+ +
+ formRenderOutsideFields() ?> + formRenderPrimaryTabs() ?> +
+ +
+
+ +
+
+ +
+ + + +
formRenderSecondaryTabs() ?>
+ + + + 'layout stretch']) ?> + makeLayout('form-with-sidebar') ?> + + + + +
+ +
+
+

fatalError)) ?>

+

+
+ diff --git a/modules/backend/lang/en/lang.php b/modules/backend/lang/en/lang.php index 9ee591a56f..d01ad6c24f 100644 --- a/modules/backend/lang/en/lang.php +++ b/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.', diff --git a/modules/backend/models/User.php b/modules/backend/models/User.php index 0995fad184..10da7209bc 100644 --- a/modules/backend/models/User.php +++ b/modules/backend/models/User.php @@ -128,33 +128,71 @@ public function getAvatarThumb($size = 25, $options = null) '&d='. urlencode($default); } + /** + * Determine whether the given user (or the currently authenticated user) + * is authorized to manage this user record. + * + * Returns true when no user is provided and no user is authenticated (CLI/queue), + * or when the user has `backend.manage_users` and (if this record is a superuser) + * the user is also a superuser. + */ + public function canBeManagedByUser(?User $user = null): bool + { + $user = $user ?? BackendAuth::getUser(); + + if (!$user) { + return true; + } + + if (!$user->hasAccess('backend.manage_users')) { + return false; + } + + if (!$user->isSuperUser() && ($this->is_superuser || $this->getOriginal('is_superuser'))) { + return false; + } + + return true; + } + /** * Before save event — enforce authorization rules to prevent privilege escalation. - * @return void */ public function beforeSave() { $actor = BackendAuth::getUser(); - $isCurrentUser = $this->exists && $actor && $actor->getKey() === $this->getKey(); - - // No authenticated user (CLI, artisan, queue, seeders) — allow everything if (!$actor) { return; } - // Rule 1: Self-escalation — users cannot modify their own role, superuser status, or permissions + $isCurrentUser = $this->exists && $actor->getKey() === $this->getKey(); + if ($isCurrentUser && $this->isDirty(['role_id', 'is_superuser', 'permissions'])) { throw new AuthorizationException(Lang::get('backend::lang.user.self_escalation_denied')); } - // Rule 2: Must have backend.manage_users to manage other users - if (!$isCurrentUser && !$actor->hasAccess('backend.manage_users')) { - throw new AuthorizationException(Lang::get('backend::lang.user.manage_users_denied')); + if (!$isCurrentUser && !$this->canBeManagedByUser($actor)) { + throw new AuthorizationException(Lang::get('backend::lang.user.cannot_manage_user')); } + } - // Rule 3: Only superusers can grant superuser status or edit existing superusers - if (!$actor->isSuperUser() && ($this->is_superuser || $this->getOriginal('is_superuser'))) { - throw new AuthorizationException(Lang::get('backend::lang.user.superuser_grant_denied')); + /** + * Before delete event — enforce authorization rules. + */ + public function beforeDelete() + { + if (!$this->canBeManagedByUser()) { + throw new AuthorizationException(Lang::get('backend::lang.user.cannot_manage_user')); + } + } + + /** + * Before restore event — enforce authorization rules. + */ + public function beforeRestore() + { + if (!$this->canBeManagedByUser()) { + throw new AuthorizationException(Lang::get('backend::lang.user.cannot_manage_user')); } } @@ -244,13 +282,37 @@ public function isSuspended() /** * Remove the suspension on this user. - * @return void + * + * @throws AuthorizationException if the current user lacks permission */ public function unsuspend() { + if (!$this->canBeManagedByUser()) { + throw new AuthorizationException(Lang::get('backend::lang.user.cannot_manage_user')); + } + BackendAuth::findThrottleByUserId($this->id)->unsuspend(); } + /** + * Get a reset password code for this user. + * + * When called by an authenticated user targeting a different account, + * the actor must have `backend.manage_users` permission. + * Self-service resets (Auth controller restore flow) are allowed. + * + * @throws AuthorizationException if the current user lacks permission + */ + public function getResetPasswordCode() + { + $actor = BackendAuth::getUser(); + if ($actor && $actor->getKey() !== $this->getKey() && !$this->canBeManagedByUser($actor)) { + throw new AuthorizationException(Lang::get('backend::lang.user.cannot_manage_user')); + } + + return parent::getResetPasswordCode(); + } + // // Impersonation // diff --git a/modules/backend/tests/classes/ControllerPostbackTest.php b/modules/backend/tests/classes/ControllerPostbackTest.php new file mode 100644 index 0000000000..6124f27d03 --- /dev/null +++ b/modules/backend/tests/classes/ControllerPostbackTest.php @@ -0,0 +1,159 @@ +getMockBuilder('Illuminate\Http\Request') + ->disableOriginalConstructor() + ->setMethods(['ajax', 'method', 'header', 'secure', 'path', 'getScheme', 'getHost', 'getPort', 'getBaseUrl']) + ->getMock(); + + $map = [ + ['X_WINTER_REQUEST_HANDLER', null, $handler], + ['X_WINTER_REQUEST_PARTIALS', null, ''], + ['X-CSRF-TOKEN', null, null], + ['X-XSRF-TOKEN', null, null], + ]; + + $requestMock->expects($this->any())->method('ajax')->willReturn(true); + $requestMock->expects($this->any())->method('method')->willReturn('POST'); + $requestMock->expects($this->any())->method('header')->willReturnMap($map); + $requestMock->expects($this->any())->method('secure')->willReturn(false); + $requestMock->expects($this->any())->method('path')->willReturn('backend/auth/signin'); + $requestMock->expects($this->any())->method('getScheme')->willReturn('http'); + $requestMock->expects($this->any())->method('getHost')->willReturn('localhost'); + $requestMock->expects($this->any())->method('getPort')->willReturn(80); + $requestMock->expects($this->any())->method('getBaseUrl')->willReturn(''); + + return $requestMock; + } + + /** + * Builds a mock Request that simulates a non-AJAX POST with _handler in POST data. + */ + protected function configPostbackRequestMock(string $handler) + { + $requestMock = $this + ->getMockBuilder('Illuminate\Http\Request') + ->disableOriginalConstructor() + ->setMethods(['ajax', 'method', 'header', 'post', 'input', 'secure', 'path', 'getScheme', 'getHost', 'getPort', 'getBaseUrl']) + ->getMock(); + + $requestMock->expects($this->any())->method('ajax')->willReturn(false); + $requestMock->expects($this->any())->method('method')->willReturn('POST'); + $requestMock->expects($this->any())->method('header')->willReturn(null); + $requestMock->expects($this->any())->method('secure')->willReturn(false); + $requestMock->expects($this->any())->method('path')->willReturn('backend/auth/signin'); + $requestMock->expects($this->any())->method('getScheme')->willReturn('http'); + $requestMock->expects($this->any())->method('getHost')->willReturn('localhost'); + $requestMock->expects($this->any())->method('getPort')->willReturn(80); + $requestMock->expects($this->any())->method('getBaseUrl')->willReturn(''); + + $postData = ['_handler' => $handler]; + $requestMock->expects($this->any())->method('post')->willReturnCallback( + function ($key = null, $default = null) use ($postData) { + return $key === null ? $postData : ($postData[$key] ?? $default); + } + ); + $requestMock->expects($this->any())->method('input')->willReturnCallback( + function ($key = null, $default = null) use ($postData) { + return $key === null ? $postData : ($postData[$key] ?? $default); + } + ); + + return $requestMock; + } + + // + // AJAX header path — validates handler name (existing behavior) + // + + public function testAjaxPathRejectsInvalidHandlerName(): void + { + $this->actingAs((new UserFixture)->asSuperUser()); + Config::set('cms.enableCsrfProtection', false); + + // Build controller with real Request, then swap for mock before run() + $controller = new Auth; + Request::swap($this->configAjaxRequestMock('update_onDelete')); + + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: update_onDelete.'); + $controller->run('signin'); + } + + public function testAjaxPathAcceptsValidHandlerName(): void + { + $this->actingAs((new UserFixture)->asSuperUser()); + Config::set('cms.enableCsrfProtection', false); + + $controller = new Auth; + Request::swap($this->configAjaxRequestMock('onSave')); + + try { + $controller->run('signin'); + } catch (SystemException $e) { + // Handler not found is fine — name validation passed + $this->assertStringNotContainsString('Invalid AJAX handler name', $e->getMessage()); + return; + } + $this->assertTrue(true); + } + + // + // Postback _handler path — validates handler name (our fix) + // + + public function testPostbackPathRejectsInvalidHandlerName(): void + { + $this->actingAs((new UserFixture)->asSuperUser()); + Config::set('cms.enableCsrfProtection', false); + + $controller = new Auth; + Request::swap($this->configPostbackRequestMock('update_onDelete')); + + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: update_onDelete.'); + $controller->run('signin'); + } + + public function testPostbackPathRejectsMethodName(): void + { + $this->actingAs((new UserFixture)->asSuperUser()); + Config::set('cms.enableCsrfProtection', false); + + $controller = new Auth; + Request::swap($this->configPostbackRequestMock('generatePermissionsField')); + + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: generatePermissionsField.'); + $controller->run('signin'); + } + + public function testPostbackPathRejectsActionPrefixedHandler(): void + { + $this->actingAs((new UserFixture)->asSuperUser()); + Config::set('cms.enableCsrfProtection', false); + + $controller = new Auth; + Request::swap($this->configPostbackRequestMock('create_onSave')); + + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: create_onSave.'); + $controller->run('signin'); + } +} diff --git a/modules/backend/tests/controllers/MyAccountTest.php b/modules/backend/tests/controllers/MyAccountTest.php new file mode 100644 index 0000000000..cba45fc6fb --- /dev/null +++ b/modules/backend/tests/controllers/MyAccountTest.php @@ -0,0 +1,29 @@ +actingAs($user); + + $controller = new MyAccount; + $this->assertEmpty($controller->requiredPermissions); + } + + public function testUsersControllerNoLongerNullifiesPermissionsForMyaccount(): void + { + $user = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($user); + + $controller = new Users; + $this->assertEquals(['backend.manage_users'], $controller->requiredPermissions); + } +} diff --git a/modules/backend/tests/models/UserAuthorizationTest.php b/modules/backend/tests/models/UserAuthorizationTest.php new file mode 100644 index 0000000000..ab2927e99e --- /dev/null +++ b/modules/backend/tests/models/UserAuthorizationTest.php @@ -0,0 +1,356 @@ +targetUser = User::create([ + 'first_name' => 'Target', + 'last_name' => 'User', + 'login' => 'targetuser', + 'email' => 'target@test.com', + 'password' => 'TestPassword1', + 'password_confirmation' => 'TestPassword1', + 'is_activated' => true, + 'is_superuser' => false, + ]); + Model::reguard(); + } + + // + // canBeManagedByUser() + // + + public function testCanBeManagedByUserReturnsTrueForCli(): void + { + // No authenticated user (CLI context) + BackendAuth::logout(); + $this->assertTrue($this->targetUser->canBeManagedByUser()); + } + + public function testCanBeManagedByUserReturnsFalseWithoutPermission(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', false); + $this->assertFalse($this->targetUser->canBeManagedByUser($actor)); + } + + public function testCanBeManagedByUserReturnsTrueWithPermission(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->assertTrue($this->targetUser->canBeManagedByUser($actor)); + } + + public function testCanBeManagedByUserReturnsFalseForNonSuperuserTargetingSuperuser(): void + { + $this->targetUser->is_superuser = true; + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->assertFalse($this->targetUser->canBeManagedByUser($actor)); + } + + public function testCanBeManagedByUserReturnsTrueForSuperuserTargetingSuperuser(): void + { + $this->targetUser->is_superuser = true; + $actor = (new UserFixture)->asSuperUser()->withPermission('backend.manage_users', true); + $this->assertTrue($this->targetUser->canBeManagedByUser($actor)); + } + + public function testCanBeManagedByUserChecksPreviousSuperuserStatus(): void + { + // Simulate a record that WAS a superuser (getOriginal returns true) + $this->targetUser->syncOriginal(); + $this->targetUser->is_superuser = true; + $this->targetUser->syncOriginal(); + $this->targetUser->is_superuser = false; + + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + // getOriginal('is_superuser') is true, so non-superuser can't manage + $this->assertFalse($this->targetUser->canBeManagedByUser($actor)); + } + + public function testCanBeManagedByUserFallsBackToAuthenticatedUser(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + // No explicit user passed — falls back to authenticated user + $this->assertTrue($this->targetUser->canBeManagedByUser()); + } + + // + // beforeSave() + // + + public function testBeforeSaveAllowsCliContext(): void + { + BackendAuth::logout(); + $this->targetUser->first_name = 'Updated'; + $this->targetUser->save(); + $this->assertEquals('Updated', $this->targetUser->first_name); + } + + public function testBeforeSaveAllowsSelfNonEscalatingChanges(): void + { + $this->actingAs($this->targetUser); + $this->targetUser->first_name = 'NewName'; + $this->targetUser->save(); + $this->assertEquals('NewName', $this->targetUser->first_name); + } + + public function testBeforeSaveBlocksSelfEscalation(): void + { + $this->actingAs($this->targetUser); + $this->targetUser->is_superuser = true; + + $this->expectException(AuthorizationException::class); + $this->targetUser->save(); + } + + public function testBeforeSaveBlocksUnauthorizedUserModifyingOthers(): void + { + $actor = new UserFixture; + $this->actingAs($actor); + + $this->targetUser->first_name = 'Hacked'; + $this->expectException(AuthorizationException::class); + $this->targetUser->save(); + } + + public function testBeforeSaveAllowsManageUsersActorModifyingOthers(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $this->targetUser->first_name = 'AdminUpdated'; + $this->targetUser->save(); + $this->assertEquals('AdminUpdated', $this->targetUser->first_name); + } + + public function testBeforeSaveBlocksNonSuperuserModifyingSuperuser(): void + { + // Make the target a superuser via direct DB update to bypass model events + $this->targetUser->newQuery()->where('id', $this->targetUser->id)->update(['is_superuser' => true]); + + // Re-fetch the model fresh to avoid stale password hash triggering validation + $superTarget = User::find($this->targetUser->id); + + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $superTarget->first_name = 'Hacked'; + $this->expectException(AuthorizationException::class); + $superTarget->save(); + } + + public function testBeforeSaveSuperuserCanModifyAnyone(): void + { + $actor = (new UserFixture)->asSuperUser(); + $this->actingAs($actor); + + $this->targetUser->first_name = 'SuperUpdated'; + $this->targetUser->save(); + $this->assertEquals('SuperUpdated', $this->targetUser->first_name); + } + + // + // beforeDelete() + // + + public function testBeforeDeleteAllowsCliContext(): void + { + BackendAuth::logout(); + $this->targetUser->delete(); + $this->assertTrue($this->targetUser->trashed()); + } + + public function testBeforeDeleteBlocksUnauthorizedUser(): void + { + $actor = new UserFixture; + $this->actingAs($actor); + + $this->expectException(AuthorizationException::class); + $this->targetUser->delete(); + } + + public function testBeforeDeleteAllowsManageUsersActor(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $this->targetUser->delete(); + $this->assertTrue($this->targetUser->trashed()); + } + + public function testBeforeDeleteBlocksNonSuperuserDeletingSuperuser(): void + { + $this->targetUser->newQuery()->where('id', $this->targetUser->id)->update(['is_superuser' => true]); + $this->targetUser->refresh(); + + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $this->expectException(AuthorizationException::class); + $this->targetUser->delete(); + } + + public function testBeforeDeleteSuperuserCanDeleteAnyone(): void + { + $actor = (new UserFixture)->asSuperUser(); + $this->actingAs($actor); + + $this->targetUser->delete(); + $this->assertTrue($this->targetUser->trashed()); + } + + // + // beforeRestore() + // + + public function testBeforeRestoreAllowsCliContext(): void + { + BackendAuth::logout(); + $this->targetUser->delete(); + $this->targetUser->restore(); + $this->assertFalse($this->targetUser->trashed()); + } + + public function testBeforeRestoreBlocksUnauthorizedUser(): void + { + BackendAuth::logout(); + $this->targetUser->delete(); + + $actor = new UserFixture; + $this->actingAs($actor); + + $this->expectException(AuthorizationException::class); + $this->targetUser->restore(); + } + + public function testBeforeRestoreAllowsManageUsersActor(): void + { + BackendAuth::logout(); + $this->targetUser->delete(); + + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $this->targetUser->restore(); + $this->assertFalse($this->targetUser->trashed()); + } + + public function testBeforeRestoreBlocksNonSuperuserRestoringSuperuser(): void + { + $this->targetUser->newQuery()->where('id', $this->targetUser->id)->update(['is_superuser' => true]); + $this->targetUser->refresh(); + BackendAuth::logout(); + $this->targetUser->delete(); + + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $this->expectException(AuthorizationException::class); + $this->targetUser->restore(); + } + + public function testBeforeRestoreSuperuserCanRestoreAnyone(): void + { + BackendAuth::logout(); + $this->targetUser->delete(); + + $actor = (new UserFixture)->asSuperUser(); + $this->actingAs($actor); + + $this->targetUser->restore(); + $this->assertFalse($this->targetUser->trashed()); + } + + // + // unsuspend() + // + + public function testUnsuspendBlocksUnauthorizedUser(): void + { + $actor = new UserFixture; + $this->actingAs($actor); + + $this->expectException(AuthorizationException::class); + $this->targetUser->unsuspend(); + } + + public function testUnsuspendAllowsManageUsersActor(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + // Should not throw — the throttle record may not exist but that's fine + // for testing the authorization gate + try { + $this->targetUser->unsuspend(); + } catch (AuthorizationException $e) { + $this->fail('Should not throw AuthorizationException for manage_users actor'); + } + $this->assertTrue(true); + } + + public function testUnsuspendAllowsCliContext(): void + { + BackendAuth::logout(); + + try { + $this->targetUser->unsuspend(); + } catch (AuthorizationException $e) { + $this->fail('Should not throw AuthorizationException in CLI context'); + } + $this->assertTrue(true); + } + + // + // getResetPasswordCode() + // + + public function testGetResetPasswordCodeAllowsSelfService(): void + { + $this->actingAs($this->targetUser); + $code = $this->targetUser->getResetPasswordCode(); + $this->assertNotEmpty($code); + } + + public function testGetResetPasswordCodeBlocksUnauthorizedUserForOthers(): void + { + $actor = new UserFixture; + $this->actingAs($actor); + + $this->expectException(AuthorizationException::class); + $this->targetUser->getResetPasswordCode(); + } + + public function testGetResetPasswordCodeAllowsManageUsersActorForOthers(): void + { + $actor = (new UserFixture)->withPermission('backend.manage_users', true); + $this->actingAs($actor); + + $code = $this->targetUser->getResetPasswordCode(); + $this->assertNotEmpty($code); + } + + public function testGetResetPasswordCodeAllowsCliContext(): void + { + BackendAuth::logout(); + $code = $this->targetUser->getResetPasswordCode(); + $this->assertNotEmpty($code); + } +} diff --git a/modules/cms/classes/Controller.php b/modules/cms/classes/Controller.php index fa0d38f0fc..93cc9f790e 100644 --- a/modules/cms/classes/Controller.php +++ b/modules/cms/classes/Controller.php @@ -386,11 +386,16 @@ public function runPage($page, $useAjax = true) if ( $useAjax && ($handler = post('_handler')) && - $this->verifyCsrfToken() && - ($handlerResponse = $this->runAjaxHandler($handler)) && - $handlerResponse !== true + $this->verifyCsrfToken() ) { - return $handlerResponse; + $this->validateHandlerName($handler); + + if ( + ($handlerResponse = $this->runAjaxHandler($handler)) && + $handlerResponse !== true + ) { + return $handlerResponse; + } } /* @@ -681,6 +686,18 @@ public function getAjaxHandler() return null; } + /** + * Validates the AJAX handler name follows the expected format. + * + * @throws 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('cms::lang.ajax_handler.invalid_name', ['name' => $handler]), 400); + } + } + /** * Executes the page, layout, component and plugin AJAX handlers. * @@ -694,9 +711,7 @@ protected function execAjaxHandlers() /* * Validate the handler name */ - if (!preg_match('/^(?:\w+\:{2})?on[A-Z]{1}[\w+]*$/', $handler)) { - throw new SystemException(Lang::get('cms::lang.ajax_handler.invalid_name', ['name'=>$handler]), 400); - } + $this->validateHandlerName($handler); /* * Validate the handler partial list diff --git a/modules/cms/tests/classes/ControllerPostbackTest.php b/modules/cms/tests/classes/ControllerPostbackTest.php new file mode 100644 index 0000000000..8f18741908 --- /dev/null +++ b/modules/cms/tests/classes/ControllerPostbackTest.php @@ -0,0 +1,175 @@ +getMockBuilder('Illuminate\Http\Request') + ->disableOriginalConstructor() + ->setMethods(['ajax', 'method', 'header']) + ->getMock(); + + $map = [ + ['X_WINTER_REQUEST_HANDLER', null, $handler], + ['X_WINTER_REQUEST_PARTIALS', null, $partials], + ]; + + $requestMock->expects($this->any()) + ->method('ajax') + ->will($this->returnValue(true)); + + $requestMock->expects($this->any()) + ->method('method') + ->will($this->returnValue('POST')); + + $requestMock->expects($this->any()) + ->method('header') + ->will($this->returnValueMap($map)); + + return $requestMock; + } + + /** + * Builds a mock Request that simulates a non-AJAX POST with _handler in POST data. + */ + protected function configPostbackRequestMock(string $handler) + { + $requestMock = $this + ->getMockBuilder('Illuminate\Http\Request') + ->disableOriginalConstructor() + ->setMethods(['ajax', 'method', 'header', 'post', 'input']) + ->getMock(); + + $requestMock->expects($this->any()) + ->method('ajax') + ->will($this->returnValue(false)); + + $requestMock->expects($this->any()) + ->method('method') + ->will($this->returnValue('POST')); + + $requestMock->expects($this->any()) + ->method('header') + ->will($this->returnValue(null)); + + $postData = ['_handler' => $handler]; + $requestMock->expects($this->any()) + ->method('post') + ->will($this->returnCallback(function ($key = null, $default = null) use ($postData) { + if ($key === null) { + return $postData; + } + return $postData[$key] ?? $default; + })); + + $requestMock->expects($this->any()) + ->method('input') + ->will($this->returnCallback(function ($key = null, $default = null) use ($postData) { + if ($key === null) { + return $postData; + } + return $postData[$key] ?? $default; + })); + + return $requestMock; + } + + // + // AJAX header path — validates handler name (existing behavior) + // + + public function testAjaxPathRejectsInvalidHandlerName(): void + { + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: update_onDelete.'); + + Request::swap($this->configAjaxRequestMock('update_onDelete')); + + $theme = Theme::load('test'); + $controller = new Controller($theme); + $controller->run('/ajax-test'); + } + + public function testAjaxPathAcceptsValidHandlerName(): void + { + // onTest exists on the ajax-test page, so this should not throw SystemException for invalid name + // It may throw for other reasons (missing partials, etc.) but not for handler name validation + Request::swap($this->configAjaxRequestMock('onTest', '')); + + $theme = Theme::load('test'); + $controller = new Controller($theme); + + try { + $controller->run('/ajax-test'); + } catch (SystemException $e) { + $this->assertStringNotContainsString('Invalid AJAX handler name', $e->getMessage()); + } + $this->assertTrue(true); + } + + // + // Postback _handler path — validates handler name (our fix) + // + + public function testPostbackPathRejectsInvalidHandlerName(): void + { + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: update_onDelete.'); + + Config::set('cms.enableCsrfProtection', false); + Request::swap($this->configPostbackRequestMock('update_onDelete')); + + $theme = Theme::load('test'); + $controller = new Controller($theme); + $controller->run('/ajax-test'); + } + + public function testPostbackPathRejectsMethodName(): void + { + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: execPageCycle.'); + + Config::set('cms.enableCsrfProtection', false); + Request::swap($this->configPostbackRequestMock('execPageCycle')); + + $theme = Theme::load('test'); + $controller = new Controller($theme); + $controller->run('/ajax-test'); + } + + public function testPostbackPathRejectsActionPrefixedHandler(): void + { + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid AJAX handler name: index_onSave.'); + + Config::set('cms.enableCsrfProtection', false); + Request::swap($this->configPostbackRequestMock('index_onSave')); + + $theme = Theme::load('test'); + $controller = new Controller($theme); + $controller->run('/ajax-test'); + } +}