From 6e222baa5d2839b880495714225208bd2475aa01 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:00:49 +0200 Subject: [PATCH 1/7] fix(media-browser): reject disguised and mismatched image uploads Validate every dot-separated part of an uploaded, renamed, copied or moved file name against the denied extensions list, not just the final extension - a name like shell.php.jpg passed before. Also require that a name claiming a raster image format (jpg, png, gif, webp, bmp, ico, avif, tif, psd) actually decodes as that format. Extend the file manager's ZIP extractor to skip entries whose name would be treated as server-executable (a PHP-like extension anywhere in the name, or .htaccess/.user.ini/etc.), closing the same bypass there. Co-Authored-By: Claude Sonnet 5 --- core/functions/actions/files.php | 34 ++++++++++- .../FileManagerExecutableNameTest.php | 11 ++++ .../MediaBrowserDoubleExtensionTest.php | 47 +++++++++++++++ manager/media/browser/mcpuk/core/browser.php | 6 +- manager/media/browser/mcpuk/core/uploader.php | 57 ++++++++++++++++++- 5 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 core/tests/Unit/Security/FileManagerExecutableNameTest.php create mode 100644 core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php diff --git a/core/functions/actions/files.php b/core/functions/actions/files.php index 789590611b..640918e97e 100644 --- a/core/functions/actions/files.php +++ b/core/functions/actions/files.php @@ -418,10 +418,38 @@ function fileManagerRemoveLink($path) } } +if(!function_exists('fileManagerIsExecutableName')) { + /** + * Whether a file name would be run by the web server or changes how it serves a folder: + * a PHP-like extension anywhere in the name ("shell.php.jpg" runs under AddHandler), or + * one of the per-directory configuration files. + * + * @param string $name + * @return bool + */ + function fileManagerIsExecutableName($name) + { + $base = strtolower(basename(str_replace(chr(92), '/', (string) $name))); + if (in_array($base, ['.htaccess', '.htpasswd', '.user.ini', '.env', 'web.config'], true)) { + return true; + } + $parts = explode('.', $base); + array_shift($parts); + + foreach ($parts as $part) { + if (preg_match('/^(?:php\d*|phps|phtml|pht|phar|inc|cgi|pl|py|sh|asp|aspx|jsp)$/', trim($part))) { + return true; + } + } + + return false; + } +} + if(!function_exists('fileManagerExtractZip')) { /** * Extracts $file into $path, skipping any entry that would land outside it, go through a - * symlink, or reach a protected folder. + * symlink, reach a protected folder, or be a server-executable file. * * @param string $file * @param string $path @@ -461,6 +489,10 @@ function fileManagerExtractZip($file, $path, array $protectedPaths = [], $dirMod if (!fileManagerIsSafeWriteTarget($root, $target) || fileManagerPathIsProtected($target, $protectedPaths)) { continue; } + // the upload form refuses these; unpacking must not be the way around it + if (!$isDir && fileManagerIsExecutableName(end($segments))) { + continue; + } if ($isDir) { if (!is_dir($target)) { mkdir($target, $dirMode, true); diff --git a/core/tests/Unit/Security/FileManagerExecutableNameTest.php b/core/tests/Unit/Security/FileManagerExecutableNameTest.php new file mode 100644 index 0000000000..4d706e8e3d --- /dev/null +++ b/core/tests/Unit/Security/FileManagerExecutableNameTest.php @@ -0,0 +1,11 @@ +toBeTrue(); +})->with(['shell.php', 'shell.PHP.jpg', 'a/b/x.phtml', '.htaccess', '.user.ini', 'x.phar.png', 'dir\x.php5']); + +it('accepts ordinary files', function (string $name) { + expect(fileManagerIsExecutableName($name))->toBeFalse(); +})->with(['photo.jpg', 'notes.txt', 'archive.tar.gz', 'php-logo.png', 'readme']); diff --git a/core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php b/core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php new file mode 100644 index 0000000000..1a9ef5ab99 --- /dev/null +++ b/core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php @@ -0,0 +1,47 @@ +newInstanceWithoutConstructor(); + foreach (['types' => ['images' => $types], 'config' => ['deniedExts' => $denied]] as $name => $value) { + $property = new ReflectionProperty(uploader::class, $name); + $property->setAccessible(true); + $property->setValue($uploader, $value); + } + + return $uploader; +} + +function uploaderAllowsName(uploader $uploader, string $name): bool +{ + $method = new ReflectionMethod(uploader::class, 'validateFilename'); + $method->setAccessible(true); + + return $method->invoke($uploader, $name, 'images'); +} + +it('rejects executable extensions hidden before the final extension', function (string $name) { + $uploader = uploaderForNames('jpg png gif', 'exe php phtml'); + + expect(uploaderAllowsName($uploader, $name))->toBeFalse(); +})->with(['shell.php.jpg', 'shell.PHP.jpg', 'a.b.phtml.png', 'shell.php.', 'dir\\shell.php.gif']); + +it('accepts ordinary image names', function (string $name) { + $uploader = uploaderForNames('jpg png gif', 'exe php phtml'); + + expect(uploaderAllowsName($uploader, $name))->toBeTrue(); +})->with(['photo.jpg', 'my.holiday.photo.PNG', 'php-logo.gif']); + +it('still rejects a denied or unlisted final extension', function (string $name) { + $uploader = uploaderForNames('jpg png gif', 'exe php phtml'); + + expect(uploaderAllowsName($uploader, $name))->toBeFalse(); +})->with(['shell.php', 'doc.txt']); diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index 59f7dabf0a..c08479fc2e 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -496,7 +496,7 @@ protected function act_rename() $this->errorMsg("A file or folder with that name already exists."); } $ext = file::getExtension($newName); - if (!$this->validateExtension($ext, $this->type)) { + if (!$this->validateFilename($newName, $this->type)) { $this->errorMsg("Denied file extension."); } if (is_array($evtOut) && !empty($evtOut)) { @@ -622,7 +622,7 @@ protected function act_cp_cbd() $error[] = $this->label("The file '{file}' does not exist.", $replace); } elseif (substr($base, 0, 1) == ".") { $error[] = "$base: " . $this->label("File name shouldn't begins with '.'"); - } elseif (!$this->validateExtension($ext, $type)) { + } elseif (!$this->validateFilename($base, $type)) { $error[] = "$base: " . $this->label("Denied file extension."); } elseif (file_exists("$dir/$base")) { $error[] = "$base: " . $this->label("A file or folder with that name already exists."); @@ -711,7 +711,7 @@ protected function act_mv_cbd() $error[] = $this->label("The file '{file}' does not exist.", $replace); } elseif (substr($base, 0, 1) == ".") { $error[] = "$base: " . $this->label("File name shouldn't begins with '.'"); - } elseif (!$this->validateExtension($ext, $type)) { + } elseif (!$this->validateFilename($base, $type)) { $error[] = "$base: " . $this->label("Denied file extension."); } elseif (file_exists("$dir/$base")) { $error[] = "$base: " . $this->label("A file or folder with that name already exists."); diff --git a/manager/media/browser/mcpuk/core/uploader.php b/manager/media/browser/mcpuk/core/uploader.php index 8069b8f522..b24a393f9f 100755 --- a/manager/media/browser/mcpuk/core/uploader.php +++ b/manager/media/browser/mcpuk/core/uploader.php @@ -406,9 +406,13 @@ protected function checkUploadedFile(array $aFile = null) return $this->label("File name shouldn't begins with '.'"); // EXTENSION CHECK - elseif (!$this->validateExtension($extension, $this->type)) + elseif (!$this->validateFilename($file['name'], $this->type)) return $this->label("Denied file extension."); + // RASTER IMAGE NAMES MUST HOLD IMAGE DATA OF THE KIND THEY CLAIM + elseif (!$this->imageDataMatchesExtension($extension, $file['tmp_name'])) + return $this->label("Unknown error."); + // SPECIAL DIRECTORY TYPES CHECK (e.g. *img) elseif (preg_match('/^\*([^ ]+)(.*)?$/s', $typePatt, $patt)) { list($typePatt, $type, $params) = $patt; @@ -473,6 +477,57 @@ protected function checkInputDir($dir, $inclType = true, $existing = true) return (is_dir($path) && is_readable($path)) ? $return : false; } + /** + * A name that claims a raster image format has to hold that format: getimagesize() must read + * it and report the type the extension stands for. + * + * @param string $ext + * @param string $path + * @return bool + */ + protected function imageDataMatchesExtension($ext, $path) + { + $kinds = [ + 'jpg' => [IMAGETYPE_JPEG], 'jpeg' => [IMAGETYPE_JPEG], 'jpe' => [IMAGETYPE_JPEG], + 'png' => [IMAGETYPE_PNG], 'gif' => [IMAGETYPE_GIF], 'bmp' => [IMAGETYPE_BMP], + 'webp' => [IMAGETYPE_WEBP], 'ico' => [IMAGETYPE_ICO], 'avif' => [defined('IMAGETYPE_AVIF') ? IMAGETYPE_AVIF : -1], + 'tif' => [IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM], 'tiff' => [IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM], + 'psd' => [IMAGETYPE_PSD], + ]; + $ext = strtolower(trim($ext)); + if (!isset($kinds[$ext])) { + return true; + } + $info = @getimagesize($path); + + return $info !== false && in_array($info[2], $kinds[$ext], true); + } + + /** + * Like validateExtension() for a whole file name: every inner dot-separated part is also checked + * against the denied list, since Apache's AddHandler runs PHP for "shell.php.jpg". + * + * @param string $name + * @param string $type + * @return bool + */ + protected function validateFilename($name, $type) + { + $parts = explode('.', basename(str_replace('\\', '/', (string)$name))); + $ext = array_pop($parts); + array_shift($parts); + + $denied = strtolower(text::clearWhitespaces($this->config['deniedExts'])); + $denied = strlen($denied) ? explode(' ', $denied) : []; + foreach ($parts as $part) { + if (in_array(strtolower(trim($part)), $denied, true)) { + return false; + } + } + + return $this->validateExtension($ext, $type); + } + /** * @param $ext * @param $type From 60d8e07ef01d3a0f10b85975dfa3ae719bb0fdb3 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:01:01 +0200 Subject: [PATCH 2/7] fix(tv): require save_snippet before a TV binding can run code @EVAL (PHP eval()) and @SELECT (raw SQL) were reachable from any template variable value or definition field, and from resource content via @DOCUMENT/@INHERIT, with no gate beyond save_template or save_document. A manager with only those permissions could store a binding that runs when the TV renders. Add TvBindingGuard, which recognizes @EVAL/@SELECT by the same prefix match the parser uses (no word boundary, since the parser has none either - "@EVALreturn 1;" still reaches eval()), looks through any @INHERIT chain, and also checks the page text fields a binding's [*field*] placeholder can pull in. A manager without save_snippet may not add or change a binding; an existing one some other editor stored is left alone. Apply it in DocumentSaveService (resource save) and save_tmplvars.processor.php (TV definition save). Co-Authored-By: Claude Sonnet 5 --- core/src/Services/DocumentSaveService.php | 25 +++++ core/src/Support/TvBindingGuard.php | 97 +++++++++++++++++++ .../Unit/Security/TvBindingGuardTest.php | 59 +++++++++++ core/vendor/composer/autoload_classmap.php | 1 + core/vendor/composer/autoload_static.php | 1 + .../processors/save_tmplvars.processor.php | 12 +++ 6 files changed, 195 insertions(+) create mode 100644 core/src/Support/TvBindingGuard.php create mode 100644 core/tests/Unit/Security/TvBindingGuardTest.php diff --git a/core/src/Services/DocumentSaveService.php b/core/src/Services/DocumentSaveService.php index d30ef82752..9ba02fd7d1 100644 --- a/core/src/Services/DocumentSaveService.php +++ b/core/src/Services/DocumentSaveService.php @@ -12,6 +12,7 @@ use EvolutionCMS\Support\DocumentSave\PublishState; use EvolutionCMS\Support\DocumentSave\TemplateVariableInput; use EvolutionCMS\Support\DocumentSave\TemplateVariableValues; +use EvolutionCMS\Support\TvBindingGuard; use Illuminate\Support\Facades\DB; /** @@ -81,6 +82,15 @@ public function save(array $input): DocumentSaveResult $tvs = TemplateVariableValues::forTemplate($template, $id, !$ctx->isAdministrator(), $ctx->managerDocgroups); $tvValues = TemplateVariableInput::values($tvs, $input); + // @EVAL and @SELECT in a TV value run code when it is rendered: that takes the PHP permission + $storedValues = []; + foreach ($tvs as $tv) { + $storedValues[$tv['id']] = $tv['value']; + } + if (!TvBindingGuard::allowsValues($ctx->can('save_snippet'), $tvValues, $storedValues, (string) ($input['ta'] ?? ''), (string) ($existing->content ?? ''), $this->textFields($input), $existing ? $this->textFields($existing->getAttributes()) : [])) { + throw new DocumentSaveDenied($ctx->lang('error_no_privileges')); + } + $now = $ctx->now; $pubDate = empty($input['pub_date']) ? 0 : $ctx->toTimestamp((string) $input['pub_date']); $unpubDate = empty($input['unpub_date']) ? 0 : $ctx->toTimestamp((string) $input['unpub_date']); @@ -300,4 +310,19 @@ private function transaction(callable $callback): mixed { return SiteContent::resolveConnection()->transaction($callback); } + + /** + * The text fields a TV binding can pull in with [*field*]. + * + * @return array + */ + private function textFields(array $source): array + { + $fields = []; + foreach (['pagetitle', 'longtitle', 'description', 'introtext', 'menutitle', 'link_attributes', 'alias'] as $name) { + $fields[$name] = (string) ($source[$name] ?? ''); + } + + return $fields; + } } diff --git a/core/src/Support/TvBindingGuard.php b/core/src/Support/TvBindingGuard.php new file mode 100644 index 0000000000..1c17e6c841 --- /dev/null +++ b/core/src/Support/TvBindingGuard.php @@ -0,0 +1,97 @@ + column) a binding can sit in. */ + public const FIELDS = ['elements' => 'elements', 'default_text' => 'default_text', 'display_params' => 'display_params']; + + public static function hasCodeBinding(?string $value): bool + { + $value = (string) $value; + // the parser trims with trim(), which also drops NUL and vertical tab, and matches a command + // by prefix ("@EVALreturn 1;" runs), so no boundary may be required after the name. + // @INHERIT hands its argument back to the parser, so it is looked through. + for ($depth = 0; $depth < 20; $depth++) { + $value = ltrim($value, " \t\n\r\0\x0B"); + if (preg_match('/^@{1,2}(?:EVAL|SELECT)/i', $value)) { + return true; + } + if (!preg_match('/^@INHERIT/i', $value)) { + return false; + } + $value = substr($value, 8); + } + + return true; // deeper than any real value nests: do not guess + } + + /** + * A TV value or resource field that can reach a code binding. Bindings nest (@INHERIT hands + * its argument back to the binding parser), so one is also caught behind another binding. + */ + public static function reachesCodeBinding(?string $value): bool + { + return self::hasCodeBinding($value); + } + + /** + * Whether a manager may store these TV values and the resource content. Values that did not + * change are kept, so an existing binding stays editable around by whoever may edit the page. + * + * @param array $desired TV id => submitted value + * @param array $stored TV id => current value + */ + public static function allowsValues(bool $mayWritePhp, array $desired, array $stored, string $content = '', string $storedContent = '', array $fields = [], array $storedFields = []): bool + { + if ($mayWritePhp) { + return true; + } + // content only runs as a binding when a TV pulls it in with @DOCUMENT, and then from its start + if ($content !== $storedContent && self::hasCodeBinding($content)) { + return false; + } + // [*field*] in a binding's argument pulls in any other field of the page, so each is checked + foreach ($fields as $name => $value) { + $value = (string) $value; + if (self::hasCodeBinding($value) && $value !== (string) ($storedFields[$name] ?? '')) { + return false; + } + } + foreach ($desired as $id => $value) { + $value = (string) $value; + if (self::reachesCodeBinding($value) && $value !== (string) ($stored[$id] ?? '')) { + return false; + } + } + + return true; + } + + /** + * @param array $submitted values keyed by column + * @param array $stored current values keyed by column, empty for a new TV + */ + public static function allows(bool $mayWritePhp, array $submitted, array $stored = []): bool + { + if ($mayWritePhp) { + return true; + } + foreach (self::FIELDS as $column) { + $value = (string) ($submitted[$column] ?? ''); + if (self::hasCodeBinding($value) && $value !== (string) ($stored[$column] ?? '')) { + return false; + } + } + + return true; + } +} diff --git a/core/tests/Unit/Security/TvBindingGuardTest.php b/core/tests/Unit/Security/TvBindingGuardTest.php new file mode 100644 index 0000000000..edc15d6378 --- /dev/null +++ b/core/tests/Unit/Security/TvBindingGuardTest.php @@ -0,0 +1,59 @@ +toBeTrue(); +})->with(['@EVAL return 1;', ' @eval return 1;', '@@EVAL return time();', "@SELECT id FROM t"]); + +it('lets harmless values through', function (?string $value) { + expect(TvBindingGuard::hasCodeBinding($value))->toBeFalse(); +})->with([null, '', 'Red==red||Blue==blue', '@CHUNK header', '@FILE assets/x.html', 'mail@EVALUATE.com', 'text @EVAL later']); + +it('refuses a new code binding without the PHP permission', function (string $field) { + expect(TvBindingGuard::allows(false, [$field => '@EVAL return 1;']))->toBeFalse(); +})->with(['elements', 'default_text', 'display_params']); + +it('allows a code binding for a manager who may write PHP', function () { + expect(TvBindingGuard::allows(true, ['elements' => '@EVAL return 1;']))->toBeTrue(); +}); + +it('keeps an existing binding editable around it, but not changeable, without the permission', function () { + $stored = ['elements' => '@EVAL return 1;']; + + expect(TvBindingGuard::allows(false, ['elements' => '@EVAL return 1;', 'default_text' => 'x'], $stored))->toBeTrue() + ->and(TvBindingGuard::allows(false, ['elements' => '@EVAL system("id");'], $stored))->toBeFalse(); +}); + +it('catches a code binding in a TV value even behind another binding', function (string $value) { + expect(TvBindingGuard::reachesCodeBinding($value))->toBeTrue(); +})->with(['@EVAL return 1;', '@INHERIT @EVAL return 1;', "@INHERIT\n@eval x();", '@@SELECT 1']); + +it('leaves ordinary TV text alone', function (string $value) { + expect(TvBindingGuard::reachesCodeBinding($value))->toBeFalse(); +})->with(['', 'write to me@select.com', '@CHUNK header', 'plain']); + +it('refuses new code in TV values or resource content without the PHP permission', function () { + expect(TvBindingGuard::allowsValues(false, [3 => '@INHERIT @EVAL 1;'], []))->toBeFalse() + ->and(TvBindingGuard::allowsValues(false, [], [], '@EVAL return 1;', 'old'))->toBeFalse() + ->and(TvBindingGuard::allowsValues(true, [3 => '@EVAL 1;'], [], '@EVAL 1;'))->toBeTrue() + ->and(TvBindingGuard::allowsValues(false, [3 => '@EVAL 1;'], [3 => '@EVAL 1;']))->toBeTrue() + ->and(TvBindingGuard::allowsValues(false, [3 => 'text'], [3 => '@EVAL 1;']))->toBeTrue(); +}); + +it('catches the forms the parser accepts but a word-boundary check misses', function (string $value) { + expect(TvBindingGuard::hasCodeBinding($value))->toBeTrue(); +})->with([ + 'no boundary' => '@EVALreturn 1;', + 'select no boundary' => '@SELECTid FROM x', + 'inherit chain' => '@INHERIT@EVAL x', + 'nested inherit' => "@INHERIT @INHERIT\n@evalx", + 'nul prefix' => "\0@EVAL x", + 'vertical tab prefix' => "\x0B@EVAL x", +]); + +it('checks the page text fields a binding can pull in with a placeholder', function () { + expect(TvBindingGuard::allowsValues(false, [], [], '', '', ['pagetitle' => '@EVALreturn 1;'], ['pagetitle' => 'old']))->toBeFalse() + ->and(TvBindingGuard::allowsValues(false, [], [], '', '', ['pagetitle' => 'Hello'], ['pagetitle' => 'old']))->toBeTrue() + ->and(TvBindingGuard::allowsValues(true, [], [], '', '', ['pagetitle' => '@EVAL 1;'], []))->toBeTrue(); +}); diff --git a/core/vendor/composer/autoload_classmap.php b/core/vendor/composer/autoload_classmap.php index 2d46a890c9..d50e7b6ae1 100644 --- a/core/vendor/composer/autoload_classmap.php +++ b/core/vendor/composer/autoload_classmap.php @@ -1443,6 +1443,7 @@ 'EvolutionCMS\\Support\\SqliteDumper' => $baseDir . '/src/Support/SqliteDumper.php', 'EvolutionCMS\\Support\\SystemSettingPathNormalizer' => $baseDir . '/src/Support/SystemSettingPathNormalizer.php', 'EvolutionCMS\\Support\\TemplateFileEngines' => $baseDir . '/src/Support/TemplateFileEngines.php', + 'EvolutionCMS\\Support\\TvBindingGuard' => $baseDir . '/src/Support/TvBindingGuard.php', 'EvolutionCMS\\TemplateProcessor' => $baseDir . '/src/TemplateProcessor.php', 'EvolutionCMS\\Tracy\\ConnectionTiming' => $baseDir . '/src/Tracy/ConnectionTiming.php', 'EvolutionCMS\\Tracy\\Debugger' => $baseDir . '/src/Tracy/Debugger.php', diff --git a/core/vendor/composer/autoload_static.php b/core/vendor/composer/autoload_static.php index 8c0e8dd377..4bd613dbec 100644 --- a/core/vendor/composer/autoload_static.php +++ b/core/vendor/composer/autoload_static.php @@ -2105,6 +2105,7 @@ class ComposerStaticInit925fea465a58fa69f06ccf2629003e87 'EvolutionCMS\\Support\\SqliteDumper' => __DIR__ . '/../..' . '/src/Support/SqliteDumper.php', 'EvolutionCMS\\Support\\SystemSettingPathNormalizer' => __DIR__ . '/../..' . '/src/Support/SystemSettingPathNormalizer.php', 'EvolutionCMS\\Support\\TemplateFileEngines' => __DIR__ . '/../..' . '/src/Support/TemplateFileEngines.php', + 'EvolutionCMS\\Support\\TvBindingGuard' => __DIR__ . '/../..' . '/src/Support/TvBindingGuard.php', 'EvolutionCMS\\TemplateProcessor' => __DIR__ . '/../..' . '/src/TemplateProcessor.php', 'EvolutionCMS\\Tracy\\ConnectionTiming' => __DIR__ . '/../..' . '/src/Tracy/ConnectionTiming.php', 'EvolutionCMS\\Tracy\\Debugger' => __DIR__ . '/../..' . '/src/Tracy/Debugger.php', diff --git a/manager/processors/save_tmplvars.processor.php b/manager/processors/save_tmplvars.processor.php index 49312f1782..ebf119cad8 100755 --- a/manager/processors/save_tmplvars.processor.php +++ b/manager/processors/save_tmplvars.processor.php @@ -21,6 +21,7 @@ $originId = isset($_REQUEST['oid']) ? (int)$_REQUEST['oid'] : null; $currentdate = time() + $modx->config['server_offset_time']; $properties = $_POST['properties']; +$tvBindingFields = ['elements' => $elements, 'default_text' => $default_text, 'display_params' => $params]; //Kyle Jaebker - added category support if (empty($_POST['newcategory']) && $_POST['categoryid'] > 0) { @@ -47,6 +48,11 @@ "id" => $id ]); + // @EVAL and @SELECT run code, which save_template alone does not grant + if (!\EvolutionCMS\Support\TvBindingGuard::allows($modx->hasPermission('save_snippet'), $tvBindingFields)) { + $modx->webAlertAndQuit($_lang["error_no_privileges"]); + } + // disallow duplicate names for new tvs if (EvolutionCMS\Models\SiteTmplvar::where('name', '=', $name)->first()) { $modx->getManagerApi()->saveFormValues(300); @@ -113,6 +119,12 @@ "id" => $id ]); + // @EVAL and @SELECT run code, which save_template alone does not grant + $storedTv = EvolutionCMS\Models\SiteTmplvar::find($id); + if (!\EvolutionCMS\Support\TvBindingGuard::allows($modx->hasPermission('save_snippet'), $tvBindingFields, $storedTv ? $storedTv->getAttributes() : [])) { + $modx->webAlertAndQuit($_lang["error_no_privileges"]); + } + // disallow duplicate names for tvs if (EvolutionCMS\Models\SiteTmplvar::where('name', '=', $name)->where('id', '!=', $id)->first()) { $modx->getManagerApi()->saveFormValues(300); From 0c19bcdf45239c8af2f9225af9e91dd6e7f5cf2d Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:01:08 +0200 Subject: [PATCH 3/7] fix(manager): check permissions on action 118's settings/plugin ajax Page 118 maps to a plain view (ManagerTheme::handle() renders it without a controller permission check), and that view includes mutate_settings.ajax.php, which let any authenticated manager change a system setting or update/delete a plugin - no settings, save_plugin or delete_plugin check. Require settings for setsetting, except keys starting with _hide_ (client-side notice-dismiss flags, harmless and meant to be open to every manager), and require save_plugin or delete_plugin for updateplugin, limited to the disabled column or the _delete_ action. Co-Authored-By: Claude Sonnet 5 --- manager/includes/mutate_settings.ajax.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/manager/includes/mutate_settings.ajax.php b/manager/includes/mutate_settings.ajax.php index ccfeb0ef5b..ba547ce1cf 100644 --- a/manager/includes/mutate_settings.ajax.php +++ b/manager/includes/mutate_settings.ajax.php @@ -15,6 +15,17 @@ $str = ''; $emptyCache = false; +// page 118 is a plain view, so nothing before this file checked what the manager may do +$core = EvolutionCMS(); +if ($action === 'setsetting' && strpos($key, '_hide_') !== 0 && !$core->hasPermission('settings')) { + $action = ''; // only the dismiss flags of notices are open to every manager +} elseif ($action === 'updateplugin') { + $needed = $key === '_delete_' ? 'delete_plugin' : 'save_plugin'; + if (!$core->hasPermission($needed) || ($key !== '_delete_' && $key !== 'disabled')) { + $action = ''; + } +} + switch (true) { case ($action == 'get' && preg_match('/^[A-z0-9_-]+$/', $lang) && file_exists(EVO_CORE_PATH . 'lang/' . $lang . '/global.php')): { From 18de1c12c6d63bca31290ae88e55fb089ed2ee1e Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:01:15 +0200 Subject: [PATCH 4/7] fix(tv): restrict a TV by its own access group, not the document's TemplateVariableValues::forTemplate() joined document_groups on the saved resource and used that join to decide whether a non-admin manager could see a TV restricted by site_tmplvar_access. That checks the document's groups, not the TV's: a manager whose groups overlap the resource's groups could get a TV restricted to some other, unrelated group into the editable set. Join site_tmplvar_access.documentgroup against the manager's own groups instead, dropping the document_groups join entirely. Co-Authored-By: Claude Sonnet 5 --- .../Support/DocumentSave/TemplateVariableValues.php | 12 ++++++------ .../DocumentSave/TemplateVariableValuesTest.php | 11 +++++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/core/src/Support/DocumentSave/TemplateVariableValues.php b/core/src/Support/DocumentSave/TemplateVariableValues.php index 77d3b674f3..52201163cd 100644 --- a/core/src/Support/DocumentSave/TemplateVariableValues.php +++ b/core/src/Support/DocumentSave/TemplateVariableValues.php @@ -13,7 +13,7 @@ final class TemplateVariableValues { /** * TVs of a template with the value stored for the document. Non-administrators only - * get the TVs without access rules or whose document belongs to one of their groups. + * get the TVs without access rules or whose access rows name one of their document groups. * * @return array */ @@ -31,12 +31,12 @@ public static function forTemplate(int $template, int $documentId, bool $restric ->where('site_tmplvar_templates.templateid', $template) ->orderBy('site_tmplvars.rank'); + // the TV's own access rows decide, not the groups of the document it is saved on if ($restrictToGroups) { - $query->leftJoin('document_groups', 'site_tmplvar_contentvalues.contentid', '=', 'document_groups.document') - ->where(function ($q) use ($managerGroups) { - $q->whereNull('site_tmplvar_access.documentgroup') - ->orWhereIn('document_groups.document_group', $managerGroups); - }); + $query->where(function ($q) use ($managerGroups) { + $q->whereNull('site_tmplvar_access.documentgroup') + ->orWhereIn('site_tmplvar_access.documentgroup', $managerGroups); + }); } $rows = []; diff --git a/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php b/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php index ed36a77ea7..f3cc8824ad 100644 --- a/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php +++ b/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php @@ -60,15 +60,18 @@ function bootTvFixture(): Capsule ->and(array_keys($rows[0]))->toBe(['id', 'name', 'type', 'default_text', 'value_id', 'value']); })->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); -test('a restricted tv is hidden from a user whose document is not in its group', function () { +test('a restricted tv is hidden from a manager outside its own access group', function () { bootTvFixture(); - expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [5]), 'id'))->toBe([2, 1]); - + // TV 3 is restricted to document group 9. A manager in group 5 does not get it, even once + // the document itself is put into group 5 - the document's groups are not the TV's groups. Capsule::table('document_groups')->insert(['document_group' => 5, 'document' => 100]); Capsule::table('site_tmplvar_contentvalues')->insert(['tmplvarid' => 3, 'contentid' => 100, 'value' => 'v3']); - expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [5]), 'id'))->toBe([2, 1, 3]); + expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [5]), 'id'))->toBe([2, 1]); + + // Only membership in the TV's own access group (9) exposes it. + expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [9]), 'id'))->toBe([2, 1, 3]); })->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); test('sync writes only the differences and leaves other documents alone', function () { From d7597ba80536ee53380ce206af25e52c67df3d3b Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:01:30 +0200 Subject: [PATCH 5/7] fix(core): confine @FILE/@INCLUDE/file_get_contents to safe content resolveAtBindFilePath() blocked traversal out of the install and reads from manager/, but not from core/ - a @FILE or @INCLUDE binding, or the file_get_contents/readfile modifier, could still reach core/.env, core/config, core/custom and other core files that happen to sit below the web root. Refuse core/ the same way manager/ already was, falling back to doing nothing when it is not actually below the base path (a relocated core). Add Core::atBindFileIsReadable(): even inside the allowed area, @FILE and the file_get_contents/readfile modifier must not hand out PHP-like source (snippets, plugins, modules - anywhere, not just a fixed extension list), hidden files/folders, assets/cache, assets/backup, or files like .env/.ini/.sql/.log/.bak/.conf/.pem/.key. Apply it to the TV custom widget's @FILE branch too, which had no extension check of its own. @INCLUDE is unaffected beyond the core/ block - it exists to run PHP, so it still may. The modifier's error messages now say why a read was refused (denied extension, hidden/generated file, or outside the allowed area) instead of a single generic message, without naming the path. Co-Authored-By: Claude Sonnet 5 --- core/functions/actions/mutate_content.php | 4 +- core/functions/tv.php | 4 +- core/src/Core.php | 47 +++++++++++++--- core/src/Legacy/Modifiers.php | 22 ++++---- .../Unit/Security/ParserEvalHardeningTest.php | 53 +++++++++++++++++++ 5 files changed, 110 insertions(+), 20 deletions(-) diff --git a/core/functions/actions/mutate_content.php b/core/functions/actions/mutate_content.php index 3985badd14..fb6ff17252 100644 --- a/core/functions/actions/mutate_content.php +++ b/core/functions/actions/mutate_content.php @@ -547,7 +547,7 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = "", $tvtype = their own, not the one that grants arbitrary PHP. */ if (substr($output, 0, 5) == "@FILE") { $file_name = $modx->atBindFilePath(substr($output, 6)); - if ($file_name === false) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { $widget_output = file_get_contents($file_name); @@ -905,7 +905,7 @@ function renderFormElement( /* If we are loading a file */ if (substr($field_elements, 0, 5) == "@FILE") { $file_name = $modx->atBindFilePath(substr($field_elements, 6)); - if ($file_name === false) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $custom_output = 'Could not retrieve file for this TV.'; } else { $custom_output = file_get_contents($file_name); diff --git a/core/functions/tv.php b/core/functions/tv.php index 08f011b9d0..8b3dfd4e20 100644 --- a/core/functions/tv.php +++ b/core/functions/tv.php @@ -523,7 +523,7 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = '', $tvtype = their own, not the one that grants arbitrary PHP. */ if (strpos($output, '@FILE') === 0) { $file_name = $modx->atBindFilePath(substr($output, 6)); - if ($file_name === false) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { $widget_output = file_get_contents($file_name); @@ -899,7 +899,7 @@ function renderFormElement( /* If we are loading a file */ if (strpos($field_elements, '@FILE') === 0) { $file_name = $modx->atBindFilePath(substr($field_elements, 6)); - if ($file_name === false) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $custom_output = 'Could not retrieve file for this TV.'; } else { $custom_output = file_get_contents($file_name); diff --git a/core/src/Core.php b/core/src/Core.php index a780e97bae..9873372c15 100644 --- a/core/src/Core.php +++ b/core/src/Core.php @@ -6615,11 +6615,15 @@ public function resolveAtBindFilePath($candidate) return false; } - $manager = realpath(EVO_MANAGER_PATH); - if ($manager !== false) { - $manager = rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $manager), '/') . '/'; - if (strpos($resolved, $manager) === 0) { - return false; + // the manager and the core hold source, configuration and .env, none of it content + foreach ([EVO_MANAGER_PATH, EVO_CORE_PATH] as $protected) { + $protected = realpath($protected); + if ($protected !== false) { + $protected = rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $protected), '/') . '/'; + // a core that is not below the base (a relocated one) is not what the base serves + if ($protected !== $base && strpos($protected, $base) === 0 && strpos($resolved, $protected) === 0) { + return false; + } } } @@ -6657,6 +6661,37 @@ public function atBindFilePath($relative, array $searchPaths = ['']) return false; } + /** + * Whether a file resolved by atBindFilePath() may be handed out as text by @FILE or the + * file_get_contents modifier: no source (PHP-like files, wherever they live: snippets, + * plugins, modules), no hidden files or folders, no generated or backup data. + * + * @param string|false $path + * @return bool + */ + public function atBindFileIsReadable($path) + { + if ($path === false || $path === '') { + return false; + } + $path = str_replace(DIRECTORY_SEPARATOR, '/', (string) $path); + $base = realpath(EVO_BASE_PATH); + $relative = $base === false ? $path : ltrim(substr($path, strlen(rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $base), '/'))), '/'); + + foreach (explode('/', $relative) as $segment) { + if (strpos($segment, '.') === 0) { + return false; + } + } + if (preg_match('~^assets/(?:cache|backup)(?:/|$)~i', $relative)) { + return false; + } + $ext = '.' . strtolower(pathinfo($path, PATHINFO_EXTENSION)); + $denied = array_merge(self::AT_BIND_FILE_DENIED_EXTENSIONS, ['.pht', '.cgi', '.pl', '.py', '.sh', '.env', '.ini', '.sql', '.log', '.bak', '.conf', '.pem', '.key']); + + return !in_array($ext, $denied, true); + } + public function atBindFileContent($str = '') { @@ -6688,7 +6723,7 @@ public function atBindFileContent($str = '') '' ]); - if ($file_path === false) { + if ($file_path === false || !$this->atBindFileIsReadable($file_path)) { return $errorMsg; } diff --git a/core/src/Legacy/Modifiers.php b/core/src/Legacy/Modifiers.php index d8141a9ccb..fff17dc0b3 100644 --- a/core/src/Legacy/Modifiers.php +++ b/core/src/Legacy/Modifiers.php @@ -1123,17 +1123,19 @@ public function getValueFromPreset($key, $value, $cmd, $opt) if (!is_file($value)) { return $value; } - $value = realpath($value); - if (strpos($value, EVO_MANAGER_PATH) !== false) { - exit('Can not read core file'); - } - $ext = strtolower(substr($value, -4)); - if ($ext === '.php') { - exit('Can not read php file'); - } - if ($ext === '.cgi') { - exit('Can not read cgi file'); + // same rules as @FILE: inside the installation, not manager/ or core/, no source or secrets + $resolved = $modx->resolveAtBindFilePath($value); + if ($resolved === false) { + exit('Can not read file: outside the site, or inside manager/ or core/'); + } + if (!$modx->atBindFileIsReadable($resolved)) { + $reason = 'hidden, generated or backup file'; + if (preg_match('/\.(php\d*|phps|phtml|pht|phar|inc|cgi|pl|py|sh|env|ini|sql|log|bak|conf|pem|key)$/i', $resolved, $matches)) { + $reason = 'denied extension .' . strtolower($matches[1]); + } + exit('Can not read file: ' . $reason); } + $value = $resolved; return file_get_contents($value); case 'filesize': diff --git a/core/tests/Unit/Security/ParserEvalHardeningTest.php b/core/tests/Unit/Security/ParserEvalHardeningTest.php index 8b55b9dac2..8cd8518748 100644 --- a/core/tests/Unit/Security/ParserEvalHardeningTest.php +++ b/core/tests/Unit/Security/ParserEvalHardeningTest.php @@ -238,6 +238,28 @@ function parserHardeningCore(): Core @unlink($absolute); } }); + + test('files under the core directory are refused, also through traversal', function () { + $core = parserHardeningCore(); + + // an earlier test may have pinned the constants to a tree whose core is not below its base + if (strpos(rtrim(EVO_CORE_PATH, '/') . '/', rtrim(EVO_BASE_PATH, '/') . '/core/') !== 0) { + $this->markTestSkipped('EVO_CORE_PATH is not below EVO_BASE_PATH/core here'); + } + + $relative = 'core/evo_atfile_' . bin2hex(random_bytes(6)) . '.txt'; + $absolute = EVO_BASE_PATH . $relative; + is_dir(dirname($absolute)) || mkdir(dirname($absolute), 0777, true); + file_put_contents($absolute, 'secret'); + + try { + expect($core->atBindFileContent('@FILE:' . $relative))->not->toBe('secret') + ->and($core->atBindFileContent('@FILE:assets/../' . $relative))->not->toBe('secret') + ->and($core->atBindFilePath($relative))->toBeFalse(); + } finally { + @unlink($absolute); + } + }); }); describe('@INCLUDE binding', function () { @@ -325,3 +347,34 @@ function parserHardeningCore(): Core ->toBeFalse(); }); }); + +describe('@FILE readability', function () { + + test('source, hidden, generated and secret files are not handed out as text', function (string $relative) { + $core = parserHardeningCore(); + $absolute = EVO_BASE_PATH . $relative; + is_dir(dirname($absolute)) || mkdir(dirname($absolute), 0777, true); + file_put_contents($absolute, 'x'); + + try { + expect($core->atBindFileIsReadable($core->atBindFilePath($relative)))->toBeFalse(); + } finally { + @unlink($absolute); + } + })->with(['assets/snippets/evo_x.php', 'assets/plugins/evo_x.phtml', 'assets/evo_x/.secret.txt', 'assets/cache/evo_x.txt', 'assets/backup/evo_x.sql', 'assets/evo_x.env']); + + test('ordinary content files stay readable', function () { + $core = parserHardeningCore(); + $relative = 'assets/templates/evo_x_' . bin2hex(random_bytes(4)) . '.html'; + $absolute = EVO_BASE_PATH . $relative; + is_dir(dirname($absolute)) || mkdir(dirname($absolute), 0777, true); + file_put_contents($absolute, '

