From 9ddbbb0e223079d0418a9b7453eaea171b1ef9a9 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Mon, 8 Dec 2025 17:13:12 +1100 Subject: [PATCH 1/7] Use data-canonical-src in preference to data-src or src --- app/assets/javascripts/behaviors/markdown/render_iframe.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/behaviors/markdown/render_iframe.js b/app/assets/javascripts/behaviors/markdown/render_iframe.js index ef20e5e97ebccd..f2a1720f696ad7 100644 --- a/app/assets/javascripts/behaviors/markdown/render_iframe.js +++ b/app/assets/javascripts/behaviors/markdown/render_iframe.js @@ -6,9 +6,9 @@ const IFRAME_SANDBOX_RESTRICTIONS = 'allow-scripts allow-popups allow-same-origi const elsProcessingMap = new WeakMap(); function renderIframeEl(el) { - // Obtain src from data-src first, in case image lazy loading hasn't - // resolved this yet. See Banzai::Filter::ImageLazyLoadFilter. - const src = el.dataset.src || el.src; + // Obtain src from data-canonical-src, then data-src, then src. + // See Banzai::Filter::AssetProxyFilter and Banzai::Filter::ImageLazyLoadFilter. + const src = el.dataset.canonicalSrc || el.dataset.src || el.src; if (!src) return; const srcUrl = new URL(src); -- GitLab From 857f412df6be60594e76aa556dde853df5bd17d0 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Mon, 8 Dec 2025 17:13:12 +1100 Subject: [PATCH 2/7] Push iframe feature flag to frontend based on project/group, not user It's enabled by project and group, as the render is cached per Markdown field -- it doesn't make sense to do this per user. --- app/controllers/groups/application_controller.rb | 1 + app/controllers/projects/application_controller.rb | 1 + lib/gitlab/gon_helper.rb | 1 - 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 1cad4ebc99f611..9fc92a96bfa4cb 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -17,6 +17,7 @@ class Groups::ApplicationController < ApplicationController before_action do push_namespace_setting(:math_rendering_limits_enabled, @group) + push_frontend_feature_flag(:allow_iframes_in_markdown, @group) end private diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 66c3b3db0c6e73..0121b0bd63e236 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -20,6 +20,7 @@ class Projects::ApplicationController < ApplicationController before_action do push_namespace_setting(:math_rendering_limits_enabled, @project&.parent) + push_frontend_feature_flag(:allow_iframes_in_markdown, @project) end helper_method :repository, :can_collaborate_with_project?, :user_access diff --git a/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb index fc6c0f5b9505ce..4a6e8a6f239711 100644 --- a/lib/gitlab/gon_helper.rb +++ b/lib/gitlab/gon_helper.rb @@ -107,7 +107,6 @@ def add_gon_feature_flags push_frontend_feature_flag(:paneled_view, current_user) push_frontend_feature_flag(:archive_group) push_frontend_feature_flag(:accessible_loading_button, current_user) - push_frontend_feature_flag(:allow_iframes_in_markdown, current_user) # Expose the Project Studio user preference as if it were a feature flag push_force_frontend_feature_flag(:project_studio_enabled, Users::ProjectStudio.new(current_user).enabled?) -- GitLab From a88a836340eff5548f48927b8bf3d5f99b0350b3 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Mon, 8 Dec 2025 17:19:55 +1100 Subject: [PATCH 3/7] Add frontend spec for iframe rendering, cover asset proxy settings --- .../behaviors/markdown/render_iframe_spec.js | 127 ++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 spec/frontend/behaviors/markdown/render_iframe_spec.js diff --git a/spec/frontend/behaviors/markdown/render_iframe_spec.js b/spec/frontend/behaviors/markdown/render_iframe_spec.js new file mode 100644 index 00000000000000..52c871cd8b3e9f --- /dev/null +++ b/spec/frontend/behaviors/markdown/render_iframe_spec.js @@ -0,0 +1,127 @@ +import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; +import renderIframes from '~/behaviors/markdown/render_iframe'; + +describe('Embedded iframe renderer', () => { + const findEmbeddedIframes = (src = null) => { + const iframes = document.querySelectorAll('iframe'); + if (src === null) return iframes; + + return Array.from(iframes).filter((iframe) => iframe.src === src); + }; + + const renderAllIframes = () => { + renderIframes([...document.querySelectorAll('.js-render-iframe')]); + jest.runAllTimers(); + }; + + beforeEach(() => { + window.gon = { + iframe_rendering_enabled: true, + iframe_rendering_allowlist: ['www.youtube.com'], + features: { + allowIframesInMarkdown: true, + }, + }; + }); + + afterEach(() => { + resetHTMLFixture(); + }); + + const target = 'https://www.youtube.com/embed/FIWD2qvNQHM'; + + // The fixtures are exactly what we get handed by the backend and need to transform. + const fixtureWithoutAssetProxy = ` +

