Skip to content

fix(file-manager): shared storage and operator links in both file managers (3.5.x) - #2549

Merged
Seiger merged 5 commits into
evolution-cms:3.5.xfrom
elcreator:fix-group-permissions-for-shared-storage-3.5
Nov 3, 2026
Merged

Seiger merged 5 commits into
evolution-cms:3.5.xfrom
elcreator:fix-group-permissions-for-shared-storage-3.5

Conversation

@elcreator

Copy link
Copy Markdown

Backport of #2546 (MiddleSokilAI) to 3.5.x, with the follow-up fixes and the file-manager link model from the matching 3.6 PR.

The hole on 3.5.x

When a type folder of the media browser lives in shared storage (assets/images a symlink into /shared/..., or rb_base_dir set outside the release, as Deployer and many hosting setups do), the browser built file-group keys from the real path. That path lies outside the file manager root, so the key came out empty, which means public: every group restriction under assets/images was ignored for managers who are not administrators. Reproduced on 3.5.x: a manager outside the group lists and reads private/secret.jpg.

The dashboard also warned "File base path seems incorrect" for any rb_base_dir outside the base path, although shared storage is a valid setup.

Changes

  1. fix(file-browser): preserve group permissions for shared storage #2546 (cherry-picked). It maps the shared type folder back to its old assets/images/... keys, refuses access when the mapping cannot be proven, and replaces the dashboard check with a real usability check.
  2. Map shared roots from the configured URL. fix(file-browser): preserve group permissions for shared storage #2546 took the site-side spelling from KCFinder's typeURL, which KCFinder resolves against the browser script (/manager/media/browser/mcpuk/assets/images, or /images when DOCUMENT_ROOT is a symlink). The mapping therefore never matched, and restricted managers got an empty, read-only browser; on 3.6 this happened after fix(file-browser): preserve group permissions for shared storage #2546 was merged. config.php now passes the configured rb_base_url as _siteUploadURL, and the browser maps from it. A type folder served from outside a narrowed filemanager_path carries no groups again, instead of refusing every action.
  3. Operator links in both file managers. The CMS never creates links, so a link below a root is the operator's: a shared upload folder, a plugin checked out from a separate repository, images from another disk. A link whose real target exists and is new 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, the type folder.
    • Still inert: dangling links, loops, and links that would give a file a second name, which would bypass file groups.
    • .. past a link is refused.
    • Removing a passable link removes the link only and keeps its groups.
    • Code under a passable link counts for the "may not remove code" rule.
  4. Both managers give the same answer for a shared type folder. The classic manager enters it with the browser's keys, so its groups can be edited again; the link itself is never deleted, renamed or unlinked.
  5. Tests drive the real start-up path. SharedTypeRootRequestTest builds the browser with the real constructor, config.php, $_SERVER and the working directory of browse.php, through a public_html -> evolution -> release chain. The older link tests now use links that stay inert whatever the environment.

The file-manager section of AGENTS.md describes the link model.

Verification

  • tests/Unit tests/Feature on PHP 8.4 (Linux): all green except PhpThumbCacheImagesTest, which reads ng.inx and fails only in a git archive copy (the file is export-ignore).
  • PHPStan: no errors.
  • The same changes are verified in Docker on 3.6 (Deployer-style shared images, restricted manager).

🤖 Generated with Claude Code

MiddleSokilAI and others added 4 commits November 3, 2026 05:29
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
@MiddleSokilAI

Copy link
Copy Markdown
Collaborator

Thanks for preserving support for operator-managed shared folders. Two security boundaries still need attention before merging. Reviewed statically at c8f817c; I have not run this PR.

  1. The text editor still writes through a final symbolic link. In core/functions/actions/files.php:2402, textsave() uses fileManagerPathContainsLink() and fileManagerCanonicalPath(), which now accept passable links and preserve their names, then reaches file_put_contents() at :2427. It never calls fileManagerIsSafeWriteTarget(), whose final-component check at :912 explicitly rejects this case. With an existing passable file link, write permissions and the executable-name restriction are checked against the link's name while the write follows its target. A manager user allowed to edit files but not run code can consequently modify a target that those checks were intended to protect. This requires an existing operator/filesystem-provided link; the manager need not create one.

