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

Command icon 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

stefanos.petrakis created an issue. See original summary.

wim leers’s picture

Very 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 🤓

stefanos.petrakis’s picture

Title: Possibly faulty validation sequence for single image upload » 3328547-possibly-faulty-validation-ckeditor-image-upload

All righty, thanks for the helping hand, here is the simplest test I could come up with.

stefanos.petrakis’s picture

Title: 3328547-possibly-faulty-validation-ckeditor-image-upload » Possibly faulty validation sequence for single image upload
Manoj Raj.R’s picture

yes i could agree with Wim Leers.

maxilein’s picture

Maybe 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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kim.pepper’s picture

This 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.