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

Embeds can be more secure and potentially more performant if wrapped in iframe with srcdoc #1626

Open
westonruter opened this issue Oct 24, 2024 · 0 comments
Labels
[Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@westonruter
Copy link
Member

westonruter commented Oct 24, 2024

I was just discussing with @adamsilverstein the behavior of how oEmbeds are inserted into a page. In particular, when an oEmbed has a type of rich (or video) the oEmbed markup may not just contain an IFRAME but it may include a SCRIPT tag as well, which can be seen in Twitter embeds.

The problem here is that third-party JavaScript is being (intentionally) injected into the page. If the embed provider is compromised, they could do XSS attacks on site visitors (as well as do nefarious tracking). To help mitigate this, WordPress core's wp_filter_oembed_result() function has an allowlist check: "Don't modify the HTML for trusted providers." This handles the case of untrusted oEmbed providers, but again, even trusted providers could end up being malicious.

This might also facilitate the use of Strict CSP on pages with embeds without having to indiscriminately add nonce attributes to the SCRIPT tags.

Adam noted how Drupal has a Security section in its Media settings screen for oEmbeds:

Image

The oEmbed Security Considerations are:

When a consumer displays any URLs, they will probably want to filter the URL scheme to be one of http, https or mailto, although providers are free to specify any valid URL. Without filtering, Javascript:... style URLs could be used for XSS attacks.

When a consumer displays HTML (as with video embeds), there's a vector for XSS attacks from the provider. To avoid this, it is recommended that consumers display the HTML in an iframe, hosted from another domain. This ensures that the HTML cannot access cookies from the consumer domain.

By wrapping the oEmbed HTML in an IFRAME (likely wrapping another IFRAME already in the embed markup) pointing to a cross-origin URL for a page contains that embed markup, then the cross-origin security policy will prevent any malicious markup from executing on the host page. This isn't really feasible for most WordPress sites which don't have an extra domain handy just for serving embeds (and perhaps it isn't so common for Drupal either since this doesn't seem to be enabled by default).

There is an alternative now to IFRAME sandboxing using another domain, and that is in the srcdoc attribute paired with the sandbox attribute. For example (Codepen):

<iframe sandbox="allow-scripts" srcdoc="
    &lt;script>
        try { 
            document.write('The host page URL is: ' + parent.location.href); 
        } catch ( e ) {
            document.write('Unable to access parent window: ' + e.message); }
    &lt;/script>
"></iframe>

This results in the following being printed:

Unable to access parent window: Failed to read a named property 'href' from 'Location': Blocked a frame with origin "null" from accessing a cross-origin frame.

When the allow-same-origin token is supplied in the sandbox attribute (or the sandbox attribute is removed entirely), the cross-origin restriction is removed:

The host page URL is: https://cdpn.io/cpe/boomboom/index.html?key=index.html-0b1822c3-6ddc-3529-6e3d-c04823c8716f

Naturally, additional sandbox tokens would be required in embeds, in particular allow-top-navigation-by-user-activation. Additionally, the markup inside the srcdoc would need to also include a ResizeObserver which would send messages to the host page to resize the IFRAME when the content size is changed.

Wrapping the embed markup in an IFRAME would have an additional benefit beyond security hardening in that it would improve the ability to lazy-load embeds. Every time a Tweet is embedded on a page, for example, it includes the BLOCKQUOTE containing the fallback Tweet content as well as the Twitter SCRIPT that turns that into an embed. If there is a Tweet in the initial viewport which does not get lazy-loaded, then the SCRIPT loaded for that Tweet instance will then look for all other Tweet BLOCKQUOTE instances and construct them as well. So lazy-loading Twitter SCRIPT tags currently has no effect at present. By wrapping each Tweet embed in an IFRAME with a srcdoc we can dynamically populate that srcdoc attribute with the contents of the Tweet when it enters the viewport, thus ensuring each embed is lazy-loaded correctly. (There will surely be additional overhead for wrapping embeds in a srcdoc IFRAME but I imagine the pros outweigh the cons.)

I'm not sure why Drupal doesn't just use srcdoc instead of the cross-origin IFRAME URL. Maybe it's because srcdoc wasn't supported in IE11? Or maybe there's another security benefit I'm not aware of.

@westonruter westonruter added [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Enhancement A suggestion for improvement of an existing feature labels Oct 24, 2024
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
Status: Not Started/Backlog 📆
Development

No branches or pull requests

1 participant