From 0b6ba6fbb754eb8e856844b8f3cc11b8b282e9b4 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Fri, 12 Sep 2025 18:17:22 -0500 Subject: [PATCH 1/2] Add Banzai filter for deteting potential iframes --- app/models/group.rb | 4 + app/models/project.rb | 4 + .../wip/allow_iframes_in_markdown.yml | 10 ++ lib/banzai/filter/iframe_link_filter.rb | 51 +++++++ lib/banzai/filter/playable_link_filter.rb | 4 +- lib/banzai/pipeline/gfm_pipeline.rb | 1 + .../banzai/filter/audio_link_filter_spec.rb | 2 +- .../banzai/filter/iframe_link_filter_spec.rb | 142 ++++++++++++++++++ .../banzai/filter/video_link_filter_spec.rb | 2 +- 9 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 config/feature_flags/wip/allow_iframes_in_markdown.yml create mode 100644 lib/banzai/filter/iframe_link_filter.rb create mode 100644 spec/lib/banzai/filter/iframe_link_filter_spec.rb diff --git a/app/models/group.rb b/app/models/group.rb index 4f717613675d9e..c63b02cfa19142 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 c5534f99395566..a09925cbcda2f4 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 00000000000000..faf41918f04247 --- /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 00000000000000..5c355c5b502d6d --- /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 8dc419380e6ba4..c094be78b15867 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 9d0edf01a41910..da16276ab9938d 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 2307b05d636402..1059b55d2cdaba 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 00000000000000..27f1f57e0aaca5 --- /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 meadia 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 2bf0c692027f74..aec32218b66c4e 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) } -- GitLab From 492f521432ddcba6b3f49051e6b29c859f499d14 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Sun, 28 Sep 2025 00:00:25 -0500 Subject: [PATCH 2/2] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Piotr Skorupa --- spec/lib/banzai/filter/iframe_link_filter_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/banzai/filter/iframe_link_filter_spec.rb b/spec/lib/banzai/filter/iframe_link_filter_spec.rb index 27f1f57e0aaca5..5ebc97370991b2 100644 --- a/spec/lib/banzai/filter/iframe_link_filter_spec.rb +++ b/spec/lib/banzai/filter/iframe_link_filter_spec.rb @@ -24,7 +24,7 @@ def link_to_image(path, height = nil, width = nil) shared_examples 'an iframe element' do let(:image) { link_to_image(src, height, width) } - it 'replaces the image tag with a meadia container and image tag' do + it 'replaces the image tag with a media container and image tag' do container = filter(image).children.first expect(container.name).to eq 'span' -- GitLab