From 94e60f311d7843ff9a3a6e07c1062f0e21ced3a8 Mon Sep 17 00:00:00 2001 From: MiddleSokilAI Date: Mon, 2 Nov 2026 19:52:09 +0200 Subject: [PATCH 1/5] 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 0bd022d0216d3fa324ea920733b2fb86dc39fe59 Mon Sep 17 00:00:00 2001 From: MiddleSokilAI Date: Mon, 2 Nov 2026 20:04:12 +0200 Subject: [PATCH 2/5] 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 From 1dc336ca29d77194ed6f1d299c48f47f2baf2642 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 03:04:25 +0100 Subject: [PATCH 3/5] fix(file-browser): map shared roots from the configured upload URL The shared-root mapping read KCFinder's typeURL, which resolves a relative rb_base_url against the browser script (or gives up when DOCUMENT_ROOT is a symlink), so it never named the folder: restricted managers got an empty, read-only browser on shared storage. Use the configured URL instead. A type root the site serves from outside a narrowed filemanager_path carries no file groups again instead of refusing every action. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + core/src/Support/FileBrowserAccess.php | 16 +++++--- core/tests/Fixtures/shared-browser-access.php | 38 ++++++++++++++++--- .../Unit/Security/SharedBrowserAccessTest.php | 21 +++++++++- manager/media/browser/mcpuk/config.php | 3 ++ manager/media/browser/mcpuk/core/browser.php | 9 ++++- 6 files changed, 74 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d4d78d6e21..4f0c0bf796 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -449,6 +449,7 @@ The name check also catches the spellings Windows turns into such a file: traili - Path comparison (`FileManagerAccess::isWithin()`) ignores case on Windows, and ZIP entries are resolved to the stored letter case (`fileManagerCanonicalCase()`) before the checks. - The media browser (KCFinder) follows the same rules: it stays out of the folders of layer 1 (`isInProtectedFolder()`), applies the executable-name rule to every name it writes (`validateFilename()`, including the final name of an upload), its temporary download archives carry the `kcf-download-` prefix and are the only `.zip` files it ever cleans up, and a folder rename never replaces an existing destination. +- The media browser keys file groups by the site-side spelling of its type root (`getFileGroupsRelPath()`, `FileBrowserAccess::resolveAclPath()`), so a type root moved to shared storage (`rb_base_dir` outside the release, `assets/images` a symlink to it) keeps its `assets/images/...` rows. That spelling comes from the configured `rb_base_url` (`_siteUploadURL`), never from KCFinder's `typeURL`, which it resolves against the browser script. The symlink must resolve exactly to the type root; an unproven spelling inside the ACL root refuses every group-checked action and logs it, while a type root the site serves from outside the ACL root (a narrowed `filemanager_path`) carries no groups, as anything else outside it. Test: `core/tests/Unit/Security/SharedBrowserAccessTest.php`. - ZIP entries with absolute paths, `:` or `..` segments are skipped. **Refusals are logged.** Every refusal above is written to the event log (a warning from source `File manager`, with the manager as its user), where administrators read it: `fileManagerLogDenied()` / `fileManagerDenied()` in the classic manager and its ZIP extraction, `logDenied()` in the media browser. Log the rule that refused, not routine misses (an extension that is merely not on the allowed list is not logged; an executable name is). Values from the request are escaped, one line per refusal, at most 20 per request. A new refusal that only returns a message is incomplete. diff --git a/core/src/Support/FileBrowserAccess.php b/core/src/Support/FileBrowserAccess.php index 2953fd3852..e2b96b4814 100644 --- a/core/src/Support/FileBrowserAccess.php +++ b/core/src/Support/FileBrowserAccess.php @@ -129,7 +129,8 @@ public static function localUploadPath(string $url, string $basePath, string $ba * 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. + * @return string|null Key relative to the site-wide ACL root; an empty key is that root itself, + * or a type root the site serves from outside it, where no group applies. * @since 3.6.0 */ public static function resolveAclPath(string $aclRoot, string $typeDir, string $absolutePath, ?string $localTypeDir = null): ?string @@ -170,13 +171,16 @@ public static function resolveAclPath(string $aclRoot, string $typeDir, string $ 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)) { + } elseif ($localTypeDir === null || ($localRoot = realpath($localTypeDir)) === false + || !FileManagerAccess::isWithin($typeRoot, str_replace('\\', '/', $localRoot)) + || !FileManagerAccess::isWithin(str_replace('\\', '/', $localRoot), $typeRoot)) { + return null; + } elseif (FileManagerAccess::isWithin($root, $localTypeDir)) { $key = ltrim(substr($localTypeDir, strlen($root)), '/'); } else { - return null; + // the site serves this folder from outside the ACL root (a narrowed filemanager_path), + // where no file_groups row can name it (FileManagerAccess::aclKey()): nothing applies + return ''; } return trim($key . '/' . $suffix, '/'); diff --git a/core/tests/Fixtures/shared-browser-access.php b/core/tests/Fixtures/shared-browser-access.php index 64cff8b429..bd41c1bac2 100644 --- a/core/tests/Fixtures/shared-browser-access.php +++ b/core/tests/Fixtures/shared-browser-access.php @@ -2,7 +2,7 @@ /** Exercise the real browser guards with release symlinks, SQLite ACLs and an editor session. */ $tmp = sys_get_temp_dir() . '/evo-shared-browser-' . bin2hex(random_bytes(6)); -foreach (['release/assets/local', 'release/assets/cache', 'shared/assets/images/private', 'shared/assets/images/public', 'shared/assets/files', 'outside'] as $dir) { +foreach (['release/assets/local', 'release/assets/cache', 'release/assets/pictures', 'shared/assets/images/private', 'shared/assets/images/public', 'shared/assets/files', 'outside'] as $dir) { mkdir($tmp . '/' . $dir, 0777, true); } define('EVO_BASE_PATH', $tmp . '/release/'); @@ -112,8 +112,9 @@ function evolutionCMS() { return evo(); } $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) { + // typeURL as KCFinder derives a relative rb_base_url (against the browser script): never the folder + foreach (['modx' => evo(), 'typeDir' => $typeDir, 'typeURL' => '/manager/media/browser/mcpuk/assets/images', + 'type' => 'images', 'config' => ['uploadDir' => $tmp . '/shared/assets', '_siteUploadURL' => 'assets']] as $name => $value) { (new ReflectionProperty(uploader::class, $name))->setValue($browser, $value); } (new ReflectionProperty(browser::class, 'protectedPaths'))->setValue($browser, []); @@ -157,7 +158,7 @@ function evolutionCMS() { return evo(); } 'wrongTargetKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( EVO_BASE_PATH, $typeDir, $open, EVO_BASE_PATH . 'assets/local' ), - 'unconfinedKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( + 'outsideSpellingKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( EVO_BASE_PATH, $typeDir, $open, $typeDir ), 'personalKey' => EvolutionCMS\Support\FileBrowserAccess::resolveAclPath( @@ -170,7 +171,8 @@ function evolutionCMS() { return evo(); } $output['memberSubtree'] = $invoke('subtreeAccessFilter', $typeDir)($secret); $output['memberDescendants'] = $invoke('hasInaccessibleDescendants', $typeDir); - (new ReflectionProperty(uploader::class, 'typeURL'))->setValue($browser, 'https://example.test/unmapped/images'); + $setConfig = static fn (array $config) => (new ReflectionProperty(uploader::class, 'config'))->setValue($browser, $config); + $setConfig(['uploadDir' => $tmp . '/shared/assets', '_siteUploadURL' => 'unmapped']); $output['unmappedKey'] = $invoke('getFileGroupsRelPath', $open); $output['unmappedRead'] = $invoke('isPathAccessible', $open); $output['unmappedWrite'] = $invoke('isWriteAllowed', 'public'); @@ -179,6 +181,32 @@ function evolutionCMS() { return evo(); } $output['unmappedDescendants'] = $invoke('hasInaccessibleDescendants', $typeDir); $output['unmappedLink'] = $invoke('isLinkVisible', $typeDir . '/escape'); $output['log'] = evo()->log; + + // A narrowed site-wide filemanager_path: rows can only name what lies below it, so a type root + // the site serves from outside it carries no groups, linked to shared storage or not. + evo()->configGlobal['filemanager_path'] = EVO_BASE_PATH . 'assets/local'; + $setConfig(['uploadDir' => $tmp . '/shared/assets', '_siteUploadURL' => 'assets']); + $output['narrowedSharedKey'] = $invoke('getFileGroupsRelPath', $secret); + $output['narrowedSharedRead'] = $invoke('isPathAccessible', $secret); + $pictures = EVO_BASE_PATH . 'assets/pictures'; + file_put_contents($pictures . '/cat.jpg', 'cat'); + foreach (['typeDir' => $pictures, 'type' => 'pictures'] as $name => $value) { + (new ReflectionProperty(uploader::class, $name))->setValue($browser, $value); + } + $setConfig(['uploadDir' => EVO_BASE_PATH . 'assets', '_siteUploadURL' => 'assets']); + $output['narrowedPlainKey'] = $invoke('getFileGroupsRelPath', $pictures . '/cat.jpg'); + $output['narrowedPlainRead'] = $invoke('isPathAccessible', $pictures . '/cat.jpg'); + $output['narrowedPlainWrite'] = $invoke('isWriteAllowed', ''); + $output['narrowedPlainListing'] = array_map('basename', $invoke('filterAccessiblePaths', [$pictures . '/cat.jpg'])); + $setConfig(['uploadDir' => EVO_BASE_PATH . 'assets', '_siteUploadURL' => 'elsewhere']); + $output['narrowedUnprovenKey'] = $invoke('getFileGroupsRelPath', $pictures . '/cat.jpg'); + $output['narrowedDiagnostic'] = EvolutionCMS\Support\FileBrowserAccess::isUsableRoot( + EVO_BASE_PATH . 'assets', 'assets/', EVO_BASE_PATH . 'assets/local', EVO_BASE_PATH, '/', EVO_SITE_URL + ); + unset(evo()->configGlobal['filemanager_path']); + foreach (['typeDir' => $typeDir, 'type' => 'images'] as $name => $value) { + (new ReflectionProperty(uploader::class, $name))->setValue($browser, $value); + } $_SESSION['mgrRole'] = 1; $output['adminRead'] = $invoke('isPathAccessible', $open); $output['adminEscape'] = $invoke('isPathAccessible', $typeDir . '/escape'); diff --git a/core/tests/Unit/Security/SharedBrowserAccessTest.php b/core/tests/Unit/Security/SharedBrowserAccessTest.php index 7019869bc1..fdf1cf82f0 100644 --- a/core/tests/Unit/Security/SharedBrowserAccessTest.php +++ b/core/tests/Unit/Security/SharedBrowserAccessTest.php @@ -78,7 +78,9 @@ function sharedBrowserResult(): array ->and($result['unmappedDiagnostic'])->toBeFalse(); expect($result['standardDiagnostic'])->toBeTrue() ->and($result['wrongTargetKey'])->toBeNull() - ->and($result['unconfinedKey'])->toBeNull(); + // a spelling outside the ACL root names nothing a row could restrict (the browser derives + // it from the configured URL below the site root, so this needs a narrowed filemanager_path) + ->and($result['outsideSpellingKey'])->toBe(''); }); it('does not derive local roots from unsafe or remote URLs', function (string $url) { @@ -113,3 +115,20 @@ function sharedBrowserResult(): array ->toBe($root . '/assets/images'); } }); + +it('maps the shared root from the configured upload URL, not from KCFinder typeURL', function () { + // the fixture gives typeURL the value KCFinder computes for rb_base_url "assets/" + expect(sharedBrowserResult()['key'])->toBe('assets/images/private/secret.txt'); +}); + +it('leaves type roots outside a narrowed filemanager_path open as before, but not unproven ones', function () { + $result = sharedBrowserResult(); + expect($result['narrowedPlainKey'])->toBe('') + ->and($result['narrowedPlainRead'])->toBeTrue() + ->and($result['narrowedPlainWrite'])->toBeTrue() + ->and($result['narrowedPlainListing'])->toBe(['cat.jpg']) + ->and($result['narrowedSharedKey'])->toBe('') + ->and($result['narrowedSharedRead'])->toBeTrue() + ->and($result['narrowedUnprovenKey'])->toBeNull() + ->and($result['narrowedDiagnostic'])->toBeTrue(); +}); diff --git a/manager/media/browser/mcpuk/config.php b/manager/media/browser/mcpuk/config.php index be59b1d9e3..57094f3ebd 100755 --- a/manager/media/browser/mcpuk/config.php +++ b/manager/media/browser/mcpuk/config.php @@ -37,6 +37,9 @@ 'theme' => "evo", 'uploadURL' => $uploadUrl, 'uploadDir' => $uploadDir, + // the URL as configured: the uploader rewrites uploadURL against the script's cwd and + // DOCUMENT_ROOT. A leading "_" keeps it out of the session override. + '_siteUploadURL' => $uploadUrl, 'siteURL' => EVO_SITE_URL, 'assetsURL' => $uploadUrl, 'dirPerms' => intval(EvolutionCMS()->getConfig('new_folder_permissions'), 8), diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index c447495694..abf5dd2f5e 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -1984,8 +1984,13 @@ protected function getFileGroupsRelPath(string $absPath): ?string $aclRoot, $this->typeDir, $absPath, + // not $this->typeURL: KCFinder resolves a relative upload URL against the browser + // script (or gives up when DOCUMENT_ROOT is a symlink), so it never names the folder \EvolutionCMS\Support\FileBrowserAccess::localUploadPath( - (string) $this->typeURL, EVO_BASE_PATH, EVO_BASE_URL, EVO_SITE_URL + rtrim((string) ($this->config['_siteUploadURL'] ?? ''), '/') . '/' . $this->type, + EVO_BASE_PATH, + EVO_BASE_URL, + EVO_SITE_URL ) ); } @@ -2029,7 +2034,7 @@ protected function filterAccessiblePaths(array $absPaths): array } $relativePath = $itemRelativePaths[$ap]; if ($relativePath === '') { - $result[] = $ap; // the ACL root itself has no per-entry row + $result[] = $ap; // the ACL root itself, or a folder outside it: no row applies continue; } if (\EvolutionCMS\Support\FileManagerAccess::isAccessible($relativePath, $userGroups, $restricted)) { From c8f817cb81f55006d9b4edbefbd9e7579b09a8c9 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 06:15:54 +0100 Subject: [PATCH 4/5] upd(file-manager): pass operator links to somewhere new in both file managers Links below a file manager root are made by the operator, never by the CMS: a shared upload folder, a plugin checked out elsewhere, images from another disk. A link whose real target exists and is new (not inside or around the ACL root, not a loop, not inside anything the walk already named or a shared type folder) is now a folder or file like any other, named by its own path, with every rule applied by that name: protected folders, file groups, the executable-name rule and the media browser type folder. Links that dangle, loop or would give a file a second name stay inert entries, and `..` past a link is refused. Both managers key a shared type folder the same way, so the classic manager can now enter it and edit its groups; the link that stands for a type folder is never deleted, renamed or unlinked. Removing any other passable link removes the link only and keeps its groups. A new fixture builds the media browser the way a web request does ($_SERVER, the working directory of browse.php, config.php), through a public_html -> evolution -> release chain, which is how the typeURL bug slipped through. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 13 +- core/functions/actions/files.php | 387 ++++++++++++++++-- core/src/Support/FileBrowserAccess.php | 123 ++++-- .../Fixtures/shared-type-root-request.php | 302 ++++++++++++++ .../FileManagerAclBypassSourceTest.php | 10 +- .../Security/FileManagerCodeRemovalTest.php | 10 +- .../Security/FileManagerLinkDisplayTest.php | 5 +- .../FileManagerPathContainmentTest.php | 20 +- .../Security/FileManagerSymlinkWritesTest.php | 3 +- .../MediaBrowserSymlinkContainmentTest.php | 15 +- .../Unit/Security/SharedBrowserAccessTest.php | 14 +- .../Security/SharedTypeRootRequestTest.php | 111 +++++ manager/actions/files.dynamic.php | 14 +- manager/media/browser/mcpuk/core/browser.php | 46 +-- .../media/browser/mcpuk/lib/helper_dir.php | 5 + 15 files changed, 953 insertions(+), 125 deletions(-) create mode 100644 core/tests/Fixtures/shared-type-root-request.php create mode 100644 core/tests/Unit/Security/SharedTypeRootRequestTest.php diff --git a/AGENTS.md b/AGENTS.md index 4f0c0bf796..ca19e8c4e7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -419,8 +419,8 @@ A Windows device name (`CON`, `NUL`, `COM1.jpg` ...; `fileManagerIsReservedDevic The executable-name ban does not depend on those lists. `textsave()` applies it too: an existing executable file cannot be edited without the code permission. Nor can it be removed or displaced: whoever may not write code may not delete, rename or move it either. Deleting or renaming a file or -link of an executable name, or a folder holding one at any depth (links count by their own name and -are not followed), needs the code permission in both file managers (`fileManagerCodeRemovalDenied()` +link of an executable name, or a folder holding one at any depth (a link the file managers pass is +a folder whose code counts; any other link counts by its own name and is not followed), needs the code permission in both file managers (`fileManagerCodeRemovalDenied()` in the classic one, `refuseCodeRemoval()` in the media browser, both on `fileManagerCodeEntry()`). The name check also catches the spellings Windows turns into such a file: trailing dots and spaces (`.htaccess.`) and NTFS streams (`.htaccess:x`, `shell.php::$DATA`). @@ -432,7 +432,7 @@ The name check also catches the spellings Windows turns into such a file: traili | Browse or enter a path | membership in a group of every restricted ancestor folder | | Modify or delete an existing file | the same, and the file is not at the top level | | Rename, move or delete an existing entry (media browser too) | the top-level rule of the classic manager, measured from the manager's own `filemanager_path` (`canModifyExisting()` in `browser.php`) | -| Delete a link | removing a symlink or junction never forgets the groups stored under its target's key (the key of a link is its target's) | +| Delete a link | removing a symlink or junction (a passable one through the folder or file delete, any other through `deletelink`) removes the link only and never forgets the groups stored under its name, which apply again if the link returns | | Upload over an existing file | the file's own restrictions, not just the folder's (`fileManagerCanModifyExistingPath()`); the file keeps them and is **not** given the folder's groups | | Copy / duplicate (both file managers) | access is an AND over the restricted folders on the way and the file itself, each an OR over its groups, which one set of groups on one key cannot say. `FileManagerAccess::carriedRestrictions()` keeps the groups that satisfy every set the destination does not already enforce; the copy is refused when there are none, and when the destination is the ACL root itself or lies outside it (no per-entry row can be stored there). Never store the union of the groups. A copy takes `replaceRestrictions()` and a rename or move takes `moveRestrictions()`, which first drops rows left at the free destination by files removed outside the CMS. Rows are matched by the exact stored path (`exactRows()`), and the `file_groups.file` column is case-sensitive (`utf8mb4_bin` on MySQL), because on a case-sensitive file system `report.txt` and `REPORT.txt` are two files | | Any name the media browser writes (copy, move, upload, thumbnail) | the destination is free, a dangling symlink counts as taken (`isTaken()`), and copies are made exclusively with `copyExclusive()` (`fopen` mode `x`), never `copy()` through a link | @@ -444,12 +444,13 @@ The name check also catches the spellings Windows turns into such a file: traili **Always enforced** -- Paths outside `filemanager_path` are rejected; `..`, symlinks and Windows junctions are not followed. -- A path that passes through a symlink or junction below the root is rejected as a whole (`fileManagerPathContainsLink()`): in the classic manager, uploads, direct edit and delete, and the media browser (`isInsideTypeDir()`). Both file managers list a link as an inert entry: no web URL, drill-down, thumbnail or selection (a link may lead anywhere, and the web server would serve its target unchecked). The one action is removing the link itself (`mode=deletelink`, `act_deleteLink`), judged by the link's own path, never resolved: the top-level and group rules of an existing entry, and no link where a protected folder belongs; the target and its groups stay. Its target is shown only as a path relative to the link, and only when it lies inside the manager's root (the media browser: the type folder) and the manager may reach it (`fileManagerRelativeLinkTarget()`); a `../../` or absolute target outside, or a restricted one, is shown as "outside the file manager or not accessible", never as a path. Links are never created: a link entry in a ZIP is not extracted (neither as a link nor as a file holding its path) but reported (`fileManagerExtractZip()` `$links`), a directory archive stores a link as a link entry with that same relative target or leaves it out, and a folder copy leaves it out. The media browser serves no thumbnail whose cached path or original runs through one. Nor does it write, rename, copy, move or empty the thumbnail cache through one (`isSafeThumbPath()`, measured from the upload root, so a link at the cache root itself is refused too, before anything is probed or created). +- Paths outside `filemanager_path` are rejected, and `..` never climbs above the root. +- Links below a root are the operator's (the CMS never creates one), so a link to somewhere new is a folder (or file) like any other: a shared upload folder, a plugin checked out elsewhere, images linked in from another disk. Absolute or relative, whatever its spelling, it is judged by its real target (`fileManagerPassableLinkTarget()`), and passed when that target exists and is new: not inside or around the ACL root, not around the folder the link sits in (a loop), not inside or around anything the walk has named already (where it started, the links it passed), not inside shared storage that a type-folder link names (`fileManagerSharedTypeRoots()`). Any of those would give a file a second name, and file groups are keyed by name, so a group on `team` would not hold `public/link-to-team`. A passed link is named by its own path, never resolved (`fileManagerCanonicalPath()`, which also puts each part into its stored letter case and accepts any spelling of the root: a hosting `public_html` chain or a release link above it), and every rule applies by that name: the folders of layer 1 (`fileManagerProtectedPaths()` holds each by its real path and by its name, so a protected folder may itself be a link), file groups, the executable-name rule, the type folder of the media browser. Past a link, `..` is refused: it climbs in the link's target, not back where the server walks. Two separate links to one outside folder cannot be told apart from here; the operator should not make them, and a server without `FollowSymLinks` does not serve what the CMS then opens. Tests: `SharedTypeRootRequestTest.php` (a web request through the real browser constructor), `SharedBrowserAccessTest.php`. +- A path that passes through any other link below the root is rejected as a whole (`fileManagerPathContainsLink()`): in the classic manager, uploads, direct edit and delete, and the media browser (`isInsideTypeDir()`); a write never goes to a path that is itself a link, dangling or not (`fileManagerIsSafeWriteTarget()`). Both file managers list such a link as an inert entry: no web URL, drill-down, thumbnail or selection (a link may lead anywhere, and the web server would serve its target unchecked). The one action is removing the link itself (`mode=deletelink`, `act_deleteLink`), judged by the link's own path, never resolved: the top-level and group rules of an existing entry, and no link where a protected folder belongs; the target and its groups stay. Its target is shown only as a path relative to the link, and only when it lies inside the manager's root (the media browser: the type folder) and the manager may reach it (`fileManagerRelativeLinkTarget()`); a `../../` or absolute target outside, or a restricted one, is shown as "outside the file manager or not accessible", never as a path. Links are never created: a link entry in a ZIP is not extracted (neither as a link nor as a file holding its path) but reported (`fileManagerExtractZip()` `$links`), a directory archive stores a link as a link entry with that same relative target or leaves it out, and a folder copy leaves it out, passable or not. The media browser serves no thumbnail whose cached path or original runs through a link it does not pass. Nor does it write, rename, copy, move or empty the thumbnail cache through one (`isSafeThumbPath()`, measured from the upload root, so a link at the cache root itself is refused too, before anything is probed or created). - Path comparison (`FileManagerAccess::isWithin()`) ignores case on Windows, and ZIP entries are resolved to the stored letter case (`fileManagerCanonicalCase()`) before the checks. - The media browser (KCFinder) follows the same rules: it stays out of the folders of layer 1 (`isInProtectedFolder()`), applies the executable-name rule to every name it writes (`validateFilename()`, including the final name of an upload), its temporary download archives carry the `kcf-download-` prefix and are the only `.zip` files it ever cleans up, and a folder rename never replaces an existing destination. -- The media browser keys file groups by the site-side spelling of its type root (`getFileGroupsRelPath()`, `FileBrowserAccess::resolveAclPath()`), so a type root moved to shared storage (`rb_base_dir` outside the release, `assets/images` a symlink to it) keeps its `assets/images/...` rows. That spelling comes from the configured `rb_base_url` (`_siteUploadURL`), never from KCFinder's `typeURL`, which it resolves against the browser script. The symlink must resolve exactly to the type root; an unproven spelling inside the ACL root refuses every group-checked action and logs it, while a type root the site serves from outside the ACL root (a narrowed `filemanager_path`) carries no groups, as anything else outside it. Test: `core/tests/Unit/Security/SharedBrowserAccessTest.php`. +- The media browser keys file groups by the name of its type folder below the ACL root, then the path below it by name (`getFileGroupsRelPath()`, `FileBrowserAccess::resolveAclPath()`), so a type folder moved to shared storage (`rb_base_dir` outside the release, `assets/images` a link to it) keeps its `assets/images/...` rows, the same keys the classic manager uses for it. The type folder is named by its real path below the ACL root, else by a passable link to it below the root, else by the site-side spelling of the configured `rb_base_url` (`_siteUploadURL`) once realpath proves it is the same folder; never by KCFinder's `typeURL`, which it resolves against the browser script. An unproven name inside the ACL root refuses every group-checked action and logs it, while a type folder the site serves from outside the ACL root (a narrowed `filemanager_path`) carries no groups, as anything else outside it. In the classic manager the link that stands for a type folder (`fileManagerSharedTypeRootAt()`) is listed as the folder and entered, but never deleted, renamed or unlinked. - ZIP entries with absolute paths, `:` or `..` segments are skipped. **Refusals are logged.** Every refusal above is written to the event log (a warning from source `File manager`, with the manager as its user), where administrators read it: `fileManagerLogDenied()` / `fileManagerDenied()` in the classic manager and its ZIP extraction, `logDenied()` in the media browser. Log the rule that refused, not routine misses (an extension that is merely not on the allowed list is not logged; an executable name is). Values from the request are escaped, one line per refusal, at most 20 per request. A new refusal that only returns a message is incomplete. diff --git a/core/functions/actions/files.php b/core/functions/actions/files.php index dd46c7985e..6207080315 100644 --- a/core/functions/actions/files.php +++ b/core/functions/actions/files.php @@ -1,4 +1,5 @@ getConfig('filemanager_path')) ?: realpath(EVO_BASE_PATH); + $userRoot = fileManagerCanonicalPath(evolutionCMS()->getConfig('filemanager_path')) ?: realpath(EVO_BASE_PATH); $roots = [FileManagerAccess::aclRoot(), rtrim(str_replace('\\', '/', $userRoot), '/')]; } @@ -220,11 +221,10 @@ function fileManagerResolvePath($filemanagerPath, $requestedPath) return null; } - $path = realpath($requested); + $path = fileManagerCanonicalPath($requested); if ($path === false) { return null; } - $path = rtrim(str_replace('\\', '/', $path), '/'); if (!FileManagerAccess::isWithin($filemanagerPath, $path)) { return null; } @@ -275,10 +275,15 @@ function fileManagerProtectedPaths() } } - return array_map( - static fn ($path) => rtrim(str_replace('\\', '/', realpath($path) ?: $path), '/'), - $paths - ); + // by its real path and by its name below the root: a protected folder may be a link + $spellings = []; + foreach ($paths as $path) { + foreach ([realpath($path) ?: $path, fileManagerCanonicalPath($path) ?: $path] as $spelling) { + $spellings[] = rtrim(str_replace('\\', '/', $spelling), '/'); + } + } + + return array_values(array_unique($spellings)); } } @@ -366,9 +371,25 @@ function fileManagerCodeEntry($path) if (fileManagerIsExecutableName($path)) { return ''; } - if (fileManagerIsLink($path) || !is_dir($path)) { + // a link the file managers pass is a folder whose code goes with it (a plugin checked out + // elsewhere); any other link counts by its own name only. Each real folder is read once. + $named = []; + $passes = static function ($link) use (&$named) { + $target = fileManagerPassableLinkTarget($link, $named); + if ($target === null || !is_dir($target)) { + return false; + } + $named[] = $target; + + return true; + }; + if (!is_dir($path) || (fileManagerIsLink($path) && !$passes($path))) { return null; } + $real = realpath($path); + if ($real !== false) { + $named[] = rtrim(str_replace('\\', '/', $real), '/'); + } $stack = ['']; while ($stack) { $rel = array_pop($stack); @@ -388,7 +409,7 @@ function fileManagerCodeEntry($path) if (fileManagerIsReservedDeviceName($entry)) { continue; } - if (!fileManagerIsLink("$path/$entryRel") && is_dir("$path/$entryRel")) { + if (is_dir("$path/$entryRel") && (!fileManagerIsLink("$path/$entryRel") || $passes("$path/$entryRel"))) { $stack[] = $entryRel; } } @@ -534,16 +555,276 @@ function fileManagerIsLink($path) } } +if(!function_exists('fileManagerSiteAclRoot')) { + /** + * FileManagerAccess::aclRoot() through evolutionCMS(), as everything here: the site-wide + * file manager root that file groups are keyed by, canonical, no trailing slash. Null without + * a CMS to ask (the helpers run on their own in tests): then no link is passed. + * + * @return string|null + */ + function fileManagerSiteAclRoot() + { + if (!defined('EVO_BASE_PATH')) { + return null; + } + try { + $evo = evolutionCMS(); + } catch (\Throwable $e) { + return null; + } + $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)), '/'); + } +} + +if(!function_exists('fileManagerSharedTypeRoots')) { + /** + * The links that stand for media browser type folders kept in shared storage (see + * FileBrowserAccess::sharedTypeRoots()), for the site-wide settings. + * + * @return array [link => real folder] + */ + function fileManagerSharedTypeRoots() + { + static $cache = []; + static $computing = false; + $aclRoot = $computing ? null : fileManagerSiteAclRoot(); + if ($aclRoot === null) { + // while the list is being built, or without a CMS, it is empty + return []; + } + $evo = evolutionCMS(); + $setting = static fn ($key) => (string) ($evo->configGlobal[$key] ?? $evo->getConfig($key, '')); + $args = [ + $aclRoot, + $setting('rb_base_dir'), + $setting('rb_base_url'), + EVO_BASE_PATH, + defined('EVO_BASE_URL') ? EVO_BASE_URL : '/', + defined('EVO_SITE_URL') ? EVO_SITE_URL : '', + ]; + $key = implode("\0", $args); + if (!array_key_exists($key, $cache)) { + $computing = true; + try { + $cache[$key] = FileBrowserAccess::sharedTypeRoots(...$args); + } finally { + $computing = false; + } + } + + return $cache[$key]; + } +} + +if(!function_exists('fileManagerSharedTypeRootAt')) { + /** + * The shared type-folder link $path names, checked again against its target, or null. + * + * @param string $path absolute path in any spelling of its parent + * @return string|null the link as fileManagerSharedTypeRoots() lists it + */ + function fileManagerSharedTypeRootAt($path) + { + $roots = fileManagerSharedTypeRoots(); + if ($roots === []) { + return null; + } + $path = rtrim(str_replace('\\', '/', $path), '/'); + $parent = realpath(dirname($path)); + $real = realpath($path); + if ($parent === false || $real === false) { + return null; + } + $spelling = rtrim(str_replace('\\', '/', $parent), '/') . '/' . basename($path); + $real = rtrim(str_replace('\\', '/', $real), '/'); + foreach ($roots as $link => $target) { + // isWithin() both ways: the same path, by the platform's letter-case rule + if (FileManagerAccess::isWithin($link, $spelling) && FileManagerAccess::isWithin($spelling, $link) + && FileManagerAccess::isWithin($target, $real) && FileManagerAccess::isWithin($real, $target)) { + return $link; + } + } + + return null; + } +} + +if(!function_exists('fileManagerPassableLinkTarget')) { + /** + * Where a link below a file manager root leads, when the file managers may pass it: then it + * is a folder (or file) like any other, named by its own path, and every rule applies by that + * name. The CMS never creates links, so these are the operator's: a shared upload folder, a + * plugin checked out elsewhere. + * + * A link is passed only into somewhere new. Not when it dangles; not into the ACL root or + * around it; not around the folder it sits in (a loop); not into or around anything the walk + * has named already ($named: where it started and the links it passed); not into shared + * storage that a type-folder link names. Each of those would give a file a second name, and + * file groups are keyed by name. Two separate links to one outside folder cannot be told + * apart from here: the operator should not make them. + * + * @param string $link + * @param string[] $named real paths, forward slashes + * @return string|null its real target, forward slashes + */ + function fileManagerPassableLinkTarget($link, array $named = []) + { + $target = realpath($link); + $parent = realpath(dirname($link)); + if ($target === false || $parent === false) { + return null; + } + $aclRoot = fileManagerSiteAclRoot(); + if ($aclRoot === null) { + return null; + } + $target = rtrim(str_replace('\\', '/', $target), '/'); + $parent = rtrim(str_replace('\\', '/', $parent), '/'); + $overlaps = static fn ($a, $b) => FileManagerAccess::isWithin($a, $b) || FileManagerAccess::isWithin($b, $a); + if ($overlaps($aclRoot, $target) || FileManagerAccess::isWithin($target, $parent)) { + return null; + } + foreach ($named as $real) { + if ($overlaps($real, $target)) { + return null; + } + } + $self = fileManagerSharedTypeRootAt($link); + foreach (fileManagerSharedTypeRoots() as $typeLink => $typeTarget) { + if ($typeLink !== $self && $overlaps($typeTarget, $target)) { + return null; + } + } + + return $target; + } +} + +if(!function_exists('fileManagerStoredName')) { + /** + * $name with the letter case $dir stores it with. Windows matches names in any case, while + * file groups are matched exactly, so a name from a request is put into the stored case + * before it becomes a key. Elsewhere $name itself. + * + * @param string $dir + * @param string $name + */ + function fileManagerStoredName($dir, $name) + { + static $listings = []; + if (PHP_OS_FAMILY !== 'Windows') { + return $name; + } + if (!array_key_exists($dir, $listings)) { + $listings[$dir] = @scandir($dir) ?: []; + } + if (in_array($name, $listings[$dir], true)) { + return $name; + } + foreach ($listings[$dir] as $entry) { + if (strcasecmp($entry, $name) === 0) { + return $entry; + } + } + + return $name; + } +} + +if(!function_exists('fileManagerCanonicalPath')) { + /** + * The name the file managers know an existing $path by. Below the ACL root (by any spelling + * of it: a hosting public_html, a release link), it is the root's own path followed by each + * part in its stored letter case, through passable links but never resolving them, since a + * linked folder is named by its link (fileManagerPassableLinkTarget()). Elsewhere realpath(). + * + * @param string $path + * @param string|null $root the ACL root, fileManagerSiteAclRoot() by default + * @return string|false forward slashes, no trailing slash; false when it does not exist, + * climbs above the root or out of a link, or passes a link not passed + */ + function fileManagerCanonicalPath($path, $root = null) + { + $path = rtrim(str_replace('\\', '/', (string) $path), '/'); + if ($path === '' || strpos($path, "\0") !== false || fileManagerRefuseReservedName($path)) { + return false; + } + $root = $root === null + ? fileManagerSiteAclRoot() + : rtrim(str_replace('\\', '/', realpath($root) ?: $root), '/'); + if ($root === null) { + $real = realpath($path); + + return $real === false ? false : rtrim(str_replace('\\', '/', $real), '/'); + } + $segments = explode('/', $path); + $rest = null; + $lead = ''; + // the shortest leading part of $path that is the ACL root + foreach ($segments as $i => $segment) { + $lead = $i === 0 ? $segment : $lead . '/' . $segment; + if ($lead === '' || substr($lead, -1) === ':') { + continue; // "/" or a bare drive letter, which realpath() reads as the working directory + } + $real = realpath($lead); + if ($real !== false && FileManagerAccess::isWithin($root, $real) && FileManagerAccess::isWithin($real, $root)) { + $rest = array_slice($segments, $i + 1); + break; + } + } + if ($rest === null) { + $real = realpath($path); + + return $real === false ? false : rtrim(str_replace('\\', '/', $real), '/'); + } + + $current = $root; + $named = [$root]; + $throughLink = false; + foreach ($rest as $segment) { + if ($segment === '' || $segment === '.') { + continue; + } + if ($segment === '..') { + // past a link, .. climbs in its target, not back where the server walks + if ($current === $root || $throughLink) { + return false; + } + $current = dirname($current); + continue; + } + $next = $current . '/' . fileManagerStoredName($current, $segment); + if (fileManagerIsLink($next)) { + $target = fileManagerPassableLinkTarget($next, $named); + if ($target === null) { + return false; + } + $named[] = $target; + $throughLink = true; + } elseif (!file_exists($next)) { + return false; + } + $current = $next; + } + + return $current; + } +} + if(!function_exists('fileManagerPathContainsLink')) { /** - * Whether a path below $root passes through a symlink or junction. The root itself is a - * configured boundary and may be a symlink (for example, public_html); links below it are - * never followed. Accept either the configured root spelling or its canonical spelling. + * Whether a path below $root cannot be used: it climbs above $root, names a Windows device, + * or passes a link the file managers do not pass (fileManagerPassableLinkTarget(): one that + * dangles, loops, or gives something reachable a second name). The root itself is a + * configured boundary and may be a link (for example, public_html). Accept either the + * configured root spelling or its canonical spelling. * * @param string $root absolute root * @param string $path absolute path, possibly not created yet - * - * A Windows device name below the root counts as a link here: callers refuse both. */ function fileManagerPathContainsLink($root, $path) { @@ -567,12 +848,15 @@ function fileManagerPathContainsLink($root, $path) $current = $base; $relative = substr($path, strlen($base)); + $named = $canonicalRoot !== '' ? [$canonicalRoot] : []; + $throughLink = false; foreach (explode('/', trim($relative, '/')) as $segment) { if ($segment === '' || $segment === '.') { continue; } if ($segment === '..') { - if ($current === $base) { + // past a link, .. climbs in its target, not back where the server walks + if ($current === $base || $throughLink) { return true; } $current = dirname($current); @@ -581,7 +865,12 @@ function fileManagerPathContainsLink($root, $path) $current = rtrim($current, '/') . '/' . $segment; if (fileManagerIsLink($current)) { - return true; + $target = fileManagerPassableLinkTarget($current, $named); + if ($target === null) { + return true; + } + $named[] = $target; + $throughLink = true; } } @@ -592,8 +881,9 @@ function fileManagerPathContainsLink($root, $path) if(!function_exists('fileManagerIsSafeWriteTarget')) { /** * Whether writing to $target, which need not exist yet, stays under $root. Every part of - * the path that already exists is checked, so a symlink anywhere on the way (a folder - * whose children do not exist yet, or the file itself) cannot redirect the write. + * the path that already exists is checked: a link on the way must be one the file managers + * pass (fileManagerPassableLinkTarget()), and the target itself must not be a link, dangling + * or not, which the write would follow. * * @param string $root canonical absolute path * @param string $target absolute path below $root @@ -610,6 +900,8 @@ function fileManagerIsSafeWriteTarget($root, $target) return false; } + $rootReal = realpath($root); + $named = $rootReal !== false ? [rtrim(str_replace('\\', '/', $rootReal), '/')] : []; $current = $root; foreach (explode('/', substr($target, strlen($root) + 1)) as $segment) { if ($segment === '' || $segment === '.' || $segment === '..') { @@ -617,7 +909,10 @@ function fileManagerIsSafeWriteTarget($root, $target) } $current .= '/' . $segment; if (fileManagerIsLink($current)) { - return false; + if ($current === $target || ($real = fileManagerPassableLinkTarget($current, $named)) === null) { + return false; + } + $named[] = $real; } if (!file_exists($current)) { // everything below is created by this write, so none of it can be a link @@ -887,8 +1182,8 @@ function fileManagerCreateDirectoryZip( array $fileGroupsMap, array $protectedPaths ): bool { - $sourcePath = rtrim(str_replace('\\', '/', realpath($sourcePath)), '/'); - $filemanagerPath = rtrim(str_replace('\\', '/', realpath($filemanagerPath)), '/'); + $sourcePath = rtrim((string) fileManagerCanonicalPath($sourcePath), '/'); + $filemanagerPath = rtrim((string) fileManagerCanonicalPath($filemanagerPath), '/'); $archiveRoot = basename($relativePath !== '' ? $relativePath : $sourcePath); $archiveRoot = trim(preg_replace('/[^\w.-]+/', '-', $archiveRoot), '.-') ?: 'files'; @@ -1180,7 +1475,7 @@ function fileManagerCanonicalCase($path) $path = rtrim(str_replace('\\', '/', $path), '/'); $rest = []; $existing = $path; - while ($existing !== '' && ($real = realpath($existing)) === false) { + while ($existing !== '' && ($real = fileManagerCanonicalPath($existing)) === false) { $rest[] = basename($existing); $parent = dirname($existing); if ($parent === $existing) { @@ -1262,7 +1557,7 @@ function fileManagerZipWriteGuard($filemanagerPath, $destination): callable function fileManagerExtractZip($file, $path, array $protectedPaths = [], $dirMode = 0777, ?callable $mayWrite = null, ?array &$created = null, ?array &$links = null) { $links = []; - $root = realpath($path); + $root = fileManagerCanonicalPath($path); if ($root === false) { return false; } @@ -1493,7 +1788,15 @@ function ls($curpath, array $options = []) } $rel_newpath = ltrim(substr($newpath, strlen($filemanager_path)), '/'); $rel_web = ltrim(substr($newpath, strlen($base_path)), '/'); - if (fileManagerIsLink($newpath)) { + // a link the file managers pass is listed as the folder or file it stands for, named by + // its own path (fileManagerPassableLinkTarget()); the site's shared type folder among them + $isLink = fileManagerIsLink($newpath); + $passableLink = $isLink && !fileManagerPathContainsLink($filemanager_path, $newpath); + $sharedTypeRoot = $passableLink && fileManagerSharedTypeRootAt($newpath) !== null; + $linkMark = $passableLink + ? ' ' + : ''; + if ($isLink && !$passableLink) { // shown, never followed: a link may lead anywhere, and the web server would serve its // target without any check here, so it gets no URL, no drill-down and no action if (fileManagerIsAccessible($rel_newpath, $userGroups ?? [], $fileGroupsMap ?? [])) { @@ -1535,7 +1838,7 @@ function ls($curpath, array $options = []) continue; } elseif (!in_array($file, $excludes) && !in_array($newpath, $protected_path)) { $dirs_array[$dircounter]['text'] = ' ' - . '' + . $linkMark . '' . htmlspecialchars($file, ENT_QUOTES, 'UTF-8') . '' . $groupSuffix; $dfiles = scandir($newpath); @@ -1548,7 +1851,8 @@ function ls($curpath, array $options = []) } } $file_exists = (0 < count($dfiles)) ? 'file_exists' : ''; - $canModifyDir = $currentDirWritable + // the site's shared type folder is entered, never removed or renamed + $canModifyDir = $currentDirWritable && !$sharedTypeRoot && fileManagerCanModifyExistingPath($rel_newpath, $userGroups ?? [], $fileGroupsMap ?? []) && is_writable($newpath); @@ -1569,7 +1873,7 @@ function ls($curpath, array $options = []) . $_style['icon_trash'] . '" title="' . $_lang['file_delete_folder'] . '">' : ''; } - $dirs_array[$dircounter]['rename'] = ($currentDirWritable + $dirs_array[$dircounter]['rename'] = ($currentDirWritable && !$sharedTypeRoot && fileManagerCanModifyExistingPath($rel_newpath, $userGroups ?? [], $fileGroupsMap ?? []) && is_writable($newpath)) ? 'Invalid path.

'; } - $filename = str_replace('\\', '/', realpath($requested)); + $filename = (string) fileManagerCanonicalPath($requested); if (!FileManagerAccess::isWithin($filemanager_path, $filename) || !is_file($filename)) { return 'Invalid path.

'; } @@ -2142,14 +2445,14 @@ function delete_file() { global $_lang; - $filemanager_path = rtrim(str_replace('\\', '/', realpath(evolutionCMS() - ->getConfig('filemanager_path', EVO_BASE_PATH))), '/'); + $filemanager_path = rtrim((string) fileManagerCanonicalPath(evolutionCMS() + ->getConfig('filemanager_path', EVO_BASE_PATH)), '/'); $requested_path = ltrim($_REQUEST['path'] ?? '', '/'); $requested = $filemanager_path . '/' . $requested_path; if (fileManagerPathContainsLink($filemanager_path, $requested)) { return 'Invalid path.

'; } - $file = str_replace('\\', '/', realpath($requested)); + $file = (string) fileManagerCanonicalPath($requested); if (!FileManagerAccess::isWithin($filemanager_path, $file) || !is_file($file)) { return 'Invalid path.

'; } @@ -2165,11 +2468,15 @@ function delete_file() // the path below the root, never the server's absolute one $msg = sprintf($_lang['deleting_file'], htmlspecialchars($fileRel, ENT_QUOTES, 'UTF-8')); + // a linked file goes as a link (unlink() never follows one): its groups stay for its return + $wasLink = fileManagerIsLink($file); if (!evolutionCMS()->hasPermission('file_manager') || !@unlink($file)) { $msg .= '' . $_lang['file_not_deleted'] . '

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

'; - \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions(fileManagerAclKey($fileRel)); + if (!$wasLink) { + \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions(fileManagerAclKey($fileRel)); + } } // Log the change diff --git a/core/src/Support/FileBrowserAccess.php b/core/src/Support/FileBrowserAccess.php index e2b96b4814..f87491bda1 100644 --- a/core/src/Support/FileBrowserAccess.php +++ b/core/src/Support/FileBrowserAccess.php @@ -11,6 +11,9 @@ final class FileBrowserAccess { private const IMAGE_TYPES = ['images', 'image']; + /** Folders below rb_base_dir that the browser opens as types (config.php `types`). */ + public const TYPE_FOLDERS = ['files', 'images', 'file', 'image', 'media']; + /** * The manager permission that opens a browser type, besides file_manager which opens all. * The browser falls back to its first type (files) for a missing or unknown one, so those @@ -124,10 +127,13 @@ public static function localUploadPath(string $url, string $basePath, string $ba } /** - * 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. + * The file_groups key of a path in a browser type folder: the type folder's name below the + * site-wide ACL root, then the path below it by name. Links below the type folder count by + * their own name when the file managers pass them (fileManagerPassableLinkTarget()); any + * other link makes the path unusable. The type folder itself is named by its real path when + * that lies below the ACL root, else by a link to it below the root (a shared folder), else + * by the site-side spelling of rb_base_url ($localTypeDir) once realpath proves it is the + * same folder. No name 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, * or a type root the site serves from outside it, where no group applies. @@ -150,40 +156,48 @@ public static function resolveAclPath(string $aclRoot, string $typeDir, string $ $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)) { + if (!FileManagerAccess::isWithin($spelling, $absolutePath) + || \fileManagerPathContainsLink($spelling, $absolutePath)) { return null; } - $existing = realpath($absolutePath); - if ($existing !== false) { - $existing = str_replace('\\', '/', $existing); - if (!FileManagerAccess::isWithin($typeRoot, $existing)) { - return null; + $parts = []; + $current = $spelling; + foreach (explode('/', substr($absolutePath, strlen($spelling))) as $segment) { + if ($segment === '' || $segment === '.') { + continue; } - $suffix = ltrim(substr($existing, strlen($typeRoot)), '/'); - } else { - $suffix = implode('/', array_filter(explode('/', $suffix), static fn ($part) => $part !== '' && $part !== '.')); + if ($segment === '..') { + // fileManagerPathContainsLink() let it through, so it stays below the type folder + array_pop($parts); + $current = dirname($current); + continue; + } + $segment = \fileManagerStoredName($current, $segment); + $parts[] = $segment; + $current .= '/' . $segment; } if (FileManagerAccess::isWithin($root, $typeRoot)) { $key = ltrim(substr($typeRoot, strlen($root)), '/'); + } elseif (($typeName = \fileManagerCanonicalPath($typeDir, $root)) !== false + && FileManagerAccess::isWithin($root, $typeName)) { + $key = ltrim(substr($typeName, strlen($root)), '/'); } elseif ($localTypeDir === null || ($localRoot = realpath($localTypeDir)) === false || !FileManagerAccess::isWithin($typeRoot, str_replace('\\', '/', $localRoot)) || !FileManagerAccess::isWithin(str_replace('\\', '/', $localRoot), $typeRoot)) { return null; - } elseif (FileManagerAccess::isWithin($root, $localTypeDir)) { - $key = ltrim(substr($localTypeDir, strlen($root)), '/'); - } else { + } elseif (($localName = \fileManagerCanonicalPath($localTypeDir, $root)) !== false + && FileManagerAccess::isWithin($root, $localName)) { + $key = ltrim(substr($localName, strlen($root)), '/'); + } elseif (!FileManagerAccess::isWithin($root, $localTypeDir)) { // the site serves this folder from outside the ACL root (a narrowed filemanager_path), // where no file_groups row can name it (FileManagerAccess::aclKey()): nothing applies return ''; + } else { + return null; // below the root, but only through a link the file managers do not pass } - return trim($key . '/' . $suffix, '/'); + return trim($key . '/' . implode('/', $parts), '/'); } /** @@ -209,7 +223,7 @@ public static function isUsableRoot(string $uploadDir, string $uploadUrl, string } $found = false; - foreach (['files', 'images', 'file', 'image', 'media'] as $type) { + foreach (self::TYPE_FOLDERS as $type) { $typeDir = rtrim($uploadDir, '/\\') . '/' . $type; if (!is_dir($typeDir)) { continue; @@ -223,4 +237,67 @@ public static function isUsableRoot(string $uploadDir, string $uploadUrl, string return $found; } + + /** + * Type folders a deployment keeps in shared storage, by the site-side link the file manager may + * pass: [link => real folder], both absolute with forward slashes. A link qualifies when it is + * the site-side spelling of a configured type folder (rb_base_url below the site root), lies + * below the ACL root with no other link on the way, and resolves exactly to that type folder + * (rb_base_dir/) outside the ACL root. Its key is the browser's (resolveAclPath()), so + * both file managers read and write the same file_groups rows. Links above the ACL root (a + * hosting public_html, a release switch) belong to the root and do not count. + * + * @return array + * @since 3.6.0 + */ + public static function sharedTypeRoots(string $aclRoot, string $uploadDir, string $uploadUrl, string $basePath, string $baseUrl, string $siteUrl): array + { + $uploadDir = rtrim(str_replace('\\', '/', str_replace('[(base_path)]', $basePath, $uploadDir)), '/'); + $aclRoot = str_replace('[(base_path)]', $basePath, $aclRoot); + if ($uploadDir === '' || strpos($uploadDir . $aclRoot, "\0") !== false + || \fileManagerRefuseReservedName($uploadDir) || \fileManagerRefuseReservedName($aclRoot)) { + return []; + } + $root = realpath($aclRoot); + if ($root === false) { + return []; + } + $root = rtrim(str_replace('\\', '/', $root), '/'); + + $links = []; + foreach (self::TYPE_FOLDERS as $type) { + $typeDir = $uploadDir . '/' . $type; + $real = realpath($typeDir); + if ($real === false || !is_dir($real)) { + continue; + } + $real = rtrim(str_replace('\\', '/', $real), '/'); + if (FileManagerAccess::isWithin($root, $real)) { + continue; // reachable without a link: nothing to pass + } + $local = self::localUploadPath(rtrim($uploadUrl, '/') . '/' . $type, $basePath, $baseUrl, $siteUrl); + $key = $local === null ? null : self::resolveAclPath($root, $typeDir, $typeDir, $local); + if ($key === null || $key === '') { + continue; + } + $link = $root . '/' . $key; + if (!\fileManagerIsLink($link)) { + continue; + } + // only the type folder itself may be the link + $parent = $root; + foreach (explode('/', dirname($key)) as $segment) { + if ($segment === '.' || $segment === '') { + continue; + } + $parent .= '/' . $segment; + if (\fileManagerIsLink($parent)) { + continue 2; + } + } + $links[$link] = $real; + } + + return $links; + } } diff --git a/core/tests/Fixtures/shared-type-root-request.php b/core/tests/Fixtures/shared-type-root-request.php new file mode 100644 index 0000000000..a6032a7f72 --- /dev/null +++ b/core/tests/Fixtures/shared-type-root-request.php @@ -0,0 +1,302 @@ + evolution -> evo-3.6.2 sits above + * the site; assets/images links to ../../shared/assets/images; assets/plugins/myplugin links (by + * an absolute path) to a checkout elsewhere, whose images are linked into the shared image folder. + * + * argv: base64 JSON {base: canonical|linked, upload: site|shared} + * Prints JSON with both managers' answers. + */ + +$options = json_decode(base64_decode($argv[1] ?? ''), true) ?: []; +$home = sys_get_temp_dir() . '/evo-shared-root-' . bin2hex(random_bytes(6)); +$site = $home . '/evo-3.6.2'; +$shared = $home . '/shared/assets/images'; +$repo = $home . '/repos/myplugin'; +foreach ([$site . '/assets/files/team', $site . '/assets/plugins', $shared . '/private', $shared . '/public', + $repo . '/images', $home . '/outside'] as $dir) { + mkdir($dir, 0777, true); +} +file_put_contents($shared . '/private/secret.png', 'secret'); +file_put_contents($shared . '/public/open.png', 'open'); +file_put_contents($repo . '/images/logo.png', 'logo'); +file_put_contents($repo . '/plugin.php', ' 'symlinks are not available']); + return; +} +symlink($repo, $site . '/assets/plugins/myplugin'); // absolute: a plugin checked out elsewhere +symlink('../../../repos/myplugin/images', $shared . '/plugin-images'); // relative: its images, for the browser +symlink($home . '/outside', $shared . '/outside'); // somewhere new: passed +symlink($shared . '/private', $shared . '/internal'); // a second name for private: not passed +symlink($site . '/assets/files/team', $shared . '/team'); // a second name for a site folder: not passed +symlink('..', $shared . '/public/loop'); // a loop: not passed +symlink($home . '/missing', $shared . '/dangling'); // leads nowhere: not passed + +$basePath = ($options['base'] ?? 'canonical') === 'linked' ? $home . '/public_html/' : $site . '/'; +$uploadDir = ($options['upload'] ?? 'site') === 'shared' ? $home . '/shared/assets/' : '[(base_path)]assets/'; +$mcpuk = dirname(__DIR__, 3) . '/manager/media/browser/mcpuk'; + +define('EVO_BASE_PATH', $basePath); +define('EVO_BASE_URL', '/'); +define('EVO_SITE_URL', 'https://example.test/'); +define('EVO_MANAGER_PATH', $basePath . 'manager/'); +define('EVO_CORE_PATH', $basePath . 'core/'); +define('IN_MANAGER_MODE', true); +define('IN_INSTALL_MODE', false); +define('EVO_API_MODE', false); + +/** + * The CMS object the browser is constructed with: settings of the site above, nothing else. + */ +class DocumentParser +{ + public array $configGlobal = []; + public array $config; + public array $log = []; + public array $denied = []; + + public function __construct(string $uploadDir) + { + $this->config = [ + 'filemanager_path' => '[(base_path)]', + 'rb_base_dir' => $uploadDir, + 'rb_base_url' => 'assets/', + 'use_udperms' => 1, + 'upload_images' => 'png,jpg', + 'upload_files' => 'txt,pdf', + 'upload_media' => 'mp4', + 'upload_maxsize' => 1048576, + 'new_folder_permissions' => '0777', + 'new_file_permissions' => '0666', + 'thumbsDir' => '.thumbs', + 'thumbWidth' => 100, + 'thumbHeight' => 100, + 'jpegQuality' => 90, + ]; + } + + public function getConfig($key, $default = null) + { + $value = $this->config[$key] ?? $default; + + return is_string($value) ? str_replace('[(base_path)]', EVO_BASE_PATH, $value) : $value; + } + + public function hasPermission($permission) + { + return !in_array($permission, $this->denied, true); + } + + public function invokeEvent(...$args) + { + return []; + } + + public function logEvent($id, $type, $message, $source) + { + $this->log[] = strip_tags($message); + } + + public function toDateFormat($timestamp, $mode = '') + { + return date('Y-m-d H:i', (int) $timestamp); + } +} + +/** Lexicon and style entries ls() prints; any key reads as its own name. */ +final class FixtureLexicon implements ArrayAccess +{ + public function offsetExists($offset): bool + { + return true; + } + + public function offsetGet($offset): mixed + { + return (string) $offset; + } + + public function offsetSet($offset, $value): void + { + } + + public function offsetUnset($offset): void + { + } +} + +final class ManagerTheme +{ + public static function getLexicon() + { + return new FixtureLexicon(); + } + + public static function getStyle() + { + return new FixtureLexicon(); + } +} + +$GLOBALS['evo'] = new DocumentParser($uploadDir); +function evo() +{ + return $GLOBALS['evo']; +} +function evolutionCMS() +{ + return evo(); +} + +require dirname(__DIR__, 2) . '/vendor/autoload.php'; +spl_autoload_register(static function ($class) use ($mcpuk) { + $map = [ + 'browser' => 'core/browser.php', 'uploader' => 'core/uploader.php', 'dir' => 'lib/helper_dir.php', + 'file' => 'lib/helper_file.php', 'path' => 'lib/helper_path.php', 'text' => 'lib/helper_text.php', + 'input' => 'lib/class_input.php', 'image' => 'lib/class_image.php', 'image_gd' => 'lib/class_image_gd.php', + 'image_imagick' => 'lib/class_image_imagick.php', 'image_gmagick' => 'lib/class_image_gmagick.php', + 'gd' => 'lib/class_gd.php', 'type_img' => 'core/types/type_img.php', 'type_mime' => 'core/types/type_mime.php', + ]; + if (isset($map[$class])) { + require $mcpuk . '/' . $map[$class]; + } +}); + +$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'], + ['id' => 2, 'document_group' => 7, 'file' => 'assets/files/team'], +]); + +try { + // what Apache hands browse.php on such hosting: the document root by its public spelling, + // the working directory by its real one + $_SERVER = array_merge($_SERVER, [ + 'DOCUMENT_ROOT' => $home . '/public_html', + 'SCRIPT_NAME' => '/manager/media/browser/mcpuk/browse.php', + 'PHP_SELF' => '/manager/media/browser/mcpuk/browse.php', + 'SCRIPT_FILENAME' => $mcpuk . '/browse.php', + 'HTTP_HOST' => 'example.test', + 'REQUEST_METHOD' => 'GET', + ]); + $_GET = ['type' => 'images']; + $_POST = []; + $_COOKIE = []; + $_SESSION = ['mgrRole' => 2, 'mgrDocgroups' => []]; + chdir($mcpuk); + $browser = new browser(evo()); + $invoke = static fn ($name, ...$args) => (new ReflectionMethod(browser::class, $name))->invoke($browser, ...$args); + $typeDir = $browser->typeDir; + $secret = $typeDir . '/private/secret.png'; + $open = $typeDir . '/public/open.png'; + $pluginImage = $typeDir . '/plugin-images/logo.png'; + + $output = [ + 'typeURL' => $browser->typeURL, + 'browserKey' => $invoke('getFileGroupsRelPath', $secret), + 'browserOutsiderReadsSecret' => $invoke('isPathAccessible', $secret), + 'browserListing' => array_map('basename', $invoke('filterAccessiblePaths', array_map( + static fn ($name) => $typeDir . '/' . $name, + ['private', 'public', 'plugin-images', 'outside', 'internal', 'team', 'dangling'] + ))), + 'browserPluginImage' => $invoke('isPathAccessible', $pluginImage), + 'browserPluginImageKey' => $invoke('getFileGroupsRelPath', $pluginImage), + 'browserPluginUpload' => $invoke('isWriteAllowed', 'plugin-images'), + 'browserSecondName' => $invoke('isPathAccessible', $typeDir . '/internal/secret.png'), + 'browserSiteName' => $invoke('isPathAccessible', $typeDir . '/team'), + 'browserLoop' => $invoke('isPathAccessible', $typeDir . '/public/loop/private/secret.png'), + 'browserDangling' => $invoke('isPathAccessible', $typeDir . '/dangling'), + 'browserClimb' => $invoke('isPathAccessible', $typeDir . '/outside/../private/secret.png'), + 'browserInertLinks' => array_column($invoke('getLinks', 'images'), 'name'), + ]; + + // the classic file manager, as files.dynamic.php sets it up + $root = rtrim(str_replace('\\', '/', fileManagerCanonicalPath(evo()->getConfig('filemanager_path')) ?: realpath(EVO_BASE_PATH)), '/'); + $relative = static fn ($request) => fileManagerResolvePath($root, $request)['relative'] ?? null; + $resolved = fileManagerResolvePath($root, 'assets/images/private/secret.png'); + $output += [ + 'sharedRoots' => array_map( + static fn ($link) => substr($link, strlen($root) + 1), + array_keys(fileManagerSharedTypeRoots()) + ), + 'classicKey' => $resolved === null ? null : fileManagerAclKey($resolved['relative']), + 'classicOutsiderReadsSecret' => $resolved !== null && fileManagerIsAccessible($resolved['relative'], []), + 'classicMemberReadsSecret' => $resolved !== null && fileManagerIsAccessible($resolved['relative'], [7]), + 'classicPluginImage' => $relative('assets/images/plugin-images/logo.png'), + 'classicPluginFolder' => $relative('assets/plugins/myplugin/images/logo.png'), + 'classicOutside' => $relative('assets/images/outside/loot.txt'), + 'classicSecondName' => $relative('assets/images/internal/secret.png'), + 'classicSiteName' => $relative('assets/images/team'), + 'classicLoop' => $relative('assets/images/public/loop/private'), + 'classicDangling' => $relative('assets/images/dangling'), + 'classicClimb' => $relative('assets/images/../files'), + 'classicClimbOutOfPlugin' => $relative('assets/plugins/myplugin/../../files'), + 'writeInside' => fileManagerIsNewWriteTarget($root, $root . '/assets/images/public/new.png'), + 'writeIntoPlugin' => fileManagerIsNewWriteTarget($root, $root . '/assets/plugins/myplugin/images/new.png'), + 'writeOverLink' => fileManagerIsSafeWriteTarget($root, $root . '/assets/images'), + 'writeThroughSecondName' => fileManagerIsSafeWriteTarget($root, $root . '/assets/images/internal/new.png'), + 'isSharedRoot' => fileManagerSharedTypeRootAt($root . '/assets/images') !== null, + 'isSharedRootPlugin' => fileManagerSharedTypeRootAt($root . '/assets/plugins/myplugin') !== null, + 'pluginCode' => fileManagerCodeEntry($root . '/assets/plugins/myplugin'), + 'pluginCodeFromAssets' => fileManagerCodeEntry($root . '/assets/plugins'), + ]; + evo()->denied = ['save_plugin', 'save_snippet', 'save_module']; + $output['pluginProtected'] = fileManagerPathIsProtected($root . '/assets/plugins/myplugin/images', fileManagerProtectedPaths()); + evo()->denied = []; + + // the listing of assets/images: linked folders are folders, the rest stay inert links + $listingOf = static function ($path) use ($root) { + ob_start(); + ls($root . '/' . $path, [ + 'len' => 0, 'editablefiles' => [], 'enablefileunzip' => false, 'inlineviewablefiles' => [], + 'uploadablefiles' => [], 'enablefiledownload' => true, 'viewablefiles' => [], 'protected_path' => [], + 'excludes' => [], 'filemanager_path' => $root, 'base_path' => rtrim(str_replace('\\', '/', realpath(EVO_BASE_PATH)), '/'), + 'showFileGroups' => true, 'fileGroupsMap' => [], 'allDocGroups' => [], 'userGroups' => [], + ]); + + return ob_get_clean(); + }; + $assets = $listingOf('assets'); + $images = $listingOf('assets/images'); + $output += [ + 'imagesListedAsFolder' => strpos($assets, 'mode=drill&path=assets%2Fimages"') !== false, + 'imagesOffersDelete' => strpos($assets, 'deleteFolder(\'images\'') !== false, + 'imagesOffersRename' => strpos($assets, 'renameFolder(\'images\')') !== false, + 'pluginImagesListedAsFolder' => strpos($images, 'mode=drill&path=assets%2Fimages%2Fplugin-images"') !== false, + 'pluginImagesOffersDelete' => strpos($images, 'deleteFolder(\'plugin-images\'') !== false, + 'inertListedAsLinks' => array_values(array_filter( + ['internal', 'team', 'dangling', 'outside'], + static fn ($name) => strpos($images, 'deleteLink(\'' . $name . '\')') !== false + )), + ]; + + // deleting a linked folder removes the link, never what it leads to + $output['removedLink'] = rrmdir($root . '/assets/plugins/myplugin'); + $output['linkGone'] = !file_exists($site . '/assets/plugins/myplugin') && !is_link($site . '/assets/plugins/myplugin'); + $output['checkoutKept'] = is_file($repo . '/plugin.php') && is_file($repo . '/images/logo.png'); + $output['log'] = evo()->log; + echo json_encode($output, JSON_UNESCAPED_SLASHES); +} finally { + chdir(__DIR__); + $items = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($home, FilesystemIterator::SKIP_DOTS), RecursiveIteratorIterator::CHILD_FIRST); + foreach ($items as $item) { + if ($item->isLink() || !$item->isDir()) { + unlink($item->getPathname()); + } else { + rmdir($item->getPathname()); + } + } + rmdir($home); +} diff --git a/core/tests/Unit/Security/FileManagerAclBypassSourceTest.php b/core/tests/Unit/Security/FileManagerAclBypassSourceTest.php index f3fe5ac855..040d210697 100644 --- a/core/tests/Unit/Security/FileManagerAclBypassSourceTest.php +++ b/core/tests/Unit/Security/FileManagerAclBypassSourceTest.php @@ -170,14 +170,16 @@ function fmAclSource(string $relative): string ->and($browser)->toContain("'xb'"); }); -it('does not list links of the classic file manager nor serve a thumbnail through one', function () { +it('lists only passable links of the classic file manager as entries, nor serves a thumbnail through another', function () { $functions = fmAclSource('core/functions/actions/files.php'); $browser = fmAclSource('manager/media/browser/mcpuk/core/browser.php'); - $ls = substr($functions, strpos($functions, '// first, get info'), 700); + $ls = substr($functions, strpos($functions, '// first, get info'), 2000); $thumb = substr($browser, strpos($browser, 'function act_thumb'), 1500); - expect($ls)->toContain('if (fileManagerIsLink($newpath)) {') - ->and(strpos($ls, 'fileManagerIsLink($newpath)'))->toBeLessThan(strpos($ls, 'fileManagerIsAccessible(')) + // a link the file managers do not pass is set aside before any ACL lookup by its name + expect($ls)->toContain('$passableLink = $isLink && !fileManagerPathContainsLink($filemanager_path, $newpath);') + ->toContain('if ($isLink && !$passableLink) {') + ->and(strpos($ls, 'if ($isLink && !$passableLink) {'))->toBeLessThan(strpos($ls, 'fileManagerIsAccessible(')) ->and($thumb)->toContain('!$this->isSafeThumbPath($file)') ->and(strpos($thumb, 'isSafeThumbPath($file)'))->toBeLessThan(strpos($thumb, 'is_file($file)')); }); diff --git a/core/tests/Unit/Security/FileManagerCodeRemovalTest.php b/core/tests/Unit/Security/FileManagerCodeRemovalTest.php index eaa177d422..80f890c28c 100644 --- a/core/tests/Unit/Security/FileManagerCodeRemovalTest.php +++ b/core/tests/Unit/Security/FileManagerCodeRemovalTest.php @@ -65,19 +65,21 @@ function removeCodeRemovalTree(string $dir): void } }); -it('counts a link by its own name and never follows one', function () { +it('counts a link it does not pass by its own name and never follows it', function () { $base = codeRemovalTree(); try { + // a loop (it leads to a folder around it), which the file managers never pass; a link + // they pass is a folder whose code counts (shared-type-root-request.php fixture) $link = "$base/clean/to-outside"; - if (!@symlink("$base/outside", $link) && PHP_OS_FAMILY === 'Windows') { + if (!@symlink($base, $link) && PHP_OS_FAMILY === 'Windows') { // without the symlink privilege a junction does the same evoRunCommand('mklink /J ' . escapeshellarg(str_replace('/', '\\', $link)) . ' ' - . escapeshellarg(str_replace('/', '\\', "$base/outside")) . ' 2>&1'); + . escapeshellarg(str_replace('/', '\\', $base)) . ' 2>&1'); } if (!fileManagerIsLink($link)) { $this->markTestSkipped('links cannot be created here'); } - // the folder it leads to holds shell.php, but the link is not followed + // the folder it leads to holds outside/shell.php, but the link is not followed expect(fileManagerCodeEntry("$base/clean"))->toBeNull() ->and(fileManagerCodeEntry("$base/clean/to-outside"))->toBeNull(); diff --git a/core/tests/Unit/Security/FileManagerLinkDisplayTest.php b/core/tests/Unit/Security/FileManagerLinkDisplayTest.php index 03618d65a1..ad3f96a16d 100644 --- a/core/tests/Unit/Security/FileManagerLinkDisplayTest.php +++ b/core/tests/Unit/Security/FileManagerLinkDisplayTest.php @@ -167,12 +167,13 @@ function linkDisplayRemove(string $path): void ->not->toHaveKey('app/current/index.txt'); })->skip(!class_exists('ZipArchive'), 'zip is required'); -it('lists links in both managers without a URL, an action or a selection', function () { +it('lists links it does not pass in both managers without a URL, an action or a selection', function () { $root = dirname(__DIR__, 4); $functions = (string) file_get_contents($root . '/core/functions/actions/files.php'); $browser = (string) file_get_contents($root . '/manager/media/browser/mcpuk/core/browser.php'); $files = (string) file_get_contents($root . '/manager/media/browser/mcpuk/js/browser/files.js'); - $start = strpos($functions, 'if (fileManagerIsLink($newpath)) {'); + // the links the file managers do not pass (a passable one is listed as a folder or file) + $start = strpos($functions, 'if ($isLink && !$passableLink) {'); $branch = substr($functions, $start, strpos($functions, 'continue;', $start) - $start); $linksHtml = substr($files, strpos($files, 'browser.linksHtml = function'), 1600); diff --git a/core/tests/Unit/Security/FileManagerPathContainmentTest.php b/core/tests/Unit/Security/FileManagerPathContainmentTest.php index a674f5b5cd..4504edfb94 100644 --- a/core/tests/Unit/Security/FileManagerPathContainmentTest.php +++ b/core/tests/Unit/Security/FileManagerPathContainmentTest.php @@ -86,12 +86,15 @@ function fmContainmentZip(string $path, array $entries): void ->and(fileManagerIsSafeWriteTarget($root, $this->tmp . '/root-old/file.txt'))->toBeFalse(); }); -it('rejects write targets that pass through a symlinked folder', function () { - mkdir($this->tmp . '/root', 0777, true); +it('rejects write targets that pass through a link the file managers do not pass', function () { + mkdir($this->tmp . '/root/inside', 0777, true); mkdir($this->tmp . '/outside', 0777, true); - fmContainmentSymlink($this->tmp . '/outside', $this->tmp . '/root/link'); + // a loop (around the folder it sits in) and a second name for a folder below the root + fmContainmentSymlink($this->tmp, $this->tmp . '/root/link'); + fmContainmentSymlink($this->tmp . '/root/inside', $this->tmp . '/root/alias'); - expect(fileManagerIsSafeWriteTarget($this->tmp . '/root', $this->tmp . '/root/link/new/file.txt'))->toBeFalse(); + expect(fileManagerIsSafeWriteTarget($this->tmp . '/root', $this->tmp . '/root/link/outside/file.txt'))->toBeFalse() + ->and(fileManagerIsSafeWriteTarget($this->tmp . '/root', $this->tmp . '/root/alias/file.txt'))->toBeFalse(); }); it('rejects a write target that is itself a symlink', function () { @@ -211,20 +214,21 @@ function fmContainmentZip(string $path, array $entries): void ->and(file_get_contents($this->tmp . '/dest/images/ok.png'))->toBe('png'); }); -it('does not follow a symlinked folder or file while extracting', function () { +it('does not follow a link it does not pass, nor write over a linked file, while extracting', function () { mkdir($this->tmp . '/dest', 0777, true); mkdir($this->tmp . '/outside', 0777, true); file_put_contents($this->tmp . '/victim.txt', 'original'); - fmContainmentSymlink($this->tmp . '/outside', $this->tmp . '/dest/link'); + // a loop: it leads to a folder around the one it sits in + fmContainmentSymlink($this->tmp, $this->tmp . '/dest/link'); fmContainmentSymlink($this->tmp . '/victim.txt', $this->tmp . '/dest/file.txt'); fmContainmentZip($this->tmp . '/a.zip', [ - 'link/new/escaped.txt' => 'x', + 'link/outside/escaped.txt' => 'x', 'file.txt' => 'overwritten', ]); fileManagerExtractZip($this->tmp . '/a.zip', $this->tmp . '/dest'); - expect(file_exists($this->tmp . '/outside/new/escaped.txt'))->toBeFalse() + expect(file_exists($this->tmp . '/outside/escaped.txt'))->toBeFalse() ->and(file_get_contents($this->tmp . '/victim.txt'))->toBe('original'); }); diff --git a/core/tests/Unit/Security/FileManagerSymlinkWritesTest.php b/core/tests/Unit/Security/FileManagerSymlinkWritesTest.php index b843b52f77..713f765337 100644 --- a/core/tests/Unit/Security/FileManagerSymlinkWritesTest.php +++ b/core/tests/Unit/Security/FileManagerSymlinkWritesTest.php @@ -201,7 +201,8 @@ function fmLinksBrowser(string $typeDir): browser it('accepts only a new name inside the type folder as an upload target', function () { fmLinksSymlink($this->tmp . '/outside/shell.php', $this->root . '/dangling.png'); - fmLinksSymlink($this->tmp . '/outside', $this->root . '/linked'); + // a loop, which the file managers never pass (a link they pass is a folder of the type) + fmLinksSymlink($this->tmp, $this->root . '/linked'); $browser = fmLinksBrowser($this->root); $method = new ReflectionMethod(browser::class, 'isNewUploadTarget'); diff --git a/core/tests/Unit/Security/MediaBrowserSymlinkContainmentTest.php b/core/tests/Unit/Security/MediaBrowserSymlinkContainmentTest.php index 548263e75a..2d3af496da 100644 --- a/core/tests/Unit/Security/MediaBrowserSymlinkContainmentTest.php +++ b/core/tests/Unit/Security/MediaBrowserSymlinkContainmentTest.php @@ -82,16 +82,19 @@ function mediaBrowserSymlink(string $target, string $link): void ->and(mediaBrowserIsInside($this->browser, $this->tmp . '/images/../outside/secret.txt'))->toBeFalse(); }); -it('rejects a symlinked folder that points outside the type folder', function () { - mediaBrowserSymlink($this->tmp . '/outside', $this->tmp . '/images/link'); +// A link to somewhere new is a folder of the type, named by its own path: see the +// shared-type-root-request.php fixture, which runs with the CMS settings that decision needs. +it('rejects a symlinked folder that loops back around the type folder', function () { + mediaBrowserSymlink($this->tmp, $this->tmp . '/images/link'); expect(mediaBrowserIsInside($this->browser, $this->tmp . '/images/link'))->toBeFalse() - ->and(mediaBrowserIsInside($this->browser, $this->tmp . '/images/link/secret.txt'))->toBeFalse() - ->and(mediaBrowserIsInside($this->browser, $this->tmp . '/images/link/new.png'))->toBeFalse(); + ->and(mediaBrowserIsInside($this->browser, $this->tmp . '/images/link/outside/secret.txt'))->toBeFalse() + ->and(mediaBrowserIsInside($this->browser, $this->tmp . '/images/link/outside/new.png'))->toBeFalse(); }); -it('rejects a symlinked file and a dangling symlink', function () { - mediaBrowserSymlink($this->tmp . '/outside/secret.txt', $this->tmp . '/images/secret.txt'); +it('rejects a symlinked file that names a file a second time, and a dangling symlink', function () { + file_put_contents($this->tmp . '/images/team/secret.txt', 'secret'); + mediaBrowserSymlink($this->tmp . '/images/team/secret.txt', $this->tmp . '/images/secret.txt'); mediaBrowserSymlink($this->tmp . '/outside/missing.txt', $this->tmp . '/images/dangling'); expect(mediaBrowserIsInside($this->browser, $this->tmp . '/images/secret.txt'))->toBeFalse() diff --git a/core/tests/Unit/Security/SharedBrowserAccessTest.php b/core/tests/Unit/Security/SharedBrowserAccessTest.php index fdf1cf82f0..6a55455b38 100644 --- a/core/tests/Unit/Security/SharedBrowserAccessTest.php +++ b/core/tests/Unit/Security/SharedBrowserAccessTest.php @@ -60,15 +60,19 @@ function sharedBrowserResult(): array ->and(implode('\n', $result['log']))->toContain('file groups root cannot be resolved'); }); -it('keeps symlinks and traversal refused including for administrators', function () { +it('passes links to somewhere new, and keeps second names, dangling links and traversal refused', function () { $result = sharedBrowserResult(); - foreach (['escape', 'internal', 'dangling', 'traversal', 'adminEscape'] as $key) { + // internal names the private folder a second time, which would bypass its file groups + foreach (['internal', 'dangling', 'traversal'] as $key) { expect($result[$key])->toBeFalse($key); } - expect($result['adminRead'])->toBeTrue(); - expect($result['visibleLink'])->toBeTrue(); // inert link entry; opening it is still refused + // escape leads somewhere new: a folder of the type, named by its own path + expect($result['escape'])->toBeTrue() + ->and($result['adminEscape'])->toBeTrue() + ->and($result['adminRead'])->toBeTrue() + ->and($result['visibleLink'])->toBeTrue(); expect($result['safeThumbs'])->toBeTrue() - ->and($result['linkedThumbs'])->toBeFalse(); + ->and($result['linkedThumbs'])->toBeTrue(); }); it('accepts a proven shared root but reports missing and unmapped roots', function () { diff --git a/core/tests/Unit/Security/SharedTypeRootRequestTest.php b/core/tests/Unit/Security/SharedTypeRootRequestTest.php new file mode 100644 index 0000000000..685185caa5 --- /dev/null +++ b/core/tests/Unit/Security/SharedTypeRootRequestTest.php @@ -0,0 +1,111 @@ + $base, 'upload' => $upload])), + ], $exitCode); + $results[$key] = json_decode($output, true); + expect($exitCode)->toBe(0, $output) + ->and($results[$key])->toBeArray($output); + } + if (isset($results[$key]['skip'])) { + test()->markTestSkipped($results[$key]['skip']); + } + + return $results[$key]; +} + +dataset('site layouts', [ + 'site root by its real path, upload folder in the site' => ['canonical', 'site'], + 'site root through public_html -> evolution -> release' => ['linked', 'site'], + 'upload folder configured in shared storage' => ['linked', 'shared'], +]); + +it('keys a shared image folder the same way in both managers, from the configured URL', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + // what KCFinder makes of rb_base_url "assets/" here: not the folder, so nothing may rely on it + expect($result['typeURL'])->not->toBe('/assets/images') + ->and($result['sharedRoots'])->toBe(['assets/images']) + ->and($result['browserKey'])->toBe('assets/images/private/secret.png') + ->and($result['classicKey'])->toBe('assets/images/private/secret.png') + ->and($result['browserOutsiderReadsSecret'])->toBeFalse() + ->and($result['classicOutsiderReadsSecret'])->toBeFalse() + ->and($result['classicMemberReadsSecret'])->toBeTrue(); +})->with('site layouts'); + +it('passes links to somewhere new as folders named by their own path', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + expect($result['browserListing'])->toBe(['public', 'plugin-images', 'outside']) + ->and($result['browserPluginImage'])->toBeTrue() + ->and($result['browserPluginImageKey'])->toBe('assets/images/plugin-images/logo.png') + ->and($result['browserPluginUpload'])->toBeTrue() + ->and($result['classicPluginImage'])->toBe('assets/images/plugin-images/logo.png') + ->and($result['classicPluginFolder'])->toBe('assets/plugins/myplugin/images/logo.png') + ->and($result['classicOutside'])->toBe('assets/images/outside/loot.txt') + ->and($result['writeInside'])->toBeTrue() + ->and($result['writeIntoPlugin'])->toBeTrue(); +})->with('site layouts'); + +it('refuses second names, loops, dangling links and climbing out of a link', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + foreach (['browserSecondName', 'browserSiteName', 'browserLoop', 'browserDangling', 'browserClimb'] as $key) { + expect($result[$key])->toBeFalse($key); + } + foreach (['classicSecondName', 'classicSiteName', 'classicLoop', 'classicDangling', 'classicClimb', 'classicClimbOutOfPlugin'] as $key) { + expect($result[$key])->toBeNull($key); + } + expect($result['writeOverLink'])->toBeFalse() + ->and($result['writeThroughSecondName'])->toBeFalse() + ->and($result['browserInertLinks'])->toBe(['dangling', 'internal', 'team']) + ->and($result['inertListedAsLinks'])->toBe(['internal', 'team', 'dangling']); +})->with('site layouts'); + +it('applies the rules of the place a link sits in, code included', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + // a plugin checked out elsewhere stays behind save_plugin, and its code counts as code + expect($result['pluginProtected'])->toBeTrue() + ->and($result['pluginCode'])->toBe('plugin.php') + ->and($result['pluginCodeFromAssets'])->toBe('myplugin/plugin.php'); +})->with('site layouts'); + +it('lists the shared image folder as a folder that is entered but never removed or renamed', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + expect($result['isSharedRoot'])->toBeTrue() + ->and($result['isSharedRootPlugin'])->toBeFalse() + ->and($result['imagesListedAsFolder'])->toBeTrue() + ->and($result['imagesOffersDelete'])->toBeFalse() + ->and($result['imagesOffersRename'])->toBeFalse() + ->and($result['pluginImagesListedAsFolder'])->toBeTrue() + ->and($result['pluginImagesOffersDelete'])->toBeTrue(); +})->with('site layouts'); + +it('removes a linked folder as a link, never what it leads to', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + expect($result['removedLink'])->toBeTrue() + ->and($result['linkGone'])->toBeTrue() + ->and($result['checkoutKept'])->toBeTrue() + ->and($result['log'])->toBe([]); +})->with('site layouts'); + +it('refuses deleting, renaming or unlinking the shared type folder in the classic manager', function () { + $source = (string) file_get_contents(dirname(__DIR__, 4) . '/manager/actions/files.dynamic.php'); + + expect(substr_count($source, 'fileManagerSharedTypeRootAt($folder) !== null'))->toBe(1) + ->and(substr_count($source, 'fileManagerSharedTypeRootAt($dirname) !== null'))->toBe(1) + ->and(substr_count($source, 'fileManagerSharedTypeRootAt($linkPath) !== null'))->toBe(1); +}); diff --git a/manager/actions/files.dynamic.php b/manager/actions/files.dynamic.php index c85f79dac6..04f59147da 100755 --- a/manager/actions/files.dynamic.php +++ b/manager/actions/files.dynamic.php @@ -40,7 +40,7 @@ $uploadablefiles = add_dot($uploadablefiles); $upload_maxsize = evo()->getConfig('upload_maxsize'); $filemanager_path = rtrim(str_replace('\\', '/', - realpath(evo()->getConfig('filemanager_path')) ?: realpath(EVO_BASE_PATH)), '/'); + fileManagerCanonicalPath(evo()->getConfig('filemanager_path')) ?: realpath(EVO_BASE_PATH)), '/'); $base_path = rtrim(str_replace('\\', '/', realpath(EVO_BASE_PATH)), '/'); // end settings // get the current work directory @@ -50,7 +50,7 @@ $selFile = ''; if (is_file($fullpath)) { $selFile = $requested_path; - $startpath = rtrim(str_replace('\\', '/', realpath(dirname($fullpath))), '/'); + $startpath = rtrim((string) fileManagerCanonicalPath(dirname($fullpath)), '/'); } elseif (is_dir($fullpath)) { $startpath = $fullpath; } else { @@ -533,9 +533,11 @@ function renameFile(file) { if ($token_check) { $folderTarget = fileManagerResolvePath($filemanager_path, $_REQUEST['folderpath'] ?? ''); $folder = $folderTarget['path'] ?? ''; + $wasLink = $folder !== '' && fileManagerIsLink($folder); if ($folderTarget === null || !is_dir($folder)) { echo 'Invalid path.

'; } elseif (!fileManagerCanModifyExistingPath($folderTarget['relative'], $userGroups) + || fileManagerSharedTypeRootAt($folder) !== null || fileManagerPathTouchesProtected($folder, $protected_path) || fileManagerHasInaccessibleDescendants($folderTarget['relative']) || !is_writable($folder)) { @@ -547,7 +549,10 @@ function renameFile(file) { echo '' . $_lang['file_folder_not_deleted'] . '

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

'; - \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions(fileManagerAclKey($folderTarget['relative'])); + // a linked folder goes as a link: its content and groups stay for the link's return + if (!$wasLink) { + \EvolutionCMS\Support\FileManagerAccess::forgetRestrictions(fileManagerAclKey($folderTarget['relative'])); + } } } else { echo $tokenRefusedMessage; @@ -732,6 +737,8 @@ function renameFile(file) { } elseif (!fileManagerCanModifyExistingPath($linkRelative, $userGroups) || fileManagerPathIsProtected($linkPath, $protected_path)) { echo fileManagerDenied('file group, top level or protected folder'); + } elseif (fileManagerSharedTypeRootAt($linkPath) !== null) { + echo fileManagerDenied('shared type folder'); } elseif (($denied = fileManagerCodeRemovalDenied($linkPath, $linkRelative)) !== null) { // a link named .htaccess is read by the web server like the file echo $denied; @@ -754,6 +761,7 @@ function renameFile(file) { if ($dirTarget === null || !is_dir($dirname)) { echo 'Invalid path.

'; } elseif (!fileManagerCanModifyExistingPath($dirTarget['relative'], $userGroups) + || fileManagerSharedTypeRootAt($dirname) !== null || fileManagerPathTouchesProtected($dirname, $protected_path) || !is_writable($dirname)) { echo fileManagerDenied(); diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index abf5dd2f5e..2aa89b671f 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -1423,9 +1423,10 @@ protected function getFiles($dir) } /** - * The links in a folder, shown but never followed: they get no URL, thumbnail or action, and - * their target only as a path relative to the link, inside the type folder and reachable by - * the current manager (linkTarget()). + * The links in a folder that the file managers do not pass (fileManagerPassableLinkTarget()), + * shown but never followed: they get no URL, thumbnail or action, and their target only as a + * path relative to the link, inside the type folder and reachable by the current manager + * (linkTarget()). A passable link is listed with the folders and files. * * @param string $dir path below the upload folder, type included * @return array @@ -1446,7 +1447,8 @@ protected function getLinks($dir) continue; } $path = "$dir/$name"; - if (!fileManagerIsLink($path) || !$this->isLinkVisible($path)) { + // a link the file managers pass is listed as the folder or file it stands for + if (!fileManagerIsLink($path) || $this->isInsideTypeDir($path) || !$this->isLinkVisible($path)) { continue; } $stat = @lstat($path); @@ -1767,8 +1769,9 @@ protected function getDir($existent = true) /** * Whether $absPath, once symlinks are resolved, is still inside the current type folder. - * The dir and file parameters are only checked by name; a symlink below the upload folder - * would otherwise lead every action (and the file groups check) outside it. + * The dir and file parameters are only checked by name. A link below the upload folder is + * passed only when the file managers pass it (fileManagerPathContainsLink()): then it is a + * folder of the type, named by its own path, which the file groups are keyed by. * * @param string $absPath * @return bool @@ -1802,28 +1805,25 @@ protected function isInProtectedFolder($absPath) */ protected function isInsideTypeRoot($absPath) { - $root = realpath($this->typeDir); - if ($root === false) { + $realRoot = realpath($this->typeDir); + $path = fileManagerNormalizePathLexically($absPath); + if ($realRoot === false || $path === null) { + return false; + } + // by name: a link below the type folder that the file managers pass (isInsideTypeDir() + // asks fileManagerPathContainsLink() first) is a folder of it, wherever it leads + $path = rtrim($path, '/'); + $within = \EvolutionCMS\Support\FileManagerAccess::isWithin($this->typeDir, $path) + || \EvolutionCMS\Support\FileManagerAccess::isWithin(str_replace('\\', '/', $realRoot), $path); + if (!$within) { return false; } - $path = realpath($absPath); - if ($path === false) { + if (!file_exists($path)) { // nothing there yet is fine, a dangling link is not: writing to it creates its target - if (dir::isLink($absPath)) { - return false; - } - $parent = realpath(dirname($absPath)); - - return $parent !== false && \EvolutionCMS\Support\FileManagerAccess::isWithin( - str_replace('\\', '/', $root), - str_replace('\\', '/', $parent) - ); + return !fileManagerIsLink($path) && realpath(dirname($path)) !== false; } - return \EvolutionCMS\Support\FileManagerAccess::isWithin( - str_replace('\\', '/', $root), - str_replace('\\', '/', $path) - ); + return true; } /** diff --git a/manager/media/browser/mcpuk/lib/helper_dir.php b/manager/media/browser/mcpuk/lib/helper_dir.php index 0085d33d3d..d641096419 100755 --- a/manager/media/browser/mcpuk/lib/helper_dir.php +++ b/manager/media/browser/mcpuk/lib/helper_dir.php @@ -152,6 +152,11 @@ static function content(string $dir, array $options=[]) { } while ($type == "link"); } + // a Windows junction followed: its target's type + if ($options['followLinks'] && !in_array($type, ["dir", "file"], true)) { + $type = is_dir("$dir/$file") ? "dir" : (is_file("$dir/$file") ? "file" : $type); + } + if ((($type === "dir") && (($file == ".") || ($file == ".."))) || !preg_match($options['pattern'], $file) ) From 20a32a0fc3baf3a9acf176ff88d346114cc711ea Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 14:27:34 +0100 Subject: [PATCH 5/5] fix(file-manager): protect folders by real target and never write through a linked file A passed link keeps its own name, which file groups are keyed by, but the protected-folder rule compared only that name. A link to a folder protected outside the ACL root (an external view path; core/, manager/ or views/ under a narrowed filemanager_path) opened it to managers without the permission. fileManagerPathIsProtected() and fileManagerPathTouchesProtected() now also match where a path lands: the real path of its nearest existing ancestor with the rest appended, so new destinations are covered too. Removing a link resolves only the folder it sits in. textsave() checked a linked file by its name while file_put_contents() followed the link, so a manager without code permissions could write PHP into the target through a text name. It now refuses a final link through fileManagerIsSafeWriteTarget(), as the classic upload over an existing name does now too (a move across file systems copies, and the copy follows a link). Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 +- core/functions/actions/files.php | 84 +++++++++++++++++-- .../Fixtures/shared-type-root-request.php | 58 ++++++++++++- .../Security/FileManagerLinkDisplayTest.php | 4 +- .../FileManagerWritePathChecksTest.php | 4 +- .../Security/SharedTypeRootRequestTest.php | 31 +++++++ manager/actions/files.dynamic.php | 2 +- manager/media/browser/mcpuk/core/browser.php | 2 +- 8 files changed, 172 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ca19e8c4e7..64cadc01f0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -445,8 +445,8 @@ The name check also catches the spellings Windows turns into such a file: traili **Always enforced** - Paths outside `filemanager_path` are rejected, and `..` never climbs above the root. -- Links below a root are the operator's (the CMS never creates one), so a link to somewhere new is a folder (or file) like any other: a shared upload folder, a plugin checked out elsewhere, images linked in from another disk. Absolute or relative, whatever its spelling, it is judged by its real target (`fileManagerPassableLinkTarget()`), and passed when that target exists and is new: not inside or around the ACL root, not around the folder the link sits in (a loop), not inside or around anything the walk has named already (where it started, the links it passed), not inside shared storage that a type-folder link names (`fileManagerSharedTypeRoots()`). Any of those would give a file a second name, and file groups are keyed by name, so a group on `team` would not hold `public/link-to-team`. A passed link is named by its own path, never resolved (`fileManagerCanonicalPath()`, which also puts each part into its stored letter case and accepts any spelling of the root: a hosting `public_html` chain or a release link above it), and every rule applies by that name: the folders of layer 1 (`fileManagerProtectedPaths()` holds each by its real path and by its name, so a protected folder may itself be a link), file groups, the executable-name rule, the type folder of the media browser. Past a link, `..` is refused: it climbs in the link's target, not back where the server walks. Two separate links to one outside folder cannot be told apart from here; the operator should not make them, and a server without `FollowSymLinks` does not serve what the CMS then opens. Tests: `SharedTypeRootRequestTest.php` (a web request through the real browser constructor), `SharedBrowserAccessTest.php`. -- A path that passes through any other link below the root is rejected as a whole (`fileManagerPathContainsLink()`): in the classic manager, uploads, direct edit and delete, and the media browser (`isInsideTypeDir()`); a write never goes to a path that is itself a link, dangling or not (`fileManagerIsSafeWriteTarget()`). Both file managers list such a link as an inert entry: no web URL, drill-down, thumbnail or selection (a link may lead anywhere, and the web server would serve its target unchecked). The one action is removing the link itself (`mode=deletelink`, `act_deleteLink`), judged by the link's own path, never resolved: the top-level and group rules of an existing entry, and no link where a protected folder belongs; the target and its groups stay. Its target is shown only as a path relative to the link, and only when it lies inside the manager's root (the media browser: the type folder) and the manager may reach it (`fileManagerRelativeLinkTarget()`); a `../../` or absolute target outside, or a restricted one, is shown as "outside the file manager or not accessible", never as a path. Links are never created: a link entry in a ZIP is not extracted (neither as a link nor as a file holding its path) but reported (`fileManagerExtractZip()` `$links`), a directory archive stores a link as a link entry with that same relative target or leaves it out, and a folder copy leaves it out, passable or not. The media browser serves no thumbnail whose cached path or original runs through a link it does not pass. Nor does it write, rename, copy, move or empty the thumbnail cache through one (`isSafeThumbPath()`, measured from the upload root, so a link at the cache root itself is refused too, before anything is probed or created). +- Links below a root are the operator's (the CMS never creates one), so a link to somewhere new is a folder (or file) like any other: a shared upload folder, a plugin checked out elsewhere, images linked in from another disk. Absolute or relative, whatever its spelling, it is judged by its real target (`fileManagerPassableLinkTarget()`), and passed when that target exists and is new: not inside or around the ACL root, not around the folder the link sits in (a loop), not inside or around anything the walk has named already (where it started, the links it passed), not inside shared storage that a type-folder link names (`fileManagerSharedTypeRoots()`). Any of those would give a file a second name, and file groups are keyed by name, so a group on `team` would not hold `public/link-to-team`. A passed link is named by its own path, never resolved (`fileManagerCanonicalPath()`, which also puts each part into its stored letter case and accepts any spelling of the root: a hosting `public_html` chain or a release link above it), and every rule applies by that name: the folders of layer 1 (`fileManagerProtectedPaths()` holds each by its real path and by its name, so a protected folder may itself be a link), file groups, the executable-name rule, the type folder of the media browser. A protected folder is also matched by where a path lands (`fileManagerPathSpellings()`: the real path of its nearest existing ancestor, the rest appended), so a link to a view path outside the site does not open it under another name; removing a link resolves only the folder it sits in. Past a link, `..` is refused: it climbs in the link's target, not back where the server walks. Two separate links to one outside folder cannot be told apart from here; the operator should not make them, and a server without `FollowSymLinks` does not serve what the CMS then opens. Tests: `SharedTypeRootRequestTest.php` (a web request through the real browser constructor), `SharedBrowserAccessTest.php`. +- A path that passes through any other link below the root is rejected as a whole (`fileManagerPathContainsLink()`): in the classic manager, uploads, direct edit and delete, and the media browser (`isInsideTypeDir()`); a write never goes to a path that is itself a link, dangling or not (`fileManagerIsSafeWriteTarget()`, text saves and uploads over an existing name included): the checks see the link's name, the write would land in its target. Both file managers list such a link as an inert entry: no web URL, drill-down, thumbnail or selection (a link may lead anywhere, and the web server would serve its target unchecked). The one action is removing the link itself (`mode=deletelink`, `act_deleteLink`), judged by the link's own path, never resolved: the top-level and group rules of an existing entry, and no link where a protected folder belongs; the target and its groups stay. Its target is shown only as a path relative to the link, and only when it lies inside the manager's root (the media browser: the type folder) and the manager may reach it (`fileManagerRelativeLinkTarget()`); a `../../` or absolute target outside, or a restricted one, is shown as "outside the file manager or not accessible", never as a path. Links are never created: a link entry in a ZIP is not extracted (neither as a link nor as a file holding its path) but reported (`fileManagerExtractZip()` `$links`), a directory archive stores a link as a link entry with that same relative target or leaves it out, and a folder copy leaves it out, passable or not. The media browser serves no thumbnail whose cached path or original runs through a link it does not pass. Nor does it write, rename, copy, move or empty the thumbnail cache through one (`isSafeThumbPath()`, measured from the upload root, so a link at the cache root itself is refused too, before anything is probed or created). - Path comparison (`FileManagerAccess::isWithin()`) ignores case on Windows, and ZIP entries are resolved to the stored letter case (`fileManagerCanonicalCase()`) before the checks. - The media browser (KCFinder) follows the same rules: it stays out of the folders of layer 1 (`isInProtectedFolder()`), applies the executable-name rule to every name it writes (`validateFilename()`, including the final name of an upload), its temporary download archives carry the `kcf-download-` prefix and are the only `.zip` files it ever cleans up, and a folder rename never replaces an existing destination. diff --git a/core/functions/actions/files.php b/core/functions/actions/files.php index 6207080315..cacf094b17 100644 --- a/core/functions/actions/files.php +++ b/core/functions/actions/files.php @@ -486,16 +486,19 @@ function fileManagerRefuseReservedName($path) if(!function_exists('fileManagerPathIsProtected')) { /** - * Whether $path is a protected folder or lies inside one. + * Whether $path is a protected folder or lies inside one, by its name or where it lands. * * @param string $path canonical absolute path * @param string[] $protectedPaths + * @param bool $followLink false to judge a link by where it sits, not where it leads */ - function fileManagerPathIsProtected($path, array $protectedPaths) + function fileManagerPathIsProtected($path, array $protectedPaths, $followLink = true) { - foreach ($protectedPaths as $protectedPath) { - if (FileManagerAccess::isWithin($protectedPath, $path)) { - return true; + foreach (fileManagerPathSpellings($path, $followLink) as $spelling) { + foreach ($protectedPaths as $protectedPath) { + if (FileManagerAccess::isWithin($protectedPath, $spelling)) { + return true; + } } } @@ -513,9 +516,11 @@ function fileManagerPathIsProtected($path, array $protectedPaths) */ function fileManagerPathTouchesProtected($path, array $protectedPaths) { - foreach ($protectedPaths as $protectedPath) { - if (FileManagerAccess::isWithin($protectedPath, $path) || FileManagerAccess::isWithin($path, $protectedPath)) { - return true; + foreach (fileManagerPathSpellings($path) as $spelling) { + foreach ($protectedPaths as $protectedPath) { + if (FileManagerAccess::isWithin($protectedPath, $spelling) || FileManagerAccess::isWithin($spelling, $protectedPath)) { + return true; + } } } @@ -523,6 +528,59 @@ function fileManagerPathTouchesProtected($path, array $protectedPaths) } } +if(!function_exists('fileManagerPathSpellings')) { + /** + * $path as the file managers name it and as the file system resolves it. A passed link keeps + * its own name (file groups are keyed by it), but a protected folder is protected wherever it + * is reached from: a link to an external view path must not open it under another name. + * + * @param string $path absolute path, which need not exist yet + * @param bool $followLink false to resolve only the folder $path sits in + * @return string[] + */ + function fileManagerPathSpellings($path, $followLink = true) + { + $path = rtrim(str_replace('\\', '/', (string) $path), '/'); + if ($followLink) { + $resolved = fileManagerResolvedPath($path); + } else { + $parent = fileManagerResolvedPath(dirname($path)); + $resolved = $parent === null ? null : $parent . '/' . basename($path); + } + + return $resolved === null || $resolved === $path ? [$path] : [$path, $resolved]; + } +} + +if(!function_exists('fileManagerResolvedPath')) { + /** + * Where $path lands on the file system: the real path of its nearest existing ancestor with + * the rest of $path appended, so a destination that does not exist yet resolves too. Null + * when no part of it exists. + * + * @param string $path absolute path + */ + function fileManagerResolvedPath($path) + { + $current = rtrim(str_replace('\\', '/', (string) $path), '/'); + $tail = ''; + while ($current !== '') { + $real = realpath($current); + if ($real !== false) { + return rtrim(str_replace('\\', '/', $real), '/') . $tail; + } + $parent = dirname($current); + if ($parent === $current || $parent === '.') { + return null; + } + $tail = '/' . basename($current) . $tail; + $current = rtrim(str_replace('\\', '/', $parent), '/'); + } + + return null; + } +} + if(!function_exists('fileManagerIsLink')) { /** * Whether $path is a link of any kind: a symlink, or on Windows a junction or other @@ -2321,7 +2379,11 @@ function fileupload() // The restrictions are keyed by the stored name: on Windows secret.txt is Secret.txt $existingRel = ltrim(substr(fileManagerCanonicalCase($targetFile), strlen($filemanager_path)), '/'); $replacing = file_exists($targetFile); - if ($replacing && !fileManagerCanModifyExistingPath($existingRel)) { + if (!fileManagerIsSafeWriteTarget($filemanager_path, $targetFile)) { + // a move across file systems copies, and the copy follows a link + fileManagerLogDenied('upload over link', $existingRel, 'link'); + $fileMsg .= '

' . $_lang['files_access_denied'] . '

'; + } elseif ($replacing && !fileManagerCanModifyExistingPath($existingRel)) { fileManagerLogDenied('upload over existing file', $existingRel, 'file group'); $fileMsg .= '

' . $_lang['files_access_denied'] . '

'; } elseif (@move_uploaded_file($userfile['tmp_name'], $targetFile)) { @@ -2415,6 +2477,10 @@ function textsave() return 'Invalid path.

'; } $fileRel = ltrim(substr($filename, strlen($filemanager_path)), '/'); + // a linked file is checked by its name, while the write would land in its target + if (!fileManagerIsSafeWriteTarget($filemanager_path, $filename)) { + return fileManagerDenied('link', $fileRel); + } if (!fileManagerCanModifyExistingPath($fileRel) || fileManagerPathIsProtected($filename, fileManagerProtectedPaths()) || (fileManagerIsExecutableName($filename) && !fileManagerMayRunCode()) diff --git a/core/tests/Fixtures/shared-type-root-request.php b/core/tests/Fixtures/shared-type-root-request.php index a6032a7f72..16a56510a8 100644 --- a/core/tests/Fixtures/shared-type-root-request.php +++ b/core/tests/Fixtures/shared-type-root-request.php @@ -6,6 +6,8 @@ * constructor and config.php. The hosting chain public_html -> evolution -> evo-3.6.2 sits above * the site; assets/images links to ../../shared/assets/images; assets/plugins/myplugin links (by * an absolute path) to a checkout elsewhere, whose images are linked into the shared image folder. + * A configured view path lies outside the site and is linked into both managers' folders, and + * assets/files/note.txt links to a PHP file elsewhere. * * argv: base64 JSON {base: canonical|linked, upload: site|shared} * Prints JSON with both managers' answers. @@ -17,7 +19,7 @@ $shared = $home . '/shared/assets/images'; $repo = $home . '/repos/myplugin'; foreach ([$site . '/assets/files/team', $site . '/assets/plugins', $shared . '/private', $shared . '/public', - $repo . '/images', $home . '/outside'] as $dir) { + $repo . '/images', $home . '/outside', $home . '/views', $home . '/notes'] as $dir) { mkdir($dir, 0777, true); } file_put_contents($shared . '/private/secret.png', 'secret'); @@ -25,6 +27,9 @@ file_put_contents($repo . '/images/logo.png', 'logo'); file_put_contents($repo . '/plugin.php', 'denied = ['save_plugin', 'save_snippet', 'save_module']; + (new ReflectionProperty(browser::class, 'protectedPaths'))->setValue($browser, null); + $protected = fileManagerProtectedPaths(); + $textSave = static function ($path, $content) { + $_POST = ['path' => $path, 'content' => $content]; + + return strip_tags(textsave()); + }; + $deleteFile = static function ($path) { + $_REQUEST = ['path' => $path]; + + return strip_tags(delete_file()); + }; + $output += [ + 'saveThroughFileLink' => $textSave('assets/files/note.txt', ' file_get_contents($home . '/notes/run.php'), + 'savePlain' => $textSave('assets/files/plain.txt', 'edited'), + 'plain' => file_get_contents($site . '/assets/files/plain.txt'), + 'saveThroughFolderLink' => $textSave('assets/images/outside/loot.txt', 'edited'), + 'loot' => file_get_contents($home . '/outside/loot.txt'), + 'saveInViewAlias' => $textSave('assets/files/tpl/page.tpl', '{evil}'), + 'deleteInViewAlias' => $deleteFile('assets/files/tpl/page.tpl'), + 'view' => file_get_contents($home . '/views/page.tpl'), + 'createInViewAlias' => fileManagerPathIsProtected($root . '/assets/files/tpl/new.tpl', $protected), + 'createDeepInViewAlias' => fileManagerPathIsProtected($root . '/assets/files/tpl/sub/new.html', $protected), + 'renameInViewAlias' => fileManagerPathTouchesProtected($root . '/assets/files/tpl/page.tpl', $protected), + 'moveIntoViewAlias' => fileManagerPathIsProtected($root . '/assets/files/tpl/moved.txt', $protected), + 'browserViewAlias' => $invoke('isInsideTypeDir', $typeDir . '/views-alias/page.tpl'), + 'browserViewAliasNew' => $invoke('isInsideTypeDir', $typeDir . '/views-alias/new.tpl'), + 'sharedMediaOpen' => !fileManagerPathIsProtected($root . '/assets/images/public/new.png', $protected), + 'browserSharedMedia' => $invoke('isInsideTypeDir', $typeDir . '/public/new.png'), + 'aliasLog' => count(evo()->log), + ]; + evo()->log = []; + evo()->denied = []; + // deleting a linked folder removes the link, never what it leads to $output['removedLink'] = rrmdir($root . '/assets/plugins/myplugin'); $output['linkGone'] = !file_exists($site . '/assets/plugins/myplugin') && !is_link($site . '/assets/plugins/myplugin'); diff --git a/core/tests/Unit/Security/FileManagerLinkDisplayTest.php b/core/tests/Unit/Security/FileManagerLinkDisplayTest.php index ad3f96a16d..3404b24795 100644 --- a/core/tests/Unit/Security/FileManagerLinkDisplayTest.php +++ b/core/tests/Unit/Security/FileManagerLinkDisplayTest.php @@ -225,7 +225,7 @@ function linkDisplayRemove(string $path): void ->toContain("strpbrk(\$linkName, '/\\\\') !== false") ->toContain('fileManagerRefuseReservedName($linkName) || !fileManagerIsLink($linkPath)') ->toContain('fileManagerCanModifyExistingPath($linkRelative, $userGroups)') - ->toContain('fileManagerPathIsProtected($linkPath, $protected_path)') + ->toContain('fileManagerPathIsProtected($linkPath, $protected_path, false)') ->toContain('!fileManagerIsLink($linkPath) || !fileManagerRemoveLink($linkPath)') ->not->toContain('forgetRestrictions') ->not->toContain('rrmdir('); @@ -240,7 +240,7 @@ function linkDisplayRemove(string $path): void ->not->toContain('prune('); // the link's own path: canModifyExisting() would resolve it and judge the target expect($canModify)->toContain('fileManagerCanonicalCase($parent)') - ->toContain('fileManagerPathIsProtected($lexical, $this->protectedPaths)') + ->toContain('fileManagerPathIsProtected($lexical, $this->protectedPaths, false)') ->toContain('fileManagerCanModifyExistingPath(substr($lexical, strlen($root) + 1))') ->not->toContain('realpath($absPath)'); expect($functions)->toContain("'"); diff --git a/core/tests/Unit/Security/FileManagerWritePathChecksTest.php b/core/tests/Unit/Security/FileManagerWritePathChecksTest.php index 2f9f3ef6e7..9af82ea7b5 100644 --- a/core/tests/Unit/Security/FileManagerWritePathChecksTest.php +++ b/core/tests/Unit/Security/FileManagerWritePathChecksTest.php @@ -260,7 +260,9 @@ function runFileManagerWrite(string $site, string $function, array $request, arr expect($check)->not->toBeFalse() ->and($check)->toBeLessThan(strpos($body, 'move_uploaded_file')) ->and(substr($body, $check - 400, 650))->toContain('fileManagerCanonicalCase($targetFile)') - ->and(substr($body, $check, 250))->toContain('fileManagerCanModifyExistingPath('); + ->and(substr($body, $check, 600))->toContain('fileManagerCanModifyExistingPath(') + // and never over a link, which the move would follow + ->and(substr($body, $check, 250))->toContain('fileManagerIsSafeWriteTarget($filemanager_path, $targetFile)'); }); it('writes a refused save to the event log for the administrator', function () { diff --git a/core/tests/Unit/Security/SharedTypeRootRequestTest.php b/core/tests/Unit/Security/SharedTypeRootRequestTest.php index 685185caa5..aaff8ebbcb 100644 --- a/core/tests/Unit/Security/SharedTypeRootRequestTest.php +++ b/core/tests/Unit/Security/SharedTypeRootRequestTest.php @@ -81,6 +81,37 @@ function sharedTypeRootRequest(string $base, string $upload): array ->and($result['pluginCodeFromAssets'])->toBe('myplugin/plugin.php'); })->with('site layouts'); +it('never writes through a linked file, and edits through a linked folder', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + // note.txt is a text name for a PHP file elsewhere: the name passes the checks, the write would not + expect($result['saveThroughFileLink'])->toBe('files_access_denied') + ->and($result['fileLinkTarget'])->toBe('original') + ->and($result['savePlain'])->toBe('file_saved') + ->and($result['plain'])->toBe('edited') + ->and($result['saveThroughFolderLink'])->toBe('file_saved') + ->and($result['loot'])->toBe('edited'); +})->with('site layouts'); + +it('protects a view path outside the site wherever a link reaches it from', function (string $base, string $upload) { + $result = sharedTypeRootRequest($base, $upload); + + // a template file is executed by its engine whatever the extension + expect($result['saveInViewAlias'])->toBe('files_access_denied') + ->and($result['deleteInViewAlias'])->toBe('files_access_denied') + ->and($result['view'])->toBe('view') + ->and($result['createInViewAlias'])->toBeTrue() + ->and($result['createDeepInViewAlias'])->toBeTrue() + ->and($result['renameInViewAlias'])->toBeTrue() + ->and($result['moveIntoViewAlias'])->toBeTrue() + ->and($result['browserViewAlias'])->toBeFalse() + ->and($result['browserViewAliasNew'])->toBeFalse() + ->and($result['aliasLog'])->toBeGreaterThanOrEqual(3); + // shared media is not a protected folder + expect($result['sharedMediaOpen'])->toBeTrue() + ->and($result['browserSharedMedia'])->toBeTrue(); +})->with('site layouts'); + it('lists the shared image folder as a folder that is entered but never removed or renamed', function (string $base, string $upload) { $result = sharedTypeRootRequest($base, $upload); diff --git a/manager/actions/files.dynamic.php b/manager/actions/files.dynamic.php index 04f59147da..042459690f 100755 --- a/manager/actions/files.dynamic.php +++ b/manager/actions/files.dynamic.php @@ -735,7 +735,7 @@ function renameFile(file) { || fileManagerRefuseReservedName($linkName) || !fileManagerIsLink($linkPath)) { echo 'Invalid path.

'; } elseif (!fileManagerCanModifyExistingPath($linkRelative, $userGroups) - || fileManagerPathIsProtected($linkPath, $protected_path)) { + || fileManagerPathIsProtected($linkPath, $protected_path, false)) { echo fileManagerDenied('file group, top level or protected folder'); } elseif (fileManagerSharedTypeRootAt($linkPath) !== null) { echo fileManagerDenied('shared type folder'); diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index 2aa89b671f..aea26eb1cd 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -1570,7 +1570,7 @@ protected function canModifyLink($absPath) if ($this->protectedPaths === null) { $this->protectedPaths = fileManagerProtectedPaths(); } - if (fileManagerPathIsProtected($lexical, $this->protectedPaths)) { + if (fileManagerPathIsProtected($lexical, $this->protectedPaths, false)) { return false; } if (!fileManagerAclApplies()) {