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

Cover image cannot be added without unfiltered_html capability #2539

Closed
SergeyBiryukov opened this issue Aug 25, 2017 · 16 comments
Closed

Cover image cannot be added without unfiltered_html capability #2539

SergeyBiryukov opened this issue Aug 25, 2017 · 16 comments
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked.

Comments

@SergeyBiryukov
Copy link
Member

When adding a Cover Image block to a post, the resulting <section> element is supposed to have a style attribute with a background image:

<section class="wp-block-cover-image has-background-dim" style="background-image:url(https://melchoycetestingpressable.mystagingwebsite.com/wp-content/uploads/2017/02/cropped-pexels-photo-297755.jpeg)">
<h2>This is a cover block</h2>
</section>

However, testing Gutenberg 0.9.0 as an admin user on a Multisite network, the style attribute is stripped, resulting in a section with no background image:

<section class="wp-block-cover-image has-background-dim">
<h2>This is a cover block</h2>
</section>

At a glance, it's because in Multisite regular admins don't have the unfiltered_html capability, only super admins do.

@SergeyBiryukov
Copy link
Member Author

The attribute ends up being stripped by safecss_filter_attr() via wp_kses(), both because $allowed_attr doesn't contain background-image and because the brackets around image URL trigger a preg_match() check earlier.

@karmatosed karmatosed added the [Type] Bug An existing feature does not function as intended label Sep 5, 2017
@mtias mtias added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked. labels Nov 20, 2017
@karmatosed karmatosed added this to the Merge Proposal milestone Jan 25, 2018
@designsimply
Copy link
Member

Tested and confirmed that the style attribute with background-image is removed from the Cover Image block on a multisite install. I also noticed that the block can no longer be read in the editor itself after publishing, closing, and reopening a post in the editor (even though it looks right behind the notice).

screen shot 2018-06-27 at wed jun 27 1 01 25 pm
screen shot 2018-06-27 at wed jun 27 1 01 21 pm
screen shot 2018-06-27 at wed jun 27 1 03 41 pm

Tested on bh.testjetpack.blog/multi/ (as a user with administrator rights) running WordPress 4.9.6 Multisite and Gutenberg 3.1.0 using Firefox 60.0.2 on macOS 10.13.5.

@dcbx
Copy link

dcbx commented Jun 29, 2018

(after two long days of head scratching, what I did wrong in my custom block):
i can confirm this bug and want to add, that it appears to also strip any "aria-hidden"-Attribute.
May i ask if there are any news or possible workarounds, as that's a real show stopper for my current project..

@dcbx
Copy link

dcbx commented Jun 29, 2018

and for others having this problem: my very simple workaround (not a solution!) is using this very old, but still fully functional plugin by automattic: https://wordpress.org/plugins/unfiltered-mu/
In general, i don't like using plugins for one simple purpose, but since it's really only one reather short PHP file, i consider it "cleaner" then changing code and forgetting it there.. After all, i hope this will be fixed in future, and i can remove it then.
Of course, this workaround should only be used in a fully trusted multisite-network!

@wpscholar
Copy link
Member

This is not a multisite only issue. All you need to do to replicate this issue is add define( 'DISALLOW_UNFILTERED_HTML', true ); and save a cover image with a background image assigned. The markup saved will have no inline style. This means that any normal WordPress installation where the user is not an admin would also encounter this issue.

@wpscholar
Copy link
Member

Given that this functionality is actually in WordPress core and not in Gutenberg, there is no way a pull request to this repo can be created to fix this issue. However, it appears that this ticket is likely dependent on this core trac ticket: https://core.trac.wordpress.org/ticket/37134

@earnjam
Copy link
Contributor

earnjam commented Jul 30, 2018

You could get down a deep rabbit hole here of trying to modify kses.php to support more advanced CSS through that core trac ticket, but I think the more immediate solution for Gutenberg is to only allow people with unfiltered_html capability to insert cover image blocks. We can always go back and open it up later if we find a way to safely allow more inline css.

That would be possible if #4155 lands.

@earnjam
Copy link
Contributor

earnjam commented Aug 14, 2018

Summary:

Any user without the unfiltered_html capability cannot add CSS that contains \ ( & } =. This prevents them from using declarations like background-image, which require a url() property value.

