diff --git a/app/models/group.rb b/app/models/group.rb index 4f717613675d9ea37983714a99c9112836d5a8fd..c63b02cfa1914241f1c2125c614e9bc91e6f45ab 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -1177,6 +1177,10 @@ def glql_load_on_click_feature_flag_enabled? feature_flag_enabled_for_self_or_ancestor?(:glql_load_on_click, type: :ops) end + def allow_iframes_in_markdown_feature_flag_enabled? + feature_flag_enabled_for_self_or_ancestor?(:allow_iframes_in_markdown, type: :wip) + end + # overriden in EE def supports_group_work_items? false diff --git a/app/models/project.rb b/app/models/project.rb index c5534f99395566767861d9275a29d07f35f49c6d..a09925cbcda2f4274d566dbd075414cc33b4a057 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -3510,6 +3510,10 @@ def markdown_placeholders_feature_flag_enabled? group&.markdown_placeholders_feature_flag_enabled? || Feature.enabled?(:markdown_placeholders, self, type: :gitlab_com_derisk) end + def allow_iframes_in_markdown_feature_flag_enabled? + group&.allow_iframes_in_markdown_feature_flag_enabled? || Feature.enabled?(:allow_iframes_in_markdown, self, type: :wip) + end + def enqueue_record_project_target_platforms return unless Gitlab.com? diff --git a/config/feature_flags/wip/allow_iframes_in_markdown.yml b/config/feature_flags/wip/allow_iframes_in_markdown.yml new file mode 100644 index 0000000000000000000000000000000000000000..faf41918f0424703486d71e081a0545b4d2b6128 --- /dev/null +++ b/config/feature_flags/wip/allow_iframes_in_markdown.yml @@ -0,0 +1,10 @@ +--- +name: allow_iframes_in_markdown +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/282443 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204952 +rollout_issue_url: +milestone: '18.5' +group: group::knowledge +type: wip +default_enabled: false diff --git a/lib/banzai/filter/iframe_link_filter.rb b/lib/banzai/filter/iframe_link_filter.rb new file mode 100644 index 0000000000000000000000000000000000000000..5c355c5b502d6de0a5d060f1eb39d79f46ec46c3 --- /dev/null +++ b/lib/banzai/filter/iframe_link_filter.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +# Determines if an `img` tag references media to be embedded in an `iframe`. The administrator +# needs to explicitly allow the domain and consider it trusted. The `js-render-iframe` class +# will get added to allow the frontend to convert into an `iframe` +# +# Even though the `iframe` src will have been allowed by the administrator, don't insert +# the `iframe` tag here on the backend - allow the frontend to handle it. This allows for +# the administrator to remove the domain in the future if it becomes untrusted for some reason. +# The markdown cache will not need to cleared as long as the `iframe` is added on the frontend. +module Banzai + module Filter + class IframeLinkFilter < PlayableLinkFilter + extend ::Gitlab::Utils::Override + + private + + def media_type + 'img' + end + + def safe_media_ext + # TODO: will change to use the administrator defined allow list + # Gitlab::CurrentSettings.iframe_src_allowlist + ['www.youtube.com/embed'] + end + + override :has_allowed_media? + def has_allowed_media?(element) + return unless context[:project]&.allow_iframes_in_markdown_feature_flag_enabled? || + context[:group]&.allow_iframes_in_markdown_feature_flag_enabled? + + src = element.attr('data-canonical-src').presence || element.attr('src') + + return unless src.present? + + src.start_with?('https://') && safe_media_ext.any? { |domain| src.start_with?("https://#{domain}") } + end + + def extra_element_attrs(element) + attrs = {} + + attrs[:height] = element[:height] if element[:height] + attrs[:width] = element[:width] if element[:width] + attrs[:class] = 'js-render-iframe' + + attrs + end + end + end +end diff --git a/lib/banzai/filter/playable_link_filter.rb b/lib/banzai/filter/playable_link_filter.rb index 8dc419380e6ba47db711f4a13ff4d63f0f3674a1..c094be78b1586751efd9f07e5d265a0ef275fd1c 100644 --- a/lib/banzai/filter/playable_link_filter.rb +++ b/lib/banzai/filter/playable_link_filter.rb @@ -10,7 +10,7 @@ class PlayableLinkFilter < HTML::Pipeline::Filter def call doc.xpath('descendant-or-self::img[not(ancestor::a)]').each do |el| - el.replace(media_node(doc, el)) if has_media_extension?(el) + el.replace(media_node(doc, el)) if has_allowed_media?(el) end doc @@ -30,7 +30,7 @@ def extra_element_attrs(element) {} end - def has_media_extension?(element) + def has_allowed_media?(element) src = element.attr('data-canonical-src').presence || element.attr('src') return unless src.present? diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index 9d0edf01a41910005cf954f1ed517e7d22f78f18..da16276ab9938d14f9f170b813a08bf24715ef89 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -29,6 +29,7 @@ def self.filters Filter::AttributesFilter, Filter::VideoLinkFilter, Filter::AudioLinkFilter, + Filter::IframeLinkFilter, Filter::TableOfContentsTagFilter, Filter::AutolinkFilter, Filter::SuggestionFilter, diff --git a/spec/lib/banzai/filter/audio_link_filter_spec.rb b/spec/lib/banzai/filter/audio_link_filter_spec.rb index 2307b05d63640233a584473cd0bed6dcffc1c171..1059b55d2cdabaf94da47f9d10fedfd66ffd1730 100644 --- a/spec/lib/banzai/filter/audio_link_filter_spec.rb +++ b/spec/lib/banzai/filter/audio_link_filter_spec.rb @@ -17,7 +17,7 @@ def link_to_image(path) %() end - let(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository) } shared_examples 'an audio element' do let(:image) { link_to_image(src) } diff --git a/spec/lib/banzai/filter/iframe_link_filter_spec.rb b/spec/lib/banzai/filter/iframe_link_filter_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..5ebc97370991b275410b2d8ddebfedc9fb7db200 --- /dev/null +++ b/spec/lib/banzai/filter/iframe_link_filter_spec.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Banzai::Filter::IframeLinkFilter, feature_category: :markdown do + def filter(doc, contexts = {}) + contexts.reverse_merge!({ project: project }) + + described_class.call(doc, contexts) + end + + def link_to_image(path, height = nil, width = nil) + return '' if path.nil? + + attrs = %(src="#{path}") + attrs += %( width="#{width}") if width + attrs += %( height="#{height}") if height + + %() + end + + let_it_be(:project) { create(:project, :repository) } + + shared_examples 'an iframe element' do + let(:image) { link_to_image(src, height, width) } + + it 'replaces the image tag with a media container and image tag' do + container = filter(image).children.first + + expect(container.name).to eq 'span' + expect(container['class']).to eq 'media-container img-container' + + iframe, link = container.children + + expect(iframe.name).to eq 'img' + expect(iframe['src']).to eq src + expect(iframe['height']).to eq height if height + expect(iframe['width']).to eq width if width + + expect(link.name).to eq 'a' + expect(link['href']).to eq src + expect(link['target']).to eq '_blank' + end + end + + shared_examples 'an unchanged element' do + it 'leaves the document unchanged' do + element = filter(link_to_image(src)).children.first + + expect(element.name).to eq 'img' + expect(element['src']).to eq src + end + 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 + end + + context 'when the element has height or width specified' do + let(:src) { "https://www.youtube.com/embed/foo" } + + it_behaves_like 'an iframe element' do + let(:height) { '100%' } + let(:width) { '50px' } + 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 + + context 'when the element has no src attribute' do + let(:src) { nil } + + it_behaves_like 'an unchanged element' + end + + context 'when the element src does not match a domain' do + let(:src) { 'https://path/my_image.jpg' } + + it_behaves_like 'an unchanged element' + end + + context 'when data-canonical-src is empty' do + let(:image) { %() } + + 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 + + context 'and src is an image' do + let(:src) { 'https://path/my_image.jpg' } + + it_behaves_like 'an unchanged element' + end + end + + context 'when data-canonical-src is set' do + it 'uses the correct src' do + proxy_src = 'https://assets.example.com/6d8b63' + canonical_src = 'https://www.youtube.com/embed/foo' + image = %() + container = filter(image).children.first + + expect(container['class']).to eq 'media-container img-container' + + iframe, link = container.children + + expect(iframe['src']).to eq proxy_src + expect(iframe['data-canonical-src']).to eq canonical_src + + expect(link['href']).to eq proxy_src + end + 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 + + it_behaves_like 'an unchanged element' + end + + it_behaves_like 'pipeline timing check' +end diff --git a/spec/lib/banzai/filter/video_link_filter_spec.rb b/spec/lib/banzai/filter/video_link_filter_spec.rb index 2bf0c692027f74297cf43aa6bd5004716d09b8d2..aec32218b66c4e570fe3cde75e7d02d263cdd2ec 100644 --- a/spec/lib/banzai/filter/video_link_filter_spec.rb +++ b/spec/lib/banzai/filter/video_link_filter_spec.rb @@ -21,7 +21,7 @@ def link_to_image(path, height = nil, width = nil) %() end - let(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository) } shared_examples 'a video element' do let(:image) { link_to_image(src, height, width) }