-
Notifications
You must be signed in to change notification settings - Fork 104
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
Comments
After testing YouTube and Twitter embeds inside the
|
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 |
When only the
All other embeds encounter one or more errors, which results in partial or non-functional behavior. |
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?
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. |
Wrapping a HTML in an iframe how? If not using the |
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. |
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
(orvideo
) the oEmbed markup may not just contain anIFRAME
but it may include aSCRIPT
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 theSCRIPT
tags.Adam noted how Drupal has a Security section in its Media settings screen for oEmbeds:
The oEmbed Security Considerations are:
By wrapping the oEmbed HTML in an
IFRAME
(likely wrapping anotherIFRAME
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 thesrcdoc
attribute paired with thesandbox
attribute. For example (Codepen):This results in the following being printed:
When the
allow-same-origin
token is supplied in thesandbox
attribute (or thesandbox
attribute is removed entirely), the cross-origin restriction is removed:Naturally, additional
sandbox
tokens would be required in embeds, in particularallow-top-navigation-by-user-activation
. Additionally, the markup inside thesrcdoc
would need to also include aResizeObserver
which would send messages to the host page to resize theIFRAME
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 theBLOCKQUOTE
containing the fallback Tweet content as well as the TwitterSCRIPT
that turns that into an embed. If there is a Tweet in the initial viewport which does not get lazy-loaded, then theSCRIPT
loaded for that Tweet instance will then look for all other TweetBLOCKQUOTE
instances and construct them as well. So lazy-loading TwitterSCRIPT
tags currently has no effect at present. By wrapping each Tweet embed in anIFRAME
with asrcdoc
we can dynamically populate thatsrcdoc
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 asrcdoc
IFRAME
but I imagine the pros outweigh the cons.)I'm not sure why Drupal doesn't just use
srcdoc
instead of the cross-originIFRAME
URL. Maybe it's becausesrcdoc
wasn't supported in IE11? Or maybe there's another security benefit I'm not aware of.The text was updated successfully, but these errors were encountered: