Adding NY Times embed results in invalid security=restricted invalid attribute error

[westonruter]

Bug Description

When adding a NY Times article to an Embed block a validation error for an invalid security attribute is reported.

This may be a wider issue with WordPress post embeds (#809), but since the oEmbed response actually doesn't include the security attribute:

<iframe src="https://www.nytimes.com/svc/oembed/html/?url=https%3A%2F%2Fwww.nytimes.com%2F2017%2F02%2F14%2Fbusiness%2Fdealbook%2Fbundling-online-services.html" scrolling="no" frameborder="0" allowtransparency="true" title="Don’t Look Now, but the Great Unbundling Has Spun Into Reverse" style="border:none;max-width:500px;min-width:300px;min-height:550px;display:block;width:100%;"></iframe>

Perhaps WordPress core is overriding the embed as a special case? In the non-AMP version, it is generating:

<iframe title="Don’t Look Now, but the Great Unbundling Has Spun Into Reverse" class="wp-embedded-content" sandbox="allow-scripts" security="restricted" src="https://www.nytimes.com/svc/oembed/html/?url=https%3A%2F%2Fwww.nytimes.com%2F2017%2F02%2F14%2Fbusiness%2Fdealbook%2Fbundling-online-services.html#?secret=Rzr8mKi8LT" data-secret="Rzr8mKi8LT" scrolling="no" frameborder="0"></iframe>

This results in an entirely broken embed on the frontend, even outside of AMP:

So there are perhaps two issues here:

1. NY Times embeds are broken in core.
2. Post embeds should more gracefully be handled while waiting for Improve handling of WordPress post embeds #809 by just stripping out the security attribute (which I believe is obsolete in favor of sandbox).

Expected Behaviour

No validation error should occur.

Steps to reproduce

1. Enable Transitional or Standard mode.
2. Add an Embed block and provide the URL https://www.nytimes.com/2017/02/14/business/dealbook/bundling-online-services.html
3. Safe draft
4. See validation error

Screenshots

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

[swissspidy]

Back when I implemented this, the security attribute was only really needed for supporting IE9 IIRC.

[westonruter]

Cool. In any case, amp-iframe will would it automatically if needed based on the sandbox, so we can just strip it out entirely. So this this is very similar to #3425.

[swissspidy]

Perhaps WordPress core is overriding the embed as a special case? In the non-AMP version, it is generating:

Every unknown* iframe received via oEmbed gets this treatment by wp_filter_oembed_result(), as at that point WordPress doesn't know whether the iframe is for a WordPress site or not.

* Unknown meaning it is coming from an untrusted provider.

[westonruter]

@swissspidy If it doesn't know it is from WordPress (e.g. that the iframe doesn't have the wp-embedded-content class) why doesn't it just pass through the Kses-sanitized iframe in that case, preserving the original width/height attributes?

Comparing Embed block vs Custom HTML block containing response from NY Times:

Seems to have a much better result:

[swissspidy]

width and height attributes are preserved by core, just not the style attribute

[westonruter]

Perhaps certain styles should be allowed in core?

[archon810]

Any updates on this one? Streamable oEmbeds contain security as well, and we'd like to stop seeing the notices instead of ignoring them.

For example, paste this https://streamable.com/clya3 in WordPress and observe the following return value:

{"success":true,"data":{"body":"<iframe class=\"wp-embedded-content\" sandbox=\"allow-scripts\" security=\"restricted\" src=\"https:\/\/streamable.com\/o\/rxmlo#?secret=OkLpdXTQNw\" data-secret=\"OkLpdXTQNw\" frameborder=\"0\" scrolling=\"no\" width=\"166\" height=\"358\" data-aspectratio=\"padding-bottom: 80%\"><\/iframe>\n","attr":{"width":728,"height":1000},"head":"<script src=\"https:\/\/www.androidpolice.com\/wordpress\/wp-includes\/js\/wp-embed.min.js\"><\/script>","sandbox":true}}

[westonruter]

@archon810 Thanks for that example. You can suppress the validation error entirely using plugin code like this:

add_filter(
	'amp_validation_error_sanitized',
	function ( $sanitized, $error ) {
		if (
			isset( $error['code'], $error['node_name'] )
			&&
			'invalid_attribute' === $error['code']
			&&
			'security' === $error['node_name']
		) {
			$sanitized = true;
		}
		return $sanitized;
	},
	10,
	2
);

We'll open a PR to prevent it from being raised in the first place, similar to what was just in #3941 to fix #3939

[pierlon]

What's the next step for this issue? The security attribute has been omitted via #3954, but the matter of the style attribute being filtered is still a problem.

Should this be fixed by Core or should the plugin handle this specific case?

[westonruter]

@pierlon You're right. This needs a core fix. I've opened: https://core.trac.wordpress.org/ticket/49173

@swissspidy Thoughts on ☝️?