ok

'); + + try { + expect($core->atBindFileIsReadable($core->atBindFilePath($relative)))->toBeTrue() + ->and($core->atBindFileIsReadable(false))->toBeFalse(); + } finally { + @unlink($absolute); + } + }); +}); From 8fb1a879dd1c2d21f0b5b360574a59f1a14a27fd Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:01:37 +0200 Subject: [PATCH 6/7] fix(manager): sandbox the resource preview iframe The preview iframe loaded same-origin, with no sandbox, so script in previewed content (which a lower-privileged editor can supply, since HTML in resources is intentionally allowed) could reach window.top, read the manager's CSRF meta tag, and ride a previewing manager's session into a same-origin request. The session cookie is already HttpOnly, so this is request forgery via a stolen token, not cookie theft. Add sandbox="allow-scripts allow-forms allow-popups", deliberately without allow-same-origin: previewed script and form submission still work, but the frame gets an opaque origin and can no longer reach outside itself. Co-Authored-By: Claude Sonnet 5 --- .../Security/DocumentPreviewSandboxTest.php | 21 +++++++++++++++++++ manager/views/page/3.blade.php | 5 ++++- 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 core/tests/Unit/Security/DocumentPreviewSandboxTest.php diff --git a/core/tests/Unit/Security/DocumentPreviewSandboxTest.php b/core/tests/Unit/Security/DocumentPreviewSandboxTest.php new file mode 100644 index 0000000000..6e8d0a721c --- /dev/null +++ b/core/tests/Unit/Security/DocumentPreviewSandboxTest.php @@ -0,0 +1,21 @@ +toContain('id="previewIframe"') + ->and($source)->toMatch('/]*id="previewIframe"[^>]*sandbox="([^"]*)"/'); + + preg_match('/]*id="previewIframe"[^>]*sandbox="([^"]*)"/', $source, $matches); + $tokens = explode(' ', trim($matches[1])); + + expect($tokens)->not->toContain('allow-same-origin') + ->and($tokens)->toContain('allow-scripts'); +}); diff --git a/manager/views/page/3.blade.php b/manager/views/page/3.blade.php index 0df1b896d3..28d0b5da60 100644 --- a/manager/views/page/3.blade.php +++ b/manager/views/page/3.blade.php @@ -497,7 +497,10 @@ class="' . $_style['icon_move'] . '">' . $icon_pub_unpub : '') . (evo()- @if(!empty($show_preview))
{{ ManagerTheme::getLexicon('preview') }}
- + {{-- sandboxed without allow-same-origin: previewed content runs scripts but gets an opaque + origin, so it cannot reach window.top, read the manager's CSRF meta tag, or ride the + manager's session into a same-origin request. --}} +
@endif @endsection From 068766e535552881cba4df6103c099391f890f89 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Thu, 8 Oct 2026 16:39:46 +0200 Subject: [PATCH 7/7] fix(manager): stop the web user email label overlapping its field main.css stretches .warning to ~100% width whenever it sits as a direct child of td:first-child in these forms (#documentPane, #webUserPane) - meant for a label whose whole text is the warning. The web user form's required-email row only wrapped the "*" in .warning, so that rule stretched the lone asterisk across almost the entire first column and pushed "E-mail address:" out to the right, behind the input field. Wrap the whole label in an outer span so .warning is no longer a direct child of the td and the rule stops matching, same as every other label row in this form. Co-Authored-By: Claude Sonnet 5 --- .../Manager/WebUserEmailRequiredMarkerTest.php | 15 +++++++++++++-- manager/actions/mutate_web_user.dynamic.php | 7 ++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php b/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php index 2ce0feb2e8..38ab4025c1 100644 --- a/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php +++ b/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php @@ -1,8 +1,19 @@ toContain('* :'); + expect($form)->toContain('* :') + // the bare, unwrapped form is what main.css's td:first-child > .warning rule stretches + // to ~100% width, pushing the label text off to the right behind it + ->and($form)->not->toContain('* :'); }); diff --git a/manager/actions/mutate_web_user.dynamic.php b/manager/actions/mutate_web_user.dynamic.php index abd32d0855..363aaacf99 100755 --- a/manager/actions/mutate_web_user.dynamic.php +++ b/manager/actions/mutate_web_user.dynamic.php @@ -356,7 +356,12 @@ function evoRenderTvImageCheck(a) { - * : + + * :