Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#49173 new defect (bug)

Allow sanitized inline styles on oEmbed iframes

Reported by: westonruter's profile westonruter Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch
Focuses: Cc:

Description

Embeds from the New York Times are currently broken because the oEmbed <iframe> sets its height via an inline style attribute which is removed by Kses in wp_filter_oembed_result(). The result is an embed that appears broken due to the iframe not having sufficient height.

I believe the fix is simply to allow the style attribute on the iframe, and to allow the safecss_filter_attr() function to remove style properties that are deemed unsafe:

  • src/wp-includes/embed.php

    a b function wp_filter_oembed_result( $result, $data, $url ) { 
    879879                        'marginheight' => true,
    880880                        'scrolling'    => true,
    881881                        'title'        => true,
     882                        'style'        => true,
    882883                ),
    883884        );

The result of this change for NYTimes embeds is the inclusion of the CSS border:none;max-width:500px;min-width:300px;min-height:550px;width:100% on the iframe's style attribute.

Attachments (5)

49173.0.diff (371 bytes) - added by westonruter 4 years ago.
editor-before.png (159.9 KB) - added by westonruter 4 years ago.
Appearance of NYTimes embed in editor prior to patch
frontend-before.png (132.3 KB) - added by westonruter 4 years ago.
Appearance of NYTimes embed on frontend prior to patch
editor-after.png (335.1 KB) - added by westonruter 4 years ago.
Appearance of NYTimes embed in editor after patch
frontend-after.png (293.7 KB) - added by westonruter 4 years ago.
Appearance of NYTimes embed on frontend after patch

Download all attachments as: .zip

Change History (11)

@westonruter
4 years ago

#1 @westonruter
4 years ago

Note that when the style attribute is allowed by Kses, the value is then sanitized by safecss_filter_attr().

Note also that the filtered HTML is cached, so in order to see this change apply you'll need to delete the necessary oEmbed postmeta and transients. For example:

wp transient delete --all
post_id=1822
for key in $(wp post meta list $post_id --fields=meta_key --format=csv | grep _oembed_); do 
    wp post meta delete $post_id $key;
done

@westonruter
4 years ago

Appearance of NYTimes embed in editor prior to patch

@westonruter
4 years ago

Appearance of NYTimes embed on frontend prior to patch

@westonruter
4 years ago

Appearance of NYTimes embed in editor after patch

@westonruter
4 years ago

Appearance of NYTimes embed on frontend after patch

#2 @westonruter
4 years ago

  • Keywords has-patch added

#3 @joyously
4 years ago

Do you have any ideas as to why that wasn't there before?
Was there some problem with inline styles on some embeds?

#4 @westonruter
4 years ago

I do not know, but @swissspidy likely would have an idea.

#5 follow-up: @swissspidy
4 years ago

Is this related to https://github.com/WordPress/gutenberg/issues/13000 / the same issue?


I don't recall any specific issues with inline styles on oEmbed results. The style attribute wasn't whitelisted from the beginning, see https://github.com/swissspidy/oEmbed-API/commit/be484afdf308ec22cf3b10be868d1e5b52d2088c.

I would be careful with whitelisting inline styles as it could lead to issues with other providers. It would require some deliberate testing.

Looking at the inline style from this embedded content - border:none;max-width:500px;min-width:300px;min-height:550px;width:100% - I don't exactly see why it is absolutely necessary:

  • Borders can be disabled with frameborder=0
  • The width and height attributes on the iframe are whitelisted, allowing the provider to define the dimensions that way
  • Themes can support responsive embeds, removing the need to manually have something like width:100% for embed iframes

PS. clearing the oEmbed cache should work via wp embed cache clear too.

#6 in reply to: ↑ 5 @westonruter
4 years ago

Replying to swissspidy:

Is this related to https://github.com/WordPress/gutenberg/issues/13000 / the same issue?

Yes, I think so.

I would be careful with whitelisting inline styles as it could lead to issues with other providers.

I don't see the problem with allowing a sanitized style attribute to be passed through. The same styles are allowed by authors in post content. To me it seems oEmbed providers should be allowed to use the same styles that authors can.

Looking at the inline style from this embedded content - border:none;max-width:500px;min-width:300px;min-height:550px;width:100% - I don't exactly see why it is absolutely necessary:

  • Borders can be disabled with frameborder=0
  • The width and height attributes on the iframe are whitelisted, allowing the provider to define the dimensions that way
  • Themes can support responsive embeds, removing the need to manually have something like width:100% for embed iframes

The issue I think is that the oEmbed data is being provided generically independent of the platform. So the same HTML being sent from NY Times needs to work in both WordPress and any other consumer. I don't think WordPress can be too heavy-handed at restricting what markup is allowed, as long as the markup is safe (e.g. no scripts, restricted styles).

It would require some deliberate testing.

Testing needed for sure.

Note: See TracTickets for help on using tickets.