Piotr Skorupa (2d734ae6) at 19 Mar 04:44
Piotr Skorupa (e1a7e1e0) at 19 Mar 04:43
Merge branch 'push-prkpknzmzyop' into 'master'
... and 1 more commit
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:
<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>src attribute. (Huh?) after it. (????)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), 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.
Past work:
Present work: a series of 4 stacked MRs for %18.10/%18.11 to bring this closer to GA:
Check out the branch.
Enable the allow_iframes_in_markdown feature on your GDK.
Enable embedding of www.youtube.com from /admin/application_settings/general#js-iframe-settings, and ensure Enable embedded content is 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="proxy.php?url=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).
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Piotr Skorupa (a91ed7b4) at 18 Mar 21:50
Merge branch 'ngala/update-pages-version-18-Mar' into 'master'
... and 1 more commit
Piotr Skorupa (14b39aeb) at 18 Mar 21:49
Update GitLab Pages version
This is part of the Gitlab Pages update process
Changelog: other
| Before | After |
|---|---|
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Done!
@kivikakk Thank you for a very informative and exhaustive description! This made understanding the change extremely easy for me. LGTM
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:
<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>src attribute. (Huh?) after it. (????)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), 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.
Past work:
Present work: a series of 4 stacked MRs for %18.10/%18.11 to bring this closer to GA:
Check out the branch.
Enable the allow_iframes_in_markdown feature on your GDK.
Enable embedding of www.youtube.com from /admin/application_settings/general#js-iframe-settings, and ensure Enable embedded content is 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="proxy.php?url=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).
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
@ngala Thanks, LGTM
Update GitLab Pages version
This is part of the Gitlab Pages update process
Changelog: other
| Before | After |
|---|---|
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
This does some post-merge fixes to virtual registries policy specs, based on !225625.
| Before | After |
|---|---|
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Piotr Skorupa (c31719a9) at 16 Mar 22:33
Remove redundant spec group factory call
... and 7 more commits
All merged; closing
@alexbuijs Ah, sorry, I got confused by the naming. I didn't rebase in a while and these files were added in the meantime. Removed ee/app/policies/virtual_registries/packages/maven/upstream/rule_policy.rb as well.
Piotr Skorupa (df4eccbf) at 13 Mar 17:58
Remove redundant Maven::Upstream::RulePolicy
... and 2780 more commits
There's also a failing spec example for one of these registry upstream models, but it's a flaky test and not actually related.
Piotr Skorupa (9845b77a) at 13 Mar 11:30
Merge branch '587651-update-up' into 'master'
... and 1 more commit