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 · 7 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
@b1ink0
Copy link
Contributor

b1ink0 commented Dec 15, 2024

After testing YouTube and Twitter embeds inside the srcdoc attribute of an IFRAME, it was observed that two sandbox tokens—allow-scripts and allow-same-origin—are necessary for them to work properly. Without the allow-same-origin token, the embeds are unable to fetch the required JavaScript, resulting in malfunctioning embeds. However, adding both allow-scripts and allow-same-origin tokens to the sandbox can allow the embedded document to remove the sandbox attribute, rendering it as insecure as not using the sandbox attribute at all. Reference: MDN

@westonruter
Copy link
Member Author

Without the allow-same-origin token, the embeds are unable to fetch the required JavaScript, resulting in malfunctioning embeds.

I'm seeing that with this error:

Uncaught SecurityError: Failed to read the 'caches' property from 'Window': Cache storage is disabled because the context is sandboxed and lacks the 'allow-same-origin' flag.

If an embed doesn't need to access such APIs on the window, then this could still be viable, but it seems you've identified a critical problem with the approach. Apparently this is why a separate domain is involved after all.

@b1ink0
Copy link
Contributor

b1ink0 commented Dec 17, 2024

When only the allow-scripts token is provided, I observed the following issues:

  1. JavaScript Errors: Assets are being fetched, but errors occur because JavaScript attempts to access objects like window, document, or localStorage.

  2. CORS Issues: For certain embeds, assets fail to load due to CORS restrictions. For example:

  3. Content Security Policy (CSP) Errors: Some embeds trigger CSP violations. For instance:

    • Reddit Embed:

      Refused to frame 'https://embed.reddit.com/' because an ancestor violates the following Content Security Policy directive: "frame-ancestors ". Note that '' matches only URLs with network schemes ('http', 'https', 'ws', 'wss'), or URLs whose scheme matches self's scheme. The scheme 'https:' must be added explicitly.

  4. Embed Functionality: I tested all of the default embed blocks, and only the following embeds work without errors:

    • Flickr
    • Animoto
    • Pocket Casts
    • SmugMug

All other embeds encounter one or more errors, which results in partial or non-functional behavior.

@adamsilverstein
Copy link
Member

After testing YouTube and Twitter embeds inside the srcdoc attribute of an IFRAME

Have we tested the simpler approach of wrapping the full embed html in an iframe, not using the srcdoc attribute? Does this present different issues?

testing YouTube

I believe YouTube embeds are already iframes? we should probably only consider wrapping embeds that are served as HTML + JavaScript, for example TikTok embeds. We already lazy load iframes from sites like youtube when they meet our core heuristics.

@westonruter
Copy link
Member Author

Have we tested the simpler approach of wrapping the full embed html in an iframe, not using the srcdoc attribute? Does this present different issues?

Wrapping a HTML in an iframe how? If not using the srcdoc attribute, the only other option without using a src pointing to a regular URL would be to encode the embed as a data:text/html URL. But I believe this would have the same issues if not more.

@adamsilverstein
Copy link
Member

Wrapping a HTML in an iframe how?

The idea I want to try is an iframe with a URL pointing back to the site to get the embed content (maybe via a rest endpoint). I think that is what Drupal is doing (but haven't confirmed).

@adamsilverstein
Copy link
Member

Wrapping a HTML in an iframe how?

The idea I want to try is an iframe with a URL pointing back to the site to get the embed content (maybe via a rest endpoint). I think that is what Drupal is doing (but haven't confirmed).

I tried this out a bit in a branch and was able to quickly verify it breaks embeds so it isn't really a viable solution.

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

3 participants