From eaaddf5459569faca476868de0ea30a2968889e2 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:01:25 +0200 Subject: [PATCH 1/4] fix(files): enforce file groups on resolved paths in the file manager and browser - Root checks used a bare prefix match, so a root at assets/alice also admitted assets/alice-private. FileManagerAccess::isWithin() replaces them. - Classic file manager actions checked the raw request (own/../private) against the listing's restriction map, then acted on the resolved target. They now resolve first and look the target's groups up fresh. - Deleting a folder is refused when it holds entries the manager may not reach; unzip checks the archive itself; the directory zip filters against the whole subtree, not only the current folder's entries. - The embedded browser checked groups only when listing: thumbnail, download, rename, delete, clipboard copy/move/delete, folder delete and all three zip downloads now check the named entry. - file_groups rows are keyed by the site-wide filemanager_path. A manager's own filemanager_path gave every path another key, which nobody had restricted, and switched groups off for them in both tools. - Rows follow renames, moves and deletes whoever performs them; before, only group managers moved them in the file manager and the browser never did, so renaming a parent folder made everything below it public. The LIKE match no longer catches siblings through _ or %. - ls() was never given the manager's groups, hiding entries they may see. Co-Authored-By: Claude Opus 5.5 --- core/functions/actions/files.php | 168 +++++++++++--- core/src/Support/FileManagerAccess.php | 168 +++++++++++++- core/tests/Unit/FileManagerAccessTest.php | 205 ++++++++++++++++++ .../Unit/Manager/FileManagerAclSourceTest.php | 80 +++++++ manager/actions/files.dynamic.php | 137 ++++++------ manager/media/browser/mcpuk/core/browser.php | 157 +++++++++++++- .../browser/mcpuk/lib/class_zipFolder.php | 11 +- 7 files changed, 814 insertions(+), 112 deletions(-) diff --git a/core/functions/actions/files.php b/core/functions/actions/files.php index d87bfcb12a..48743df9a0 100644 --- a/core/functions/actions/files.php +++ b/core/functions/actions/files.php @@ -35,44 +35,75 @@ function fileManagerUserGroupIds() } } +if(!function_exists('fileManagerAclKey')) { + /** + * The file_groups key of a path given relative to the current manager's file manager root. + * Groups are stored against the site-wide root, so a manager with their own + * filemanager_path still hits the rows everyone else's paths hit. + * + * @param string $relativePath + * @return string|null null when the path lies outside the ACL root + */ + function fileManagerAclKey($relativePath) + { + static $roots = null; + if ($roots === null) { + $userRoot = realpath(evolutionCMS()->getConfig('filemanager_path')) ?: realpath(EVO_BASE_PATH); + $roots = [FileManagerAccess::aclRoot(), rtrim(str_replace('\\', '/', $userRoot), '/')]; + } + + return FileManagerAccess::aclKey($roots[0], $roots[1], $relativePath); + } +} + +if(!function_exists('fileManagerAclApplies')) { + /** + * @return bool false for administrators and when document permissions are off + */ + function fileManagerAclApplies() + { + return evolutionCMS()->getConfig('use_udperms') + && !(isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1); + } +} + if(!function_exists('fileManagerRestrictionMap')) { /** - * @param string[] $relativePaths - * @return array + * @param string[] $relativePaths relative to the manager's root + * @return array keyed by ACL key */ function fileManagerRestrictionMap(array $relativePaths) { - if (!evolutionCMS()->getConfig('use_udperms')) { + if (!fileManagerAclApplies()) { return []; } - if (isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1) { - return []; - } + $keys = array_filter(array_map('fileManagerAclKey', $relativePaths), static fn ($key) => $key !== null); - return FileManagerAccess::loadRestrictions($relativePaths); + return FileManagerAccess::loadRestrictions(array_values($keys)); } } if(!function_exists('fileManagerIsAccessible')) { /** - * @param string $relativePath + * @param string $relativePath relative to the manager's root * @param int[]|null $userGroups - * @param array|null $restrictionMap + * @param array|null $restrictionMap from fileManagerRestrictionMap() * @return bool */ function fileManagerIsAccessible($relativePath, ?array $userGroups = null, ?array $restrictionMap = null) { - if (!evolutionCMS()->getConfig('use_udperms')) { + if (!fileManagerAclApplies()) { return true; } - if (isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1) { + $key = fileManagerAclKey($relativePath); + if ($key === null) { return true; } return FileManagerAccess::isAccessible( - $relativePath, + $key, $userGroups ?? fileManagerUserGroupIds(), $restrictionMap ?? fileManagerRestrictionMap([$relativePath]) ); @@ -81,39 +112,106 @@ function fileManagerIsAccessible($relativePath, ?array $userGroups = null, ?arra if(!function_exists('fileManagerCanModifyExistingPath')) { /** - * @param string $relativePath + * @param string $relativePath relative to the manager's root * @param int[]|null $userGroups - * @param array|null $restrictionMap + * @param array|null $restrictionMap from fileManagerRestrictionMap() * @return bool */ function fileManagerCanModifyExistingPath($relativePath, ?array $userGroups = null, ?array $restrictionMap = null) { - if (!evolutionCMS()->getConfig('use_udperms')) { + if (!fileManagerAclApplies()) { return true; } - if (isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1) { - return true; + // top level is what this manager sees at the top of their own file manager + $relativePath = FileManagerAccess::normalizeRelativePath($relativePath); + + return $relativePath !== '' + && !FileManagerAccess::isTopLevelPath($relativePath) + && fileManagerIsAccessible($relativePath, $userGroups, $restrictionMap); + } +} + +if(!function_exists('fileManagerResolvePath')) { + /** + * Resolve a requested path under the file manager root. The ACL is keyed by the resolved + * path, so callers must check the returned 'relative', never the raw request: own/../private + * names private, not something below own. + * + * @param string $filemanagerPath canonical root, no trailing slash + * @param string $requestedPath path relative to the root, as requested + * @return array{path: string, relative: string}|null null when it does not exist or leaves the root + */ + function fileManagerResolvePath($filemanagerPath, $requestedPath) + { + $path = realpath($filemanagerPath . '/' . ltrim((string) $requestedPath, '/')); + if ($path === false) { + return null; + } + $path = rtrim(str_replace('\\', '/', $path), '/'); + if (!FileManagerAccess::isWithin($filemanagerPath, $path)) { + return null; } - return FileManagerAccess::canModifyExistingPath( - $relativePath, - $userGroups ?? fileManagerUserGroupIds(), - $restrictionMap ?? fileManagerRestrictionMap([$relativePath]) - ); + return [ + 'path' => $path, + 'relative' => FileManagerAccess::normalizeRelativePath(substr($path, strlen(rtrim($filemanagerPath, '/')))), + ]; + } +} + +if(!function_exists('fileManagerSubtreeRestrictionMap')) { + /** + * @param string $relativePath relative to the manager's root + * @return array keyed by ACL key + */ + function fileManagerSubtreeRestrictionMap($relativePath) + { + if (!fileManagerAclApplies()) { + return []; + } + + $key = fileManagerAclKey($relativePath); + if ($key === null) { + return []; + } + + return FileManagerAccess::loadSubtreeRestrictions($key); + } +} + +if(!function_exists('fileManagerHasInaccessibleDescendants')) { + /** + * Whether a recursive operation on $relativePath would reach something the user may not. + * + * @param string $relativePath relative to the manager's root + * @return bool + */ + function fileManagerHasInaccessibleDescendants($relativePath) + { + $restrictions = fileManagerSubtreeRestrictionMap($relativePath); + $key = fileManagerAclKey($relativePath); + + return $restrictions !== [] && $key !== null + && FileManagerAccess::inaccessibleDescendants($key, fileManagerUserGroupIds(), $restrictions) !== []; } } if(!function_exists('fileManagerEffectiveGroupIds')) { /** - * @param string $relativePath - * @param array|null $restrictionMap + * @param string $relativePath relative to the manager's root + * @param array|null $restrictionMap from fileManagerRestrictionMap() * @return int[] */ function fileManagerEffectiveGroupIds($relativePath, ?array $restrictionMap = null) { + $key = fileManagerAclKey($relativePath); + if ($key === null) { + return []; + } + return FileManagerAccess::effectiveGroupIds( - $relativePath, + $key, $restrictionMap ?? fileManagerRestrictionMap([$relativePath]) ); } @@ -267,7 +365,7 @@ function ls($curpath, array $options = []) $dirs_array[$dircounter]['groups'] = ($showFileGroups ?? false) ? '' : ''; @@ -324,7 +422,7 @@ function ls($curpath, array $options = []) $files_array[$filecounter]['groups'] = ($showFileGroups ?? false) ? '' : ''; @@ -538,7 +636,7 @@ function unzip($file, $path) $target = $path . '/' . $filename; // Additional check to ensure target is within path $target_dir = rtrim(str_replace('\\', '/', realpath(dirname($target)) ?: dirname($target)), '/\\'); - if (strpos($target_dir, $path) !== 0) { + if (!FileManagerAccess::isWithin($path, $target_dir)) { continue; } if (substr($filename, -1) == '/') { @@ -592,7 +690,7 @@ function fileupload() $startpath = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_path)); $startpath = rtrim($startpath, '/'); // Ensure startpath is within filemanager_path - if (strpos($startpath, $filemanager_path) !== 0 || !is_dir($startpath)) { + if (!FileManagerAccess::isWithin($filemanager_path, $startpath) || !is_dir($startpath)) { return '

Invalid path.

'; } $dirRel = ltrim(substr($startpath, strlen($filemanager_path)), '/'); @@ -666,8 +764,8 @@ function fileupload() logFileChange('upload', $targetFile); // Inherit groups from parent directory if (!empty($dirGroupIds)) { - $fileRel = ltrim(substr($targetFile, strlen($filemanager_path)), '/'); - foreach ($dirGroupIds as $gid) { + $fileRel = fileManagerAclKey(ltrim(substr($targetFile, strlen($filemanager_path)), '/')); + foreach ($fileRel === null ? [] : $dirGroupIds as $gid) { $inheritInserts[] = ['document_group' => $gid, 'file' => $fileRel]; } } @@ -722,7 +820,7 @@ function textsave() ->getConfig('filemanager_path', EVO_BASE_PATH))), '/'); $requested_path = ltrim($_POST['path'] ?? '', '/'); $filename = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_path)); - if (strpos($filename, $filemanager_path) !== 0 || !is_file($filename)) { + if (!FileManagerAccess::isWithin($filemanager_path, $filename) || !is_file($filename)) { return 'Invalid path.

'; } $fileRel = ltrim(substr($filename, strlen($filemanager_path)), '/'); @@ -757,7 +855,7 @@ function delete_file() ->getConfig('filemanager_path', EVO_BASE_PATH))), '/'); $requested_path = ltrim($_REQUEST['path'] ?? '', '/'); $file = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_path)); - if (strpos($file, $filemanager_path) !== 0 || !is_file($file)) { + if (!FileManagerAccess::isWithin($filemanager_path, $file) || !is_file($file)) { return 'Invalid path.

'; } $fileRel = ltrim(substr($file, strlen($filemanager_path)), '/'); @@ -770,9 +868,7 @@ function delete_file() $msg .= '' . $_lang['file_not_deleted'] . '

'; } else { $msg .= '' . $_lang['file_deleted'] . '

'; - if (evolutionCMS()->getConfig('use_udperms')) { - \EvolutionCMS\Models\FileGroup::query()->where('file', $fileRel)->delete(); - } + \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions(fileManagerAclKey($fileRel)); } // Log the change diff --git a/core/src/Support/FileManagerAccess.php b/core/src/Support/FileManagerAccess.php index 49df5aa316..9354931748 100644 --- a/core/src/Support/FileManagerAccess.php +++ b/core/src/Support/FileManagerAccess.php @@ -13,12 +13,70 @@ public static function normalizeRelativePath(?string $path): string return trim($path, '/'); } + /** + * Whether $path is $root itself or lies below it. A bare prefix test is not enough: with + * the root at assets/alice it would also admit assets/alice-private. + */ + public static function isWithin(string $root, string $path): bool + { + $root = rtrim(str_replace('\\', '/', $root), '/'); + $path = rtrim(str_replace('\\', '/', $path), '/'); + + return $root !== '' && ($path === $root || strncmp($path, $root . '/', strlen($root) + 1) === 0); + } + + /** + * The root file groups are stored against: the site-wide filemanager_path. A manager's own + * filemanager_path only narrows what the file manager shows; keying the ACL by it would + * give the same file a different key per manager, and a key nobody restricted. + */ + public static function aclRoot(): string + { + $evo = evo(); + $root = $evo->configGlobal['filemanager_path'] ?? $evo->getConfig('filemanager_path'); + $root = str_replace('[(base_path)]', EVO_BASE_PATH, (string) $root); + + return rtrim(str_replace('\\', '/', realpath($root) ?: realpath(EVO_BASE_PATH)), '/'); + } + + /** + * The file_groups key of $relativePath, given relative to a manager's own root. + * + * @return string|null null when the path lies outside the ACL root, where no group applies + */ + public static function aclKey(string $aclRoot, string $userRoot, ?string $relativePath): ?string + { + $aclRoot = rtrim(str_replace('\\', '/', $aclRoot), '/'); + $userRoot = rtrim(str_replace('\\', '/', $userRoot), '/'); + $relativePath = self::normalizeRelativePath($relativePath); + + if (self::isWithin($aclRoot, $userRoot)) { + $prefix = self::normalizeRelativePath(substr($userRoot, strlen($aclRoot))); + + return trim($prefix . '/' . $relativePath, '/'); + } + + if (self::isWithin($userRoot, $aclRoot)) { + // the manager sees more than the ACL root: only what lies inside it has a key + $aclPrefix = self::normalizeRelativePath(substr($aclRoot, strlen($userRoot))); + if ($relativePath === $aclPrefix) { + return ''; + } + + return strncmp($relativePath, $aclPrefix . '/', strlen($aclPrefix) + 1) === 0 + ? substr($relativePath, strlen($aclPrefix) + 1) + : null; + } + + return null; + } + public static function getRelativePath(string $fileManagerRoot, string $absolutePath): string { $root = rtrim(str_replace('\\', '/', realpath($fileManagerRoot) ?: $fileManagerRoot), '/'); $path = rtrim(str_replace('\\', '/', realpath($absolutePath) ?: $absolutePath), '/'); - if ($root === '' || strpos($path, $root) !== 0) { + if (!self::isWithin($root, $path)) { return ''; } @@ -66,6 +124,114 @@ public static function loadRestrictions(array $relativePaths): array ->all(); } + /** + * Restrictions on $relativePath, its ancestors and everything below it: what a recursive + * delete or a directory archive has to respect, where loadRestrictions() only covers the + * paths it is given. + */ + public static function loadSubtreeRestrictions(?string $relativePath): array + { + $relativePath = self::normalizeRelativePath($relativePath); + $restrictions = self::loadRestrictions([$relativePath]); + + $query = FileGroup::query(); + if ($relativePath !== '') { + // LIKE reads _ and % in the path as wildcards; the prefix test below drops what they let in + $query->where('file', 'like', $relativePath . '/%'); + } + + foreach ($query->get() as $row) { + $file = self::normalizeRelativePath($row->file); + if (!self::isBelow($relativePath, $file)) { + continue; + } + $restrictions[$file][] = (int) $row->document_group; + } + + return array_map(static fn ($groups) => array_values(array_unique($groups)), $restrictions); + } + + /** + * Carry the groups of $oldKey and everything below it over to $newKey after a rename or + * move. Whoever moved it, the rows have to follow: a row left under the old path protects + * nothing, and the entry at the new path is open to everyone. + */ + public static function moveRestrictions(?string $oldKey, ?string $newKey): void + { + $oldKey = self::normalizeRelativePath($oldKey); + $newKey = self::normalizeRelativePath($newKey); + if ($oldKey === '' || $newKey === '' || $oldKey === $newKey) { + return; + } + + foreach (self::subtreeRows($oldKey) as $row) { + $file = self::normalizeRelativePath($row->file); + $row->update(['file' => $newKey . substr($file, strlen($oldKey))]); + } + } + + /** + * Drop the groups of $key and everything below it once the entry is gone. + */ + public static function forgetRestrictions(?string $key): void + { + $key = self::normalizeRelativePath($key); + if ($key === '') { + return; + } + + $ids = array_map(static fn ($row) => $row->getKey(), self::subtreeRows($key)); + if ($ids !== []) { + FileGroup::query()->whereIn('id', $ids)->delete(); + } + } + + /** + * @return FileGroup[] rows of $key and below; LIKE alone would also match a sibling through _ or % + */ + private static function subtreeRows(string $key): array + { + return FileGroup::query() + ->where('file', $key) + ->orWhere('file', 'like', $key . '/%') + ->get() + ->filter(static function ($row) use ($key) { + $file = self::normalizeRelativePath($row->file); + + return $file === $key || self::isBelow($key, $file); + }) + ->all(); + } + + /** + * Restricted paths strictly below $relativePath that $userGroups may not reach. + * + * @return string[] + */ + public static function inaccessibleDescendants(?string $relativePath, array $userGroups, array $restrictions): array + { + $relativePath = self::normalizeRelativePath($relativePath); + $blocked = []; + + foreach (array_keys($restrictions) as $path) { + $path = self::normalizeRelativePath((string) $path); + if (self::isBelow($relativePath, $path) && !self::isAccessible($path, $userGroups, $restrictions)) { + $blocked[] = $path; + } + } + + return $blocked; + } + + private static function isBelow(string $relativePath, string $path): bool + { + if ($relativePath === '') { + return $path !== ''; + } + + return strncmp($path, $relativePath . '/', strlen($relativePath) + 1) === 0; + } + public static function isAccessible(?string $relativePath, array $userGroups, array $restrictions = []): bool { $userGroups = array_values(array_unique(array_map('intval', $userGroups))); diff --git a/core/tests/Unit/FileManagerAccessTest.php b/core/tests/Unit/FileManagerAccessTest.php index 7485483118..83df9f1b12 100644 --- a/core/tests/Unit/FileManagerAccessTest.php +++ b/core/tests/Unit/FileManagerAccessTest.php @@ -36,3 +36,208 @@ ->and(FileManagerAccess::canModifyExistingPath('articles/one', [10, 11], $restrictions))->toBeTrue() ->and(FileManagerAccess::canModifyExistingPath('articles/one', [10], $restrictions))->toBeFalse(); }); + +test('a root contains itself and what lies below it, not a sibling sharing its prefix', function () { + expect(FileManagerAccess::isWithin('/site/assets/alice', '/site/assets/alice'))->toBeTrue() + ->and(FileManagerAccess::isWithin('/site/assets/alice', '/site/assets/alice/'))->toBeTrue() + ->and(FileManagerAccess::isWithin('/site/assets/alice', '/site/assets/alice/photos/a.jpg'))->toBeTrue() + ->and(FileManagerAccess::isWithin('/site/assets/alice/', '/site/assets/alice/photos'))->toBeTrue() + ->and(FileManagerAccess::isWithin('C:\\site\\alice', 'C:/site/alice/x'))->toBeTrue() + ->and(FileManagerAccess::isWithin('/site/assets/alice', '/site/assets/alice-private'))->toBeFalse() + ->and(FileManagerAccess::isWithin('/site/assets/alice', '/site/assets/alicex/a.jpg'))->toBeFalse() + ->and(FileManagerAccess::isWithin('/site/assets/alice', '/site/assets'))->toBeFalse() + ->and(FileManagerAccess::isWithin('/site/assets/alice', ''))->toBeFalse() + ->and(FileManagerAccess::isWithin('', '/site/assets'))->toBeFalse(); +}); + +function fileManagerSandbox(): string +{ + $root = sys_get_temp_dir() . '/evo-fm-' . bin2hex(random_bytes(4)); + foreach (['alice/own', 'alice/private', 'alice-private'] as $dir) { + mkdir($root . '/' . $dir, 0777, true); + } + file_put_contents($root . '/alice/private/secret.txt', 'x'); + file_put_contents($root . '/alice-private/secret.txt', 'x'); + + return str_replace('\\', '/', realpath($root)); +} + +function removeFileManagerSandbox(string $root): void +{ + $items = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($root, FilesystemIterator::SKIP_DOTS), + RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($items as $item) { + $item->isDir() ? rmdir($item->getPathname()) : unlink($item->getPathname()); + } + rmdir($root); +} + +test('the relative path of a sibling that shares the root prefix is empty, not a path', function () { + $sandbox = fileManagerSandbox(); + + try { + expect(FileManagerAccess::getRelativePath($sandbox . '/alice', $sandbox . '/alice-private/secret.txt'))->toBe('') + ->and(FileManagerAccess::getRelativePath($sandbox . '/alice', $sandbox . '/alice/private/secret.txt'))->toBe('private/secret.txt'); + } finally { + removeFileManagerSandbox($sandbox); + } +}); + +test('a requested path is resolved before the ACL sees it', function () { + $sandbox = fileManagerSandbox(); + $root = $sandbox . '/alice'; + + try { + // the raw path names own/.., whose ancestors carry no restriction; the target is private + expect(fileManagerResolvePath($root, 'own/../private/secret.txt')) + ->toBe(['path' => $root . '/private/secret.txt', 'relative' => 'private/secret.txt']) + ->and(fileManagerResolvePath($root, '/own'))->toBe(['path' => $root . '/own', 'relative' => 'own']) + ->and(fileManagerResolvePath($root, ''))->toBe(['path' => $root, 'relative' => '']) + ->and(fileManagerResolvePath($root, '../alice-private/secret.txt'))->toBeNull() + ->and(fileManagerResolvePath($root, '../../'))->toBeNull() + ->and(fileManagerResolvePath($root, 'missing'))->toBeNull(); + } finally { + removeFileManagerSandbox($sandbox); + } +}); + +test('restricted descendants the user cannot reach are reported, reachable ones are not', function () { + $restrictions = [ + 'shared' => [10], + 'shared/team' => [10], + 'shared/team/hr' => [12], + 'shared/other' => [11], + 'shared-archive/x' => [12], + ]; + + expect(FileManagerAccess::inaccessibleDescendants('shared', [10], $restrictions))->toBe(['shared/team/hr', 'shared/other']) + ->and(FileManagerAccess::inaccessibleDescendants('shared', [10, 11, 12], $restrictions))->toBe([]) + ->and(FileManagerAccess::inaccessibleDescendants('shared/team', [10], $restrictions))->toBe(['shared/team/hr']) + // the folder itself and a prefix-sharing sibling are not descendants + ->and(FileManagerAccess::inaccessibleDescendants('shared/team/hr', [10], $restrictions))->toBe([]) + ->and(FileManagerAccess::inaccessibleDescendants('', [10, 11], $restrictions))->toBe(['shared/team/hr', 'shared-archive/x']); +}); + +test('subtree restrictions cover ancestors and every level below, and nothing beside', function () { + $capsule = new Illuminate\Database\Capsule\Manager(); + $capsule->addConnection(['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => '']); + $capsule->setAsGlobal(); + $capsule->bootEloquent(); + $capsule->schema()->create('file_groups', function ($table) { + $table->increments('id'); + $table->integer('document_group'); + $table->string('file'); + }); + Illuminate\Database\Capsule\Manager::table('file_groups')->insert([ + ['document_group' => 10, 'file' => 'shared'], + ['document_group' => 12, 'file' => 'shared/team/hr/pay.pdf'], + ['document_group' => 11, 'file' => 'shared/team/hr/pay.pdf'], + ['document_group' => 13, 'file' => 'shared_x/y'], + ['document_group' => 14, 'file' => 'sharedx/y'], + ['document_group' => 16, 'file' => 'sharedax/y'], + ['document_group' => 15, 'file' => 'other'], + ]); + + try { + $restrictions = FileManagerAccess::loadSubtreeRestrictions('shared/team'); + + ksort($restrictions); + expect($restrictions)->toBe([ + 'shared' => [10], + 'shared/team/hr/pay.pdf' => [12, 11], + ]) + // nothing to cover below the root means every row + // the _ in shared_x is a LIKE wildcard that also matches sharedax + ->and(FileManagerAccess::loadSubtreeRestrictions('shared_x'))->toBe(['shared_x/y' => [13]]) + ->and(array_keys(FileManagerAccess::loadSubtreeRestrictions('')))->toHaveCount(6); + } finally { + $capsule->getConnection()->disconnect(); + } +})->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); + +test('a path below a personal root is keyed by the site-wide root', function () { + // the admin restricts assets/images/hr; alice's file manager starts at assets + expect(FileManagerAccess::aclKey('/site', '/site/assets', 'images/hr/ceo.png'))->toBe('assets/images/hr/ceo.png') + ->and(FileManagerAccess::aclKey('/site', '/site/assets/', ''))->toBe('assets') + ->and(FileManagerAccess::aclKey('/site', '/site', 'assets/x'))->toBe('assets/x') + ->and(FileManagerAccess::aclKey('/site/', '/site', ''))->toBe('') + ->and(FileManagerAccess::aclKey('C:\\site', 'C:/site/assets', 'a'))->toBe('assets/a'); +}); + +test('a personal root above the site-wide one keys only what lies inside it', function () { + expect(FileManagerAccess::aclKey('/site/public', '/site', 'public/images/a.png'))->toBe('images/a.png') + ->and(FileManagerAccess::aclKey('/site/public', '/site', 'public'))->toBe('') + ->and(FileManagerAccess::aclKey('/site/public', '/site', 'public-old/a.png'))->toBeNull() + ->and(FileManagerAccess::aclKey('/site/public', '/site', 'logs/a.log'))->toBeNull(); +}); + +test('a personal root beside the site-wide one has no keys', function () { + expect(FileManagerAccess::aclKey('/site/public', '/site/public-old', 'a.png'))->toBeNull() + ->and(FileManagerAccess::aclKey('/site/public', '/elsewhere', 'a.png'))->toBeNull(); +}); + +function fileGroupsTable(array $rows): Illuminate\Database\Capsule\Manager +{ + $capsule = new Illuminate\Database\Capsule\Manager(); + $capsule->addConnection(['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => '']); + $capsule->setAsGlobal(); + $capsule->bootEloquent(); + $capsule->schema()->create('file_groups', function ($table) { + $table->increments('id'); + $table->integer('document_group'); + $table->string('file'); + }); + Illuminate\Database\Capsule\Manager::table('file_groups')->insert($rows); + + return $capsule; +} + +function fileGroupRows(): array +{ + return Illuminate\Database\Capsule\Manager::table('file_groups')->orderBy('file')->pluck('file')->all(); +} + +test('groups follow a renamed folder, its descendants included, and nothing beside it', function () { + $capsule = fileGroupsTable([ + ['document_group' => 1, 'file' => 'files/team_a'], + ['document_group' => 1, 'file' => 'files/team_a/hr/pay.pdf'], + ['document_group' => 2, 'file' => 'files/teamXa/other.txt'], + ['document_group' => 2, 'file' => 'files/team_ab'], + ]); + + try { + FileManagerAccess::moveRestrictions('files/team_a', 'files/team_b'); + + // _ is a LIKE wildcard: teamXa and team_ab must stay where they are + expect(fileGroupRows())->toBe(['files/teamXa/other.txt', 'files/team_ab', 'files/team_b', 'files/team_b/hr/pay.pdf']); + + FileManagerAccess::moveRestrictions('files/team_b', 'files/team_b'); + FileManagerAccess::moveRestrictions('', 'files/x'); + expect(fileGroupRows())->toHaveCount(4); + } finally { + $capsule->getConnection()->disconnect(); + } +})->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); + +test('groups of a removed entry and everything below it are dropped, and nothing beside it', function () { + $capsule = fileGroupsTable([ + ['document_group' => 1, 'file' => 'files/team_a'], + ['document_group' => 1, 'file' => 'files/team_a/hr/pay.pdf'], + ['document_group' => 2, 'file' => 'files/teamXa/other.txt'], + ['document_group' => 2, 'file' => 'files/team_ab'], + ]); + + try { + FileManagerAccess::forgetRestrictions('files/team_a'); + expect(fileGroupRows())->toBe(['files/teamXa/other.txt', 'files/team_ab']); + + // the ACL root is never dropped wholesale + FileManagerAccess::forgetRestrictions(''); + FileManagerAccess::forgetRestrictions(null); + expect(fileGroupRows())->toHaveCount(2); + } finally { + $capsule->getConnection()->disconnect(); + } +})->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); diff --git a/core/tests/Unit/Manager/FileManagerAclSourceTest.php b/core/tests/Unit/Manager/FileManagerAclSourceTest.php index 8293456d39..b2b849ab6b 100644 --- a/core/tests/Unit/Manager/FileManagerAclSourceTest.php +++ b/core/tests/Unit/Manager/FileManagerAclSourceTest.php @@ -26,6 +26,86 @@ public function testKcfinderSourceDoesNotBypassWriteAclForFileManagerPermission( self::assertStringContainsString("'files' => \$this->getEntries(\$this->session['dir'])", $source); } + public function testClassicManagerChecksTheResolvedPathOfEveryNamedEntry(): void + { + $source = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/actions/files.dynamic.php'); + + // a bare prefix test admits a sibling such as alice-private next to the root alice + self::assertDoesNotMatchRegularExpression('/strpos\(\$\w+, \$filemanager_path\)/', $source); + self::assertStringContainsString('FileManagerAccess::isWithin($filemanager_path, $startpath)', $source); + + // the raw request is not what the ACL is keyed by: own/../private names private + self::assertStringNotContainsString('fileManagerCanModifyExistingPath($requested_', $source); + self::assertStringNotContainsString('$fileGroupsMap) || !is_writable', $source); + foreach (['$folderTarget', '$fileTarget', '$dirTarget'] as $target) { + self::assertStringContainsString("fileManagerCanModifyExistingPath({$target}['relative'], \$userGroups)", $source); + } + self::assertStringContainsString("fileManagerIsAccessible(\$viewTarget['relative'], \$userGroups)", $source); + self::assertStringContainsString("\$groupsTargetPath = \$groupsTarget['relative'];", $source); + + self::assertStringContainsString("fileManagerHasInaccessibleDescendants(\$folderTarget['relative'])", $source); + self::assertStringContainsString("fileManagerIsAccessible(\$zipTarget['relative'], \$userGroups)", $source); + self::assertStringContainsString('$subtreeGroupsMap = fileManagerSubtreeRestrictionMap($relative_path);', $source); + self::assertStringContainsString("'allDocGroups', 'userGroups')), EXTR_OVERWRITE);", $source); + } + + public function testKcfinderChecksFileGroupsOnEveryReadAndChangeOfANamedEntry(): void + { + $source = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/media/browser/mcpuk/core/browser.php'); + + $acts = [ + 'act_thumb' => 'isPathAccessible("{$this->typeDir}/{$this->get[\'dir\']}/$file")', + 'act_download' => '!$this->isPathAccessible($file)', + 'act_rename' => '!$this->isPathAccessible($file)', + 'act_delete' => '!$this->isPathAccessible($file)', + 'act_deleteDir' => '$this->hasInaccessibleDescendants($dir)', + 'act_cp_cbd' => '!$this->isPathAccessible($path)', + 'act_mv_cbd' => '!$this->isPathAccessible($path)', + 'act_rm_cbd' => '!$this->isPathAccessible($path)', + 'act_downloadDir' => 'new zipFolder($file, $dir, null, $this->subtreeAccessFilter($dir));', + 'act_downloadSelected' => '!$this->isPathAccessible($file)', + 'act_downloadClipboard' => '!$this->isPathAccessible($file)', + ]; + foreach ($acts as $act => $check) { + self::assertSame(1, preg_match('/function ' . $act . '\(\)\n \{\n(.*?)\n \}\n/s', $source, $body), "$act not found"); + self::assertStringContainsString($check, $body[1], "$act does not check file groups"); + } + } + + public function testFileGroupsAreKeyedBySiteRootAndFollowEveryRenameAndDelete(): void + { + $classic = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/actions/files.dynamic.php'); + $helpers = (string) file_get_contents(dirname(__DIR__, 4) . '/core/functions/actions/files.php'); + $browser = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/media/browser/mcpuk/core/browser.php'); + + // a personal filemanager_path must not change which rows a path hits + self::assertStringContainsString('FileManagerAccess::aclKey($roots[0], $roots[1], $relativePath)', $helpers); + self::assertStringContainsString("\$groupsKey = \$groupsTarget === null ? null : fileManagerAclKey(\$groupsTargetPath);", $classic); + self::assertStringContainsString("FileGroup::query()->where('file', \$groupsKey)->get();", $classic); + self::assertStringNotContainsString("where('file', \$groupsTargetPath)", $classic); + self::assertStringContainsString('\EvolutionCMS\Support\FileManagerAccess::aclRoot(),', $browser); + self::assertStringNotContainsString("getConfig('filemanager_path', EVO_BASE_PATH)", $browser); + + // rows follow the entry whoever renames or deletes it, not only group managers + self::assertSame(2, substr_count($classic, 'FileManagerAccess::moveRestrictions(')); + self::assertStringContainsString("FileManagerAccess::forgetRestrictions(fileManagerAclKey(\$folderTarget['relative']));", $classic); + self::assertStringContainsString('FileManagerAccess::forgetRestrictions(fileManagerAclKey($fileRel));', $helpers); + self::assertStringNotContainsString("orWhere('file', 'like'", $classic); + + $acts = [ + 'act_renameDir' => '$this->moveFileGroups($oldKey,', + 'act_rename' => '$this->moveFileGroups($oldKey, $newName);', + 'act_mv_cbd' => '$this->moveFileGroups($oldKey, "$dir/$base");', + 'act_delete' => '$this->forgetFileGroups($oldKey);', + 'act_rm_cbd' => '$this->forgetFileGroups($oldKey);', + 'act_deleteDir' => '$this->forgetFileGroups($oldKey);', + ]; + foreach ($acts as $act => $call) { + self::assertSame(1, preg_match('/function ' . $act . '\(\)\n \{\n(.*?)\n \}\n/s', $browser, $body), "$act not found"); + self::assertStringContainsString($call, $body[1], "$act leaves file groups behind"); + } + } + public function testClassicManagerHelpersExposeAclEnforcementHooks(): void { $source = (string) file_get_contents(dirname(__DIR__, 4) . '/core/functions/actions/files.php'); diff --git a/manager/actions/files.dynamic.php b/manager/actions/files.dynamic.php index c4c4466e48..33e3ab3426 100755 --- a/manager/actions/files.dynamic.php +++ b/manager/actions/files.dynamic.php @@ -82,7 +82,9 @@ } else { $startpath = $filemanager_path; } -if ($startpath === false || strpos($startpath, $filemanager_path) !== 0 || !is_readable($startpath)) { +if ($startpath === false + || !\EvolutionCMS\Support\FileManagerAccess::isWithin($filemanager_path, $startpath) + || !is_readable($startpath)) { evo()->webAlertAndQuit($_lang["files_access_denied"]); } // Raymond: get web start path for showing pictures @@ -258,7 +260,9 @@ function fileManagerCreateDirectoryZip( $directoryZipMessage = '' . $_lang['files_zip_in_progress'] . '

'; } else { fclose($lockHandle); - if (fileManagerCreateDirectoryZip($startpath, $relative_path, $filemanager_path, $directoryZipPaths['zip'], $userGroups, $fileGroupsMap, $protected_path)) { + // $fileGroupsMap only covers this folder's own entries; the archive walks the whole subtree + $subtreeGroupsMap = fileManagerSubtreeRestrictionMap($relative_path); + if (fileManagerCreateDirectoryZip($startpath, $relative_path, $filemanager_path, $directoryZipPaths['zip'], $userGroups, $subtreeGroupsMap, $protected_path)) { register_shutdown_function('fileManagerDeleteDirectoryZip', $directoryZipPaths); while (ob_get_level() > 0) { ob_end_clean(); @@ -418,10 +422,12 @@ function renameFile(file) { } elseif (get_by_key($_POST, 'mode') == 'savegroups') { if ($token_check) { if ($showFileGroups) { - $groupsTargetPath = ltrim($_POST['groupspath'] ?? $_POST['path'] ?? '', '/'); - // Validate path is within filemanager_path - $fullGroupsPath = str_replace('\\', '/', realpath($filemanager_path . '/' . $groupsTargetPath) ?: ($filemanager_path . '/' . $groupsTargetPath)); - if (strpos($fullGroupsPath, $filemanager_path) !== 0) { + // Rows are keyed by the resolved path, so a/../b must be stored and checked as b + $groupsTarget = fileManagerResolvePath($filemanager_path, $_POST['groupspath'] ?? $_POST['path'] ?? ''); + $groupsTargetPath = $groupsTarget['relative'] ?? ''; + // and by the site-wide root, whatever this manager's own root is + $groupsKey = $groupsTarget === null ? null : fileManagerAclKey($groupsTargetPath); + if ($groupsKey === null) { echo 'Invalid path.

'; } elseif (!fileManagerIsAccessible($groupsTargetPath, $userGroups)) { echo '' . $_lang['files_access_denied'] . '

'; @@ -429,7 +435,7 @@ function renameFile(file) { $submittedGroups = isset($_POST['docgroups']) ? (array)$_POST['docgroups'] : []; $chkAllFiles = isset($_POST['chkallfiles']) && $_POST['chkallfiles'] === 'on'; $canManageAllGroups = evo()->hasPermission('manage_groups'); - $existingFG = \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsTargetPath)->get(); + $existingFG = \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsKey)->get(); $existingGroupIds = $existingFG->pluck('document_group')->map(static fn ($groupId) => (int)$groupId)->all(); $manageableExistingGroups = $canManageAllGroups ? $existingGroupIds @@ -441,7 +447,7 @@ function renameFile(file) { echo '' . $_lang['files_access_denied'] . '

'; } elseif ($chkAllFiles) { // Make public: only full group managers may remove all restrictions - \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsTargetPath)->delete(); + \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsKey)->delete(); } else { // Determine which group IDs are submitted $submittedGroupIds = []; @@ -454,14 +460,14 @@ function renameFile(file) { } $submittedGroupIds[] = $gid; if (!in_array($gid, $existingGroupIds)) { - $insertRows[] = ['document_group' => $gid, 'file' => $groupsTargetPath]; + $insertRows[] = ['document_group' => $gid, 'file' => $groupsKey]; } } // Delete removed groups only from the set the current user manages $toDelete = array_diff($manageableExistingGroups, $submittedGroupIds); if (!empty($toDelete)) { - \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsTargetPath) + \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsKey) ->whereIn('document_group', $toDelete) ->delete(); } @@ -539,7 +545,7 @@ function safe_unzip($file, $path) { } $target = $path . '/' . $filename; $target_real = rtrim(str_replace('\\', '/', realpath(dirname($target)) ?: dirname($target)), '/\\'); - if (strpos($target_real, $path) !== 0) { + if (!\EvolutionCMS\Support\FileManagerAccess::isWithin($path, $target_real)) { continue; } if (substr($filename, -1) == '/') { @@ -561,11 +567,14 @@ function safe_unzip($file, $path) { // Unzip .zip files - by Raymond, with safe_unzip if ($enablefileunzip && get_by_key($_REQUEST, 'mode') == 'unzip' && $currentPathWritable) { if ($token_check) { - $zipfile = str_replace('\\', '/', realpath($startpath . '/' . $_REQUEST['file'])); - if (strpos($zipfile, $filemanager_path) !== 0) { + $zipTarget = fileManagerResolvePath($filemanager_path, $relative_path . '/' . ($_REQUEST['file'] ?? '')); + if ($zipTarget === null || !is_file($zipTarget['path'])) { echo 'Invalid path.

'; + } elseif (!fileManagerIsAccessible($zipTarget['relative'], $userGroups) || !is_readable($zipTarget['path'])) { + // Unpacking a restricted archive into this folder would publish its contents here + echo '' . $_lang['files_access_denied'] . '

'; } else { - $success = safe_unzip($zipfile, $startpath); + $success = safe_unzip($zipTarget['path'], $startpath); if (!$success) { echo '' . $_lang['file_unzip_fail'] . '

'; } else { @@ -579,7 +588,10 @@ function safe_unzip($file, $path) { foreach ($iterator as $f) { if ($f->isFile()) { $fp = str_replace('\\', '/', $f->getPathname()); - $relP = ltrim(substr($fp, strlen($filemanager_path)), '/'); + $relP = fileManagerAclKey(ltrim(substr($fp, strlen($filemanager_path)), '/')); + if ($relP === null) { + continue; + } foreach ($dirGroupIds as $gid) { $insertRows[] = ['document_group' => $gid, 'file' => $relP]; } @@ -602,22 +614,19 @@ function safe_unzip($file, $path) { // Delete Folder if (get_by_key($_REQUEST, 'mode') == 'deletefolder') { if ($token_check) { - $requested_folderpath = ltrim($_REQUEST['folderpath'] ?? '', '/'); - $folder = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_folderpath)); - if (strpos($folder, $filemanager_path) !== 0 || !is_dir($folder)) { + $folderTarget = fileManagerResolvePath($filemanager_path, $_REQUEST['folderpath'] ?? ''); + $folder = $folderTarget['path'] ?? ''; + if ($folderTarget === null || !is_dir($folder)) { echo 'Invalid path.

'; - } elseif (!fileManagerCanModifyExistingPath($requested_folderpath, $userGroups, $fileGroupsMap) || !is_writable($folder)) { + } elseif (!fileManagerCanModifyExistingPath($folderTarget['relative'], $userGroups) + || fileManagerHasInaccessibleDescendants($folderTarget['relative']) + || !is_writable($folder)) { echo '' . $_lang['files_access_denied'] . '

'; } elseif (!@rrmdir($folder)) { echo '' . $_lang['file_folder_not_deleted'] . '

'; } else { echo '' . $_lang['file_folder_deleted'] . '

'; - if ($showFileGroups) { - $delRelPath = ltrim(substr($folder, strlen($filemanager_path)), '/'); - \EvolutionCMS\Models\FileGroup::query()->where('file', $delRelPath) - ->orWhere('file', 'like', $delRelPath . '/%') - ->delete(); - } + \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions(fileManagerAclKey($folderTarget['relative'])); } } else { echo 'Invalid token

'; @@ -673,11 +682,11 @@ function safe_unzip($file, $path) { if (get_by_key($_REQUEST, 'mode') == 'duplicate') { if ($token_check) { $old_umask = umask(0); - $requested_file = ltrim($_REQUEST['path'] ?? '', '/'); - $filename = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_file)); - if (strpos($filename, $filemanager_path) !== 0 || !is_file($filename)) { + $fileTarget = fileManagerResolvePath($filemanager_path, $_REQUEST['path'] ?? ''); + $filename = $fileTarget['path'] ?? ''; + if ($fileTarget === null || !is_file($filename)) { echo 'Invalid path.

'; - } elseif (!fileManagerCanModifyExistingPath($requested_file, $userGroups, $fileGroupsMap) || !is_writable($filename)) { + } elseif (!fileManagerCanModifyExistingPath($fileTarget['relative'], $userGroups) || !is_writable($filename)) { echo '' . $_lang['files_access_denied'] . '

'; } else { $newFilename = str_replace([ '..\\', '../', '\\', '/' ], '', $_REQUEST['newFilename']); @@ -702,11 +711,11 @@ function safe_unzip($file, $path) { if (get_by_key($_REQUEST, 'mode') == 'renameFolder') { if ($token_check) { $old_umask = umask(0); - $requested_dir = ltrim($_REQUEST['path'] ?? '', '/'); - $dirname = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_dir . '/' . $_REQUEST['dirname'])); - if (strpos($dirname, $filemanager_path) !== 0 || !is_dir($dirname)) { + $dirTarget = fileManagerResolvePath($filemanager_path, ltrim($_REQUEST['path'] ?? '', '/') . '/' . ($_REQUEST['dirname'] ?? '')); + $dirname = $dirTarget['path'] ?? ''; + if ($dirTarget === null || !is_dir($dirname)) { echo 'Invalid path.

'; - } elseif (!fileManagerCanModifyExistingPath(trim($requested_dir . '/' . $_REQUEST['dirname'], '/'), $userGroups, $fileGroupsMap) || !is_writable($dirname)) { + } elseif (!fileManagerCanModifyExistingPath($dirTarget['relative'], $userGroups) || !is_writable($dirname)) { echo '' . $_lang['files_access_denied'] . '

'; } else { $newDirname = str_replace([ '..\\', '../', '\\', '/' ], '', $_REQUEST['newDirname']); @@ -715,18 +724,11 @@ function safe_unzip($file, $path) { } else if (!rename($dirname, dirname($dirname) . '/' . $newDirname)) { echo '', $_lang['file_folder_not_created'], '

'; } else { - if ($showFileGroups) { - $oldRelPath = ltrim(substr($dirname, strlen($filemanager_path)), '/'); - $newRelPath = ltrim(substr(dirname($dirname) . '/' . $newDirname, strlen($filemanager_path)), '/'); - \EvolutionCMS\Models\FileGroup::query()->where('file', $oldRelPath) - ->update(['file' => $newRelPath]); - $oldPrefix = $oldRelPath . '/'; - $newPrefix = $newRelPath . '/'; - $affected = \EvolutionCMS\Models\FileGroup::query()->where('file', 'like', $oldPrefix . '%')->get(); - foreach ($affected as $fg) { - $fg->update(['file' => $newPrefix . substr($fg->file, strlen($oldPrefix))]); - } - } + // whoever renames it, the groups follow, or everything below turns public + \EvolutionCMS\Support\FileManagerAccess::moveRestrictions( + fileManagerAclKey(ltrim(substr($dirname, strlen($filemanager_path)), '/')), + fileManagerAclKey(ltrim(substr(dirname($dirname) . '/' . $newDirname, strlen($filemanager_path)), '/')) + ); } umask($old_umask); } @@ -738,11 +740,11 @@ function safe_unzip($file, $path) { if (get_by_key($_REQUEST, 'mode') == 'renameFile') { if ($token_check) { $old_umask = umask(0); - $requested_file = ltrim($_REQUEST['path'] ?? '', '/'); - $filename = str_replace('\\', '/', realpath($filemanager_path . '/' . $requested_file)); - if (strpos($filename, $filemanager_path) !== 0 || !is_file($filename)) { + $fileTarget = fileManagerResolvePath($filemanager_path, $_REQUEST['path'] ?? ''); + $filename = $fileTarget['path'] ?? ''; + if ($fileTarget === null || !is_file($filename)) { echo 'Invalid path.

'; - } elseif (!fileManagerCanModifyExistingPath($requested_file, $userGroups, $fileGroupsMap) || !is_writable($filename)) { + } elseif (!fileManagerCanModifyExistingPath($fileTarget['relative'], $userGroups) || !is_writable($filename)) { echo '' . $_lang['files_access_denied'] . '

'; } else { $path = dirname($filename); @@ -755,12 +757,10 @@ function safe_unzip($file, $path) { if (!rename($filename, $path . '/' . $newFilename)) { echo $_lang['files.dynamic.php5']; } else { - if ($showFileGroups) { - $oldRelPath = ltrim(substr($filename, strlen($filemanager_path)), '/'); - $newRelPath = ltrim(substr($path . '/' . $newFilename, strlen($filemanager_path)), '/'); - \EvolutionCMS\Models\FileGroup::query()->where('file', $oldRelPath) - ->update(['file' => $newRelPath]); - } + \EvolutionCMS\Support\FileManagerAccess::moveRestrictions( + fileManagerAclKey($fileTarget['relative']), + fileManagerAclKey(ltrim(substr($path . '/' . $newFilename, strlen($filemanager_path)), '/')) + ); } umask($old_umask); } @@ -786,7 +786,7 @@ function safe_unzip($file, $path) { - ' . $_lang['files_directory_is_empty'] . ' '; } ?> @@ -854,14 +854,22 @@ function safe_unzip($file, $path) {
:
webAlertAndQuit('Invalid path.'); + } + $groupsTargetPath = $groupsTarget['relative']; + $groupsKey = fileManagerAclKey($groupsTargetPath); + if ($groupsKey === null) { + evo()->webAlertAndQuit('Invalid path.'); + } + if (!fileManagerIsAccessible($groupsTargetPath, $userGroups)) { evo()->webAlertAndQuit($_lang["files_access_denied"]); } - $existingFileGroups = \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsTargetPath)->get(); + $existingFileGroups = \EvolutionCMS\Models\FileGroup::query()->where('file', $groupsKey)->get(); $directGroupIds = $existingFileGroups->pluck('document_group')->map(static fn ($groupId) => (int)$groupId)->all(); - $effectiveGroupIds = fileManagerEffectiveGroupIds($groupsTargetPath, $fileGroupsMap); + $effectiveGroupIds = fileManagerEffectiveGroupIds($groupsTargetPath); $effectiveGroupNames = empty($effectiveGroupIds) ? [$_lang['all_file_groups']] : $allDocGroups->whereIn('id', $effectiveGroupIds)->pluck('name')->toArray(); @@ -959,11 +967,12 @@ function makeFilesPublic(b) {
webAlertAndQuit("Invalid path."); } - if (!fileManagerIsAccessible($requested_path, $userGroups, $fileGroupsMap)) { + if (!fileManagerIsAccessible($viewTarget['relative'], $userGroups)) { evo()->webAlertAndQuit($_lang["files_access_denied"]); } $buffer = file_get_contents($filename); diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index e78888762d..aa61c90892 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -187,6 +187,10 @@ protected function act_thumb() if (basename($file) != $file) { $this->sendDefaultThumb(); } + // a cached thumbnail is still a picture of the original + if (!$this->isPathAccessible("{$this->typeDir}/{$this->get['dir']}/$file")) { + $this->sendDefaultThumb(); + } $file = "{$this->thumbsDir}/{$this->type}/{$this->get['dir']}/$file"; if (!is_file($file) || !is_readable($file)) { $file = "{$this->config['uploadDir']}/{$this->type}/{$this->get['dir']}/" . basename($file); @@ -310,9 +314,11 @@ protected function act_renameDir() if (is_array($evtOut) && !empty($evtOut)) { $this->errorMsg(implode('\n', $evtOut)); } + $oldKey = $this->getFileGroupsRelPath($dir); if (!@rename($dir, dirname($dir) . "/$newName")) { $this->errorMsg("Cannot rename the folder."); } + $this->moveFileGroups($oldKey, dirname($dir) . "/$newName"); $thumbDir = "$this->thumbsTypeDir/{$this->post['dir']}"; if (is_dir($thumbDir)) { @rename($thumbDir, dirname($thumbDir) . "/$newName"); @@ -347,6 +353,10 @@ protected function act_deleteDir() if (!dir::isWritable($dir)) { $this->errorMsg("Cannot delete the folder."); } + // prune() takes everything below with it, including entries the listing hid + if ($this->hasInaccessibleDescendants($dir)) { + $this->errorMsg("You don't have permissions to write to this folder."); + } $evtOut = $this->modx->invokeEvent('OnBeforeFileBrowserDelete', [ 'element' => 'dir', @@ -356,7 +366,11 @@ protected function act_deleteDir() die(json_encode(['error' => $evtOut])); } + $oldKey = $this->getFileGroupsRelPath($dir); $result = !dir::prune($dir, false); + if (!is_dir($dir)) { + $this->forgetFileGroups($oldKey); + } if (is_array($result) && count($result)) { $this->errorMsg("Failed to delete {count} files/folders.", ['count' => count($result)]); @@ -409,7 +423,8 @@ protected function act_download() !isset($this->post['file']) || strpos($this->post['file'], '../') !== false || (false === ($file = "$dir/{$this->post['file']}")) || - !file_exists($file) || !is_readable($file) + !file_exists($file) || !is_readable($file) || + !$this->isPathAccessible($file) ) { $this->errorMsg("Unknown error."); } @@ -445,6 +460,9 @@ protected function act_rename() ) { $this->errorMsg("Unknown error."); } + if (!$this->isPathAccessible($file)) { + $this->errorMsg("You don't have permissions to write to this folder."); + } if (isset($this->config['denyExtensionRename']) && $this->config['denyExtensionRename'] && @@ -483,9 +501,11 @@ protected function act_rename() if (is_array($evtOut) && !empty($evtOut)) { $this->errorMsg(implode('\n', $evtOut)); } + $oldKey = $this->getFileGroupsRelPath($file); if (!@rename($file, $newName)) { $this->errorMsg("Unknown error."); } + $this->moveFileGroups($oldKey, $newName); $this->modx->invokeEvent('OnFileBrowserRename', [ 'element' => 'file', 'filepath' => $dir, @@ -517,7 +537,8 @@ protected function act_delete() !isset($this->post['file']) || strpos($this->post['file'], '../') !== false || (false === ($file = "$dir/{$this->post['file']}")) || - !file_exists($file) || !is_readable($file) || !file::isWritable($file) + !file_exists($file) || !is_readable($file) || !file::isWritable($file) || + !$this->isPathAccessible($file) ) { $this->errorMsg("Cannot delete '{file}'.", ['file' => basename($file)]); } @@ -532,7 +553,10 @@ protected function act_delete() die(json_encode(['error' => $evtOut])); } - @unlink($file); + $oldKey = $this->getFileGroupsRelPath($file); + if (@unlink($file)) { + $this->forgetFileGroups($oldKey); + } $thumb = "{$this->thumbsTypeDir}/{$this->post['dir']}/{$this->post['file']}"; if (file_exists($thumb)) { @@ -580,6 +604,11 @@ protected function act_cp_cbd() $path = "{$this->config['uploadDir']}/$file"; $base = basename($file); $replace = ['file' => $base]; + if (!$this->isPathAccessible($path)) { + // a copy into an open folder would hand out a file the listing hides + $error[] = $this->label("Cannot read '{file}'.", $replace); + continue; + } $ext = file::getExtension($base); $evtOut = $this->modx->invokeEvent('OnBeforeFileBrowserCopy', [ 'oldpath' => $path, @@ -664,6 +693,11 @@ protected function act_mv_cbd() $path = "{$this->config['uploadDir']}/$file"; $base = basename($file); $replace = ['file' => $base]; + if (!$this->isPathAccessible($path)) { + $error[] = $this->label("Cannot move '{file}'.", $replace); + continue; + } + $oldKey = $this->getFileGroupsRelPath($path); $ext = file::getExtension($base); $evtOut = $this->modx->invokeEvent('OnBeforeFileBrowserMove', [ 'oldpath' => $path, @@ -685,6 +719,7 @@ protected function act_mv_cbd() } elseif (!file::isWritable($path) || !@rename($path, "$dir/$base")) { $error[] = $this->label("Cannot move '{file}'.", $replace); } else { + $this->moveFileGroups($oldKey, "$dir/$base"); if (function_exists("chmod")) { @chmod("$dir/$base", $this->config['filePerms']); } @@ -744,7 +779,9 @@ protected function act_rm_cbd() $base = basename($file); $filepath = str_replace('/' . $base, '', $path); $replace = ['file' => $base]; - if (!is_file($path)) { + if (!$this->isPathAccessible($path)) { + $error[] = $this->label("Cannot delete '{file}'.", $replace); + } elseif (!is_file($path)) { $error[] = $this->label("The file '{file}' does not exist.", $replace); } else { $evtOut = $this->modx->invokeEvent('OnBeforeFileBrowserDelete', [ @@ -756,9 +793,11 @@ protected function act_rm_cbd() if (is_array($evtOut) && !empty($evtOut)) { $error[] = implode("\n", $evtOut); } else { + $oldKey = $this->getFileGroupsRelPath($path); if (!@unlink($path)) { $error[] = $this->label("Cannot delete '{file}'.", $replace); } else { + $this->forgetFileGroups($oldKey); $this->modx->invokeEvent('OnFileBrowserDelete', [ 'element' => 'file', 'filename' => $base, @@ -785,7 +824,7 @@ protected function act_rm_cbd() protected function act_downloadDir() { $dir = $this->postDir(); - if (!isset($this->post['dir']) || $this->config['denyZipDownload']) { + if (!isset($this->post['dir']) || $this->config['denyZipDownload'] || !$this->isPathAccessible($dir)) { $this->errorMsg("Unknown error."); } $filename = basename($dir) . ".zip"; @@ -793,7 +832,7 @@ protected function act_downloadDir() $file = md5(time() . session_id()); $file = "{$this->config['uploadDir']}/$file.zip"; } while (file_exists($file)); - new zipFolder($file, $dir); + new zipFolder($file, $dir, null, $this->subtreeAccessFilter($dir)); header("Content-Type: application/x-zip"); header('Content-Disposition: attachment; filename="' . str_replace('"', "_", $filename) . '"'); header("Content-Length: " . filesize($file)); @@ -823,7 +862,7 @@ protected function act_downloadSelected() continue; } $file = "$dir/$file"; - if (!is_file($file) || !is_readable($file)) { + if (!is_file($file) || !is_readable($file) || !$this->isPathAccessible($file)) { continue; } $zipFiles[] = $file; @@ -874,7 +913,7 @@ protected function act_downloadClipboard() continue; } $file = $this->config['uploadDir'] . "/$file"; - if (!is_file($file) || !is_readable($file)) { + if (!is_file($file) || !is_readable($file) || !$this->isPathAccessible($file)) { continue; } $zipFiles[] = $file; @@ -1244,8 +1283,9 @@ protected function output($data = null, $template = null) */ protected function getFileGroupsRelPath(string $absPath): string { + // the site-wide root, not the manager's own filemanager_path: groups are keyed by it return \EvolutionCMS\Support\FileManagerAccess::getRelativePath( - $this->modx->getConfig('filemanager_path', EVO_BASE_PATH), + \EvolutionCMS\Support\FileManagerAccess::aclRoot(), $absPath ); } @@ -1320,6 +1360,105 @@ protected function isWriteAllowed($relDir) return \EvolutionCMS\Support\FileManagerAccess::isAccessible($relPath, $userGroups, $rows); } + /** + * Whether the current manager user may reach this file or folder. The listing hides what + * this refuses; every act that reads or changes a named entry has to ask as well, or a + * hidden entry is one crafted request away. + * + * @param string $absPath + * @return bool + */ + protected function isPathAccessible($absPath) + { + if (isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1) { + return true; + } + if (!$this->modx->getConfig('use_udperms')) { + return true; + } + + $relPath = $this->getFileGroupsRelPath($absPath); + if ($relPath === '') { + return true; + } + $userGroups = array_map('intval', (array)($_SESSION['mgrDocgroups'] ?? [])); + + return \EvolutionCMS\Support\FileManagerAccess::isAccessible( + $relPath, + $userGroups, + \EvolutionCMS\Support\FileManagerAccess::loadRestrictions([$relPath]) + ); + } + + /** + * A filter for everything below $absDir that the current manager user may reach, built + * from one query instead of one per entry. + * + * @param string $absDir + * @return callable(string): bool + */ + protected function subtreeAccessFilter($absDir) + { + if ((isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1) || !$this->modx->getConfig('use_udperms')) { + return static fn ($path) => true; + } + + $restrictions = \EvolutionCMS\Support\FileManagerAccess::loadSubtreeRestrictions($this->getFileGroupsRelPath($absDir)); + $userGroups = array_map('intval', (array)($_SESSION['mgrDocgroups'] ?? [])); + + return function ($path) use ($restrictions, $userGroups) { + $relPath = $this->getFileGroupsRelPath($path); + + return $relPath === '' + || \EvolutionCMS\Support\FileManagerAccess::isAccessible($relPath, $userGroups, $restrictions); + }; + } + + /** + * Carry the file groups of a renamed or moved entry over to its new path. The rows have to + * follow whoever moved it, or everything below turns public at the new path. + * + * @param string $oldKey getFileGroupsRelPath() of the old path, taken before the move + * @param string $newAbsPath + */ + protected function moveFileGroups($oldKey, $newAbsPath) + { + \EvolutionCMS\Support\FileManagerAccess::moveRestrictions($oldKey, $this->getFileGroupsRelPath($newAbsPath)); + } + + /** + * @param string $oldKey getFileGroupsRelPath() of the removed path, taken before removal + */ + protected function forgetFileGroups($oldKey) + { + \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions($oldKey); + } + + /** + * Whether pruning $absDir would take something with it the current manager user may not reach. + * + * @param string $absDir + * @return bool + */ + protected function hasInaccessibleDescendants($absDir) + { + if ((isset($_SESSION['mgrRole']) && (int)$_SESSION['mgrRole'] === 1) || !$this->modx->getConfig('use_udperms')) { + return false; + } + + $relPath = $this->getFileGroupsRelPath($absDir); + if ($relPath === '') { + return false; + } + $userGroups = array_map('intval', (array)($_SESSION['mgrDocgroups'] ?? [])); + + return \EvolutionCMS\Support\FileManagerAccess::inaccessibleDescendants( + $relPath, + $userGroups, + \EvolutionCMS\Support\FileManagerAccess::loadSubtreeRestrictions($relPath) + ) !== []; + } + /** * Like isWriteAllowed(), but also requires the path to be strictly inside * the type directory (i.e. at least one segment deep), preventing structural diff --git a/manager/media/browser/mcpuk/lib/class_zipFolder.php b/manager/media/browser/mcpuk/lib/class_zipFolder.php index 1be6fb816d..a74b966e3c 100755 --- a/manager/media/browser/mcpuk/lib/class_zipFolder.php +++ b/manager/media/browser/mcpuk/lib/class_zipFolder.php @@ -17,9 +17,15 @@ class zipFolder { protected $zip; protected $root; protected $ignored; + protected $filter; - function __construct($file, $folder, $ignored=null) { + /** + * @param callable|null $filter receives each absolute path; entries it refuses are left + * out, a refused folder with everything below it + */ + function __construct($file, $folder, $ignored=null, ?callable $filter=null) { $this->zip = new ZipArchive(); + $this->filter = $filter; $this->ignored = is_array($ignored) ? $ignored @@ -47,7 +53,8 @@ function zip($folder, $parent=null) { foreach ($dir as $file) if (!$file->isDot()) { $filename = $file->getFilename(); - if (!in_array($filename, $this->ignored)) { + if (!in_array($filename, $this->ignored) + && ($this->filter === null || call_user_func($this->filter, "$full_path/$filename"))) { if ($file->isDir()) $this->zip($filename, "$zip_path/"); else From f8c2fcf830407756fe42e693bfad41151a519ac3 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:01:33 +0200 Subject: [PATCH 2/4] fix(file-browser): require a permission for every type and refuse an unusable root - browse.php checked assets_images/assets_files only for type=images and type=files. media, image, file, a missing type and an unknown one (both open the files folder) needed nothing. Every type now maps to a permission; file_manager still opens all of them. - A manager whose image_base_upload_dir could not be resolved (outside the site, or climbing out with ..) was silently given the whole rb_base_dir, with every other manager's folders in it. The browser now refuses, before the uploader creates folders or prunes archives under an empty root. - The root test was a bare prefix match; assets2 was taken for a folder below assets and got the URL assets/2. - The refusal printed an empty page: there is no global $_lang in the browser. It now uses the translator and answers 403. Co-Authored-By: Claude Opus 5.5 --- core/src/Support/FileBrowserAccess.php | 83 ++++++++++ .../Unit/Security/FileBrowserAccessTest.php | 145 ++++++++++++++++++ core/vendor/composer/autoload_classmap.php | 1 + core/vendor/composer/autoload_static.php | 1 + manager/media/browser/mcpuk/browse.php | 15 +- manager/media/browser/mcpuk/config.php | 52 +------ manager/media/browser/mcpuk/core/uploader.php | 7 + 7 files changed, 256 insertions(+), 48 deletions(-) create mode 100644 core/src/Support/FileBrowserAccess.php create mode 100644 core/tests/Unit/Security/FileBrowserAccessTest.php diff --git a/core/src/Support/FileBrowserAccess.php b/core/src/Support/FileBrowserAccess.php new file mode 100644 index 0000000000..9e236de085 --- /dev/null +++ b/core/src/Support/FileBrowserAccess.php @@ -0,0 +1,83 @@ +getPathname()); + } + rmdir($root); + } +} + +describe('FileBrowserAccess::permissionFor()', function () { + + test('image types need assets_images', function () { + expect(FileBrowserAccess::permissionFor('images'))->toBe('assets_images') + ->and(FileBrowserAccess::permissionFor('image'))->toBe('assets_images'); + }); + + test('every other type, a missing one and an unknown one need assets_files', function (?string $type) { + expect(FileBrowserAccess::permissionFor($type))->toBe('assets_files'); + })->with(['files', 'file', 'media', null, '', 'flash', 'Images', '../images']); +}); + +describe('FileBrowserAccess::resolveUploadRoot()', function () { + + test('no personal root means rb_base_dir', function () { + $site = fileBrowserSite(); + try { + expect(FileBrowserAccess::resolveUploadRoot('[(base_path)]assets/', 'assets/', '', $site, '/')) + ->toBe([$site . 'assets', 'assets']); + } finally { + removeFileBrowserSite($site); + } + }); + + test('a relative personal root lies below rb_base_dir, existing or not', function () { + $site = fileBrowserSite(); + try { + expect(FileBrowserAccess::resolveUploadRoot('[(base_path)]assets/', 'assets/', 'alice', $site, '/')) + ->toBe([$site . 'assets/alice', 'assets/alice']) + ->and(FileBrowserAccess::resolveUploadRoot('[(base_path)]assets/', 'assets/', 'carol/', $site, '/')) + ->toBe([$site . 'assets/carol', 'assets/carol']); + } finally { + removeFileBrowserSite($site); + } + }); + + test('an absolute personal root inside the site gets a URL relative to the site', function () { + $site = fileBrowserSite(); + try { + expect(FileBrowserAccess::resolveUploadRoot('[(base_path)]assets/', 'assets/', '[(base_path)]uploads/bob', $site, '/evo/')) + ->toBe([$site . 'uploads/bob', '/evo/uploads/bob']); + } finally { + removeFileBrowserSite($site); + } + }); + + test('a sibling sharing the prefix of rb_base_dir is not taken for a folder below it', function () { + $site = fileBrowserSite(); + try { + // the prefix test used to answer [.../assets2, 'assets/2'] + expect(FileBrowserAccess::resolveUploadRoot('[(base_path)]assets/', 'assets/', '[(base_path)]assets2', $site, '')) + ->toBe([$site . 'assets2', '/assets2']); + } finally { + removeFileBrowserSite($site); + } + }); + + test('a personal root that cannot be used refuses instead of falling back to rb_base_dir', function (string $custom) { + $site = fileBrowserSite(); + try { + $custom = str_replace('{outside}', rtrim($site, '/') . '-outside', $custom); + expect(FileBrowserAccess::resolveUploadRoot('[(base_path)]assets/', 'assets/', $custom, $site, '/'))->toBeNull(); + } finally { + removeFileBrowserSite($site); + } + })->with([ + 'outside the site' => ['{outside}'], + 'filesystem root' => ['/'], + 'climbing out, relative' => ['../../'], + 'climbing out, missing folder' => ['alice/../../../nowhere'], + 'climbing back in' => ['alice/../images'], + ]); +}); + +describe('call sites', function () { + + test('browse.php asks for a permission for every type', function () { + $source = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/media/browser/mcpuk/browse.php'); + + expect($source) + ->toContain('FileBrowserAccess::permissionFor(') + ->toContain("!EvolutionCMS()->hasPermission('file_manager') && !EvolutionCMS()->hasPermission(\$typePermission)") + ->not->toContain("\$_GET['type'] == 'images'") + // there is no global $_lang in the browser; the refusal used to be an empty 200 + ->toContain("header('HTTP/1.1 403 Forbidden');") + ->toContain("sprintf(__('global.files_management_no_permission'), \$role)") + ->not->toContain('global $_lang;'); + expect(strpos($source, 'permissionFor('))->toBeLessThan(strpos($source, 'new browser(')); + }); + + test('an unresolvable root disables the browser before it touches the disk', function () { + $config = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/media/browser/mcpuk/config.php'); + $uploader = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/media/browser/mcpuk/core/uploader.php'); + + expect($config) + ->toContain('FileBrowserAccess::resolveUploadRoot(') + ->toContain("'disabled' => \$uploadRoot === null,"); + expect($uploader)->toContain("__('global.files_management_no_permission'), __('global.image_base_upload_dir_title')"); + $guard = strpos($uploader, "if (!empty(\$this->config['disabled'])) {"); + expect($guard)->toBeGreaterThan(0) + ->and($guard)->toBeLessThan(strpos($uploader, '@mkdir($this->config[\'uploadDir\']')); + }); +}); diff --git a/core/vendor/composer/autoload_classmap.php b/core/vendor/composer/autoload_classmap.php index da4da0b094..5f9a00a21f 100644 --- a/core/vendor/composer/autoload_classmap.php +++ b/core/vendor/composer/autoload_classmap.php @@ -1412,6 +1412,7 @@ 'EvolutionCMS\\Support\\DocumentSave\\SaveResponse' => $baseDir . '/src/Support/DocumentSave/SaveResponse.php', 'EvolutionCMS\\Support\\DocumentSave\\TemplateVariableInput' => $baseDir . '/src/Support/DocumentSave/TemplateVariableInput.php', 'EvolutionCMS\\Support\\DocumentSave\\TemplateVariableValues' => $baseDir . '/src/Support/DocumentSave/TemplateVariableValues.php', + 'EvolutionCMS\\Support\\FileBrowserAccess' => $baseDir . '/src/Support/FileBrowserAccess.php', 'EvolutionCMS\\Support\\FileManagerAccess' => $baseDir . '/src/Support/FileManagerAccess.php', 'EvolutionCMS\\Support\\Formatter\\CSSMinify' => $baseDir . '/src/Support/Formatter/CSSMinify.php', 'EvolutionCMS\\Support\\Formatter\\HtmlFormatter' => $baseDir . '/src/Support/Formatter/HtmlFormatter.php', diff --git a/core/vendor/composer/autoload_static.php b/core/vendor/composer/autoload_static.php index 9add00eab6..9a2d61511f 100644 --- a/core/vendor/composer/autoload_static.php +++ b/core/vendor/composer/autoload_static.php @@ -2089,6 +2089,7 @@ class ComposerStaticInit925fea465a58fa69f06ccf2629003e87 'EvolutionCMS\\Support\\DocumentSave\\SaveResponse' => __DIR__ . '/../..' . '/src/Support/DocumentSave/SaveResponse.php', 'EvolutionCMS\\Support\\DocumentSave\\TemplateVariableInput' => __DIR__ . '/../..' . '/src/Support/DocumentSave/TemplateVariableInput.php', 'EvolutionCMS\\Support\\DocumentSave\\TemplateVariableValues' => __DIR__ . '/../..' . '/src/Support/DocumentSave/TemplateVariableValues.php', + 'EvolutionCMS\\Support\\FileBrowserAccess' => __DIR__ . '/../..' . '/src/Support/FileBrowserAccess.php', 'EvolutionCMS\\Support\\FileManagerAccess' => __DIR__ . '/../..' . '/src/Support/FileManagerAccess.php', 'EvolutionCMS\\Support\\Formatter\\CSSMinify' => __DIR__ . '/../..' . '/src/Support/Formatter/CSSMinify.php', 'EvolutionCMS\\Support\\Formatter\\HtmlFormatter' => __DIR__ . '/../..' . '/src/Support/Formatter/HtmlFormatter.php', diff --git a/manager/media/browser/mcpuk/browse.php b/manager/media/browser/mcpuk/browse.php index 588cc7c612..af595a1b41 100755 --- a/manager/media/browser/mcpuk/browse.php +++ b/manager/media/browser/mcpuk/browse.php @@ -15,13 +15,20 @@ require "core/autoload.php"; // Init MODX function returnNoPermissionsMessage($role) { - global $_lang; - echo sprintf($_lang['files_management_no_permission'], $role); + // there is no global $_lang here; it used to print an empty page + header('HTTP/1.1 403 Forbidden'); + echo sprintf(__('global.files_management_no_permission'), $role); exit; } -if( $_GET['type'] == 'images' && !EvolutionCMS()->hasPermission('file_manager') && !EvolutionCMS()->hasPermission('assets_images')) returnNoPermissionsMessage('assets_images'); -if( $_GET['type'] == 'files' && !EvolutionCMS()->hasPermission('file_manager') && !EvolutionCMS()->hasPermission('assets_files')) returnNoPermissionsMessage('assets_files'); +// Every type needs a permission: media, image and file used to open for anyone, and a missing or +// unknown type opens the files folder. +$typePermission = \EvolutionCMS\Support\FileBrowserAccess::permissionFor( + isset($_GET['type']) && is_string($_GET['type']) ? $_GET['type'] : null +); +if (!EvolutionCMS()->hasPermission('file_manager') && !EvolutionCMS()->hasPermission($typePermission)) { + returnNoPermissionsMessage($typePermission); +} // Only the page itself and the thumbnails it embeds are fetched without a token; every other // act, including reads, is scripted through browser.baseGetData() and carries the session one. diff --git a/manager/media/browser/mcpuk/config.php b/manager/media/browser/mcpuk/config.php index 71e328794d..61c134ed99 100755 --- a/manager/media/browser/mcpuk/config.php +++ b/manager/media/browser/mcpuk/config.php @@ -18,55 +18,19 @@ $modx = evolutionCMS(); -$resolveUploadRoot = static function (string $defaultDir, string $defaultUrl, string $customDir): array { - $defaultDir = rtrim(str_replace('\\', '/', str_replace('[(base_path)]', EVO_BASE_PATH, $defaultDir)), '/'); - $defaultUrl = rtrim(str_replace('\\', '/', $defaultUrl), '/'); - $customDir = trim($customDir); - - if ($customDir === '') { - return [$defaultDir, $defaultUrl]; - } - - $customDir = str_replace('[(base_path)]', EVO_BASE_PATH, $customDir); - $customDir = str_replace('\\', '/', $customDir); - - if (!preg_match('/^(?:[A-Za-z]:[\/\\\\]|\/)/', $customDir)) { - $customDir = $defaultDir . '/' . ltrim($customDir, '/'); - } - - $customDir = rtrim($customDir, '/'); - $resolvedDir = str_replace('\\', '/', realpath($customDir) ?: $customDir); - - if ($defaultDir !== '' && strpos($resolvedDir, $defaultDir) === 0) { - $relativePath = ltrim(substr($resolvedDir, strlen($defaultDir)), '/'); - return [ - $resolvedDir, - $relativePath === '' ? $defaultUrl : $defaultUrl . '/' . $relativePath - ]; - } - - $siteBasePath = rtrim(str_replace('\\', '/', EVO_BASE_PATH), '/'); - $siteBaseUrl = rtrim(str_replace('\\', '/', EVO_BASE_URL), '/'); - if ($siteBasePath !== '' && strpos($resolvedDir, $siteBasePath) === 0) { - $relativePath = ltrim(substr($resolvedDir, strlen($siteBasePath)), '/'); - return [ - $resolvedDir, - $relativePath === '' ? ($siteBaseUrl === '' ? '/' : $siteBaseUrl) : - ($siteBaseUrl === '' ? '' : $siteBaseUrl) . '/' . $relativePath - ]; - } - - return [$defaultDir, $defaultUrl]; -}; - -[$uploadDir, $uploadUrl] = $resolveUploadRoot( +// A manager confined by image_base_upload_dir whose folder cannot be resolved gets no browser +// at all rather than the shared rb_base_dir with everyone else's files in it. +$uploadRoot = \EvolutionCMS\Support\FileBrowserAccess::resolveUploadRoot( (string) EvolutionCMS()->getConfig('rb_base_dir'), (string) EvolutionCMS()->getConfig('rb_base_url'), - (string) EvolutionCMS()->getConfig('image_base_upload_dir', '') + (string) EvolutionCMS()->getConfig('image_base_upload_dir', ''), + EVO_BASE_PATH, + EVO_BASE_URL ); +[$uploadDir, $uploadUrl] = $uploadRoot ?? ['', '']; $_CONFIG = [ - 'disabled' => false, + 'disabled' => $uploadRoot === null, 'denyZipDownload' => EvolutionCMS()->getConfig('denyZipDownload'), 'denyExtensionRename' => EvolutionCMS()->getConfig('denyExtensionRename'), 'showHiddenFiles' => EvolutionCMS()->getConfig('showHiddenFiles'), diff --git a/manager/media/browser/mcpuk/core/uploader.php b/manager/media/browser/mcpuk/core/uploader.php index 2d3590eb0d..8069b8f522 100755 --- a/manager/media/browser/mcpuk/core/uploader.php +++ b/manager/media/browser/mcpuk/core/uploader.php @@ -181,6 +181,13 @@ public function __construct(DocumentParser $modx) } else $this->session = &$_SESSION; + // config.php disables the browser when a confined manager's root cannot be resolved; stop + // here, before the code below creates folders and prunes archives under an empty root + if (!empty($this->config['disabled'])) { + header('HTTP/1.1 403 Forbidden'); + die(sprintf(__('global.files_management_no_permission'), __('global.image_base_upload_dir_title'))); + } + // IMAGE DRIVER INIT if (isset($this->config['imageDriversPriority'])) { $this->config['imageDriversPriority'] = From 34f9c881b43dfb376c738820c37f8e95275fac4f Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:01:39 +0200 Subject: [PATCH 3/4] fix(manager): check resource access when saving an edited document The editor page refuses a document the manager's groups do not reach, but a posted save only checked save_document, and the parent only when it changed. A manager who knew a restricted document's id and parent could overwrite it. The save service now checks the document itself when use_udperms is on. Co-Authored-By: Claude Opus 5.5 --- .../DocumentSave/DocumentSaveContext.php | 9 ++++ core/src/Services/DocumentSaveService.php | 4 ++ core/tests/Support/DocumentSaveDatabase.php | 2 + .../Unit/Services/DocumentSaveServiceTest.php | 41 +++++++++++++++++++ 4 files changed, 56 insertions(+) diff --git a/core/src/Services/DocumentSave/DocumentSaveContext.php b/core/src/Services/DocumentSave/DocumentSaveContext.php index f778de6bf9..e24c3f4523 100644 --- a/core/src/Services/DocumentSave/DocumentSaveContext.php +++ b/core/src/Services/DocumentSave/DocumentSaveContext.php @@ -25,6 +25,7 @@ final class DocumentSaveContext * @param Closure(string):int $toTimestamp * @param Closure(int):array $parentIds * @param Closure(int):bool $canCreateIn udperms check for a parent + * @param Closure(int):bool $canEdit udperms check for the document being edited * @param Closure():array $userGroupsLoader document groups the user is a member of, read fresh * @param Closure(string):string $lang */ @@ -40,6 +41,7 @@ public function __construct( private readonly Closure $toTimestamp, private readonly Closure $parentIds, private readonly Closure $canCreateIn, + private readonly Closure $canEdit, private readonly Closure $userGroupsLoader, private readonly Closure $lang, ) { @@ -64,6 +66,8 @@ public static function fromManager(): self toTimestamp: fn (string $date) => (int) $evo->toTimeStamp($date), parentIds: fn (int $id) => (array) $evo->getParentIds($id), canCreateIn: fn (int $parent) => Permissions::canCreateIn($parent), + // udperms asks the same of a document as of a parent: can this user reach it + canEdit: fn (int $id) => Permissions::canCreateIn($id), userGroupsLoader: fn () => array_values(array_unique(array_map('intval', MemberGroup::query() ->join('membergroup_access', 'membergroup_access.membergroup', '=', 'member_groups.user_group') ->where('member_groups.member', $userId) @@ -115,6 +119,11 @@ public function canCreateIn(int $parent): bool return ($this->canCreateIn)($parent); } + public function canEdit(int $id): bool + { + return $id > 0 && ($this->canEdit)($id); + } + /** * Document groups the user belongs to, queried once per save. * diff --git a/core/src/Services/DocumentSaveService.php b/core/src/Services/DocumentSaveService.php index c4696ae2c9..d30ef82752 100644 --- a/core/src/Services/DocumentSaveService.php +++ b/core/src/Services/DocumentSaveService.php @@ -60,6 +60,10 @@ public function save(array $input): DocumentSaveResult if ($existing === null) { throw new DocumentSaveDenied($ctx->lang('error_no_results'), false); } + // the editor page checks this before showing the form; a posted save has to as well + if ($usePermissions && !$ctx->canEdit($id)) { + throw new DocumentSaveDenied($ctx->lang('access_permission_denied'), false); + } } $alias = $this->resolveAlias((string) ($input['alias'] ?? ''), $pagetitle, $id, $parent); diff --git a/core/tests/Support/DocumentSaveDatabase.php b/core/tests/Support/DocumentSaveDatabase.php index dc8007ee7f..3f3a63f1eb 100644 --- a/core/tests/Support/DocumentSaveDatabase.php +++ b/core/tests/Support/DocumentSaveDatabase.php @@ -137,6 +137,7 @@ public static function context( ?callable $snapshot = null, int $now = 1_700_000_000, ?callable $canCreateIn = null, + ?callable $canEdit = null, ): DocumentSaveContext { return new DocumentSaveContext( userId: 7, @@ -162,6 +163,7 @@ public static function context( return $ids; }, canCreateIn: $canCreateIn ?? fn (int $parent) => true, + canEdit: $canEdit ?? fn (int $id) => true, userGroupsLoader: fn () => $userGroups, lang: fn (string $key) => $key === 'duplicate_alias_found' ? 'duplicate %s %s' : $key, ); diff --git a/core/tests/Unit/Services/DocumentSaveServiceTest.php b/core/tests/Unit/Services/DocumentSaveServiceTest.php index 3d54360d1b..2c18a4adb1 100644 --- a/core/tests/Unit/Services/DocumentSaveServiceTest.php +++ b/core/tests/Unit/Services/DocumentSaveServiceTest.php @@ -256,6 +256,47 @@ function dbSnapshot(): array ->and((int) Capsule::table('site_content')->where('id', 2)->value('parent'))->toBe(1); })->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); +test('an edit of a document the user cannot reach is refused even when the parent stays', function () { + bootSaveFixture(); + $asked = []; + $context = DocumentSaveDatabase::context( + config: ['use_udperms' => 1], + permissions: ['publish_document'], + role: 2, + userGroups: [3], + canEdit: function (int $id) use (&$asked) { + $asked[] = $id; + return $id !== 2; + }, + ); + $service = new DocumentSaveService($context); + + $save = fn () => $service->save(saveForm(['id' => '2', 'mode' => '27', 'pagetitle' => 'hijacked', 'alias' => 'page', 'parent' => '1'])); + + expect($save)->toThrow(DocumentSaveDenied::class, 'access_permission_denied') + ->and($asked)->toBe([2]) + ->and(Capsule::table('site_content')->where('id', 2)->value('pagetitle'))->toBe('page'); + + // a reachable document still saves, and a new one is not an edit + $service->save(saveForm(['id' => '4', 'mode' => '27', 'pagetitle' => 'renamed', 'alias' => 'other', 'parent' => '0'])); + $service->save(saveForm(['pagetitle' => 'fresh'])); + expect($asked)->toBe([2, 4]) + ->and(Capsule::table('site_content')->where('id', 4)->value('pagetitle'))->toBe('renamed'); +})->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); + +test('without use_udperms the edit is not checked against document groups', function () { + bootSaveFixture(); + $service = new DocumentSaveService(DocumentSaveDatabase::context( + permissions: ['publish_document'], + role: 2, + canEdit: fn (int $id) => false, + )); + + $service->save(saveForm(['id' => '2', 'mode' => '27', 'pagetitle' => 'edited', 'alias' => 'page', 'parent' => '1'])); + + expect(Capsule::table('site_content')->where('id', 2)->value('pagetitle'))->toBe('edited'); +})->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); + test('a non administrator cannot post a group list without one of their own groups', function () { bootSaveFixture(); $service = new DocumentSaveService(DocumentSaveDatabase::context(config: ['use_udperms' => 1], role: 2, userGroups: [3])); From f27d7d5c00bcd8e6bcb6261e5b998977bbc69dbc Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:02:33 +0200 Subject: [PATCH 4/4] fix(deps): add Tracy ConnectionTiming to the committed classmap The committed autoloader is classmap-authoritative, and EvolutionCMS\Tracy\ConnectionTiming was missing from it, so TracyServiceProvider failed with "class not found" on a site while tests, with a regenerated vendor/, passed. A test now reads the committed classmap from git and requires every class under core/src in it. Co-Authored-By: Claude Opus 5.5 --- core/tests/Unit/CommittedClassmapTest.php | 49 ++++++++++++++++++++++ core/vendor/composer/autoload_classmap.php | 1 + core/vendor/composer/autoload_static.php | 1 + 3 files changed, 51 insertions(+) create mode 100644 core/tests/Unit/CommittedClassmapTest.php diff --git a/core/tests/Unit/CommittedClassmapTest.php b/core/tests/Unit/CommittedClassmapTest.php new file mode 100644 index 0000000000..0d2e5cc6b7 --- /dev/null +++ b/core/tests/Unit/CommittedClassmapTest.php @@ -0,0 +1,49 @@ +getExtension() !== 'php') { + continue; + } + $source = (string) file_get_contents($file->getPathname()); + if (!preg_match('/^\s*(?:(?:final|abstract|readonly)\s+)*(?:class|interface|trait|enum)\s+\w+/m', $source)) { + continue; + } + $relative = str_replace('\\', '/', substr($file->getPathname(), strlen($root . '/src/'))); + $names[] = 'EvolutionCMS\\' . str_replace('/', '\\', substr($relative, 0, -4)); + } + sort($names); + + return $names; +} + +test('every class under core/src is in the committed classmap', function (string $map) { + $committed = shell_exec('git -C ' . escapeshellarg(dirname(__DIR__, 2)) . ' show HEAD:core/vendor/composer/' . $map . ' 2>&1'); + if (!is_string($committed) || !str_contains($committed, 'EvolutionCMS\\\\')) { + $this->markTestSkipped('no git checkout to read the committed classmap from'); + } + + preg_match_all("/^\s*'(EvolutionCMS\\\\\\\\[^']+)' =>/m", $committed, $matches); + $classmap = array_flip(array_map('stripslashes', $matches[1])); + + $missing = array_values(array_filter(srcClassNames(), static fn ($class) => !isset($classmap[$class]))); + + expect($missing)->toBe([]); +})->with(['autoload_classmap.php', 'autoload_static.php']); diff --git a/core/vendor/composer/autoload_classmap.php b/core/vendor/composer/autoload_classmap.php index 5f9a00a21f..d0ab936a4d 100644 --- a/core/vendor/composer/autoload_classmap.php +++ b/core/vendor/composer/autoload_classmap.php @@ -1432,6 +1432,7 @@ 'EvolutionCMS\\Support\\SystemSettingPathNormalizer' => $baseDir . '/src/Support/SystemSettingPathNormalizer.php', 'EvolutionCMS\\Support\\TemplateFileEngines' => $baseDir . '/src/Support/TemplateFileEngines.php', 'EvolutionCMS\\TemplateProcessor' => $baseDir . '/src/TemplateProcessor.php', + 'EvolutionCMS\\Tracy\\ConnectionTiming' => $baseDir . '/src/Tracy/ConnectionTiming.php', 'EvolutionCMS\\Tracy\\Debugger' => $baseDir . '/src/Tracy/Debugger.php', 'EvolutionCMS\\Tracy\\Panels\\AbstractPanel' => $baseDir . '/src/Tracy/Panels/AbstractPanel.php', 'EvolutionCMS\\Tracy\\Panels\\Auth\\Panel' => $baseDir . '/src/Tracy/Panels/Auth/Panel.php', diff --git a/core/vendor/composer/autoload_static.php b/core/vendor/composer/autoload_static.php index 9a2d61511f..46575f039a 100644 --- a/core/vendor/composer/autoload_static.php +++ b/core/vendor/composer/autoload_static.php @@ -2109,6 +2109,7 @@ class ComposerStaticInit925fea465a58fa69f06ccf2629003e87 'EvolutionCMS\\Support\\SystemSettingPathNormalizer' => __DIR__ . '/../..' . '/src/Support/SystemSettingPathNormalizer.php', 'EvolutionCMS\\Support\\TemplateFileEngines' => __DIR__ . '/../..' . '/src/Support/TemplateFileEngines.php', 'EvolutionCMS\\TemplateProcessor' => __DIR__ . '/../..' . '/src/TemplateProcessor.php', + 'EvolutionCMS\\Tracy\\ConnectionTiming' => __DIR__ . '/../..' . '/src/Tracy/ConnectionTiming.php', 'EvolutionCMS\\Tracy\\Debugger' => __DIR__ . '/../..' . '/src/Tracy/Debugger.php', 'EvolutionCMS\\Tracy\\Panels\\AbstractPanel' => __DIR__ . '/../..' . '/src/Tracy/Panels/AbstractPanel.php', 'EvolutionCMS\\Tracy\\Panels\\Auth\\Panel' => __DIR__ . '/../..' . '/src/Tracy/Panels/Auth/Panel.php',