From 67c84840f8b7e784cff6afcd861a55ca15eebd0c Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 15:29:18 +0200 Subject: [PATCH 1/5] fix(manager): guard role, permission and category deletes and escape log filters Roles, permissions, permission groups and categories were deleted from GET parameters on actions that also render their edit pages, so none of them required a CSRF token. The middleware now verifies those actions when the triggering query parameter is present, and the delete links send the token. The manager log echoed the message filter raw into value="", and every filter reached the pagination links unescaped because Paginate urldecodes its extra arguments. Paginate now escapes them itself, and the stored item and user names in the filter dropdowns are escaped too. Co-Authored-By: Claude Opus 5.5 --- core/src/Middleware/VerifyCsrfToken.php | 23 ++++- core/src/Support/Paginate.php | 4 +- .../Unit/Security/ManagerCsrfCoverageTest.php | 67 ++++++++++++++ .../Security/ManagerCsrfMiddlewareTest.php | 41 ++++++++- .../Unit/Security/ManagerReflectedXssTest.php | 90 +++++++++++++++++++ .../actions/category_mgr/skin/edit.tpl.phtml | 2 +- manager/actions/logging.static.php | 23 +++-- .../user_roles/element_permission.blade.php | 2 +- .../page/user_roles/permission.blade.php | 2 +- .../user_roles/permissions_groups.blade.php | 2 +- .../views/page/user_roles/user_role.blade.php | 2 +- 11 files changed, 245 insertions(+), 13 deletions(-) diff --git a/core/src/Middleware/VerifyCsrfToken.php b/core/src/Middleware/VerifyCsrfToken.php index f46dc63ac6..f3a87265e7 100644 --- a/core/src/Middleware/VerifyCsrfToken.php +++ b/core/src/Middleware/VerifyCsrfToken.php @@ -64,6 +64,23 @@ class VerifyCsrfToken 501, // delete_category ]; + /** + * Manager actions that render an ordinary page on GET but mutate state once a particular + * query parameter is present. + * + * Listing these in MUTATING_GET_ACTIONS would demand a token for plain navigation to the + * edit forms, so only requests carrying the triggering parameter are verified. + */ + protected const MUTATING_GET_PARAMETERS = [ + 35 => 'action', // UserRole - ?action=delete removes the role + 36 => 'action', // UserRole + 38 => 'action', // UserRole + 120 => 'module_categories_manager', // category manager - [delete] removes a category + 121 => 'module_categories_manager', // category manager + 135 => 'action', // Permission - ?action=delete removes the permission + 136 => 'action', // PermissionsGroups - ?action=delete removes the group and its permissions + ]; + /** * @param \Illuminate\Http\Request $request * @param Closure $next @@ -98,9 +115,13 @@ protected function requiresVerification($request): bool return true; } + $action = $this->getActionId($request); + // Explicitly stale tokens must not be ignored even on read-only pages. return $request->input('_token', $request->header('X-CSRF-TOKEN')) !== null - || in_array($this->getActionId($request), self::MUTATING_GET_ACTIONS, true); + || in_array($action, self::MUTATING_GET_ACTIONS, true) + || (isset(self::MUTATING_GET_PARAMETERS[$action]) + && $request->query->has(self::MUTATING_GET_PARAMETERS[$action])); } /** diff --git a/core/src/Support/Paginate.php b/core/src/Support/Paginate.php index 44052b8a58..d32b873caf 100644 --- a/core/src/Support/Paginate.php +++ b/core/src/Support/Paginate.php @@ -75,7 +75,9 @@ public function __construct($int_nbr_row, $int_cur_position, $int_num_result, $s $this->int_nbr_row = $int_nbr_row; $this->int_num_result = $int_num_result; $this->int_cur_position = $int_cur_position; - $this->str_ext_argv = urldecode($str_ext_argv); + // Every link writes this into href="", and urldecode() undoes any encoding the caller + // applied, so it is escaped here rather than trusted to arrive safe. + $this->str_ext_argv = htmlspecialchars(urldecode((string)$str_ext_argv), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8', false); } /** diff --git a/core/tests/Unit/Security/ManagerCsrfCoverageTest.php b/core/tests/Unit/Security/ManagerCsrfCoverageTest.php index bdbbcf2c66..813e505117 100644 --- a/core/tests/Unit/Security/ManagerCsrfCoverageTest.php +++ b/core/tests/Unit/Security/ManagerCsrfCoverageTest.php @@ -216,6 +216,73 @@ function isLoginTemplate(string $path): bool expect($unguarded)->toBe([]); }); +it('guards the page actions that delete when a query parameter is present', function () { + // Role, permission and category pages are also plain edit screens, so their action ids + // cannot go into MUTATING_GET_ACTIONS; the triggering parameter is guarded instead. + $parameters = (new ReflectionClass(VerifyCsrfToken::class))->getConstant('MUTATING_GET_PARAMETERS'); + + $deleteBranches = [ + 'core/src/Controllers/UserRoles/UserRole.php' => [[35, 36, 38], 'action', "\$_GET['action'] == 'delete'"], + 'core/src/Controllers/UserRoles/Permission.php' => [[135], 'action', "\$_GET['action'] == 'delete'"], + 'core/src/Controllers/UserRoles/PermissionsGroups.php' => [[136], 'action', "\$_GET['action'] == 'delete'"], + 'manager/actions/category_mgr/inc/request_trigger.inc.php' => [ + [120, 121], + 'module_categories_manager', + "\$_GET[\$cm->get('request_key')]['delete']", + ], + ]; + + $unguarded = []; + + foreach ($deleteBranches as $relative => [$actions, $parameter, $trigger]) { + expect((string)file_get_contents(evoRoot() . '/' . $relative))->toContain($trigger); + + foreach ($actions as $action) { + if (($parameters[$action] ?? null) !== $parameter) { + $unguarded[] = $relative . ' (a=' . $action . ')'; + } + } + } + + $categories = (string)file_get_contents(evoRoot() . '/manager/actions/mutate_categories.dynamic.php'); + expect($categories)->toContain("'request_key' => 'module_categories_manager'"); + + expect($unguarded)->toBe([]); +}); + +it('appends a token to every role, permission and category delete link', function () { + $patterns = [ + '/\ba=(35|36|38|135|136)&action=delete/', + "/makeUrl\('actions\.delete'\) }}&action=delete/", + "/request_key'\); \?>\[delete\]=/", + ]; + + $found = 0; + $untokenised = []; + + foreach (managerTemplateFiles() as $path) { + $lines = explode("\n", (string)file_get_contents($path)); + + foreach ($lines as $index => $line) { + foreach ($patterns as $pattern) { + if (!preg_match($pattern, $line)) { + continue; + } + + $found++; + + if (!str_contains($line, '_token')) { + $untokenised[] = str_replace(evoRoot() . '/', '', $path) . ':' . ($index + 1); + } + } + } + } + + // user_role, permission, permissions_groups, element_permission and the category editor. + expect($found)->toBeGreaterThanOrEqual(5) + ->and($untokenised)->toBe([]); +}); + it('guards the disable toggle branch of the element save processors', function () { // Each save_* processor acts on ?disabled= before it ever looks at the POST body. $toggles = [ diff --git a/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php b/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php index 964ff402bc..0c1eca77f7 100644 --- a/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php +++ b/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php @@ -101,7 +101,7 @@ function csrfPassed(Request $request): bool $request = csrfRequest('POST', [ 'a' => 109, 'mode' => 'new', - 'post' => '', + 'post' => '', ]); expect(csrfPassed($request))->toBeFalse(); @@ -149,6 +149,45 @@ function csrfPassed(Request $request): bool 303, // delete_tmplvars ]); +it('rejects page actions whose delete parameter is sent without a token', function (array $params) { + // Roles, permissions and categories are deleted by the same actions that render their + // edit pages, so a single query parameter decides whether the request is destructive. + expect(csrfPassed(csrfRequest('GET', $params)))->toBeFalse(); +})->with([ + 'role (a=35)' => [['a' => 35, 'id' => 3, 'action' => 'delete']], + 'role (a=36)' => [['a' => 36, 'id' => 3, 'action' => 'delete']], + 'role (a=38)' => [['a' => 38, 'id' => 3, 'action' => 'delete']], + 'permission (a=135)' => [['a' => 135, 'id' => 3, 'action' => 'delete']], + 'permission group (a=136)' => [['a' => 136, 'id' => 3, 'action' => 'delete']], + 'category (a=120)' => [['a' => 120, 'module_categories_manager' => ['delete' => 3, 'category' => 'x']]], + 'category (a=121)' => [['a' => 121, 'module_categories_manager' => ['delete' => 3]]], +]); + +it('accepts page action deletes carrying the session token', function (array $params) { + $params['_token'] = str_repeat('a', 40); + + expect(csrfPassed(csrfRequest('GET', $params)))->toBeTrue(); +})->with([ + 'role' => [['a' => 35, 'id' => 3, 'action' => 'delete']], + 'permission' => [['a' => 135, 'id' => 3, 'action' => 'delete']], + 'category' => [['a' => 120, 'module_categories_manager' => ['delete' => 3]]], +]); + +it('keeps the role, permission and category pages reachable without a token', function (array $params) { + expect(csrfPassed(csrfRequest('GET', $params)))->toBeTrue(); +})->with([ + 'edit role' => [['a' => 35, 'id' => 3]], + 'new role' => [['a' => 38]], + 'edit permission' => [['a' => 135, 'id' => 3]], + 'edit permission group' => [['a' => 136, 'id' => 3]], + 'category manager' => [['a' => 120]], +]); + +it('does not treat the delete parameter as destructive on unrelated pages', function () { + // Only the role, permission and category actions act on ?action=delete. + expect(csrfPassed(csrfRequest('GET', ['a' => 27, 'id' => 3, 'action' => 'delete'])))->toBeTrue(); +}); + it('accepts destructive GET actions carrying the session token', function () { $request = csrfRequest('GET', ['a' => 112, 'id' => 3, '_token' => str_repeat('a', 40)]); diff --git a/core/tests/Unit/Security/ManagerReflectedXssTest.php b/core/tests/Unit/Security/ManagerReflectedXssTest.php index b7e9397943..d7d824528e 100644 --- a/core/tests/Unit/Security/ManagerReflectedXssTest.php +++ b/core/tests/Unit/Security/ManagerReflectedXssTest.php @@ -131,3 +131,93 @@ function managerSource(string $relative): string ->not->toContain("' ' . \$e->getMessage() . ''") ->not->toContain("' ' . print_r(\$result->errorInfo(), true) . ''"); }); + +it('escapes the manager log filters echoed back into the search form', function () { + // ?message= was written raw into value="", and the sanitizer only rewrites "toContain("value=\"not->toContain("value=\"\""); + + // No request value may be echoed without going through entities(). + preg_match_all('/<\?=((?:(?!\?>).)*\$_REQUEST(?:(?!\?>).)*)\?>/s', $source, $matches); + + $unescaped = array_values(array_filter( + $matches[1], + static fn ($expr) => !str_contains($expr, 'entities(') && !str_contains($expr, '(int)') + )); + + expect($matches[1])->not->toBeEmpty() + ->and($unescaped)->toBe([]); +}); + +it('encodes the manager log filters carried into the pagination links', function () { + // Every filter used to be concatenated raw into $extargv, which Paginate writes into href="". + $source = managerSource('manager/actions/logging.static.php'); + + expect($source) + ->toContain("\$extargv = '&' . str_replace('%', '%25', http_build_query([") + ->toContain("], '', '&', PHP_QUERY_RFC3986));") + ->not->toContain("\"&message=\" . get_by_key(\$_REQUEST, 'message')") + ->not->toContain("\"&dateto=\" . \$_REQUEST['dateto']"); +}); + +it('escapes stored log values listed in the manager log filter dropdowns', function () { + // Item names are document titles and element names, set by lower privileged editors. + $source = managerSource('manager/actions/logging.static.php'); + + expect($source) + ->not->toContain("'>' . \$row['username'] . \"") + ->not->toContain("'>' . \$row['itemname'] . \"") + ->not->toContain("'\n"; + echo "\t\t" . '\n"; } ?> @@ -66,7 +66,7 @@ $logs_items = record_sort(array_unique_multi($logs, 'itemid'), 'itemid'); foreach ($logs_items as $row) { $selectedtext = $row['itemid'] == get_by_key($_REQUEST, 'itemid') ? ' selected="selected"' : ''; - echo "\t\t" . '\n"; + echo "\t\t" . '\n"; } ?> @@ -82,7 +82,7 @@ $logs_names = record_sort(array_unique_multi($logs, 'itemname'), 'itemname'); foreach ($logs_names as $row) { $selectedtext = $row['itemname'] == get_by_key($_REQUEST, 'itemname') ? ' selected="selected"' : ''; - echo "\t\t" . '\n"; + echo "\t\t" . '\n"; } ?> @@ -92,7 +92,7 @@
+ value="getConfig('modx_charset')) ?>"/>
@@ -187,7 +187,20 @@ class=""> // Number of result to display on the page, will be in the LIMIT of the sql query also $int_num_result = is_numeric($_REQUEST['nrresults']) ? $_REQUEST['nrresults'] : EvolutionCMS()->getConfig('number_of_logs'); - $extargv = "&a=13&searchuser=" . get_by_key($_REQUEST, 'searchuser') . "&action=" . get_by_key($_REQUEST, 'action') . "&itemid=" . get_by_key($_REQUEST, 'itemid') . "&itemname=" . get_by_key($_REQUEST, 'itemname') . "&message=" . get_by_key($_REQUEST, 'message') . "&dateto=" . $_REQUEST['dateto'] . "&datefrom=" . $_REQUEST['datefrom'] . "&nrresults=" . $int_num_result . "&log_submit=" . $_REQUEST['log_submit']; // extra argv here (could be anything depending on your page) + // Paginate urldecodes this before escaping it into href="", so '%' is encoded once more to + // keep filter values that contain '&' or '=' intact. + $extargv = '&' . str_replace('%', '%25', http_build_query([ + 'a' => 13, + 'searchuser' => (string)get_by_key($_REQUEST, 'searchuser', '', 'is_scalar'), + 'action' => (string)get_by_key($_REQUEST, 'action', '', 'is_scalar'), + 'itemid' => (string)get_by_key($_REQUEST, 'itemid', '', 'is_scalar'), + 'itemname' => (string)get_by_key($_REQUEST, 'itemname', '', 'is_scalar'), + 'message' => (string)get_by_key($_REQUEST, 'message', '', 'is_scalar'), + 'dateto' => (string)get_by_key($_REQUEST, 'dateto', '', 'is_scalar'), + 'datefrom' => (string)get_by_key($_REQUEST, 'datefrom', '', 'is_scalar'), + 'nrresults' => (int)$int_num_result, + 'log_submit' => (string)get_by_key($_REQUEST, 'log_submit', '', 'is_scalar'), + ], '', '&', PHP_QUERY_RFC3986)); // build the sql $limit = $num_rows = $logs->count(); diff --git a/manager/views/page/user_roles/element_permission.blade.php b/manager/views/page/user_roles/element_permission.blade.php index a91633f323..388f687d7c 100644 --- a/manager/views/page/user_roles/element_permission.blade.php +++ b/manager/views/page/user_roles/element_permission.blade.php @@ -37,7 +37,7 @@
  • - +
    • diff --git a/manager/views/page/user_roles/permission.blade.php b/manager/views/page/user_roles/permission.blade.php index 40f86e9168..9b8c18758e 100644 --- a/manager/views/page/user_roles/permission.blade.php +++ b/manager/views/page/user_roles/permission.blade.php @@ -98,7 +98,7 @@ function changestate(element) { }, delete: function () { if (confirm("{{ ManagerTheme::getLexicon('confirm_delete_permission') }}") === true) { - document.location.href = "index.php?id=" + document.userform.id.value + "&a=135&action=delete"; + document.location.href = "index.php?id=" + document.userform.id.value + "&a=135&action=delete&_token={{ csrf_token() }}"; } }, cancel: function () { diff --git a/manager/views/page/user_roles/permissions_groups.blade.php b/manager/views/page/user_roles/permissions_groups.blade.php index 57f09c3ccf..7b34433401 100644 --- a/manager/views/page/user_roles/permissions_groups.blade.php +++ b/manager/views/page/user_roles/permissions_groups.blade.php @@ -58,7 +58,7 @@ function changestate(element) { }, delete: function () { if (confirm("{{ ManagerTheme::getLexicon('confirm_delete_category') }}") === true) { - document.location.href = "index.php?id=" + document.userform.id.value + "&a=136&action=delete"; + document.location.href = "index.php?id=" + document.userform.id.value + "&a=136&action=delete&_token={{ csrf_token() }}"; } }, cancel: function () { diff --git a/manager/views/page/user_roles/user_role.blade.php b/manager/views/page/user_roles/user_role.blade.php index 599c239be1..be8553d4af 100644 --- a/manager/views/page/user_roles/user_role.blade.php +++ b/manager/views/page/user_roles/user_role.blade.php @@ -201,7 +201,7 @@ function changestate(element) { }, delete: function () { if (confirm("{{ ManagerTheme::getLexicon('confirm_delete_role') }}") === true) { - document.location.href = "index.php?id=" + document.userform.id.value + "&a=35&action=delete"; + document.location.href = "index.php?id=" + document.userform.id.value + "&a=35&action=delete&_token={{ csrf_token() }}"; } }, cancel: function () { From dec8ea78260b5ed0d47175eb0a7ccb0040b2f334 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 15:29:18 +0200 Subject: [PATCH 2/5] upd(tests): replace antivirus-flagged payloads in security tests Octal-obfuscated function calls, a cookie-exfiltration script and an rm -rf string match antivirus signatures. The arithmetic payloads are now built at runtime from harmless function names, and the others use inert markers. Also repairs the stored error report payload, whose unescaped quotes had turned most of it into a PHP comment. Co-Authored-By: Claude Opus 5.5 --- .../Unit/Security/BackupManagerShellTest.php | 2 +- .../Unit/Security/BladeStoredXssTest.php | 6 +++--- core/tests/Unit/Security/SafeHtmlTest.php | 4 ++-- .../Unit/Support/ArithmeticExpressionTest.php | 21 +++++++++++++------ 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/core/tests/Unit/Security/BackupManagerShellTest.php b/core/tests/Unit/Security/BackupManagerShellTest.php index c4ca9a4d39..0fd81ab766 100644 --- a/core/tests/Unit/Security/BackupManagerShellTest.php +++ b/core/tests/Unit/Security/BackupManagerShellTest.php @@ -54,7 +54,7 @@ public function inspectProcess($binary, array $arguments) }); test('an argument carrying shell metacharacters stays a single argument', function () { - $table = 'evo_users; rm -rf /'; + $table = 'evo_users; echo pwned'; $line = (new InspectableBackupService())->inspectProcess('pg_dump', ['--table', $table]) ->getCommandLine(); diff --git a/core/tests/Unit/Security/BladeStoredXssTest.php b/core/tests/Unit/Security/BladeStoredXssTest.php index bb51aaeda1..1523d71e3c 100644 --- a/core/tests/Unit/Security/BladeStoredXssTest.php +++ b/core/tests/Unit/Security/BladeStoredXssTest.php @@ -52,7 +52,7 @@ function renderBladeFragment(string $template, array $data = []): string */ function storedXssPayload(): string { - return 'Import failed
    bad value:
    '; + return 'Import failed
    bad value:
    '; } test('the old raw echo really did execute a stored payload', function () { @@ -61,7 +61,7 @@ function storedXssPayload(): string $output = renderBladeFragment('{!! $description !!}', ['description' => storedXssPayload()]); expect($output)->toContain(''; + . '
    row 12:
    '; $rendered = (string) safe_html($stored); @@ -30,7 +30,7 @@ // ...but the injected element is inert text, not an element. expect($rendered)->not->toContain('') - ->and($rendered)->toContain('<script>alert(document.cookie)</script>'); + ->and($rendered)->toContain('<script>alert(document.domain)</script>'); }); test('attributes cannot survive the allow list', function () { diff --git a/core/tests/Unit/Support/ArithmeticExpressionTest.php b/core/tests/Unit/Support/ArithmeticExpressionTest.php index 9eda282bed..f892f35972 100644 --- a/core/tests/Unit/Support/ArithmeticExpressionTest.php +++ b/core/tests/Unit/Support/ArithmeticExpressionTest.php @@ -86,15 +86,24 @@ }); }); +/** + * Spells a string as PHP octal escapes, so a payload holds no letters for the callers' filter to + * strip. Built at runtime to keep obfuscated call literals - an antivirus signature - out of the file. + */ +function octalEscape(string $value): string +{ + return implode('', array_map(static fn (string $char) => '\\' . decoct(ord($char)), str_split($value))); +} + describe('rejects everything that is not arithmetic', function () { // Every payload below survives `preg_replace('@([a-zA-Z\n\r\t\s])@', '', $filter)` - the filter // the callers apply before handing the string over - because it contains no letters at all. $payloads = [ - 'octal escaped system() call' => '"\163\171\163\164\145\155"("\151\144")', - 'octal escaped phpinfo' => '"\160\150\160\151\156\146\157"()', - 'backtick shell operator' => '1 . `\151\144`', - 'variable variable' => '${"\137\107\105\124"}', + 'octal escaped call with an argument' => '"' . octalEscape('strrev') . '"("' . octalEscape('x') . '")', + 'octal escaped call without arguments' => '"' . octalEscape('phpversion') . '"()', + 'backtick shell operator' => '1 . `' . octalEscape('true') . '`', + 'variable variable' => '${"' . octalEscape('_GET') . '"}', 'statement separator' => '1;print_r($_SERVER)', 'superglobal read' => '$_SERVER', 'string concatenation' => '"1"."2"', @@ -125,9 +134,9 @@ $marker = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'evo_arith_' . bin2hex(random_bytes(6)); // file_put_contents("", "x") spelled without a single letter. - $call = '"\146\151\154\145\137\160\165\164\137\143\157\156\164\145\156\164\163"("' + $call = '"' . octalEscape('file_put_contents') . '"("' . addcslashes($marker, "\\\"") - . '","\170")'; + . '","' . octalEscape('x') . '")'; ArithmeticExpression::evaluate($call); From ab52b94d05b9bf43c5d17df77857efd6e0873d2a Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 15:41:02 +0200 Subject: [PATCH 3/5] fix(manager): require a CSRF token for the OPcache reset SystemInfo (a=53) resets OPcache whenever opcache_reset is in the request, and neither the action nor its reset link carried a token. The middleware now verifies a=53 when the parameter is present, and the link sends the token. Co-Authored-By: Claude Opus 5.5 --- core/src/Controllers/SystemInfo.php | 2 +- core/src/Middleware/VerifyCsrfToken.php | 1 + .../Unit/Security/ManagerCsrfCoverageTest.php | 14 ++++++++++++++ .../Unit/Security/ManagerCsrfMiddlewareTest.php | 3 +++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/core/src/Controllers/SystemInfo.php b/core/src/Controllers/SystemInfo.php index 660dcae672..18b8e606cc 100644 --- a/core/src/Controllers/SystemInfo.php +++ b/core/src/Controllers/SystemInfo.php @@ -194,7 +194,7 @@ protected function opcacheStatus(): string $label = $this->managerTheme->getLexicon('enabled'); if (function_exists('opcache_reset') && $this->canUseOpcacheApi()) { - $label = '' . $label . ''; + $label = '' . $label . ''; } $details = sprintf($this->managerTheme->getLexicon('opcache_memory_details'), $this->formatBytes($used), diff --git a/core/src/Middleware/VerifyCsrfToken.php b/core/src/Middleware/VerifyCsrfToken.php index f3a87265e7..4a910a48b7 100644 --- a/core/src/Middleware/VerifyCsrfToken.php +++ b/core/src/Middleware/VerifyCsrfToken.php @@ -75,6 +75,7 @@ class VerifyCsrfToken 35 => 'action', // UserRole - ?action=delete removes the role 36 => 'action', // UserRole 38 => 'action', // UserRole + 53 => 'opcache_reset', // SystemInfo - ?opcache_reset=1 resets OPcache 120 => 'module_categories_manager', // category manager - [delete] removes a category 121 => 'module_categories_manager', // category manager 135 => 'action', // Permission - ?action=delete removes the permission diff --git a/core/tests/Unit/Security/ManagerCsrfCoverageTest.php b/core/tests/Unit/Security/ManagerCsrfCoverageTest.php index 813e505117..2f1db6c333 100644 --- a/core/tests/Unit/Security/ManagerCsrfCoverageTest.php +++ b/core/tests/Unit/Security/ManagerCsrfCoverageTest.php @@ -225,6 +225,7 @@ function isLoginTemplate(string $path): bool 'core/src/Controllers/UserRoles/UserRole.php' => [[35, 36, 38], 'action', "\$_GET['action'] == 'delete'"], 'core/src/Controllers/UserRoles/Permission.php' => [[135], 'action', "\$_GET['action'] == 'delete'"], 'core/src/Controllers/UserRoles/PermissionsGroups.php' => [[136], 'action', "\$_GET['action'] == 'delete'"], + 'core/src/Controllers/SystemInfo.php' => [[53], 'opcache_reset', "request()->boolean('opcache_reset')"], 'manager/actions/category_mgr/inc/request_trigger.inc.php' => [ [120, 121], 'module_categories_manager', @@ -250,6 +251,19 @@ function isLoginTemplate(string $path): bool expect($unguarded)->toBe([]); }); +it('appends a token to the OPcache reset link', function () { + // The link is built in the controller, so the template scan below does not see it. + $source = (string)file_get_contents(evoRoot() . '/core/src/Controllers/SystemInfo.php'); + + preg_match_all('/^.*opcache_reset=1.*$/m', $source, $links); + + expect($links[0])->not->toBeEmpty(); + + foreach ($links[0] as $link) { + expect($link)->toContain('_token='); + } +}); + it('appends a token to every role, permission and category delete link', function () { $patterns = [ '/\ba=(35|36|38|135|136)&action=delete/', diff --git a/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php b/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php index 0c1eca77f7..765b27e576 100644 --- a/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php +++ b/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php @@ -157,6 +157,7 @@ function csrfPassed(Request $request): bool 'role (a=35)' => [['a' => 35, 'id' => 3, 'action' => 'delete']], 'role (a=36)' => [['a' => 36, 'id' => 3, 'action' => 'delete']], 'role (a=38)' => [['a' => 38, 'id' => 3, 'action' => 'delete']], + 'opcache reset (a=53)' => [['a' => 53, 'opcache_reset' => 1]], 'permission (a=135)' => [['a' => 135, 'id' => 3, 'action' => 'delete']], 'permission group (a=136)' => [['a' => 136, 'id' => 3, 'action' => 'delete']], 'category (a=120)' => [['a' => 120, 'module_categories_manager' => ['delete' => 3, 'category' => 'x']]], @@ -169,6 +170,7 @@ function csrfPassed(Request $request): bool expect(csrfPassed(csrfRequest('GET', $params)))->toBeTrue(); })->with([ 'role' => [['a' => 35, 'id' => 3, 'action' => 'delete']], + 'opcache reset' => [['a' => 53, 'opcache_reset' => 1]], 'permission' => [['a' => 135, 'id' => 3, 'action' => 'delete']], 'category' => [['a' => 120, 'module_categories_manager' => ['delete' => 3]]], ]); @@ -178,6 +180,7 @@ function csrfPassed(Request $request): bool })->with([ 'edit role' => [['a' => 35, 'id' => 3]], 'new role' => [['a' => 38]], + 'system info' => [['a' => 53]], 'edit permission' => [['a' => 135, 'id' => 3]], 'edit permission group' => [['a' => 136, 'id' => 3]], 'category manager' => [['a' => 120]], From 0adb94d89995f6e35c90e99eeb6b0c9152c0f206 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 16:09:58 +0200 Subject: [PATCH 4/5] fix(manager): require CSRF tokens for logout and module dependency changes Module dependencies (a=113) are added and removed from $_REQUEST['op'], so a plain GET changed them. The middleware now verifies a=113 when op is present. Logout (a=8) destroyed the manager session on GET without a token. It is now a guarded action, and the logout links in the frame, the welcome page, the lockout placeholder and the keepalive redirect carry the token. Co-Authored-By: Claude Opus 5.5 --- core/src/ManagerTheme.php | 3 ++- core/src/Middleware/VerifyCsrfToken.php | 2 ++ core/tests/Unit/Security/ManagerCsrfCoverageTest.php | 12 ++++++++++++ .../Unit/Security/ManagerCsrfMiddlewareTest.php | 4 ++++ manager/media/style/default/js/evo.js | 2 +- manager/views/frame/1.blade.php | 2 +- manager/views/page/2.blade.php | 2 +- 7 files changed, 23 insertions(+), 4 deletions(-) diff --git a/core/src/ManagerTheme.php b/core/src/ManagerTheme.php index ec759f4048..8aa184097f 100644 --- a/core/src/ManagerTheme.php +++ b/core/src/ManagerTheme.php @@ -717,7 +717,8 @@ public function getTemplatePlaceholders(): array 'evo_charset' => $this->getCharset(), 'favicon' => (file_exists(EVO_BASE_PATH . 'favicon.ico') ? EVO_SITE_URL : $this->getThemeUrl() . 'images/') . 'favicon.ico', 'homeurl' => $this->getCore()->makeUrl($this->getManagerStartupPageId()), - 'logouturl' => EVO_MANAGER_URL . 'index.php?a=8', + // Logout is CSRF-verified; csrf_token() throws when no session has been started. + 'logouturl' => EVO_MANAGER_URL . 'index.php?a=8' . (isset($_SESSION) ? '&_token=' . rawurlencode(csrf_token()) : ''), 'year' => date('Y'), 'theme' => $this->getTheme(), 'manager_theme_url' => $this->getThemeUrl(), diff --git a/core/src/Middleware/VerifyCsrfToken.php b/core/src/Middleware/VerifyCsrfToken.php index 4a910a48b7..5bbd8e4b3e 100644 --- a/core/src/Middleware/VerifyCsrfToken.php +++ b/core/src/Middleware/VerifyCsrfToken.php @@ -30,6 +30,7 @@ class VerifyCsrfToken */ protected const MUTATING_GET_ACTIONS = [ 6, // delete_content + 8, // LogInOut - logout destroys the manager session 21, // delete_template 24, // save_snippet (?disabled= toggle) 25, // delete_snippet @@ -76,6 +77,7 @@ class VerifyCsrfToken 36 => 'action', // UserRole 38 => 'action', // UserRole 53 => 'opcache_reset', // SystemInfo - ?opcache_reset=1 resets OPcache + 113 => 'op', // module dependencies - ?op=add|del changes them from $_REQUEST 120 => 'module_categories_manager', // category manager - [delete] removes a category 121 => 'module_categories_manager', // category manager 135 => 'action', // Permission - ?action=delete removes the permission diff --git a/core/tests/Unit/Security/ManagerCsrfCoverageTest.php b/core/tests/Unit/Security/ManagerCsrfCoverageTest.php index 2f1db6c333..d054c2a37d 100644 --- a/core/tests/Unit/Security/ManagerCsrfCoverageTest.php +++ b/core/tests/Unit/Security/ManagerCsrfCoverageTest.php @@ -189,6 +189,7 @@ function isLoginTemplate(string $path): bool $guarded = (new ReflectionClass(VerifyCsrfToken::class))->getConstant('MUTATING_GET_ACTIONS'); $mutatingControllers = [ + 8 => 'Users/LogInOut', // logout destroys the manager session 26 => 'RefreshSite', // publishes/unpublishes pending documents, clears the cache 52 => 'MoveDocument', // reparents a document from $_REQUEST 90 => 'Users/DeleteUser', // deletes a manager user from $_GET @@ -226,6 +227,7 @@ function isLoginTemplate(string $path): bool 'core/src/Controllers/UserRoles/Permission.php' => [[135], 'action', "\$_GET['action'] == 'delete'"], 'core/src/Controllers/UserRoles/PermissionsGroups.php' => [[136], 'action', "\$_GET['action'] == 'delete'"], 'core/src/Controllers/SystemInfo.php' => [[53], 'opcache_reset', "request()->boolean('opcache_reset')"], + 'manager/actions/mutate_module_resources.dynamic.php' => [[113], 'op', "switch (\$_REQUEST['op'])"], 'manager/actions/category_mgr/inc/request_trigger.inc.php' => [ [120, 121], 'module_categories_manager', @@ -264,6 +266,16 @@ function isLoginTemplate(string $path): bool } }); +it('appends a token to the logout links built outside the templates', function () { + // The template scan covers the Blade links; these are assembled in PHP and in the theme script. + $theme = (string)file_get_contents(evoRoot() . '/core/src/ManagerTheme.php'); + $script = (string)file_get_contents(evoRoot() . '/manager/media/style/default/js/evo.js'); + + expect($theme)->toContain("'logouturl' => EVO_MANAGER_URL . 'index.php?a=8' . (isset(\$_SESSION) ? '&_token=' . rawurlencode(csrf_token()) : '')") + ->and($script)->toContain("'?a=8&_token=' + encodeURIComponent(evo.tabsCsrfToken())") + ->and($script)->not->toMatch("/\?a=8';/"); +}); + it('appends a token to every role, permission and category delete link', function () { $patterns = [ '/\ba=(35|36|38|135|136)&action=delete/', diff --git a/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php b/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php index 765b27e576..2bb649bbac 100644 --- a/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php +++ b/core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php @@ -141,6 +141,7 @@ function csrfPassed(Request $request): bool expect(csrfPassed($request))->toBeFalse(); })->with([ 6, // delete_content + 8, // logout - destroys the manager session 61, // publish_content 94, // duplicate_content 110, // delete_module @@ -158,6 +159,8 @@ function csrfPassed(Request $request): bool 'role (a=36)' => [['a' => 36, 'id' => 3, 'action' => 'delete']], 'role (a=38)' => [['a' => 38, 'id' => 3, 'action' => 'delete']], 'opcache reset (a=53)' => [['a' => 53, 'opcache_reset' => 1]], + 'module dependency add (a=113)' => [['a' => 113, 'id' => 3, 'op' => 'add', 'newids' => '5', 'rt' => 'snip']], + 'module dependency delete (a=113)' => [['a' => 113, 'id' => 3, 'op' => 'del', 'depid' => [5]]], 'permission (a=135)' => [['a' => 135, 'id' => 3, 'action' => 'delete']], 'permission group (a=136)' => [['a' => 136, 'id' => 3, 'action' => 'delete']], 'category (a=120)' => [['a' => 120, 'module_categories_manager' => ['delete' => 3, 'category' => 'x']]], @@ -181,6 +184,7 @@ function csrfPassed(Request $request): bool 'edit role' => [['a' => 35, 'id' => 3]], 'new role' => [['a' => 38]], 'system info' => [['a' => 53]], + 'module dependencies' => [['a' => 113, 'id' => 3]], 'edit permission' => [['a' => 135, 'id' => 3]], 'edit permission group' => [['a' => 136, 'id' => 3]], 'category manager' => [['a' => 120]], diff --git a/manager/media/style/default/js/evo.js b/manager/media/style/default/js/evo.js index 7e03f98c99..669f901fde 100644 --- a/manager/media/style/default/js/evo.js +++ b/manager/media/style/default/js/evo.js @@ -1881,7 +1881,7 @@ keepMeAlive: function () { evo.get('includes/session_keepalive.php?tok=' + d.getElementById('sessTokenInput').value + '&o=' + Math.random(), function (r) { r = JSON.parse(r); - if (r.status !== 'ok') w.location.href = evo.EVO_MANAGER_URL + '?a=8'; + if (r.status !== 'ok') w.location.href = evo.EVO_MANAGER_URL + '?a=8&_token=' + encodeURIComponent(evo.tabsCsrfToken()); }); }, openWindow: function (a) { diff --git a/manager/views/frame/1.blade.php b/manager/views/frame/1.blade.php index 6ebba7a1f0..4a748205ed 100644 --- a/manager/views/frame/1.blade.php +++ b/manager/views/frame/1.blade.php @@ -314,7 +314,7 @@ function jsIconMarkup($icon) { @endif
  • - + {{ icon_html($_style['icon_logout']) }} {{ManagerTheme::getLexicon('logout')}}
    • diff --git a/manager/views/page/2.blade.php b/manager/views/page/2.blade.php index d34fdc4a90..76ace9dddb 100644 --- a/manager/views/page/2.blade.php +++ b/manager/views/page/2.blade.php @@ -462,7 +462,7 @@ : '') . ' - + ' . $_style['icon_logout'] . ' From 52586364860f1c66dfcfe895e1c042958c2f9a0f Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 16:09:58 +0200 Subject: [PATCH 5/5] fix(manager): escape web user, page title and category manager output - category manager: cast ?id= before it is written into every form action - web user list: escape the reflected search box and the stored username, full name, email and role names - web user editor: escape the username shown in the header - menu index sort and the template/TV delete screens: escape page titles and sanitise summaries while keeping their inline formatting Escaping never double-encodes, so values stored entity-encoded by older installations still render as the same text. Co-Authored-By: Claude Opus 5.5 --- .../Unit/Security/ManagerReflectedXssTest.php | 102 ++++++++++++++++++ manager/actions/mutate_categories.dynamic.php | 4 +- .../actions/mutate_menuindex_sort.dynamic.php | 2 +- manager/actions/mutate_web_user.dynamic.php | 2 +- .../actions/web_user_management.static.php | 14 +-- .../processors/delete_template.processor.php | 2 +- .../processors/delete_tmplvars.processor.php | 2 +- 7 files changed, 115 insertions(+), 13 deletions(-) diff --git a/core/tests/Unit/Security/ManagerReflectedXssTest.php b/core/tests/Unit/Security/ManagerReflectedXssTest.php index d7d824528e..fdef55a81b 100644 --- a/core/tests/Unit/Security/ManagerReflectedXssTest.php +++ b/core/tests/Unit/Security/ManagerReflectedXssTest.php @@ -221,3 +221,105 @@ function paginateLinks(string $extargv): array 'itemname' => 'Home page', ]); }); + +it('casts the category manager id before building its form url', function () { + // ?id= was written into every form action="" and link of the category manager. + $source = managerSource('manager/actions/mutate_categories.dynamic.php'); + + expect($source) + ->toContain("'index.php?a=120&id=' . (int)get_by_key(\$_GET, 'id', 0, 'is_scalar')") + ->toContain("'module_id' => (int)get_by_key(\$_GET, 'id', 0, 'is_scalar')") + ->not->toContain("'index.php?a=120&id=' . get_by_key(\$_GET, 'id', 0)"); +}); + +it('escapes the web user list, including its reflected search box', function () { + // Web users can register themselves, so these fields are chosen by anonymous visitors. + $source = managerSource('manager/actions/web_user_management.static.php'); + + expect($source) + ->not->toContain('value=""') + ->not->toContain("'\">' . \$el['username'] . ''") + ->not->toContain("'user_full_name' => \$el['fullname'],") + ->not->toContain("'email' => \$el['email'],") + ->not->toContain("'>'.\$row['name'].''") + ->toContain("htmlspecialchars((string)\$query['search'], ENT_QUOTES, ManagerTheme::getCharset(), false)") + ->toContain("htmlspecialchars((string)\$el['username'], ENT_QUOTES, ManagerTheme::getCharset(), false)") + ->toContain("htmlspecialchars((string)\$el['fullname'], ENT_QUOTES, ManagerTheme::getCharset(), false)") + ->toContain("htmlspecialchars((string)\$el['email'], ENT_QUOTES, ManagerTheme::getCharset(), false)"); +}); + +it('escapes the username in the web user editor header', function () { + // The stored name is html_entity_decode()d first, so an encoded payload came back live. + $source = managerSource('manager/actions/mutate_web_user.dynamic.php'); + + expect($source) + ->toContain("entities(\$usernamedata['username'], \$modx->getConfig('modx_charset')) . (isset(\$usernamedata['id'])") + ->not->toContain("not->toContain($raw); +})->with([ + 'menu index sort' => ['manager/actions/mutate_menuindex_sort.dynamic.php', "\$icon . \$row['pagetitle'] ."], + 'template in use' => ['manager/processors/delete_template.processor.php', "'&a=27\">' . \$row->pagetitle ."], + 'template in use intro' => ['manager/processors/delete_template.processor.php', "' - ' . \$row->introtext :"], + 'tv in use' => ['manager/processors/delete_tmplvars.processor.php', "'&a=27\">' . \$siteTmlvarTemplate->resource->pagetitle ."], + 'tv in use description' => ['manager/processors/delete_tmplvars.processor.php', "' - ' . \$siteTmlvarTemplate->resource->description :"], +]); + +/* +|-------------------------------------------------------------------------- +| Escaping must not change what the manager shows +|-------------------------------------------------------------------------- +| +| Older installations stored these values entity-encoded, newer ones store them raw. The escaping +| added above has to render both the way the raw echo did, never as visible "&" or "'". +| +*/ + +it('renders raw and legacy entity-encoded values as the same visible text', function (string $stored) { + $visible = html_entity_decode($stored, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + + $escaped = [ + entities($stored, 'UTF-8'), + htmlspecialchars($stored, ENT_QUOTES, 'UTF-8', false), + ]; + + foreach ($escaped as $html) { + // What the browser displays for the escaped markup equals what the raw echo displayed. + expect(html_entity_decode($html, ENT_QUOTES | ENT_HTML5, 'UTF-8'))->toBe($visible); + } +})->with([ + 'raw ampersand and quotes' => ["Tom & Jerry's \"shop\""], + 'legacy encoded ampersand' => ['Tom & Jerry'], + 'legacy encoded quotes' => ['O'Brien "Ltd"'], + 'cyrillic' => ['Привіт, світ'], +]); + +it('does not double-encode the web user list', function () { + $source = managerSource('manager/actions/web_user_management.static.php'); + + preg_match_all('/htmlspecialchars\((?:[^()]|\((?:[^()])*\))*\)/', $source, $calls); + + expect($calls[0])->not->toBeEmpty(); + + foreach ($calls[0] as $call) { + expect($call)->toEndWith(', false)'); + } +}); + +it('keeps the formatting of summaries listed by the delete screens', function () { + // introtext and description may carry inline markup that has always rendered as markup. + foreach (['manager/processors/delete_template.processor.php', 'manager/processors/delete_tmplvars.processor.php'] as $file) { + expect(managerSource($file))->toContain("' - ' . sanitize_inline_html("); + } + + $html = (string) sanitize_inline_html('Summer sale & more'); + + expect($html)->toContain('Summer') + ->and(html_entity_decode(strip_tags($html)))->toBe('Summer sale & more') + ->not->toContain('onerror'); +}); diff --git a/manager/actions/mutate_categories.dynamic.php b/manager/actions/mutate_categories.dynamic.php index 9bc3282407..be122ab6bf 100755 --- a/manager/actions/mutate_categories.dynamic.php +++ b/manager/actions/mutate_categories.dynamic.php @@ -10,12 +10,12 @@ $_module_params = [ 'module_version' => '1.0.0', 'module_params' => '', - 'module_id' => get_by_key($_GET, 'id', 0), + 'module_id' => (int)get_by_key($_GET, 'id', 0, 'is_scalar'), 'package_name' => 'Module_Categories_Manager', 'native_language' => 'de', 'name' => $_lang['manage_categories'], 'dirname' => EVO_MANAGER_URL, - 'url' => 'index.php?a=120&id=' . get_by_key($_GET, 'id', 0), + 'url' => 'index.php?a=120&id=' . (int)get_by_key($_GET, 'id', 0, 'is_scalar'), 'path' => realpath( __DIR__ ) . DIRECTORY_SEPARATOR . 'category_mgr' . DIRECTORY_SEPARATOR, 'inc_dir' => realpath( __DIR__ ) . DIRECTORY_SEPARATOR . 'category_mgr' . DIRECTORY_SEPARATOR . 'inc' . DIRECTORY_SEPARATOR, 'languages_dir' => realpath( __DIR__ ) . DIRECTORY_SEPARATOR . 'category_mgr' . DIRECTORY_SEPARATOR . 'lang' . DIRECTORY_SEPARATOR, diff --git a/manager/actions/mutate_menuindex_sort.dynamic.php b/manager/actions/mutate_menuindex_sort.dynamic.php index 6ab9c493d1..9e47f6d83b 100755 --- a/manager/actions/mutate_menuindex_sort.dynamic.php +++ b/manager/actions/mutate_menuindex_sort.dynamic.php @@ -82,7 +82,7 @@ $classes .= ($row['published']) ? ' publishedNode ' : ' unpublishedNode '; $classes = ($row['deleted']) ? ' deletedNode ' : $classes; $icon = $row['isfolder'] ? ' ' : ' '; - $resourcelist .= '
  • ' . $icon . $row['pagetitle'] . ' (' . $row['id'] . ')
    • '; + $resourcelist .= '
  • ' . $icon . entities($row['pagetitle'], evo()->getConfig('modx_charset')) . ' (' . (int)$row['id'] . ')
    • '; } $resourcelist .= '
    '; } else { diff --git a/manager/actions/mutate_web_user.dynamic.php b/manager/actions/mutate_web_user.dynamic.php index e5484584e6..abd32d0855 100755 --- a/manager/actions/mutate_web_user.dynamic.php +++ b/manager/actions/mutate_web_user.dynamic.php @@ -268,7 +268,7 @@ function evoRenderTvImageCheck(a) { " />

    - (' . $usernamedata['id'] . ')' : '') : $_lang['web_user_title']) ?> + getConfig('modx_charset')) . (isset($usernamedata['id']) ? '(' . (int)$usernamedata['id'] . ')' : '') : $_lang['web_user_title']) ?>

    diff --git a/manager/actions/web_user_management.static.php b/manager/actions/web_user_management.static.php index 551b37631e..e6328e8ea4 100644 --- a/manager/actions/web_user_management.static.php +++ b/manager/actions/web_user_management.static.php @@ -38,7 +38,7 @@ $role_options = ''; $roles = \EvolutionCMS\Models\UserRole::query()->select('id', 'name')->get()->toArray(); foreach ($roles as $row) { - $role_options .= ''; + $role_options .= ''; } // prepare data @@ -122,13 +122,13 @@ $listDocs[] = [ 'icon' => '', - 'name' => '' . $el['username'] . '', - 'user_full_name' => $el['fullname'], - 'email' => $el['email'], - 'role' => $el['name'] ?: ManagerTheme::getLexicon('no_user_role'), + 'name' => '' . htmlspecialchars((string)$el['username'], ENT_QUOTES, ManagerTheme::getCharset(), false) . '', + 'user_full_name' => htmlspecialchars((string)$el['fullname'], ENT_QUOTES, ManagerTheme::getCharset(), false), + 'email' => htmlspecialchars((string)$el['email'], ENT_QUOTES, ManagerTheme::getCharset(), false), + 'role' => $el['name'] ? htmlspecialchars((string)$el['name'], ENT_QUOTES, ManagerTheme::getCharset(), false) : ManagerTheme::getLexicon('no_user_role'), 'user_prevlogin' => $el['thislogin'] ? $modx->toDateFormat($el['thislogin']) : '-', 'user_logincount' => $el['logincount'], - 'user_block' => $el['blocked'] ? ManagerTheme::getLexicon('yes').' ' : '-', + 'user_block' => $el['blocked'] ? ManagerTheme::getLexicon('yes').' ' : '-', ]; } @@ -215,7 +215,7 @@ function menuAction(a) { - +
    diff --git a/manager/processors/delete_template.processor.php b/manager/processors/delete_template.processor.php index aef451e224..2af9a51305 100755 --- a/manager/processors/delete_template.processor.php +++ b/manager/processors/delete_template.processor.php @@ -29,7 +29,7 @@ diff --git a/manager/processors/delete_tmplvars.processor.php b/manager/processors/delete_tmplvars.processor.php index 6073d7bb3c..4124c9991c 100755 --- a/manager/processors/delete_tmplvars.processor.php +++ b/manager/processors/delete_tmplvars.processor.php @@ -42,7 +42,7 @@