Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media Manager upload size and file name validation #1248

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Media Manager upload size and file name validation #1248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73de039
MediaManager upload size/file name validation
Denoder Nov 4, 2024
f15d982
Update mediamanager.js
Denoder Nov 4, 2024
43cf718
Update UploadableWidget.php
Denoder Nov 4, 2024
c75c762
Update mediamanager.js
Denoder Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion modules/backend/traits/UploadableWidget.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ trait UploadableWidget
// */
// public $uploadPath;

/**
* @var bool $validateFileName Determines whether the file name should be validated.
*/
public bool $validateMediaFileName = true;

/**
* Returns the disk that will be used to store the uploaded file
*/
Expand Down Expand Up @@ -171,7 +176,7 @@ public function validateMediaFileName(string $fileName, string $extension): stri
/*
* File name contains non-latin characters, attempt to slug the value
*/
if (!$this->validateFileName($fileName)) {
if (!$this->validateFileName($fileName) && $this->validateMediaFileName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to allow a filename to skip validation? The validation exists for a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I'm adding multiple files and their have name formatting that i use elsewhere in a plugin so they can be associated with a relation for upload after batch uploading (An argument can be made to use fileupload, but the batch uploading is for individual relations that use file upload not just one using multiupload). I made it opt-out, but I do need it to keep the name formatting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't recommend abusing the media library system for that. I'd need to know more about your exact use case, but I could see you using an attachMany to accept multiple files and then re-assigning them to the relevant attachOne relationships post upload. Also why can't you have the files renamed when they get moved out of the medialibrary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to create a clone of the mediamanager as a plugin/widget rather than using that media manager.

$fileName = $this->cleanFileName(File::name($fileName)) . '.' . $extension;
}

Expand Down
27 changes: 27 additions & 0 deletions modules/backend/widgets/MediaManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public function __construct($controller, $alias, $readOnly = false)
{
$this->alias = $alias;
$this->readOnly = $readOnly;
$this->validateMediaFileName = Config::get('cms.storage.media.validateMediaFileName', true);

parent::__construct($controller, []);
}
Expand Down Expand Up @@ -109,6 +110,32 @@ public function render(): string
// AJAX handlers
//

/**
* Process file uploads submitted via AJAX
*
* @throws ApplicationException If the file "file_data" wasn't detected in the request or if the file failed to pass validation / security checks
*/
public function onUpload(): ?\Illuminate\Http\Response
{
ini_set('memory_limit', Config::get('cms.storage.media.memoryLimit', ini_get('memory_limit')));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have this here? Why not just put this code in the event listener instead if you really need it for your application? Why do you even need to increase the memory limit here in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had explained earlier when i was uploading large files I got a memory exhausted error (this was after fixing the error above). I can move it out if it works in the event listener (at the same time i was unsure if the event listener even work because everything else uses $this->fireSystemEvent that I thought that Event was unused.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A memory exhausted error feels more like the image is being processed in some way, I don't think I've seen it happen just from a file being uploaded before. I'd need more evidence that this is actually a problem that affects most Winter users before I'd want it in the core, you should be able to implement for your own project just with the existing event listener for now though.


if ($this->readOnly) {
return null;
}

/**
* @event backend.widgets.uploadable.onUpload
* Provides an opportunity to process the file upload using custom logic.
*
* Example usage ()
*/
if ($result = Event::fire('backend.widgets.uploadable.onUpload', [$this], true)) {
return $result;
}

return $this->onUploadDirect();
}

/**
* Perform a search with the query specified in the request ("search")
*/
Expand Down
8 changes: 5 additions & 3 deletions modules/backend/widgets/mediamanager/assets/js/mediamanager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,8 @@
paramName: 'file_data',
timeout: 0,
headers: {},
createImageThumbnails: false
createImageThumbnails: false,
maxFilesize: this.options.maxFilesize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably just populate this from an ini_get(post_max_size) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure how i'd proceed with this part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you're using the latest version of winter/storm and add data-max-filesize="<?= e(\File::getMaxUploadSize()); ?>" to https://github.com/wintercms/winter/blob/develop/modules/backend/widgets/mediamanager/partials/_body.php#L13

// fallback: implement method that would set a flag that the uploader is not supported by the browser
}

Expand Down Expand Up @@ -837,7 +838,7 @@
MediaManager.prototype.uploadError = function(file, message) {
this.updateUploadBar('error', 'progress-bar progress-bar-danger');

if (file.xhr.status === 413) {
if (file?.xhr?.status === 413) {
LukeTowers marked this conversation as resolved.
Show resolved Hide resolved
message = 'Server rejected the file because it was too large, try increasing post_max_size';
}
if (!message) {
Expand Down Expand Up @@ -1309,7 +1310,8 @@
selectSingleImage: 'Please select a single image.',
selectionNotImage: 'The selected item is not an image.',
bottomToolbar: false,
cropAndInsertButton: false
cropAndInsertButton: false,
maxFileSize: 0,
}

var old = $.fn.mediaManager
Expand Down
Loading