Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG…

That's intentional: SVGs are insecure to serve.

Nothing in Drupal allows SVG uploads: not the image field type, not Media, not CKEditor 4, not user profile picture, not site logo, nothing. Because it is insecure.

At least today. See linked issues. So for now, there's nothing to do here.

Sorry 😞

That's intentional: SVGs are insecure to serve.

@Wim Leers, that's correct but if we want to use a module such as https://www.drupal.org/project/issues/svg_image, which is sanitizing the file? What's the API that such a module is triggering to allow SVG upload without patching the core?

As far as I can see the CKE config can be update to support svg+xml by implementing hook_ckeditor5_plugin_info_alter(), even, for some reasons is not straight because there's no setter for "ckeditor5" definition. But how about CKEditor5ImageController::upload()? Could we, at least, pass the extensions as route options or defaults instead of hardcoding them? BTW, they are now 'gif png jpg jpeg', not even following CKE, which accepts also WebP.

BTW, they are now 'gif png jpg jpeg', not even following CKE, which accepts also WebP.

They're matching what was allowed for CKEditor 4 — see \Drupal\editor\Form\EditorImageDialog::buildForm().

Could we, at least, pass the extensions as route options or defaults instead of hardcoding them?

I like that! 😊 That'd make it easy for people Who Know What They're Doing like yourself to allow SVG easily. That's the reason you'd like it to work like this, right? 🤓

Here's a rough draft of how to programmatically allow uploading any type of file to CKEditor5. If the svg_image module implemented this, it would solve @Claudiu.cristea's issue, but also add support for ".webp", ".avif", etc..., as suggested by @wim-leers. The issue title probably needs to be changed now?
This doesn't have any tests yet, I'm just looking for feedback on the general idea.
I did not base this on the previous patch, so I'm not including an interdiff, but if you would like, I can include one :). Feedback welcome.

Can the issue summary be updated with the proposed solution please. Easier for reviews when the default template is used.

Thanks!

Updated issue summary

Applied the patch #11 but don't appear able to upload an image via image upload button in ckeditor.

Also noticed test coverage was missing we will need some

Thanks.

@smustgrave will add test coverage soon, you'll need some code like this to upload new file types and to be running CKEditor 5:

function my_module_ckeditor5_image_controller_extensions_alter(array &$extensions): void {
  $extensions[] = 'svg';
}

function my_module_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
  // Add a custom file type to the image upload plugin. Note that 'svg+xml' below
  // should be an IANA media type Name.
  // https://www.iana.org/assignments/media-types/media-types.xhtml#image 
    $imageUploadPlugin = $plugin_definitions['ckeditor5_imageUpload']->toArray();
    $imageUploadPlugin['ckeditor5']['config']['image']['upload']['types'][] = 'svg+xml';
    $plugin_definitions['ckeditor5_imageUpload'] = new \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition($imageUploadPlugin);
}

See also comments #8, #9 and #10 for why this custom code is necessary

I applied #11 and tested with the suggested snippet (from @AleexGreen on #15). Notice i created a custom module named svg-test with the snippet, and also tested on a new, fresh Drupal 10 installation.

Patch applied with no issues and solution works great! Only tests left to make it RTBC i guess.

Evidence attached.

Thanks.

Let's see if Test Bot likes this test.

Sorry, bad patch. Let's see if this one works.

Fixed some lints, now ready for review. Feedback welcome!

1. +++ b/core/modules/ckeditor5/ckeditor5.api.php
   @@ -272,6 +281,16 @@ function hook_ckeditor4to5upgrade_plugin_info_alter(array &$plugin_definitions):
   + * Modify the list of allowed extensions for the image upload plugin.
   

   s/Modify/Modifies/

2. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
   @@ -144,8 +156,12 @@ class CKEditor5ImageController extends ControllerBase {
   +    $extensions = ['gif', 'png', 'jpg', 'jpeg'];
   +    $this->moduleHandler
   +      ->alter('ckeditor5_image_controller_extensions', $extensions);
   +
   

   🤔 This is only modifying the Drupal-side validation logic.

   Shouldn't we also update

   ckeditor5_imageUpload:
     ckeditor5:
       plugins:
         - image.ImageUpload
         - drupalImage.DrupalImageUpload
       config:
         image:
           upload:
             types: ["jpeg", "png", "gif"]
   

   ?

   See https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imageconfig-....

Here is a patch for problem #1.
@wim-leers for problem #2, we adjust ckeditor5_imageUpload.ckeditor5.config.image.upload.types in ckeditor5_test_module_allowed_image_ckeditor5_plugin_info_alter() i.e.: hook_ckeditor5_plugin_info_alter()

Added a draft Change record

CR reads well I think

+    $extensions = ['gif', 'png', 'jpg', 'jpeg'];
+    $this->moduleHandler
+      ->alter('ckeditor5_image_controller_extensions', $extensions);

I think .webp should be officially supported, so it should be in the $extensions list? Setting to NR to weight on that.

EDIT: Or should be treated in a followup?

Personally I would say follow up. Think expanding the allowed list could be larger conversation vs around this new hook.

OK, agree. Just keep in mind that WebP is officially adopted as image type by Drupal core, so we need to add the followup only for .webp. A future expansion of accepted types is out of scope for the followup.

Setting to NR to have the followup created and linked here.

Follow-up issue added: #3386605: [PP-1] Add CKEditor 5 support for WebP images

Thank you

#21.2: Right, ckeditor5_test_module_allowed_image_ckeditor5_plugin_info_alter() only modifies the plugin definition. And ckeditor5_test_module_allowed_image_ckeditor5_image_controller_extensions_alter() modifies only the Drupal file upload validation logic.

My bad!

But what's missing here is explicit docs in:

1. the ckeditor5_imageUpload plugin definition that points out this connection (which was not necessary before because it was all hard-coded)
2. the change record
3. last but not least: ckeditor5.api.php, since this issue is increasing the API surface!

It's critical that these two are in sync. And it's also critical to describe why in one place you need svg and in the other svg+xml.

I've made the documentation clear in both cases (filename extension vs image media type name) and the change record. Feedback is welcome :).

I'm sorry to be so annoying, but I still think the connection isn't quite clear enough 😅

1. +++ b/modules/ckeditor5/ckeditor5.api.php
   @@ -255,8 +255,9 @@ function hook_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
      // Add a custom file type to the image upload plugin. Note that 'tiff' below
   -  // should be an IANA media type Name.
   -  // https://www.iana.org/assignments/media-types/media-types.xhtml#image
   +  // should be an IANA image media type Name.
   +  // @see https://www.iana.org/assignments/media-types/media-types.xhtml#image
   +  // @see https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imageconfig-ImageUploadConfig.html#member-types
      assert($plugin_definitions['ckeditor5_imageUpload'] instanceof CKEditor5PluginDefinition);
      $imageUploadPlugin = $plugin_definitions['ckeditor5_imageUpload']->toArray();
      $imageUploadPlugin['ckeditor5']['config']['image']['upload']['types'][] = 'tiff';
   

   This should have:

   // @see hook_ckeditor5_image_controller_extensions_alter()
   

2. +++ b/modules/ckeditor5/ckeditor5.api.php
   @@ -288,6 +289,9 @@ function hook_ckeditor4to5upgrade_plugin_info_alter(array &$plugin_definitions):
   +  // The following array values should be the file name extensions to be
   +  // validated by the image upload controller.
   +  // @see https://www.drupal.org/node/3363700
   

   This should have:

   // @see hook_ckeditor5_plugin_info_alter
   

3. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
   @@ -144,8 +156,12 @@ class CKEditor5ImageController extends ControllerBase {
   +    $extensions = ['gif', 'png', 'jpg', 'jpeg'];
   +    $this->moduleHandler
   +      ->alter('ckeditor5_image_controller_extensions', $extensions);
   

   Can we move this logic into a protected function getAllowedExtensions() helper method? Then that provides us a point to point towards.

   Also, can we add an @see ckeditor5_imageUpload in ckeditor5.ckeditor5.yml just above the $extensions variable declaration, to make the connection clear?

4. +++ b/modules/ckeditor5/ckeditor5.ckeditor5.yml
   @@ -532,6 +532,8 @@ ckeditor5_imageUpload:
   +          # https://www.iana.org/assignments/media-types/media-types.xhtml#image
   

   • Nit: missing leading @see 😅
   • Thanks to the previous point, we can now do @see \Drupal\ckeditor5\Controller\CKEditor5ImageController::getAllowedExtensions() 😊

As I wrote #31, I was thinking: SURELY there must be some way to define this only once and figure out one from the other?!

Then we wouldn't need hook_ckeditor5_image_controller_extensions_alter() (aka the API addition!) at all. Which is much better: less API surface, and no need to manually keep the two in sync, so it also removes the need for documenting that tricky aspect, which is what I've been worried about in #29 and #31!

And … sure enough, core has the necessary infra. AFAICT this pseudocode should work:

$mimetypes = new \Symfony\Component\Mime\MimeTypes();
$imageUploadPlugin = $plugin_definitions['ckeditor5_imageUpload']->toArray();
$extensions = [];
foreach ($imageUploadPlugin['ckeditor5']['config']['image']['upload']['types'] as $mime_type) {
  $extension = $mimetypes->getExtensions($mime_type);
}

That'd make this issue trivial to land 😊

@wim-leers, after some manual testing, I figured out that symfony/mime expects to be passed the fully-qualified mime type with subtype and type ( i.e. the Template (second) column in this table) in order to look up the file extensions.
However, CKEditor5 expects to be passed the IANA image type name (i.e. the Name (first) column in the table)

Internally, CKEditor5 adds/removes the 'image/' string to the beginning of the IANA image type to convert between fully-qualified mime types and image types, but as far as I can tell this assumption isn't guaranteed by the IANA, so it may be risky for us to use it too. I'm unsure on how else to achieve this without patching CKEditor5 (i.e. so it always handles fully-qualified mime types) or symfony/mime (i.e. to make it aware of how IANA image type names map to fully-qualified mime types) .

Thanks for your very thorough work here, @AleexGreen — it does not go unnoticed and is deeply appreciated! 🤩 👏

Given the very long list in the table you link, I do think that's a safe assumption to make. Especially because in December 2020, some new top-level types were added.

Any image file type that does not use the image top-level type will not work correctly in many applications.

All I had to do here was fix nits and copy/paste bits and pieces from \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::addImage() to make the test coverage actually do what it claims 👍

P.S.: looking forward to having our paths cross again here in the d.o issue queue! 😊

Updated issue summary 👍

Added a little bit of feedback but overall this looks good to me.

I think this is ready for re-review

Seems all open threads have been addressed and already received reviews from @longwave and @Wim Leers

Posted one last nit, but it's really something that is already in HEAD.

This MR looks great — thanks @AleexGreen!

Last thread resolved, thanks @wim-leers!

This looks great!

However the MR needs rebasing against 11.x following #3221793: Move file upload validation from file.module to constraint validators which also added to the CKEditor5ImageController constructor. It might be easier to avoid changing all arguments to use constructor property promotion, there is an issue somewhere to do this in bulk at some point in the future.

RTBC++ 😊

EDIT: cross-posted with @longwave and … shoot, he's right, even though @AleexGreen rebased on latest upstream. Root cause: this MR was created before the 11.x branch existed 😬

@longwave Could you please change the target branch to 11.x? Only committers can do that unfortunately 🫣

Might also be worth waiting for #3388985: Make CKEditor5ImageController reuse FileUploadHandler?

Didn't realise that the linked issue above only went into 11.x (10.3.x) and so I'm not sure how to get this into 10.2.x without having to untangle it again in 10.3.x given both want to change the constructor :/

Maybe this just has to be 11.x/10.3.x only as well?

@longwave I didn't either! 🤪🙈

I think this issue/feature request is hard-blocking some sites (i.e. those sites that absolutely need SVG or webp images).

On the other hand, #3221793: Move file upload validation from file.module to constraint validators and #3388985: Make CKEditor5ImageController reuse FileUploadHandler are "only" changing implementation details: zero end user impact.

Conclusion: this issue should land in both 10.2.x (in its current MR form) and 11.x (in a rebased form).

Do you agree? If you do, then @AleexGreen can just open a second merge request that targets 11.x!

(@AleexGreen: the policy is that everything must first land in 11.x before it lands in 10.x.y — otherwise we risk making the future major less capable than the current major 😅)

Given #3388985: Make CKEditor5ImageController reuse FileUploadHandler is a standard refactoring task with no deadline and this is a major feature request blocking other sites, and they both want to touch the same controller, I think we should postpone that one on this, and try to get this in to 10.2.0 before next week's beta deadline.

💯

I opened a new MR for 11.x since the current MR would need to go in as-is into 10.2.x anywya.

Since this is just applying the existing MR + resolving conflicts, I squashed all of the existing commits to a single one, applied that as a commit, and resolved the conflicts. (No point in resolving the same conflict in many subtly different ways dozens of times.)

@longwave Could you please change the target branch of the original MR back to 10.2.x? 🙏

#50 Done, but that still needs rebasing.

In fact the 11.x MR applies cleanly to 10.2.x (as it should), so we don't need the older MR any more.

I'm confused, the 11.x MR must necessarily be different compared to 10.2.x because #3221793: Move file upload validation from file.module to constraint validators only exists in 11.x? 🤔

AHHHHHHHHHHH! 🤣 Now I get it! #3221793 landed in 11.x at a time when 10.2.x did not exist yet! That explains all the confusion 😅

Back to Needs work so that @AleexGreen can address @longwave's review on the MR — one new thing has come up due to the rebase 😅

I can confirm that the code in the branch named 3280279-11 applies to both 11.x (at b20ec5185c) and 10.2.x (at ffd4be4dda)

@longwave I think I understand what you were asking me to do, but I'm still pretty new to this and don't fully understand all the terminology yet.

Highlighted in the issue summary MR 5295 is the main one, this isn't always needed but when multiple MRs are on a ticket just helps

I see on MR 5295 though there are some test failures see https://git.drupalcode.org/issue/drupal-3280279/-/pipelines/55954

Believe what @longwave meant was for backwards compatibility, if contrib or custom modules could be extending this class the new parameter would instantly break their site.

    if (!$file_validator) {
      @trigger_error('Calling ' . __METHOD__ . '() without the $file_validator argument is deprecated in drupal:10.2.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/3363700', E_USER_DEPRECATED);
      $file_validator = \Drupal::service('file.validator');
    }

I see on this class there's already an example of that converage. For this ticket though it will be 10.3.0 and required in 11. The CR should be checked to make sure it includes that the class now takes an additional parameter.

Fixed broken tests, changed the CR, and added the deprecation warning, ready for re-review :).

Believe all feedback has been addressed. Not sure about 10.2 though if the tag for releases notes should be removed.

Per https://www.drupal.org/about/core/policies/core-release-cycles/schedule, the final release happens next week. https://www.drupal.org/project/drupal/releases/10.2.0-rc1 was tagged Dec 1. The 10.2 train has left the station, so this will board the next one 😊

Updated issue tag + CR.

Hi
I just used the below code & patch and this works for me. You need to turn off /on the insert image icon in CK Editor config.

/**
function fam_common_ckeditor5_image_controller_extensions_alter(array &$extensions): void {
  $extensions[] = 'svg';
}

function fam_common_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
  // Add a custom file type to the image upload plugin. Note that 'svg+xml' below
  // should be an IANA media type Name.
  // https://www.iana.org/assignments/media-types/media-types.xhtml#image 
    $imageUploadPlugin = $plugin_definitions['ckeditor5_imageUpload']->toArray();
    $imageUploadPlugin['ckeditor5']['config']['image']['upload']['types'][] = 'svg+xml';
    $plugin_definitions['ckeditor5_imageUpload'] = new \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition($imageUploadPlugin);
}

Patch code

diff --git a/web/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php b/web/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
index 47f8e1e3d..577dbeecb 100644
--- a/web/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
+++ b/web/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
@@ -145,7 +145,7 @@ public function upload(Request $request) {
     }
 
     $validators = [
-      'file_validate_extensions' => ['gif png jpg jpeg'],
+      'file_validate_extensions' => ['gif png jpg jpeg svg'],
       'file_validate_size' => [$max_filesize],
       'file_validate_image_resolution' => [$max_dimensions],
     ];

Updated issue credits.

Changed the change record to remove references to SVG because we shouldn't be recommending that, used webp instead.

Went to commit it, but it's failing phpcs as follows.

FILE: ...5/tests/src/FunctionalJavascript/CKEditor5UploadModuleAllowedImageTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 1 | ERROR | [x] Missing declare(strict_types=1).
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Time: 55ms; Memory: 10MB

#3388985: Make CKEditor5ImageController reuse FileUploadHandler is in too, so this needs a reroll

We should also remove the last hunk (Additional changes) from the change record and update https://www.drupal.org/node/3388990 instead

Just a couple of nits.

Rebased on 11.x and resolved my own feedback. :-D

Believe @larowlan did the change record updates he mentioned #62 can you confirm? I was looking at the history of the CR.

Changes here to the MR look good and all threads addressed. So +1 RTBC from me but per new approach for Needs Review queue going to leave in review for additional eyes.

Confirmed with @larowlan on slack he did make the updates.

I'm triaging RTBC issues. I read the IS, the comments, the CR and the MR. I didn't find any unanswered questions.

Everything about this issue was easy to review. Thank you to everyone for the easy to follow comment in the issue and in the MR. The CR reads well too. :-)

I pushed a small change to the description of the test module and left a question. The question does not block this so I will leave at RTBC. I will also make a comment in #needs-review-queue-initiative about the change to the module description field.

Leaving at RTBC.

Just one minor nit on the deprecation message, very close

Hey guys, just uploading a static patch from the MR #5295 for D10.2.x.

The nit was fixed, back to RTBC

@victordcp please don't change the issue version for a backport patch.

Committed and pushed 8f95f51a84 to 11.x and e1e8d87df6 to 10.3.x. Thanks!

Published the CR.