Loading ImageResize before ImageStyle might lead to crash #8270

ansorensen · 2020-10-15T22:33:14Z

I'm dealing with a computer generated HTML source that sometimes leads to poorly formed HTML. In some cases, CKEditor will ignore items that have this issues. However, the image and image-resize packages seem brittle and crash the whole editor with they encounter a problem.

📝 Provide a description of the improvement

As a short term, I'd could use some tips on how to create a plugin that can screen out tags with no source, or <figure class="image image_resized" style="width:331px;"></figure>. Create an elementToElement conversion with an upcast helper that returns null for images with no sources works fine. But then I need to handle the case of figures that got resized and no longer have an image. More details in what I have tried before.

In the long term, I suggest we check for image sources before using them (or throw an error that can be displayed in the console without crashing the entire editor). Same with checking that children of a figure element exist before performing operations on them.

How the feature works now and what you'd like to change?

I have these two test cases:

`

The first case, , I can deal with just fine with this plugin code:

editor.conversion.for( 'upcast' ).elementToElement( { view: { name: 'img', attribute: { src: false } }, model: ( viewImage, modelWriter ) => null, priority: 'high' } );

However, when I try to convert

elements with no image children, I get an error from the image-resize package (goes away when I comment out that package in the plugin list). Here's the error:

// removes figures without children editor.conversion.for( 'upcast' ).elementToElement( { view: (element) => { if(element.name === 'figure' && element.childCount === 0) { return { name: true}; } return null; }, model: ( viewFigure, modelWriter ) => null, priority: 'highest' } );

The above code does seem to run without throwing errors, but some how it still does not actually prevent image-resizing from erroring on the childless figure. Here's the error:

Is there a way without CKEditor5 to screen out junk HTML before brittle plugins like image-resize run? Admittedly, I could perform an operation outside of CKEditor5 using some fancy regex on the string... however, I'd rather have this logic packaged with ckeditor so that it doesn't get accidentally forgotten in some editor instances.

📃 Other details

Browser: Chrome
OS: Mac
CKEditor version: 15.0.0
Installed CKEditor plugins: Image, Image Resize

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

The text was updated successfully, but these errors were encountered:

Mgsy · 2020-10-19T07:23:11Z

Hi, thanks for the detailed report. In order to start investigating it, I'd like to ask you to provide steps to reproduce, so we'll be able to see the issue locally.

ansorensen · 2020-10-19T18:13:37Z

Steps to reproduce:

Use a classic editor with the image, image styling, and image resize plugins
Create an editor instance with the HTML above that crashes the editor provided on init: <img /> or <figure class="image image_resized" style="width:331px;"></figure>
See errors also listed above.

Mgsy · 2020-10-20T07:20:05Z

Thanks for the steps! I'm able to reproduce the issue and it seems that it occurs if ImageResize plugin is placed before ImageStyle plugin in plugins array. Could you please confirm that moving ImageResize after ImageStyle solves the issue?

ClassicEditor
    .create( document.querySelector( '#editor' ), {
        plugins: [
           // ...
           ImageStyle,
           ImageResize,
           // ...
        ]
   } );

ansorensen · 2020-10-20T19:22:22Z

Thanks, the change in plugin order did fix the issue. The order had been:

Image,
ImageToolbar,
ImageResize,
ImageStyle,

which broke, but this order seems to work

Image,
ImageToolbar,
ImageStyle,
ImageResize,

Mgsy · 2020-10-21T12:30:44Z

Thanks for the confirmation! I'll leave this issue open, as it might be worth investigating.

maxbarnas · 2021-01-11T11:38:26Z

Converters for plugin ImageStyle and ImageResize have the same priority (low) which makes the order of those plugins relevant. The error we see when ImageResize is the first plugin listed, happens because ImageResize converter, attributeToAttribute, creates a modelRange in one of its helper functions: 0d785a0/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L896.

Then the condition in ImageStyle converter, that checks if modelRange is empty never stops the converter and subsequently, the schema is checked using a non-existent model element: 0d785a0/packages/ckeditor5-image/src/imagestyle/converters.js#L61.

This is another case (previously: ckeditor/ckeditor5/issues/8393#issuecomment-752125780) where the code assumes that if modelRange is not falsy, we are free to play with its items without checking their existence first. To fix that I can either add another condition or implement something more elaborate - I am open to suggestions.

niegowski · 2021-01-12T11:41:52Z

We can simply skip upcasting if there is no modelImageElement because this happens only for incomplete image HTML that won't get upcasted anyway. https://github.com/ckeditor/ckeditor5/blob/0d785a0/packages/ckeditor5-image/src/imagestyle/converters.js#L61.

Fix (image): Image plugins can be loaded in any order without causing an error. Closes #8270.

ansorensen added the type:improvement This issue reports a possible enhancement of an existing feature. label Oct 15, 2020

Mgsy added the pending:feedback This issue is blocked by necessary feedback. label Oct 19, 2020

Mgsy added type:bug This issue reports a buggy (incorrect) behavior. and removed type:improvement This issue reports a possible enhancement of an existing feature. labels Oct 20, 2020

Mgsy changed the title ~~Image and image resize crash editor with malformed HTML~~ Loading ImageResize before ImageStyle might lead to crash Oct 21, 2020

Mgsy added package:image squad:core Issue to be handled by the Core team. and removed pending:feedback This issue is blocked by necessary feedback. labels Oct 21, 2020

Reinmar added this to the nice-to-have milestone Nov 9, 2020

AnnaTomanek mentioned this issue Dec 16, 2020

📃 Iteration 39 roadmap #8693

Closed

maxbarnas self-assigned this Jan 7, 2021

maxbarnas modified the milestones: nice-to-have, iteration 39 Jan 7, 2021

maxbarnas mentioned this issue Jan 12, 2021

Stop upcast conversion early when a model element is not available #8819

Merged

niegowski closed this as completed in #8819 Jan 14, 2021

niegowski added a commit that referenced this issue Jan 14, 2021

Merge pull request #8819 from ckeditor/i/8270

1c7397d

Fix (image): Image plugins can be loaded in any order without causing an error. Closes #8270.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading ImageResize before ImageStyle might lead to crash #8270

Loading ImageResize before ImageStyle might lead to crash #8270

ansorensen commented Oct 15, 2020

Mgsy commented Oct 19, 2020

ansorensen commented Oct 19, 2020 •

edited

Loading

Mgsy commented Oct 20, 2020 •

edited

Loading

ansorensen commented Oct 20, 2020

Mgsy commented Oct 21, 2020

maxbarnas commented Jan 11, 2021 •

edited

Loading

niegowski commented Jan 12, 2021

Loading ImageResize before ImageStyle might lead to crash #8270

Loading ImageResize before ImageStyle might lead to crash #8270

Comments

ansorensen commented Oct 15, 2020

📝 Provide a description of the improvement

📃 Other details

Mgsy commented Oct 19, 2020

ansorensen commented Oct 19, 2020 • edited Loading

Mgsy commented Oct 20, 2020 • edited Loading

ansorensen commented Oct 20, 2020

Mgsy commented Oct 21, 2020

maxbarnas commented Jan 11, 2021 • edited Loading

niegowski commented Jan 12, 2021

ansorensen commented Oct 19, 2020 •

edited

Loading

Mgsy commented Oct 20, 2020 •

edited

Loading

maxbarnas commented Jan 11, 2021 •

edited

Loading