Repository navigation
Replace elFinder with an own file manager for the rich text editor - #984
Merged
Merged
Conversation
The editor's "Manage pictures" opens GrandAdmin.fileManager (adminapp, Bootstrap 5 and plain JavaScript) instead of elFinder. The Admin FileManager controller lists, uploads, creates folders, renames and deletes under assets/images/uploaded through IMediaFileStore; FileManagerService keeps every path under that root, accepts only MediaSettings.AllowedFileTypes (on upload and on rename), refuses files over 4 MB and never replaces an existing file. elFinder is removed as a whole: the elFinder.Net packages (and the Microsoft.AspNetCore.Http reference that only patched their dependency graph), the connector, its JSON converter, the vendored client and its styles, and the two elFinder command settings of the media settings.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| [TestInitialize] | ||
| public void Init() | ||
| { | ||
| _webRoot = Path.Combine(Path.GetTempPath(), "filemanager-" + Guid.NewGuid().ToString("N")); |
| public void Init() | ||
| { | ||
| _webRoot = Path.Combine(Path.GetTempPath(), "filemanager-" + Guid.NewGuid().ToString("N")); | ||
| _uploaded = Path.Combine(_webRoot, "assets", "images", "uploaded"); |
| _uploaded = Path.Combine(_webRoot, "assets", "images", "uploaded"); | ||
| Directory.CreateDirectory(_uploaded); | ||
| //what a path that escapes the root would reach: the site's own scripts | ||
| Directory.CreateDirectory(Path.Combine(_webRoot, "assets", "custom")); |
| Directory.CreateDirectory(_uploaded); | ||
| //what a path that escapes the root would reach: the site's own scripts | ||
| Directory.CreateDirectory(Path.Combine(_webRoot, "assets", "custom")); | ||
| File.WriteAllText(Path.Combine(_webRoot, "assets", "custom", "site.js"), "script"); |
| [TestMethod] | ||
| public void List_Root_FoldersFirstAndOnlyAllowedTypes() | ||
| { | ||
| Directory.CreateDirectory(Path.Combine(_uploaded, "zeta")); |
| public void List_Root_FoldersFirstAndOnlyAllowedTypes() | ||
| { | ||
| Directory.CreateDirectory(Path.Combine(_uploaded, "zeta")); | ||
| File.WriteAllText(Path.Combine(_uploaded, "a.png"), "x"); |
| { | ||
| Directory.CreateDirectory(Path.Combine(_uploaded, "zeta")); | ||
| File.WriteAllText(Path.Combine(_uploaded, "a.png"), "x"); | ||
| File.WriteAllText(Path.Combine(_uploaded, "placeholder.txt"), "x"); |
| Directory.CreateDirectory(Path.Combine(_uploaded, "zeta")); | ||
| File.WriteAllText(Path.Combine(_uploaded, "a.png"), "x"); | ||
| File.WriteAllText(Path.Combine(_uploaded, "placeholder.txt"), "x"); | ||
| File.WriteAllText(Path.Combine(_uploaded, ".hidden.png"), "x"); |
| [TestMethod] | ||
| public void List_NestedFolder_RelativePathsCrumbsAndEscapedUrls() | ||
| { | ||
| Directory.CreateDirectory(Path.Combine(_uploaded, "summer sale")); |
- FileSystemStore builds a renamed entry's path from the store path; the absolute Parent.FullName lost its leading slash and landed under the root twice on Linux - a new name must be a single name, and the root check requires a separator so a sibling folder sharing the root's prefix is outside - an empty entry in the allowed file types no longer allows files without an extension - the file manager upload request has its own size ceiling
| public void GetDirectoryInfo_SiblingSharingTheRootPrefix_Throws() | ||
| { | ||
| //Arrange - "<root>-old" starts with the root's path but lies outside it | ||
| var root = Path.Combine(Path.GetTempPath(), $"store-{Guid.NewGuid():N}"); |
Scryber is gone on develop, so elFinder was the last reason for the ImageSharp, Microsoft.AspNetCore.Http and elFinder.Net pins; the ImageSharp entries in .trivyignore go with them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #(none)
Type: feature
Issue
The rich text editor's "Manage pictures" button opened elFinder. Its server side,
elFinder.Net.*1.5.0, is no longer maintained:MissingMethodExceptionforImage.Load(Stream, out IImageFormat), so the file manager shows no thumbnails (HTTP 500 on/Admin/ElFinder/Thumb/...);Microsoft.AspNetCore.Httpreference only to patch an advisory;The editor only ever used a small part of it: browse
assets/images/uploaded, upload, create a folder, rename, delete, and pick a file to insert.Solution
An own file manager, in the panel's component style.
Server
FileManagerService(Grand.Web.AdminShared) overIMediaFileStore, rooted atassets/images/uploaded. Every path is split into names and each name is checked, so nothing can leave that root (..,., empty segments, drives, reserved Windows names, hidden files,\ / : * ? " < > |). This matters becauseFileSystemStoreonly keeps a path inside wwwroot, not inside a subfolder.MediaSettings.AllowedFileTypesare listed, uploaded and kept on rename (a picture cannot be renamed to.jsor.cshtml); 4 MB per file as elFinder's volume had; an upload never replaces a file (top (1).jpg); a case-only rename works on case-insensitive file systems.FileManagerController(Grand.Web.Admin):List,Upload,CreateFolder,Rename,Delete, JSON only (camelCase, mapped explicitly - the panel's serializer keeps PascalCase for the grids). Same authorization asElFinderControllerhad:HtmlEditoron the controller andHtmlEditorManagePicturesin every action; antiforgery fromBaseAdminController. Messages come from newAdmin.FileManager.*resources (DefaultLanguage.xml+Upgrade/en_250.xml).Client
GrandAdmin.fileManager(adminapp/src/ui/filemanager.js, inadmin.ui.js), opened inGrandAdmin.modal: breadcrumb, up, new folder, upload (button or drag and drop), rename, delete throughGrandAdmin.modal.confirm, picture previews, Insert as the one primary button, keyboard navigation. Names are written withtextContentonly.EditorTemplates/Editor.cshtmlopens it instead of elFinder; a picked picture is inserted as an image, a video as an iframe, anything else as a link, as before.Removed
elFinder.Net.AspNetCore,elFinder.Net.Core,elFinder.Net.Drivers.FileSystemand theMicrosoft.AspNetCore.Httpreference that only patched their graph.ElFinderController,IElFinderViewModelService,ElFinderViewModelService,BaseInfoResponseConverter(+ its test), the vendored client underwwwroot/administration/elfinderand its rules in_custom.scss.MediaSettings.FileManagerEnabledCommands/FileManagerDisabledUICommands(elFinder command lists) from the settings class, the Media settings screen and the Store settings controller. Stored values are ignored when the settings are read.Breaking changes
/administration/elfinder/...or called$.fn.elfinder/dialogelfindermust ship its own copy.MediaSettings.FileManagerEnabledCommandsandFileManagerDisabledUICommandsno longer exist./Admin/ElFinder/Connectorand/Admin/ElFinder/Thumbare gone.Testing
dotnet test src/Tests/Grand.Web.Admin.Tests→ all pass (44 new inFileManagerServiceTests: path validation, traversal on list/upload/delete, allowed types on upload and rename, size, no overwrite, case-only rename, root locked).Grand.Web.Store.Tests,Grand.Mapping.Tests,Grand.Business.Storage.Testspass.cd src/Web/Grand.SharedUIResources/adminapp && npm ci && npm test && npm run lint→ 230 passed (12 new infilemanager.test.js), lint clean. Bundles rebuilt withnpm run build..pngand a.js→ the picture is listed and selected, the script is refused with "This file type is not allowed…";x.js→ refused; to another.png→ renamed;GET /Admin/FileManager/List?path=../..,POST Deletewithpath=../../../administration/admin.common.js,POST CreateFolderwithname=../escapeandPOST Renamewith an empty path → all answersuccess: false; a POST withoutX-CSRF-TOKEN→ 400; signed out,/Admin/FileManager/Listredirects to the login page.