CKEditor 4.21 blocks iFramed players

Nevermind, this only happens when the iFrame has an empty sandbox="" attribute, which isn't happening with this module.

After some testing on a clean D7 site with only ckeditor 7.x-1.23 installed, it turns out that by default, filling in only the required URL field, an iFrame has the following HTML attributes:

<iframe frameborder="0" sandbox="" scrolling="no" src="https://www.youtube.com/embed/3MrdU6XbJok"></iframe></p>

The empty sandbox="" is the culprit here.

Attached patch worked for me, YMMV.

Patch which allows adding multiple domains on the admin form at /admin/config/content/ckeditor/editg

@Spokje the patch solves the issue.

I have one concern though, that this change applies do the Global settings. IMHO it should be set per profile instead.

IMHO it should be set per profile instead.

I thought about that, but I see having to fill out the allowed-domains-in-an-iFrame separately for each profile as a (big) disadvantage.
Happy to be convinced otherwise BTW.

The current patch needed to be put back to NW anyway.

We need to identify all domains in all node body-fields that are in an <iframe>-tag having a sandbox="".
Then we need to/let the user fill those out in the new allowed-domains-in-an-iFrame field.

After which we to fetch-n-save every node containing a body-field with an <iframe>-tag having a sandbox="", otherwise this patch will only affect iFrames created after applying the patch, leaving the ones that are already stuck on not showing/working.

Retitling into something more general.

Took a stab at the update hook mentioned in #12.

Still needs a decision if this should be in the General Profile or individual profiles.

Still needs a decision if this should be in the General Profile or individual profiles.

I would go with individual profiles as they usually have different purpose. So for example Basic one for comments, where you don't want to allow only Twitter but no YT videos. For Full HTML you want to allow much more, as this is for content creators only.

Other solution would be to keep the General profile as the default and overwrite them with individual profiles if they are set.

Thanks @vokiel, makes sense now.

Other solution would be to keep the General profile as the default and overwrite them with individual profiles if they are set.

Personally I like this solution.

Anyway: The patch in #14 works perfect for my use-cases. I'm sure someone else can (relatively) easy refactor this into an individual profile solution. Setting to NW for that.

I'm stepping away from this one.

Thanks for this patch.

Just a heads up for anyone else coming across this issue that iframes can also occur in fields other than field_data_body and that those occurrences need intervention beyond what the update hook here does.

To deal with other fields where iframes were embedded manually, for example, we searched our database for occurrences of "

If you don't do this, CK could potentially add empty sandbox to those iframes and break them when they are edited and saved.

Not sure there is anything this module can do about that.

With @Spokje "stepping away from this one," I wonder what are next steps?

We're in tricky territory in terms of getting this out to those who might need it.

The 7.x branch is marked as, "No longer developed by its maintainers."

We do, however, have a recent update in reaction in part to a 3rd party security update: https://www.drupal.org/project/ckeditor/issues/3349695 📌 Update CKEditor 4 to 4.21.0 Needs work

That update suggests updating to 4.21.0, which we know in this thread breaks iframes in certain situations and has potential to affect users beyond the database update provided here (other CK fields beyond body).

Thanks @darkodev.

I think we first need a maintainers (@vokiel) statement that this issue is something that needs addressing and goes beyond the current "No longer developed by its maintainers" status.

If that is indeed the case, we need the code from @darkodev to "search our database for occurrences of
<iframe and ran similar code to the .install update hook to find all domains used in iframe embeds so we could add them to the "Allowed domains to be displayed in an iFrame" setting. "

On top of that we need a decision on #15 📌 CKEditor 4.21 blocks iFramed players Needs work on general vs individual profiles.

If all that is done: Code, Check, Commit :)

I've had the same issue and ended up doing something similar to the above patches listed. My only issue is that running allow-scripts and allow-same-origin is strongly discouraged. In extreme cases, it's defeating the purpose of the original security patch by making things more open.

See, https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe, for more details.

In a related post, one of my solutions was to allow users to select sandbox attributes. This doesn't quite solve the problem (it might create more unintended issues; for example, my opening statement).

@Spokje, there is no code to find occurrences of iframe, just a general db search in phpMyAdmin :-)

If this moves forward, there could be a programmatic search of the db to harvest all domains used in iframes. This might be problematic, though, in the case that users have embedded iframes from undesirable domains. It would be best to somehow authorize the addition of the iframe domains that are discovered in a db search.

Get ready for a novel...

1. Other important flags

To further @anthonyroundtree's idea of allowing users to pick those attributes:

What if the URL being embedded isn't a video? We need to consider that users might be embedding a form (allow-forms), or a webpage that would load a popup (allow-popups), or want modals (allow-modals), etc.