Please apply the safe-write-target guard to text saves before opening the file, reject final-component links consistently with other write operations, and retain the existing ACL and code-permission checks. Add a regression that invokes the actual text-save path as a user without code privileges against an existing final file link and verifies denial and unchanged target contents; also cover an ordinary allowed text file and a permitted parent-directory link.

  1. Protection of external view.paths is checked against the lexical name rather than the resolved target. fileManagerProtectedPaths() (:247) records the configured view directories and their real paths, but fileManagerPathIsProtected() (:494) and fileManagerPathTouchesProtected() (:514) only compare those against the incoming path spelling. fileManagerCanonicalPath() deliberately retains a passable link's name. When an accessible linked directory resolves into an external configured view directory, the lexical name can therefore miss the protected real directory. Such files are executable by their template engine regardless of extension, so users without code privileges must not be allowed to write there through an alias.

Please keep the lexical identity for file-group ACLs while also checking the resolved filesystem target against protected real paths for every relevant read/change guard. For new write destinations, resolve and validate the nearest existing parent and the appended destination; retain the intended allowance for unrelated shared media directories. Add regression coverage for an external configured view directory reached through a passable directory link: text-save, create/upload, rename/move and delete must respect the protection for a user without code privileges, including non-PHP template extensions. Verify that an unrelated shared media directory remains usable.

SokilAI

…ough 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 <noreply@anthropic.com>
@elcreator

Copy link
Copy Markdown
Author

Thanks, both are confirmed and fixed in 20a32a0. Both are regressions of this PR: before it, no link was ever passed, so neither alias existed.

1. Text save through a final link. textsave() checked the link's name, and file_put_contents() followed the link. It now calls fileManagerIsSafeWriteTarget() before the other checks, so a final link is refused and logged. Folder links on the way still pass. The ACL, protected-folder and code-permission checks are unchanged.

The classic upload over an existing name had the same gap: move_uploaded_file() falls back to a copy across file systems, and the copy follows the link. It now uses the same guard. That one predates the PR, because fileupload() never called the guard.

2. Protected folders reached through a link. This applies to every protected folder whose real path is outside the ACL root: an external view.paths entry, and, with a narrowed filemanager_path, also core/, manager/ and views/.

  • fileManagerPathIsProtected() and fileManagerPathTouchesProtected() now match both the name and where the path lands (fileManagerPathSpellings(): the realpath of the nearest existing ancestor, with the rest of the path appended). New destinations are covered too.
  • Every guard in both managers goes through these two functions, so this closes all of them: text save, delete, new file or folder, upload, rename or move, ZIP extraction, folder copy, and the media browser's isInsideTypeDir().
  • File-group keys still use the link's own name.
  • Removing a link (deletelink, act_deleteLink) resolves only the folder the link sits in, so it is still judged by where the link is, not where it leads.

Tests. SharedTypeRootRequestTest runs the real textsave() and delete_file() as a manager without code permissions, in three layouts: canonical root; a public_html -> evolution -> release chain; rb_base_dir in shared storage.

  • assets/files/note.txt links to a PHP file outside the site: the save is refused and the target is unchanged. An ordinary text file saves, and so does a file under a passable folder link.
  • assets/files/tpl (classic manager) and views-alias (media browser) link to an external view path:
    • saving and deleting page.tpl are refused, and the file is unchanged;
    • creating new.tpl or sub/new.html, renaming, and moving into it are all refused (create, rename and move are checked at the guard level, not through a full request);
    • shared media stays writable.
  • Without the fix, the fixture deletes views/page.tpl through the alias.

Also checked over HTTP on a Deployer-style container (assets/files linked to shared storage, an extra view.paths entry via core/custom/config/view/paths.php), as a manager with file_manager and no code permission:

Request Before After
save assets/files/note.txt (link to /data/www/notes/run.php) written: <?php evil(); Access denied
save assets/files/tpl/page.tpl (external view path) written refused
delete assets/files/tpl/page.tpl deleted refused
save assets/files/plain.txt saved saved

Full tests/Unit tests/Feature suite on PHP 8.4: all green except PhpThumbCacheImagesTest, which reads ng.inx, a file that is missing from my export copy. PHPStan: no errors. AGENTS.md now covers both rules.

@Seiger
Seiger merged commit 20d14aa into evolution-cms:3.5.x Nov 3, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants