Treat <iframe> as <img>
What does this MR do and why?
Per #282443 ("What Could Still Be Implemented"), let's allow users to drop embed iframe code directly into Markdown source.
This isn't a controversial ask, and it was highly requested. I kept it out of the MVC to keep things simple.
Right now, if you want to embed a YouTube video, for example, you have to:
- Go to your YouTube video. (OK.)
- Click the "Share" button. (Seems fair.)
- Click "Embed". (Right.)
- Copy the embed code you're given. (OK!)
<iframe width="560" height="315" src="https://www.youtube.com/embed/DwIepRJgMF4" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen></iframe> - Paste it into a Markdown field on GitLab. (Naturally.)
- Edit it to remove everything but the URL in the
srcattribute. (Huh?) - Add
after it. (????) - Done! (
😥 )
Now, steps 1 to 4 could themselves be improved upon (and will in the next stacked MR!), but in this one we're focussing on (read: removing) steps 6 and 7.
A previous draft of the whole iframe feature actually permitted <iframe>s through in the SanitizationFilter, and then filtered them based on the allowlist. This doesn't work primarily because the allowlist might change. For this reason, we don't ever currently store an <iframe> in cached rendered HTML; we just tag would-be iframes with js-render-iframe, and then the frontend references the allowlist (again!) before transforming that into a live <iframe>.
It's also just better never to store a live <iframe> in the database column; sometimes they're exposed without being processed by the correct frontend code (recent example: !227421 (merged)), and having the natural outcome of such an exposure be "an inert <div> is put on the page" is much better than something with security ramifications.
So, this MR takes the same approach and applies it to actually pasted <iframe>s: if the iframe embedding feature is enabled in the settings, we scan the document before sanitisation and change any <iframe>s into <img>s. They become harmless <img> tags which pass the sanitisation filter.
These <img>s will then be picked up by the IframeLinkFilter the same way the equivalent embed would be (i.e. it looks identical to someone who followed steps 1 to 8 above), tagged appropriately and then rendered into an <iframe> on the frontend after passing the allowlist again.
This approach is pretty nice: we don't have to touch the sanitiser, and we don't need separate codepaths for folks using the GitLab native embed syntax ![]() and those just copy-pasting their embeds and wanting it to work.
The one oddity is that non-allowlisted iframes will end up being treated as images! But I think this is an OK and pretty normal consequence: it's the exact same outcome as writing . This is a win — the syntaxes are treated identically because they actually become the same thing early on, and so we don't need separate sanitisation or rendering codepaths on the backend or frontend. If the feature is disabled (or an <iframe> not passing the allowlist is seen at the backend stage), it's sanitised out very early on as usual.
References
Past work:
- Add Banzai filter for detecting potential `ifra... (!204952 - merged) was the preliminary work for potential iframe recognition
- Allow iframe embeds in markdown from allowed sr... (!200864 - merged) carried the initial implementation
- Push iframe feature flag to frontend based on p... (!215529 - merged) fixed two bugs that stopped it from being usable on GitLab.com; since this MR the feature is configured and usable on GitLab.com, but you need to enable the FF for your group (we have it enabled in groupknowledge)
Present work: a series of 4 stacked MRs for %18.10/%18.11 to bring this closer to GA:
- Pass iframe dimensions through if supplied (!224624 - merged)
- Avoid iframes having to deal with asset proxy/l... (!224625 - merged)
- Treat `<iframe>` as `<img>` (!224626 - merged) — You are here
- Transform `<iframe>` URLs according to a registry (!223870 - merged)
How to set up and validate locally
-
Check out the branch.
-
Enable the
allow_iframes_in_markdownfeature on your GDK. -
Enable embedding of
www.youtube.comfrom/admin/application_settings/general#js-iframe-settings, and ensureEnable embedded contentis turned on. -
It may be wise to restart the GDK Rails instance here, for best chances of success.
-
Using the plain-text editor, insert the following content into a wiki page, issue description, MR comment, etc.:
<iframe width="560" height="315" src="https://www.youtube.com/embed/DwIepRJgMF4" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen></iframe> -
It should embed correctly, at a decent size (560x315 per the tag).
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.