This also means any users without that capability cannot use blocks that insert CSS using those characters, such as the Cover Image block. Default roles without that capability are Author and below on single site and Administrator and below on multisite.

Potential solutions:

  1. Move the background to an <img> tag inside the section and add .wp-block-cover-image img { position: absolute; } to the block's stylesheet. Downside here is it breaks previously inserted cover image blocks, as the generated output is different.
  2. Modifications to core to loosen up the restrictions on allowed CSS. Either inside safecss_filter_attr or by adding a new esc_css function to facilitate safely escaping CSS and stripping of unsafe protocols. Additional benefit of this method is it can also allow us to solve several open issues in core: 24157, 37134, 37248

I think 2 is the better solution here, but it will need lots of review, as it opens up potential security issues.

@aduth
Copy link
Member

aduth commented Sep 7, 2018

Since it was only linked by reference, noting that there's additional context to be found in #2540 as well (reiterates/validates much of the same findings as @earnjam in #2539 (comment)).

@aduth aduth changed the title Cover image cannot be added on Multisite Cover image cannot be added without unfiltered_html capability Sep 17, 2018
@aduth
Copy link
Member

aduth commented Sep 17, 2018

Renamed the issue to reflect that, as best I can tell, this has nothing to do with multisite, and more to do with the unfiltered_html capability.

@notnownikki
Copy link
Member

Discovered this one when I was working on tests that check that kses does not break blocks when a non-admin user saves posts.

I can confirm it is to do with the unfiltered_html capability, not multisite.

Looking at the solutions proposed in #2539 (comment) , couldn't we do both? If we change the markup saved so that it works with the current state of kses, then we can support the people who are using the plugin on 4.x and we can still get the improvements to safecss_filter_attr into 5.

Does that sound reasonable?

@aduth
Copy link
Member

aduth commented Sep 17, 2018

cc @joemcgill who'd mentioned some issues with trying the idea of an img element to support parallax (fixed background).

The main downside after a quick play at this would be supporting the parallax option. With background-image this is a fairly easy thing to implement. If we use an img element in the markup, it’s much more difficult.

https://wordpress.slack.com/archives/C02QB2JS7/p1536338278000100

@mtias mtias removed this from the Merge: Core milestone Oct 3, 2018
@mtias mtias added the [Priority] High Used to indicate top priority items that need quick attention label Oct 3, 2018
@mtias mtias added this to the WordPress 5.0 milestone Oct 3, 2018
@pento
Copy link
Member

pento commented Oct 8, 2018

To fix this, we need to modify safecss_filter_attr(). Here's some rough pseudo-code that @azaozz, @dd32, and I hashed out.

  1. Remove the preg_match() near the start of the function (keep a copy, we'll use it later).
  2. Add an array of CSS attributes, like $allowed_attr, that can use url().
  3. After it the attribute is confirmed (in_array( trim( $parts[0] ), $allowed_attr )), check if it's allowed to use url().
    1. If it can't, goto step 4.
    2. If it can, check for /url\([^)]+\)/ in the value. If that doesn't exist, goto step 4.
    3. Extract the url() from the value. Strip off the url( from the start, ) from the end, and whitespace or quotes. This leaves you with the URL.
    4. Run wp_kses_bad_protocol() on the URL. If the return value is different, return an empty string.
    5. Strip the entire url() from the value, and proceed to step 4.
  4. Run the preg_match() from step 1 on the value, return an empty string if it matches.

Some notes about this solution:

  • This deliberately doesn't try to validate the URL beyond ensuring that it has a valid protocol. If it's linking off site, for example, that's no different to how the <img> tag behaves.
  • While it might be possible to loosen the restrictions a bit (for example, by trying to remove just the problematic CSS attribute), this would be significantly harder behaviour to prove safe, so would be best left for a future WordPress release.

@peterwilsoncc
Copy link
Contributor

@mtias
Copy link
Member

mtias commented Oct 23, 2018

Can we close this based on https://core.trac.wordpress.org/changeset/43781 ?

@earnjam
Copy link
Contributor

earnjam commented Oct 23, 2018

Tested with an author user (who does not have unfiltered_html capabilities) w/ Gutenberg master and WP 5.0 and it worked correctly. I think we're good here now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked.
Projects
None yet
Development

Successfully merging a pull request may close this issue.