Problem/Motivation
CKEditor5ImageController performs a validation on the uploaded single image; that validation logic differs from the one we see in FileUploadHandler and the differences need to be investigated to figure out which one is the more robust one, or if some kind of normalization/refactoring is needed to unify the validation logic.
Steps to reproduce
An issue on a contrib module #3328277: Can't upload files with CKEditor5 file upload brought this to attention.
The findings and important differences so far are that:
(a) CKEditor5ImageController during upload and validation will create a new File entity that references the destination file that is not present yet on the file system; that causes FileUploadSecureValidator's validation logic to issue an error.
CKEditor5ImageController performs first the File entity constraint validation and then calls the file_validate function that engages hook implementations for hook_file_validate.
(b) FileUploadHandler::handleFileUpload's code (which seems quite similar to CKEditor5ImageController::upload and CKEditor5ImageController::validate) runs in a different way.
The File entity it creates references the source (temporary uploaded file) and not the destination file; that allows hook implementations of hook_file_validate to have access to a File entity that references an existing file present on the filesystem;
it also performs first the call to the file_validate function, then sets the File entity's uri to the destination filename and then proceeds to perform constrain validation on the File object itself before it's saved.
The latter (b) code flow seems more robust and would resolve the reported problem.
Proposed resolution
Figure out if CKEditor5ImageController has a faulty validation logic, comparing the implementation with that of FileUploadHandler::handleFileUpload.
Issue fork drupal-3328547
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
wim leersVery interesting. This will need careful review. To my knowledge, this was modeled to match the behavior in
\Drupal\editor\Form\EditorImageDialog.But it's possible something is wrong. File uploads and validating them is unfortunately kind of a mess 😬 See #2940383: [META] Unify file upload logic of REST and JSON:API.
A test that shows where/how it's failing would certainly make this issue move forward faster 🤓
Comment #4
stefanos.petrakisAll righty, thanks for the helping hand, here is the simplest test I could come up with.
Comment #5
stefanos.petrakisComment #6
Manoj Raj.R commentedyes i could agree with Wim Leers.
Comment #7
maxilein commentedMaybe this is related and can be solved on the go of this issue:
clamav prevents upload because:
"I don't think clamav can do anything about this if another module is not calling hook_file_validate when it processes uploads."
https://www.drupal.org/project/clamav/issues/2972967
Comment #9
kim.pepperThis looks like a duplicate of #3372385: CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. which has progressed much further along. Transferring issue credit to that one.