+ + + YouTube embed + + + + + +

+ `; + + const fixtureWithAssetProxy = ` +

+ + + YouTube iframe + + + + + +

+ `; + + it('renders an embedded iframe when the asset proxy is disabled', () => { + setHTMLFixture(fixtureWithoutAssetProxy); + + expect(findEmbeddedIframes()).toHaveLength(0); + + renderAllIframes(); + + expect(findEmbeddedIframes(target)).toHaveLength(1); + }); + + it('renders an embedded iframe when the asset proxy is enabled', () => { + setHTMLFixture(fixtureWithAssetProxy); + + expect(findEmbeddedIframes()).toHaveLength(0); + + renderAllIframes(); + + expect(findEmbeddedIframes(target)).toHaveLength(1); + }); + + it('does not render an embedded iframe when the allowlist has no match', () => { + setHTMLFixture(fixtureWithAssetProxy); + + // No YouTube. + window.gon.iframe_rendering_allowlist = ['embed.figma.com']; + + renderAllIframes(); + + expect(findEmbeddedIframes()).toHaveLength(0); + }); + + it('does not render an embedded iframe when the feature flag is not enabled for the project or group', () => { + setHTMLFixture(fixtureWithAssetProxy); + + window.gon.features.allowIframesInMarkdown = false; + + renderAllIframes(); + + expect(findEmbeddedIframes()).toHaveLength(0); + }); + + it('does not render an embedded iframe when the instance-wide setting is disabled', () => { + setHTMLFixture(fixtureWithAssetProxy); + + window.gon.iframe_rendering_enabled = false; + + renderAllIframes(); + + expect(findEmbeddedIframes()).toHaveLength(0); + }); +}); -- GitLab From a6233ae27f6396e088c261eccae7fd3137b25ff6 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Tue, 9 Dec 2025 13:19:57 +1100 Subject: [PATCH 4/7] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Jay --- app/controllers/groups/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 9fc92a96bfa4cb..3ad670146da456 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -17,7 +17,7 @@ class Groups::ApplicationController < ApplicationController before_action do push_namespace_setting(:math_rendering_limits_enabled, @group) - push_frontend_feature_flag(:allow_iframes_in_markdown, @group) + push_force_frontend_feature_flag(:allow_iframes_in_markdown, @group.allow_iframes_in_markdown_feature_flag_enabled?) end private -- GitLab From 6c2b4aecd3173bdd8361cf8076eee001d56147e2 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Tue, 9 Dec 2025 13:21:07 +1100 Subject: [PATCH 5/7] Apply similar change to projects controller Co-authored-by: Jay --- app/controllers/projects/application_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 0121b0bd63e236..1ec3996948fed5 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -20,7 +20,8 @@ class Projects::ApplicationController < ApplicationController before_action do push_namespace_setting(:math_rendering_limits_enabled, @project&.parent) - push_frontend_feature_flag(:allow_iframes_in_markdown, @project) + push_force_frontend_feature_flag(:allow_iframes_in_markdown, + @project.allow_iframes_in_markdown_feature_flag_enabled?) end helper_method :repository, :can_collaborate_with_project?, :user_access -- GitLab From c1dd8e52574a743455a61e105368bc81d14c4a0b Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Tue, 9 Dec 2025 15:13:45 +1100 Subject: [PATCH 6/7] Ensure we behave correctly with actual feature flag settings --- spec/features/markdown/iframe_spec.rb | 31 ++++++++++-- .../banzai/filter/iframe_link_filter_spec.rb | 48 +++++++++++++++---- 2 files changed, 66 insertions(+), 13 deletions(-) diff --git a/spec/features/markdown/iframe_spec.rb b/spec/features/markdown/iframe_spec.rb index 7d3635afb4b4cd..bf23cdad2a57a7 100644 --- a/spec/features/markdown/iframe_spec.rb +++ b/spec/features/markdown/iframe_spec.rb @@ -3,7 +3,9 @@ require 'spec_helper' RSpec.describe 'iframe rendering', :js, feature_category: :markdown do - let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:group) { create(:group) } + let_it_be(:subgroup) { create(:group, parent: group) } + let_it_be(:project) { create(:project, :public, :repository, group: subgroup) } # No registration of .example domains is possible: # https://en.wikipedia.org/wiki/.example @@ -22,9 +24,8 @@ img[src="https://iframe.example/some-video"]' end - context 'when feature is enabled and configured' do + shared_examples_for 'an iframe renderer' do before do - stub_feature_flags(allow_iframes_in_markdown: true) stub_application_setting(iframe_rendering_enabled: true, iframe_rendering_allowlist: ['iframe.example']) end @@ -105,6 +106,30 @@ end end + context 'when feature is configured and enabled for the project' do + before do + stub_feature_flags(allow_iframes_in_markdown: project) + end + + it_behaves_like 'an iframe renderer' + end + + context 'when feature is configured and enabled for the immediate group' do + before do + stub_feature_flags(allow_iframes_in_markdown: subgroup) + end + + it_behaves_like 'an iframe renderer' + end + + context 'when feature is configured and enabled for an ancestor group' do + before do + stub_feature_flags(allow_iframes_in_markdown: group) + end + + it_behaves_like 'an iframe renderer' + end + context 'when feature is enabled but not configured' do before do stub_feature_flags(allow_iframes_in_markdown: true) diff --git a/spec/lib/banzai/filter/iframe_link_filter_spec.rb b/spec/lib/banzai/filter/iframe_link_filter_spec.rb index cad27ed2293306..8a3cccecfd644b 100644 --- a/spec/lib/banzai/filter/iframe_link_filter_spec.rb +++ b/spec/lib/banzai/filter/iframe_link_filter_spec.rb @@ -19,7 +19,12 @@ def link_to_image(path, height = nil, width = nil) img.to_html end - let_it_be(:project) { create(:project, :repository) } + let_it_be(:group) { create(:group) } + let_it_be(:subgroup) { create(:group, parent: group) } + let_it_be(:project) { create(:project, :repository, group: subgroup) } + + let(:width) { nil } + let(:height) { nil } before do allow(Gitlab::CurrentSettings).to receive_messages( @@ -59,9 +64,6 @@ def link_to_image(path, height = nil, width = nil) end context 'when the element src has a supported iframe domain' do - let(:height) { nil } - let(:width) { nil } - it_behaves_like 'an iframe element' do let(:src) { "https://www.youtube.com/embed/foo" } end @@ -76,13 +78,11 @@ def link_to_image(path, height = nil, width = nil) end it_behaves_like 'an iframe element' do - let(:height) { nil } let(:width) { '50px' } end it_behaves_like 'an iframe element' do let(:height) { '50px' } - let(:width) { nil } end end @@ -103,8 +103,6 @@ def link_to_image(path, height = nil, width = nil) context 'and src is for an iframe' do let(:src) { "https://www.youtube.com/embed/foo" } - let(:height) { nil } - let(:width) { nil } it_behaves_like 'an iframe element' end @@ -135,15 +133,45 @@ def link_to_image(path, height = nil, width = nil) end context 'when allow_iframes_in_markdown is disabled' do - let(:src) { 'https://www.youtube.com/embed/foo' } - before do stub_feature_flags(allow_iframes_in_markdown: false) end + let(:src) { 'https://www.youtube.com/embed/foo' } + it_behaves_like 'an unchanged element' end + context 'when allow_iframes_in_markdown is set for the project' do + before do + stub_feature_flags(allow_iframes_in_markdown: project) + end + + let(:src) { 'https://www.youtube.com/embed/foo' } + + it_behaves_like 'an iframe element' + end + + context 'when allow_iframes_in_markdown is set for the immediate group' do + before do + stub_feature_flags(allow_iframes_in_markdown: subgroup) + end + + let(:src) { 'https://www.youtube.com/embed/foo' } + + it_behaves_like 'an iframe element' + end + + context 'when allow_iframes_in_markdown is set for an ancestor group' do + before do + stub_feature_flags(allow_iframes_in_markdown: group) + end + + let(:src) { 'https://www.youtube.com/embed/foo' } + + it_behaves_like 'an iframe element' + end + it_behaves_like 'pipeline timing check' do let(:context) { { project: } } end -- GitLab From ea437849ef883dc666f729c3a2e5a9b236743ef7 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Tue, 9 Dec 2025 15:34:23 +1100 Subject: [PATCH 7/7] Ensure we only push feature flags after project/group is known --- app/controllers/groups/application_controller.rb | 11 +++++++---- app/controllers/groups_controller.rb | 2 ++ app/controllers/projects/application_controller.rb | 14 ++++++++------ app/controllers/projects_controller.rb | 2 ++ spec/features/markdown/iframe_spec.rb | 13 +++++++++++-- 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 3ad670146da456..e0d7500efea5c3 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -15,10 +15,7 @@ class Groups::ApplicationController < ApplicationController before_action :set_sorting requires_cross_project_access - before_action do - push_namespace_setting(:math_rendering_limits_enabled, @group) - push_force_frontend_feature_flag(:allow_iframes_in_markdown, @group.allow_iframes_in_markdown_feature_flag_enabled?) - end + before_action :set_group_markdown_flags private @@ -111,6 +108,12 @@ def enforce_step_up_auth_for_namespace # to avoid triggering find_routable! when the :group before_action was skipped enforce_step_up_auth_for(@group) end + + def set_group_markdown_flags + push_namespace_setting(:math_rendering_limits_enabled, @group) + push_force_frontend_feature_flag(:allow_iframes_in_markdown, + @group&.allow_iframes_in_markdown_feature_flag_enabled? == true) + end end Groups::ApplicationController.prepend_mod_with('Groups::ApplicationController') diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 2503c7f097b9e4..d8f7397758b9f0 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -38,6 +38,8 @@ class GroupsController < Groups::ApplicationController # admin intervention for every affected group. Non-admin users still require step-up auth. skip_before_action :enforce_step_up_auth_for_namespace, if: :skip_step_up_auth_for_owner_on_edit_and_update? + before_action :set_group_markdown_flags + before_action :group_projects, only: [:activity, :merge_requests] before_action :event_filter, only: [:activity] diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 1ec3996948fed5..1e0ac547bad5be 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -16,13 +16,9 @@ class Projects::ApplicationController < ApplicationController # By placing it after `before_action :project`, we guarantee the correct execution order. before_action :enforce_step_up_auth_for_namespace - layout 'project' + before_action :set_project_markdown_flags - before_action do - push_namespace_setting(:math_rendering_limits_enabled, @project&.parent) - push_force_frontend_feature_flag(:allow_iframes_in_markdown, - @project.allow_iframes_in_markdown_feature_flag_enabled?) - end + layout 'project' helper_method :repository, :can_collaborate_with_project?, :user_access @@ -133,6 +129,12 @@ def enforce_step_up_auth_for_namespace enforce_step_up_auth_for_namespace_id(params[:namespace_id]) end end + + def set_project_markdown_flags + push_namespace_setting(:math_rendering_limits_enabled, @project&.parent) + push_force_frontend_feature_flag(:allow_iframes_in_markdown, + @project&.allow_iframes_in_markdown_feature_flag_enabled? == true) + end end Projects::ApplicationController.prepend_mod_with('Projects::ApplicationController') diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b9660ab5800481..1849e09d8c696e 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -42,6 +42,8 @@ class ProjectsController < Projects::ApplicationController skip_before_action :enforce_step_up_auth_for_namespace, only: [:create] before_action :enforce_step_up_auth_for_namespace_on_create, only: [:create] + before_action :set_project_markdown_flags + # Project Export Rate Limit before_action :check_export_rate_limit!, only: [:export, :download_export, :generate_new_export] diff --git a/spec/features/markdown/iframe_spec.rb b/spec/features/markdown/iframe_spec.rb index bf23cdad2a57a7..406a7ffa2c1c35 100644 --- a/spec/features/markdown/iframe_spec.rb +++ b/spec/features/markdown/iframe_spec.rb @@ -75,6 +75,7 @@ end context 'in a project home page' do + let(:project) { create(:project, :public, group: subgroup) } let!(:wiki) { create(:project_wiki, project: project) } let!(:wiki_page) { create(:wiki_page, wiki: wiki, title: 'home', content: markdown) } @@ -92,12 +93,18 @@ end end end + end + + shared_examples_for 'an iframe renderer in a group' do + before do + stub_application_setting(iframe_rendering_enabled: true, iframe_rendering_allowlist: ['iframe.example']) + end context 'in a group milestone' do - let(:group_milestone) { create(:group_milestone, description: markdown) } + let(:group_milestone) { create(:group_milestone, group: subgroup, description: markdown) } it 'includes iframe correctly' do - visit(group_milestone_path(group_milestone.group, group_milestone)) + visit(group_milestone_path(subgroup, group_milestone)) wait_for_requests @@ -120,6 +127,7 @@ end it_behaves_like 'an iframe renderer' + it_behaves_like 'an iframe renderer in a group' end context 'when feature is configured and enabled for an ancestor group' do @@ -128,6 +136,7 @@ end it_behaves_like 'an iframe renderer' + it_behaves_like 'an iframe renderer in a group' end context 'when feature is enabled but not configured' do -- GitLab