WYSIWYG Editor Field: Image element with width and height attributes triggers unsafe HTML error message

shanomurphy commented 6 months ago

Describe the bug

If an image element is added to a WYSIWYG Editor field it triggers the unsafe HTML error message. This only occurs when:

the <img> contains both a width and height attribute (default when using the 'Add media' button)
the template file calls get_header()

Removing either the width or height attribute (or both) fixes the issue.

To Reproduce

Steps to reproduce the behaviour:

Activate the Twenty Twenty-Four theme
Create a page called 'Test'
Use the ACF import tool and import the field group JSON file below
Edit the 'Test' page and add an image to the 'Test Field' (using 'Add media' button or simply paste <img src="https://picsum.photos/600/400" width="600" height="400" /> in text tab)
Create a template file called 'page-test.php' in the Twenty Twenty-Four theme directory (see template file code below)
View the page
Go to the WordPress Admin area and note that the unsafe HTML error message now shows

Expected behaviour

Image elements with width and height attributes should not trigger the warning.

Template File page-test.php Code

<?php get_header(); ?>

<?php the_field('test'); ?>

<?php get_footer(); ?>

ACF Group Code

acf-export-2024-05-01.json

Version Information:

WordPress Version 6.5.2
PHP Version 8.1.23
ACF Version ACF PRO 6.2.9
Browser Chrome 124

Additional context

If get_header() is not called the error doesn't appear. I was able to reproduce this error in a fresh WordPress install with the Twenty Twenty-Four theme and no other plugins apart from ACF PRO activated.

lgladdy commented 6 months ago

Hey @shanomurphy,

Thanks for taking the time to write this up.

The potentially unsafe HTML warning is powered by the WordPress kses system. Unfortunately, as it parses out the result of TinyMCE it often tides up the markup leading us to see a modification. We noted this in the release notes for ACF 6.2.5 where the warning was introduced.

The upcoming ACF 6.3 release will give you a way to turn off the warning permanently, and there is also a filter you can use now to disable if it you'd like in the same release notes linked above.

Thanks! Liam

shanomurphy commented 6 months ago

Hi @lgladdy,

Thanks for the quick reply.

Does that mean that every WYSIWYG field containing an image is going to trigger the warning? Note it happens even with an image simply added using the 'Add media' button.

I understand the warning can be disabled but doesn't that defeat the purpose of the alert? I feel like an image element shouldn't be triggering it.

Why does removing either the width or height attribute fix the issue? for example this will trigger the warning:

<img src="https://picsum.photos/600/400" width="600" height="400" />

However this will not:

<img src="https://picsum.photos/600/400" width="600" />

In both cases WordPress adds a decoding attribute so the markup is being modified in both cases.

lgladdy commented 6 months ago

Hey @shanomurphy,

Yup, there are many things in a WYSIWYG field which kses will change or encode which we'll see as a change. We have no control of what kses does, all we do a literally string comparison of that the result of get_field was before and after.

Unfortunately, we're super limited on what we could do here and log by default for performance reasons.

Because we have to do this check on every field output, if we did complex parsing to detect if it was just a "&" or a quote changing, or even logging of the full value being changed, we'd quickly crash sites on budget hosting.

We tried our best to find a balance between making sure users were aware, and gave them tools to find out when they were ready to debug, without massively increasing the CPU load of ACF running on millions of sites.

We've released plugins and all kinds of hooks and filters along the way (all detailed in the release notes) to let devs see the output that's changed when they're ready to debug the fields, and the message states it may not be a breaking change, but something about the output is changing - so it's important we flag it by default.

shanomurphy commented 6 months ago

Thanks, that makes perfect sense. I hadn't considered the performance implications. I did wonder why the field post ID wasn't logged/reported by default. Thanks for explaining.

AdvancedCustomFields / acf