From d13dccb89fe3abdfedf4768ee207e065902f7b5f Mon Sep 17 00:00:00 2001 From: MiddleSokilAI Date: Mon, 2 Nov 2026 19:52:09 +0200 Subject: [PATCH 1/2] fix(file-browser): preserve group permissions for shared storage --- core/src/Support/FileBrowserAccess.php | 139 +++++++++++ core/tests/Fixtures/shared-browser-access.php | 223 ++++++++++++++++++ .../Unit/Security/SharedBrowserAccessTest.php | 115 +++++++++ manager/includes/config_check.inc.php | 15 +- manager/media/browser/mcpuk/core/browser.php | 65 ++++- 5 files changed, 545 insertions(+), 12 deletions(-) create mode 100644 core/tests/Fixtures/shared-browser-access.php create mode 100644 core/tests/Unit/Security/SharedBrowserAccessTest.php diff --git a/core/src/Support/FileBrowserAccess.php b/core/src/Support/FileBrowserAccess.php index 9e236de085..2953fd3852 100644 --- a/core/src/Support/FileBrowserAccess.php +++ b/core/src/Support/FileBrowserAccess.php @@ -80,4 +80,143 @@ public static function resolveUploadRoot( return null; } + + /** + * Find the site-side spelling of a configured upload URL, without trusting it as a grant. + * Callers must still compare its real directory with the configured physical upload root. + * Remote URLs, traversal and encoded paths cannot establish an ACL identity. + * + * @return string|null Absolute site-side path, which may end at a deployment symlink. + * @since 3.6.0 + */ + public static function localUploadPath(string $url, string $basePath, string $baseUrl, string $siteUrl): ?string + { + $parts = parse_url($url); + $site = parse_url($siteUrl); + if ($parts === false || $site === false || isset($parts['query']) || isset($parts['fragment']) + || isset($parts['user']) || isset($parts['pass']) + || (isset($parts['scheme']) && !in_array(strtolower($parts['scheme']), ['http', 'https'], true)) + || (isset($parts['host']) && (strcasecmp($parts['host'], $site['host'] ?? '') !== 0 + || ($parts['port'] ?? null) !== ($site['port'] ?? null)))) { + return null; + } + + $path = $parts['path'] ?? ''; + if (strpbrk($path, "\\%\0:") !== false || in_array('..', explode('/', $path), true) + || \fileManagerRefuseReservedName($path)) { + return null; + } + $baseUrl = '/' . trim($baseUrl, '/'); + if (str_starts_with($path, '/')) { + if ($baseUrl !== '/' && !FileManagerAccess::isWithin($baseUrl, $path)) { + return null; + } + $path = $baseUrl === '/' ? ltrim($path, '/') : ltrim(substr($path, strlen($baseUrl)), '/'); + } + + $root = realpath($basePath); + if ($root === false) { + return null; + } + $segments = array_filter(explode('/', $path), static fn ($part) => $part !== '' && $part !== '.'); + + return rtrim(str_replace('\\', '/', $root), '/') . '/' . implode('/', $segments); + } + + /** + * Keep file_groups keys stable when a deployment exposes a configured shared type root. + * Only the type root may be an alias: the browser must separately reject every symlink + * below it. The site-side root is accepted only when realpath proves it is the same folder. + * No matching root returns null; callers must not interpret that as a public file. + * + * @return string|null Key relative to the site-wide ACL root; an empty key is that root itself. + * @since 3.6.0 + */ + public static function resolveAclPath(string $aclRoot, string $typeDir, string $absolutePath, ?string $localTypeDir = null): ?string + { + foreach ([$aclRoot, $typeDir, $absolutePath, $localTypeDir ?? ''] as $path) { + if (strpos($path, "\0") !== false || \fileManagerRefuseReservedName($path)) { + return null; + } + } + $root = realpath($aclRoot); + $typeRoot = realpath($typeDir); + if ($root === false || $typeRoot === false) { + return null; + } + $root = rtrim(str_replace('\\', '/', $root), '/'); + $typeRoot = rtrim(str_replace('\\', '/', $typeRoot), '/'); + $typeDir = rtrim(str_replace('\\', '/', $typeDir), '/'); + $absolutePath = rtrim(str_replace('\\', '/', $absolutePath), '/'); + $spelling = FileManagerAccess::isWithin($typeDir, $absolutePath) ? $typeDir : $typeRoot; + if (!FileManagerAccess::isWithin($spelling, $absolutePath)) { + return null; + } + $suffix = ltrim(substr($absolutePath, strlen($spelling)), '/'); + if (in_array('..', explode('/', $suffix), true) || strpos($suffix, "\0") !== false + || \fileManagerPathContainsLink($typeDir, $absolutePath)) { + return null; + } + $existing = realpath($absolutePath); + if ($existing !== false) { + $existing = str_replace('\\', '/', $existing); + if (!FileManagerAccess::isWithin($typeRoot, $existing)) { + return null; + } + $suffix = ltrim(substr($existing, strlen($typeRoot)), '/'); + } else { + $suffix = implode('/', array_filter(explode('/', $suffix), static fn ($part) => $part !== '' && $part !== '.')); + } + + if (FileManagerAccess::isWithin($root, $typeRoot)) { + $key = ltrim(substr($typeRoot, strlen($root)), '/'); + } elseif ($localTypeDir !== null && FileManagerAccess::isWithin($root, $localTypeDir) + && ($localRoot = realpath($localTypeDir)) !== false + && FileManagerAccess::isWithin($typeRoot, str_replace('\\', '/', $localRoot)) + && FileManagerAccess::isWithin(str_replace('\\', '/', $localRoot), $typeRoot)) { + $key = ltrim(substr($localTypeDir, strlen($root)), '/'); + } else { + return null; + } + + return trim($key . '/' . $suffix, '/'); + } + + /** + * Diagnose an upload root without opening it to an unrecognised ACL namespace. + * External shared roots need a proven site-side identity for every existing browser type. + * This read-only check never creates folders or replaces the browser's operation guards. + * + * @since 3.6.0 + */ + public static function isUsableRoot(string $uploadDir, string $uploadUrl, string $aclRoot, string $basePath, string $baseUrl, string $siteUrl): bool + { + $uploadDir = str_replace('[(base_path)]', $basePath, $uploadDir); + $aclRoot = str_replace('[(base_path)]', $basePath, $aclRoot); + if (strpos($uploadDir . $aclRoot, "\0") !== false + || \fileManagerRefuseReservedName($uploadDir) || \fileManagerRefuseReservedName($aclRoot)) { + return false; + } + if (!is_dir($uploadDir) || !is_readable($uploadDir) || !is_dir($aclRoot) || !is_readable($aclRoot)) { + return false; + } + if (self::resolveAclPath($aclRoot, $uploadDir, $uploadDir) !== null) { + return true; + } + + $found = false; + foreach (['files', 'images', 'file', 'image', 'media'] as $type) { + $typeDir = rtrim($uploadDir, '/\\') . '/' . $type; + if (!is_dir($typeDir)) { + continue; + } + $found = true; + $local = self::localUploadPath(rtrim($uploadUrl, '/') . '/' . $type, $basePath, $baseUrl, $siteUrl); + if (self::resolveAclPath($aclRoot, $typeDir, $typeDir, $local) === null) { + return false; + } + } + + return $found; + } } diff --git a/core/tests/Fixtures/shared-browser-access.php b/core/tests/Fixtures/shared-browser-access.php new file mode 100644 index 0000000000..64cff8b429 --- /dev/null +++ b/core/tests/Fixtures/shared-browser-access.php @@ -0,0 +1,223 @@ +config[$key] ?? ['filemanager_path' => EVO_BASE_PATH, 'use_udperms' => 1][$key] ?? $default; + } + + /** + * Keep the dashboard checksum check outside this filesystem regression fixture. + * + * @since 3.6.0 + */ + public function getManagerApi(): self + { + return $this; + } + + /** + * The isolated site has no tracked CMS checksum changes. + * + * @since 3.6.0 + */ + public function checkSystemChecksum(): string + { + return '0'; + } + + /** + * Capture refusals for assertions without writing a CMS event log. + * + * @since 3.6.0 + */ + public function logEvent($id, $type, $message, $source): void + { + $this->log[] = $message; + } +} + +$GLOBALS['evo'] = new SharedBrowserStub(); +/** + * Supply the fixture CMS host to the unmodified browser helpers. + * + * @since 3.6.0 + */ +function evo() { return $GLOBALS['evo']; } +/** + * Supply the legacy spelling used by the refusal logger. + * + * @since 3.6.0 + */ +function evolutionCMS() { return evo(); } + +require dirname(__DIR__, 2) . '/vendor/autoload.php'; +$browserDir = dirname(__DIR__, 3) . '/manager/media/browser/mcpuk/'; +require $browserDir . 'lib/helper_path.php'; +require $browserDir . 'lib/helper_dir.php'; +require $browserDir . 'core/uploader.php'; +require $browserDir . 'core/browser.php'; + +try { + if (!@symlink($tmp . '/shared/assets/images', EVO_BASE_PATH . 'assets/images') + || !@symlink($tmp . '/shared/assets/files', EVO_BASE_PATH . 'assets/files')) { + echo json_encode(['skip' => 'symlinks are not available']); + return; + } + file_put_contents($tmp . '/shared/assets/images/private/secret.txt', 'secret'); + file_put_contents($tmp . '/shared/assets/images/public/open.txt', 'open'); + symlink($tmp . '/outside', $tmp . '/shared/assets/images/escape'); + symlink($tmp . '/shared/assets/images/private', $tmp . '/shared/assets/images/internal'); + symlink($tmp . '/outside/missing', $tmp . '/shared/assets/images/dangling'); + mkdir($tmp . '/shared/assets/.thumbs/images', 0777, true); + symlink($tmp . '/outside', $tmp . '/shared/assets/.thumbs-link'); + + $capsule = new Illuminate\Database\Capsule\Manager(); + $capsule->addConnection(['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => '']); + $capsule->setAsGlobal(); + $capsule->bootEloquent(); + $capsule->getConnection()->statement('CREATE TABLE file_groups (id INTEGER PRIMARY KEY, document_group INTEGER, file TEXT)'); + $capsule->getConnection()->table('file_groups')->insert([ + 'id' => 1, 'document_group' => 7, 'file' => 'assets/images/private', + ]); + + $browser = (new ReflectionClass(browser::class))->newInstanceWithoutConstructor(); + $typeDir = $tmp . '/shared/assets/images'; + foreach (['modx' => evo(), 'typeDir' => $typeDir, 'typeURL' => 'https://example.test/assets/images', + 'type' => 'images', 'config' => ['uploadDir' => $tmp . '/shared/assets']] as $name => $value) { + (new ReflectionProperty(uploader::class, $name))->setValue($browser, $value); + } + (new ReflectionProperty(browser::class, 'protectedPaths'))->setValue($browser, []); + $invoke = static fn ($name, ...$args) => (new ReflectionMethod(browser::class, $name))->invoke($browser, ...$args); + $_SESSION = ['mgrRole' => 2, 'mgrDocgroups' => []]; + $secret = $typeDir . '/private/secret.txt'; + $open = $typeDir . '/public/open.txt'; + $output = [ + 'key' => $invoke('getFileGroupsRelPath', $secret), + 'newKey' => $invoke('getFileGroupsRelPath', $typeDir . '/private/new.txt'), + 'dottedKey' => $invoke('getFileGroupsRelPath', $typeDir . '/./private/secret.txt'), + 'deniedRead' => $invoke('isPathAccessible', $secret), + 'deniedWrite' => $invoke('isWriteAllowed', 'private'), + 'publicRead' => $invoke('isPathAccessible', $open), + 'publicWrite' => $invoke('isWriteAllowed', 'public'), + 'listing' => array_map('basename', $invoke('filterAccessiblePaths', [$secret, $open])), + 'subtreeSecret' => $invoke('subtreeAccessFilter', $typeDir)($secret), + 'protectedDescendants' => $invoke('hasInaccessibleDescendants', $typeDir), + 'escape' => $invoke('isPathAccessible', $typeDir . '/escape'), + 'internal' => $invoke('isPathAccessible', $typeDir . '/internal/secret.txt'), + 'visibleLink' => $invoke('isLinkVisible', $typeDir . '/escape'), + 'dangling' => $invoke('isWriteAllowed', 'dangling'), + 'traversal' => $invoke('isPathAccessible', $typeDir . '/../files'), + 'safeThumbs' => !fileManagerPathContainsLink($tmp . '/shared/assets', $tmp . '/shared/assets/.thumbs/images'), + 'linkedThumbs' => !fileManagerPathContainsLink($tmp . '/shared/assets', $tmp . '/shared/assets/.thumbs-link/images'), + 'diagnostic' => EvolutionCMS\Support\FileBrowserAccess::isUsableRoot( + $tmp . '/shared/assets', 'assets/', EVO_BASE_PATH, EVO_BASE_PATH, '/', EVO_SITE_URL + ), + 'missingDiagnostic' => EvolutionCMS\Support\FileBrowserAccess::isUsableRoot( + $tmp . '/missing', 'assets/', EVO_BASE_PATH, EVO_BASE_PATH, '/', EVO_SITE_URL + ), + 'unmappedDiagnostic' => EvolutionCMS\Support\FileBrowserAccess::isUsableRoot( + $tmp . '/shared/assets', 'unmapped/', EVO_BASE_PATH, EVO_BASE_PATH, '/', EVO_SITE_URL + ), + 'standardDiagnostic' => EvolutionCMS\Support\FileBrowserAccess::isUsableRoot( + '[(base_path)]assets/', 'assets/', '[(base_path)]', EVO_BASE_PATH, '/', EVO_SITE_URL + ), + 'standardKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + EVO_BASE_PATH, EVO_BASE_PATH . 'assets/local', EVO_BASE_PATH . 'assets/local/new.txt' + ), + 'wrongTargetKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + EVO_BASE_PATH, $typeDir, $open, EVO_BASE_PATH . 'assets/local' + ), + 'unconfinedKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + EVO_BASE_PATH, $typeDir, $open, $typeDir + ), + 'personalKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + EVO_BASE_PATH, $typeDir . '/private', $secret, EVO_BASE_PATH . 'assets/images/private' + ), + ]; + $_SESSION['mgrDocgroups'] = [7]; + $output['memberRead'] = $invoke('isPathAccessible', $secret); + $output['memberWrite'] = $invoke('isWriteAllowed', 'private'); + $output['memberSubtree'] = $invoke('subtreeAccessFilter', $typeDir)($secret); + $output['memberDescendants'] = $invoke('hasInaccessibleDescendants', $typeDir); + + (new ReflectionProperty(uploader::class, 'typeURL'))->setValue($browser, 'https://example.test/unmapped/images'); + $output['unmappedKey'] = $invoke('getFileGroupsRelPath', $open); + $output['unmappedRead'] = $invoke('isPathAccessible', $open); + $output['unmappedWrite'] = $invoke('isWriteAllowed', 'public'); + $output['unmappedListing'] = $invoke('filterAccessiblePaths', [$open]); + $output['unmappedSubtree'] = $invoke('subtreeAccessFilter', $typeDir)($open); + $output['unmappedDescendants'] = $invoke('hasInaccessibleDescendants', $typeDir); + $output['unmappedLink'] = $invoke('isLinkVisible', $typeDir . '/escape'); + $output['log'] = evo()->log; + $_SESSION['mgrRole'] = 1; + $output['adminRead'] = $invoke('isPathAccessible', $open); + $output['adminEscape'] = $invoke('isPathAccessible', $typeDir . '/escape'); + // Another release must retain the same stored key, without moving file_groups rows. + mkdir($tmp . '/next-release/assets', 0777, true); + symlink($typeDir, $tmp . '/next-release/assets/images'); + $output['nextReleaseKey'] = EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + $tmp . '/next-release', $typeDir, $secret, $tmp . '/next-release/assets/images' + ); + + // Execute the actual dashboard include, including its independent file-manager check. + $capsule->getConnection()->statement('CREATE TABLE site_content (id INTEGER, published INTEGER, privateweb INTEGER, deleted INTEGER)'); + require dirname(__DIR__, 2) . '/lang/en/global.php'; + foreach ([ + 'shared' => [$tmp . '/shared/assets/', 'assets/', '[(base_path)]'], + 'portable' => ['[(base_path)]assets/', 'assets/', '[(base_path)]'], + 'missing' => [$tmp . '/missing/', 'assets/', '[(base_path)]'], + 'unmapped' => [$tmp . '/shared/assets/', 'unmapped/', '[(base_path)]'], + 'missingManager' => ['[(base_path)]assets/', 'assets/', '[(base_path)]missing/'], + 'outsideManager' => [$tmp . '/shared/assets/', 'assets/', $tmp . '/shared/'], + 'traversalManager' => ['[(base_path)]assets/', 'assets/', '[(base_path)]../shared/'], + ] as $case => [$upload, $url, $managerRoot]) { + evo()->config = ['rb_base_dir' => $upload, 'rb_base_url' => $url, 'filemanager_path' => $managerRoot]; + include dirname(__DIR__, 3) . '/manager/includes/config_check.inc.php'; + $messages = array_column($warnings, 0); + $output['dashboard'][$case] = [ + 'browserWarning' => in_array($_lang['configcheck_rb_base_dir'], $messages, true), + 'managerWarning' => in_array($_lang['configcheck_filemanager_path'], $messages, true), + ]; + } + echo json_encode($output); +} finally { + $items = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($tmp, FilesystemIterator::SKIP_DOTS), RecursiveIteratorIterator::CHILD_FIRST); + foreach ($items as $item) { + if ($item->isLink() || !$item->isDir()) { + unlink($item->getPathname()); + } else { + rmdir($item->getPathname()); + } + } + rmdir($tmp); +} diff --git a/core/tests/Unit/Security/SharedBrowserAccessTest.php b/core/tests/Unit/Security/SharedBrowserAccessTest.php new file mode 100644 index 0000000000..7019869bc1 --- /dev/null +++ b/core/tests/Unit/Security/SharedBrowserAccessTest.php @@ -0,0 +1,115 @@ +toBe(0, $output); + expect($result)->toBeArray($output); + } + if (isset($result['skip'])) { + test()->markTestSkipped($result['skip']); + } + + return $result; +} + +it('preserves old group keys for shared roots and newly created files', function () { + $result = sharedBrowserResult(); + expect($result['key'])->toBe('assets/images/private/secret.txt') + ->and($result['dottedKey'])->toBe($result['key']) + ->and($result['nextReleaseKey'])->toBe($result['key']) + ->and($result['personalKey'])->toBe($result['key']) + ->and($result['standardKey'])->toBe('assets/local/new.txt') + ->and($result['newKey'])->toBe('assets/images/private/new.txt'); +}); + +it('enforces groups in listings direct access writes and recursive reads', function () { + $result = sharedBrowserResult(); + expect($result['deniedRead'])->toBeFalse() + ->and($result['deniedWrite'])->toBeFalse() + ->and($result['publicRead'])->toBeTrue() + ->and($result['publicWrite'])->toBeTrue() + ->and($result['listing'])->toBe(['open.txt']) + ->and($result['subtreeSecret'])->toBeFalse() + ->and($result['protectedDescendants'])->toBeTrue() + ->and($result['memberRead'])->toBeTrue() + ->and($result['memberWrite'])->toBeTrue() + ->and($result['memberSubtree'])->toBeTrue() + ->and($result['memberDescendants'])->toBeFalse(); +}); + +it('refuses and logs an unmapped ACL root instead of treating it as public', function () { + $result = sharedBrowserResult(); + expect($result['unmappedKey'])->toBeNull() + ->and($result['unmappedRead'])->toBeFalse() + ->and($result['unmappedWrite'])->toBeFalse() + ->and($result['unmappedListing'])->toBe([]) + ->and($result['unmappedSubtree'])->toBeFalse() + ->and($result['unmappedDescendants'])->toBeTrue() + ->and($result['unmappedLink'])->toBeFalse() + ->and(implode('\n', $result['log']))->toContain('file groups root cannot be resolved'); +}); + +it('keeps symlinks and traversal refused including for administrators', function () { + $result = sharedBrowserResult(); + foreach (['escape', 'internal', 'dangling', 'traversal', 'adminEscape'] as $key) { + expect($result[$key])->toBeFalse($key); + } + expect($result['adminRead'])->toBeTrue(); + expect($result['visibleLink'])->toBeTrue(); // inert link entry; opening it is still refused + expect($result['safeThumbs'])->toBeTrue() + ->and($result['linkedThumbs'])->toBeFalse(); +}); + +it('accepts a proven shared root but reports missing and unmapped roots', function () { + $result = sharedBrowserResult(); + expect($result['diagnostic'])->toBeTrue() + ->and($result['missingDiagnostic'])->toBeFalse() + ->and($result['unmappedDiagnostic'])->toBeFalse(); + expect($result['standardDiagnostic'])->toBeTrue() + ->and($result['wrongTargetKey'])->toBeNull() + ->and($result['unconfinedKey'])->toBeNull(); +}); + +it('does not derive local roots from unsafe or remote URLs', function (string $url) { + expect(FileBrowserAccess::localUploadPath($url, sys_get_temp_dir(), '/cms/', 'https://example.test/cms/'))->toBeNull(); +})->with(['https://other.test/cms/assets/images', '/other/assets/images', '../assets/images', + '/cms/assets/../images', '/cms/assets/%2e%2e/images', 'assets/images?x=1', 'assets/images#x', + 'https://user@example.test/cms/assets/images', 'file:///cms/assets/images', 'assets/CON/images']); + +it('refuses device names and NUL bytes before probing configured paths', function () { + expect(FileBrowserAccess::resolveAclPath('/NUL', '/tmp', '/tmp/test'))->toBeNull() + ->and(FileBrowserAccess::resolveAclPath('/tmp', '/tmp', "/tmp/bad\0name"))->toBeNull() + ->and(FileBrowserAccess::isUsableRoot('/CON', 'assets/', '/tmp', '/tmp', '/', 'https://example.test/'))->toBeFalse(); +}); + +it('renders dashboard path warnings independently and only accepts proven browser roots', function () { + $dashboard = sharedBrowserResult()['dashboard']; + foreach (['shared', 'portable'] as $case) { + expect($dashboard[$case])->toBe(['browserWarning' => false, 'managerWarning' => false]); + } + foreach (['missing', 'unmapped'] as $case) { + expect($dashboard[$case])->toBe(['browserWarning' => true, 'managerWarning' => false]); + } + expect($dashboard['missingManager']['managerWarning'])->toBeTrue() + ->and($dashboard['traversalManager']['managerWarning'])->toBeTrue() + ->and($dashboard['outsideManager']['managerWarning'])->toBeTrue(); +}); + +it('resolves site relative URLs for a subdirectory installation', function () { + $root = rtrim(str_replace('\\', '/', realpath(sys_get_temp_dir())), '/'); + foreach (['assets/images', '/cms/assets/images', 'https://example.test/cms/assets/images'] as $url) { + expect(FileBrowserAccess::localUploadPath($url, sys_get_temp_dir(), '/cms/', 'https://example.test/cms/')) + ->toBe($root . '/assets/images'); + } +}); diff --git a/manager/includes/config_check.inc.php b/manager/includes/config_check.inc.php index 2f4422f842..61d23f2dff 100755 --- a/manager/includes/config_check.inc.php +++ b/manager/includes/config_check.inc.php @@ -130,11 +130,22 @@ function checkSiteCache() { $warnings[] = [$_lang['configcheck_images']]; } -if(strpos(EvolutionCMS()->config['rb_base_dir'], EVO_BASE_PATH) !==0 && strpos(EvolutionCMS()->config['rb_base_dir'], '[(base_path)]') !==0) { +if (!\EvolutionCMS\Support\FileBrowserAccess::isUsableRoot( + (string) EvolutionCMS()->config['rb_base_dir'], + (string) EvolutionCMS()->config['rb_base_url'], + (string) EvolutionCMS()->config['filemanager_path'], + EVO_BASE_PATH, + EVO_BASE_URL, + EVO_SITE_URL +)) { $warnings[] = [$_lang['configcheck_rb_base_dir']]; } -if(strpos(EvolutionCMS()->config['filemanager_path'], EVO_BASE_PATH) !==0 && strpos(EvolutionCMS()->config['rb_base_dir'], '[(base_path)]assets/') !==0) { +$filemanagerRoot = str_replace('\\', '/', str_replace('[(base_path)]', EVO_BASE_PATH, (string) EvolutionCMS()->config['filemanager_path'])); +if (strpos($filemanagerRoot, "\0") !== false || fileManagerRefuseReservedName($filemanagerRoot) + || !is_dir($filemanagerRoot) || !is_readable($filemanagerRoot) + || in_array('..', explode('/', $filemanagerRoot), true) + || !\EvolutionCMS\Support\FileManagerAccess::isWithin(EVO_BASE_PATH, $filemanagerRoot)) { $warnings[] = [$_lang['configcheck_filemanager_path']]; } diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index 348f568119..c447495694 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -12,6 +12,7 @@ * @link http://kcfinder.sunhater.com */ +/** Embedded manager browser; path containment and file-group checks apply to every action. */ class browser extends uploader { /** Name prefix of the temporary download archives; the cleanup removes nothing else. */ @@ -1598,6 +1599,10 @@ protected function isLinkVisible($absPath) return true; } $parentKey = $this->getFileGroupsRelPath($parent); + if ($parentKey === null) { + $this->logDenied('file groups root cannot be resolved', $parent); + return false; + } $key = ltrim($parentKey . '/' . basename($absPath), '/'); $userGroups = array_map('intval', (array)($_SESSION['mgrDocgroups'] ?? [])); @@ -1960,17 +1965,28 @@ protected function output($data = null, $template = null) /** * Convert an absolute filesystem path to a path relative to filemanager_path, - * as stored in the file_groups table. Returns '' if the path is outside the root. + * as stored in file_groups. A shared type root keeps its proven site-side key; + * an unrecognised root returns null so group-protected operations fail closed. * * @param string $absPath - * @return string + * @return string|null */ - protected function getFileGroupsRelPath(string $absPath): string + 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( - \EvolutionCMS\Support\FileManagerAccess::aclRoot(), - $absPath + $aclRoot = \EvolutionCMS\Support\FileManagerAccess::aclRoot(); + $direct = \EvolutionCMS\Support\FileBrowserAccess::resolveAclPath($aclRoot, $this->typeDir, $absPath); + if ($direct !== null) { + return $direct; + } + + return \EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + $aclRoot, + $this->typeDir, + $absPath, + \EvolutionCMS\Support\FileBrowserAccess::localUploadPath( + (string) $this->typeURL, EVO_BASE_PATH, EVO_BASE_URL, EVO_SITE_URL + ) ); } @@ -1997,15 +2013,23 @@ protected function filterAccessiblePaths(array $absPaths): array $userGroups = array_map('intval', (array)($_SESSION['mgrDocgroups'] ?? [])); $itemRelativePaths = []; foreach ($absPaths as $ap) { - $itemRelativePaths[$ap] = $this->getFileGroupsRelPath($ap); + $key = $this->getFileGroupsRelPath($ap); + if ($key === null) { + $this->logDenied('file groups root cannot be resolved', $ap); + continue; + } + $itemRelativePaths[$ap] = $key; } $restricted = \EvolutionCMS\Support\FileManagerAccess::loadRestrictions(array_values($itemRelativePaths)); $result = []; foreach ($absPaths as $ap) { - $relativePath = $itemRelativePaths[$ap] ?? ''; + if (!array_key_exists($ap, $itemRelativePaths)) { + continue; + } + $relativePath = $itemRelativePaths[$ap]; if ($relativePath === '') { - $result[] = $ap; // rel path unknown, let through + $result[] = $ap; // the ACL root itself has no per-entry row continue; } if (\EvolutionCMS\Support\FileManagerAccess::isAccessible($relativePath, $userGroups, $restricted)) { @@ -2040,6 +2064,10 @@ protected function isWriteAllowed($relDir) } $relPath = $this->getFileGroupsRelPath($absPath); + if ($relPath === null) { + $this->logDenied('file groups root cannot be resolved', $absPath); + return false; + } if ($relPath === '') { return true; } @@ -2070,6 +2098,10 @@ protected function isPathAccessible($absPath) } $relPath = $this->getFileGroupsRelPath($absPath); + if ($relPath === null) { + $this->logDenied('file groups root cannot be resolved', $absPath); + return false; + } if ($relPath === '') { return true; } @@ -2095,7 +2127,12 @@ protected function subtreeAccessFilter($absDir) return fn ($path) => $this->isInsideTypeDir($path); } - $restrictions = \EvolutionCMS\Support\FileManagerAccess::loadSubtreeRestrictions($this->getFileGroupsRelPath($absDir)); + $rootKey = $this->getFileGroupsRelPath($absDir); + if ($rootKey === null) { + $this->logDenied('file groups root cannot be resolved', $absDir); + return static fn ($path) => false; + } + $restrictions = \EvolutionCMS\Support\FileManagerAccess::loadSubtreeRestrictions($rootKey); $userGroups = array_map('intval', (array)($_SESSION['mgrDocgroups'] ?? [])); return function ($path) use ($restrictions, $userGroups) { @@ -2103,6 +2140,10 @@ protected function subtreeAccessFilter($absDir) return false; } $relPath = $this->getFileGroupsRelPath($path); + if ($relPath === null) { + $this->logDenied('file groups root cannot be resolved', $path); + return false; + } return $relPath === '' || \EvolutionCMS\Support\FileManagerAccess::isAccessible($relPath, $userGroups, $restrictions); @@ -2168,6 +2209,10 @@ protected function hasInaccessibleDescendants($absDir) } $relPath = $this->getFileGroupsRelPath($absDir); + if ($relPath === null) { + $this->logDenied('file groups root cannot be resolved', $absDir); + return true; + } if ($relPath === '') { return false; } From 2b566219ef1228ba0af41224a1773016ce62f44f Mon Sep 17 00:00:00 2001 From: MiddleSokilAI Date: Mon, 2 Nov 2026 20:04:12 +0200 Subject: [PATCH 2/2] fix(tests): accept cached site-wide ACL root lookup --- core/tests/Unit/Manager/FileManagerAclSourceTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/tests/Unit/Manager/FileManagerAclSourceTest.php b/core/tests/Unit/Manager/FileManagerAclSourceTest.php index 9752b267e9..50ba1ce5c5 100644 --- a/core/tests/Unit/Manager/FileManagerAclSourceTest.php +++ b/core/tests/Unit/Manager/FileManagerAclSourceTest.php @@ -4,6 +4,7 @@ use Tests\TestCase; +/** Source guards for ACL entry points and the ownership of persisted file-group rows. */ final class FileManagerAclSourceTest extends TestCase { public function testClassicManagerSourceEnforcesAclOnGroupSaveAndView(): void @@ -73,6 +74,7 @@ public function testKcfinderChecksFileGroupsOnEveryReadAndChangeOfANamedEntry(): } } + /** Assert both managers use the site-wide ACL root and carry rows across entry changes. */ public function testFileGroupsAreKeyedBySiteRootAndFollowEveryRenameAndDelete(): void { $classic = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/actions/files.dynamic.php'); @@ -84,7 +86,8 @@ public function testFileGroupsAreKeyedBySiteRootAndFollowEveryRenameAndDelete(): 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); + // The global root may be stored in a variable before the shared-path mapping. + 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