Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Configuration Disallowing img[src] Attributes #2

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

schwindy
Copy link
Contributor

@schwindy schwindy commented Apr 21, 2021

Overview

I allowed all <img> element attributes in default TinyMCE configuration.

My recent change to improve Word Paste Formatting broke the Insert/Edit Image toolbar button. Attempting to insert an image into WYSIWIG content will fail because the html will be <img/>.

This is caused by not whitelisting the img[src] attribute, which is how the button inserts the image into the content.

For good measure, I added the whitelist as img[*] (all attributes) so that if some plugin were to use the data-src attribute, it would not be stripped out. Alternatively we could also compile a list of attributes we want to allow on <img> tags 🐒

@schwindy schwindy added the bug Something isn't working label Apr 21, 2021
Copy link
Member

@rreynier rreynier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -11,7 +11,7 @@
'init' => [
'allow_html_in_named_anchor' => false,
'branding' => false,
'extended_valid_elements' => 'a[href]',
'extended_valid_elements' => 'a[href],img[*]',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, want to enable everything on anchor tags, too? There are a lot of attributes like "target" and "rel" that are useful to add.

Copy link
Contributor Author

@schwindy schwindy Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I agree 100% on target/rel. The reason I did this is because when I was pasting links from Word, it kept adding this weird name="asdf1234hashxD" which for some reason rendered a weird icon on the left of all links.

Could we maybe compile a list of allowed attributes on <a> tags?

I would think these to start:

  1. href
  2. rel
  3. target

Anything else come to mind @elliottregan ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch.

How about data-*?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 👍

I confirmed locally that this works: a[data-*|href|rel|target] by manually adding all of those attributes and some others like random that I wanted to be deleted in the code view. All of the expected attributes were kept, and random was deleted 🐒

@schwindy schwindy merged commit f7ed4c2 into master Apr 21, 2021
@schwindy schwindy deleted the Configuration-FixedAllowedImgAttributes branch April 21, 2021 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants