diff --git a/app/assets/javascripts/behaviors/markdown/render_iframe.js b/app/assets/javascripts/behaviors/markdown/render_iframe.js index ef20e5e97ebccd3e5ea75ebba68dfaf153943d05..f2a1720f696ad76081cdfce41d0b7df690642239 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); diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 1cad4ebc99f6118f440996bf8e8670c87bc263fb..e0d7500efea5c3d4b46fac4b369e029351a17fc9 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -15,9 +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) - end + before_action :set_group_markdown_flags private @@ -110,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 2503c7f097b9e4fb8725db3a4cdb834cd9bd7de1..d8f7397758b9f0149197fee145ba4609ec3778f3 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 66c3b3db0c6e73811867b021fd35054cb1219bb6..1e0ac547bad5bee18887cdd1afcfc506e820c958 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -16,11 +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) - end + layout 'project' helper_method :repository, :can_collaborate_with_project?, :user_access @@ -131,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 b9660ab5800481b4123e94cfec3346d06b67bb42..1849e09d8c696e06eb405aa719d36a644a104eec 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/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb index fc6c0f5b9505ce4edd8c5e6e072d601f0b542a61..4a6e8a6f239711b7cf7a910b570dde93fe219bef 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?) diff --git a/spec/features/markdown/iframe_spec.rb b/spec/features/markdown/iframe_spec.rb index 7d3635afb4b4cd694a83e7d25799dc167a89b239..406a7ffa2c1c35a04974b634e7cd868fc565c924 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 @@ -74,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) } @@ -91,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 @@ -105,6 +113,32 @@ 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' + it_behaves_like 'an iframe renderer in a group' + 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' + it_behaves_like 'an iframe renderer in a group' + end + context 'when feature is enabled but not configured' do before do stub_feature_flags(allow_iframes_in_markdown: true) 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 0000000000000000000000000000000000000000..52c871cd8b3e9f87b0ee80413172c158f3ea4747 --- /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); + }); +}); diff --git a/spec/lib/banzai/filter/iframe_link_filter_spec.rb b/spec/lib/banzai/filter/iframe_link_filter_spec.rb index cad27ed2293306ececb67a2ccbcf972944a580e0..8a3cccecfd644b78bc63a3cd712957f3e721b1e2 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