From 176ff69029987972ff53b669e1ecd974fd5a66ab Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 03:04:25 +0100 Subject: [PATCH 1/4] 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 e06fb0102c..1f9aa5cc1d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -477,6 +477,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 0b03d8b8cbf328d6bece67f2b1275989c81aba01 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 03:18:33 +0100 Subject: [PATCH 2/4] fix(file-browser): show both list and grid toggle buttons Since the icons became masks, background-color paints the glyph and the mask clips the border: the inactive toggle was white on white, and a selected toolbar button (Settings) showed a blue icon on blue. Draw the toggle button on the label and colour the glyphs through currentColor. Co-Authored-By: Claude Opus 5.5 --- .../mcpuk/js/tests/mask-icon-colors.test.js | 35 +++++++++++++++++++ .../media/browser/mcpuk/themes/evo/style.css | 15 +++++--- 2 files changed, 45 insertions(+), 5 deletions(-) create mode 100644 manager/media/browser/mcpuk/js/tests/mask-icon-colors.test.js diff --git a/manager/media/browser/mcpuk/js/tests/mask-icon-colors.test.js b/manager/media/browser/mcpuk/js/tests/mask-icon-colors.test.js new file mode 100644 index 0000000000..da7a23ff46 --- /dev/null +++ b/manager/media/browser/mcpuk/js/tests/mask-icon-colors.test.js @@ -0,0 +1,35 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const css = fs.readFileSync(path.join(__dirname, '..', '..', 'themes', 'evo', 'style.css'), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, ''); +const rules = [...css.matchAll(/([^{}]+)\{([^{}]*)\}/g)] + .map(([, selector, body]) => ({ selector: selector.trim(), body })); + +// evo draws its icons as masks on ::before, painted with background-color: currentColor. +// Any other background there recolours the glyph itself (white on white, blue on blue). +test('no ::before icon in the evo theme gets a background colour of its own', () => { + const offending = rules + .filter(({ selector }) => /:before\b/.test(selector)) + .filter(({ body }) => [...body.matchAll(/background(?:-color)?\s*:\s*([^;]+)/g)] + .some(([, value]) => value.trim() !== 'currentColor')) + .map(({ selector }) => selector); + + assert.deepEqual(offending, []); +}); + +test('the list/grid toggle draws its button on the label, so both states stay visible', () => { + const declared = (selector, property) => rules + .filter((rule) => rule.selector.split(',').map((s) => s.trim()).includes(selector)) + .some(({ body }) => new RegExp(`(^|;|\\s)${property}\\s*:`).test(body)); + + for (const label of ['label.radio-thumbs', 'label.radio-list']) { + assert.ok(declared(label, 'border'), `${label} has the button border`); + assert.ok(declared(label, 'background-color'), `${label} has the button background`); + assert.ok(declared(label, 'color'), `${label} sets the glyph colour`); + assert.ok(declared(`${label}.labelchecked`, 'color'), `${label}.labelchecked recolours the glyph`); + assert.ok(!declared(`${label}:before`, 'border'), `${label}:before is not the button: a mask would clip its border`); + } +}); diff --git a/manager/media/browser/mcpuk/themes/evo/style.css b/manager/media/browser/mcpuk/themes/evo/style.css index 49f73493fd..0a2bf64562 100644 --- a/manager/media/browser/mcpuk/themes/evo/style.css +++ b/manager/media/browser/mcpuk/themes/evo/style.css @@ -58,7 +58,9 @@ tr.selected > td, tr.selected:hover > td { background-color: #5b9bda; color: #ff #toolbar #mobileActions .menu a { margin: 0; } #toolbar a:before { font-size: 1.3rem; display: inline-block; padding-right: 3px; margin-top: 0px; vertical-align: top; color: #404040; font-weight: bold;} #toolbar a:hover, #toolbar a.hover { color: #292b2c; background-color: #e6e6e6; border-color: #adadad } -#toolbar a.selected, #toolbar a.selected:before { background-color: #3687e2; border-color: #3687e2; color: #fff; } +#toolbar a.selected { background-color: #3687e2; border-color: #3687e2; color: #fff; } +/* icons are masks painted with background-color: the glyph takes the text colour, never the button's background */ +#toolbar a.selected:before { color: #fff; } #toolbar a[href="kcact:upload"] { color: #fff; background-color: #5cb85c; border-color: #56ac56 } #toolbar a[href="kcact:upload"]:hover { color: #fff; background-color: #449d44; border-color: #419641 } #toolbar a[href="kcact:upload"]:before { content:"";display:inline-block;width:1em;height:1em;vertical-align:middle;background-color:currentColor;-webkit-mask:var(--evo-icon-upload) center/contain no-repeat;mask:var(--evo-icon-upload) center/contain no-repeat; color: #fff; font-weight: bold;} @@ -78,10 +80,13 @@ tr.selected > td, tr.selected:hover > td { background-color: #5b9bda; color: #ff } /*fake btn*/ #viewThumbs, #viewList { display: none; float: left; margin: 0 0px 0 -30px; } -label.radio-thumbs:before { display: block; float: left; font-size: 1.4rem; line-height: 1.6rem; vertical-align: middle; content:"";display:inline-block;width:1em;height:1em;vertical-align:middle;background-color:currentColor;-webkit-mask:var(--evo-icon-layout-grid) center/contain no-repeat;mask:var(--evo-icon-layout-grid) center/contain no-repeat; color: #292b2c; background-color: #fff; border: 1px solid #d4d4d4; padding: 0.5rem 0.6rem; margin-left: 5px; font-weight: bold;} -label.radio-list:hover:before, label.radio-thumbs:hover:before { color: #292b2c; background-color: #e6e6e6; border-color: #adadad } -label.radio-list.labelchecked:before, label.radio-thumbs.labelchecked:before { background-color: #3687e2; border-color: #3687e2; color: #fff; } -label.radio-list:before { display: block; float: left; font-size: 1.4rem; line-height: 1.6rem; vertical-align: middle; content:"";display:inline-block;width:1em;height:1em;vertical-align:middle;background-color:currentColor;-webkit-mask:var(--evo-icon-list) center/contain no-repeat;mask:var(--evo-icon-list) center/contain no-repeat; color: #292b2c; background-color: #fff; border: 1px solid #d4d4d4; padding: 0.5rem 0.6rem; margin-left: 5px; font-weight: bold; } +/* the button box sits on the label: a mask on ::before would clip its border and paint the glyph with its background */ +label.radio-thumbs, label.radio-list { display: block; float: left; margin-left: 5px; padding: 0.5rem 0.6rem; color: #292b2c; background-color: #fff; border: 1px solid #d4d4d4; } +label.radio-thumbs:before, label.radio-list:before { content: ""; display: block; width: 1.4rem; height: 1.6rem; background-color: currentColor; } +label.radio-thumbs:before { -webkit-mask: var(--evo-icon-layout-grid) center/1.4rem no-repeat; mask: var(--evo-icon-layout-grid) center/1.4rem no-repeat; } +label.radio-list:before { -webkit-mask: var(--evo-icon-list) center/1.4rem no-repeat; mask: var(--evo-icon-list) center/1.4rem no-repeat; } +label.radio-list:hover, label.radio-thumbs:hover { color: #292b2c; background-color: #e6e6e6; border-color: #adadad; } +label.radio-list.labelchecked, label.radio-thumbs.labelchecked { color: #fff; background-color: #3687e2; border-color: #3687e2; } input[type=checkbox] + label { color: #ccc; font-style: italic; } input[type=checkbox]:checked + label { color: #f00; font-style: normal; } #settings { background: #fff; padding: 5px; } From 5a9b58d21094ac59443d8de71afb0369faf2b3d1 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 06:15:54 +0100 Subject: [PATCH 3/4] 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 1f9aa5cc1d..9b3838f5e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -447,8 +447,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`). @@ -460,7 +460,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 | @@ -472,12 +472,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 ffcc33bddf..79cc0b31f8 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'; @@ -1198,7 +1493,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) { @@ -1280,7 +1575,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; } @@ -1511,7 +1806,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 ?? [])) { @@ -1553,7 +1856,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); @@ -1566,7 +1869,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); @@ -1587,7 +1891,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.

'; } @@ -2160,14 +2463,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.

'; } @@ -2183,11 +2486,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 64e203e68f..cb357c1702 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 6f17a446c4..36b897a1fd 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 bf28687b83..6e71449ccf 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 5bda4fd336..d1e33106fe 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 022b8643c2dcdd4790a77c51754506460c5f5e31 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Tue, 3 Nov 2026 14:27:34 +0100 Subject: [PATCH 4/4] 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 9b3838f5e2..eb2d06257c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -473,8 +473,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 79cc0b31f8..36ea2feb59 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 @@ -2339,7 +2397,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)) { @@ -2433,6 +2495,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 cb357c1702..cd0494a067 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()) {