Here's a web.dev article that explains some of the flags in layman terms.

By default, we should really be enabling most of the flags mentioned in the MDN article because we can't assume how a site editor will be using their content.

If someone here were to code a picker interface, I'd like to see a picker that shows all flags, including some "default/recommended options", and "lesser-used options" or options that require some level of caution. Somewhere in there it should link to one or both of those articles above for admins who want to educate themselves.

2. Alternate solution: disable sandboxing

We can just turn off sandboxing entirely for whitelisted domains.

I understand this may be an unpopular opinion, but here are my reasons:

1. Until a CVE is announced for CKEditor 4 stating that all versions prior to the sandboxing behavior (released in v4.21.0) are insecure, we should treat the sandboxing as a "feature", and we should do our best to be as backwards-compatible as possible, which means either disabling the sandboxing feature, or enabling all (or nearly all) flags.
2. For the CKEditor team (thank you for your service) to add the sandboxing feature enabled by default, especially as a minor version, it feels like it's forcing our hand and causing a downstream regression/bug, and it's making us choose an acceptable security policy that we (the people in this issue and maintainers of this module) deem appropriately secure enough, but also wide enough not to break the content behaviors of an unknown number of websites out there.
3. We can't predict how a site admin wants to restrict the content, or that they even understand the security implications, or the bugs that can be introduced by too-restrictive policies
4. Making a change to sandboxing here will only affect content that is edited going forward. (See my discussion below: 4. Beware of mass-updates to content)
5. It has already been mentioned that this module is unsupported. So adding new complexity will make it harder to get a patch in.
6. Unless we chose a very permissive set of flags (i.e. nearly all flags) we should expect someone here to put even more energy into coding a picker-interface for all sandbox flags, which would make it harder to get a patch in.

Disabling of sandboxing can be accomplished in a patch.
Instead of:
return {sandbox: "allow-scripts allow-same-origin"};
Do this:
return { };

However, there's a minor bug with that, which is that if you first embed an iframe with the URL "bad-example.domain/watch/123", then edit the source code and switch the URL to "good-example.domain...", the "sandbox" attribute stays there and doesn't get removed. I don't think this is a bug in the patch, but rather in how CKEditor handles the cleanup of iframe attributes. The workaround is to remove the iframe and re-embed it with the correct URL.

3. Case-sensitive problem in the patch

The line that checks the src attribute:
if (iframe.attributes.src.indexOf(Drupal.settings.ckeditor.allowed_domains_in_iframe[i]) !== -1) {
That will fail if there is a case-mismatch between the source and whitelisted values.
Both "src" and the whitelist value need to be compared using the same case.

4. Error with YouTube videos without the "presentation" flag

As far as just videos, in addition to the two attribute flags "allow-scripts" and "allow-same-origin", we should consider adding a third:
allow-presentation: Allows embedders to have control over whether an iframe can start a presentation session.

I'm on Chrome 116.0.5845.111 on Windows 10, and loading a page with a YouTube video without the presentation flag throws this error:

cast_sender.js:84 Uncaught DOMException: Failed to construct 'PresentationRequest': The document is sandboxed and lacks the 'allow-presentation' flag.
at V.initialize (https://www.gstatic.com/eureka/clank/116/cast_sender.js:84:87)
at chrome.cast.initialize (https://www.gstatic.com/eureka/clank/116/cast_sender.js:98:162)
at w9.init (https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...)
at Owb (https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...)
at Ewb (https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...)
at https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...
at chrome.cast.ea (https://www.gstatic.com/eureka/clank/116/cast_sender.js:100:385)

4. Beware of mass-updates to content

The behavior introduced by the new version of CKEditor only affects sites that have been upgraded to CKE 4.21.0, and then only for pages with an IFrame that have been edited since that upgrade. To that end, a hook_update_N() has been proposed in the patch.

I get the intention behind making all content on the website secure, but is it really this module's job to alter potentially thousands (or millions) of pieces of content? The update could potentially take a really, really long time for so many pieces of content.

What if I have node types containing a body field with a certain input format that is set to not display a wysiwyg editor? Those nodes will be erroneously altered by a mass-update.

Also, what if a big site like YouTube or Twitter/X implements a new flag that results in console errors on all your old content? Then the flags that we changed during the update will need to be changed again and the update rerun.

If a person wants to make all past content on their site secure, there are other modules that can do a find/replace within the content (such as Search and Replace Scanner → ). Or better yet, don't even touch the database content, and instead use a Text Filter module to parse all IFrames and output specific sandboxed attributes (or no attributes) depending on the source URL/whitelisting. (I recommend building a Text Filter using code like in @Spokje's hook_update_N() that iterates over DOM elements using DOMDocument(). I already have code that does something like this if anyone wants it).