Apply 1 suggestion(s) to 1 file(s)

Co-authored-by: Janis Altherr <jaltherr@gitlab.com>

lib/banzai/filter/base_sanitization_filter.rb

@@ -15,8 +15,6 @@ class BaseSanitizationFilter < HTML::Pipeline::SanitizationFilter
       include Gitlab::Utils::StrongMemoize
       extend Gitlab::Utils::SanitizeNodeLink
 
-      RENDER_TIMEOUT = 5.seconds
-
       UNSAFE_PROTOCOLS = %w[data javascript vbscript].freeze
 
       def call
@@ -57,9 +55,8 @@ def allowlist
           allowlist[:attributes]['img'].push('data-diagram-src')
 
           # Allow any protocol in `a` elements
-          # and then remove links with unsafe protocols
+          # and then remove links with unsafe protocols in SanitizeLinkFilter
           allowlist[:protocols].delete('a')
-          allowlist[:transformers].push(self.class.method(:sanitize_unsafe_links))
 
           # Remove `rel` attribute from `a` elements
           allowlist[:transformers].push(self.class.remove_rel)

lib/banzai/filter/external_link_filter.rb

@@ -2,10 +2,10 @@
 
 module Banzai
   module Filter
-    # HTML Filter to modify the attributes of external links
+    # HTML Filter to modify the attributes of external links.
+    # This is considered a sanitization filter.
     class ExternalLinkFilter < HTML::Pipeline::Filter
       prepend Concerns::TimeoutFilterHandler
-      prepend Concerns::PipelineTimingCheck
 
       SCHEMES      = ['http', 'https', nil].freeze
       RTLO         = "\u202E"
@@ -46,7 +46,7 @@ def render_timeout
       # partial un-sanitized results.
       # It's ok to allow any following filters to run since this is safe HTML.
       def returned_timeout_value
-        HTML::Pipeline.parse(COMPLEX_MARKDOWN_MESSAGE)
+        HTML::Pipeline.parse(Banzai::Filter::SanitizeLinkFilter::TIMEOUT_MARKDOWN_MESSAGE)
       end
 
       # if this is a link to a proxied image, then `src` is already the correct

lib/banzai/filter/gollum_tags_filter.rb

@@ -77,6 +77,7 @@ def process_tag(tag)
       def sanitized_content_tag(name, content, options = {})
         html = content_tag(name, content, options)
         node = Banzai::Filter::SanitizationFilter.new(html).call
+        node = Banzai::Filter::SanitizeLinkFilter.new(node).call
 
         node&.children&.first
       end

lib/banzai/filter/references/abstract_reference_filter.rb

@@ -10,8 +10,6 @@ class AbstractReferenceFilter < ReferenceFilter
         prepend Concerns::TimeoutFilterHandler
         prepend Concerns::PipelineTimingCheck
 
-        RENDER_TIMEOUT = 2.seconds
-
         def initialize(doc, context = nil, result = nil)
           super
 

lib/banzai/filter/sanitize_link_filter.rb

+# frozen_string_literal: true
+
+module Banzai
+  module Filter
+    # Validate links and remove unsafe protocols.
+    # This can be intensive, so it was split from BaseSanitizationFilter in order
+    # for it to have its own time period.
+    class SanitizeLinkFilter < HTML::Pipeline::Filter
+      prepend Concerns::TimeoutFilterHandler
+      include Gitlab::Utils::SanitizeNodeLink
+
+      # [href], [src], [data-src], [data-canonical-src]
+      CSS = Gitlab::Utils::SanitizeNodeLink::ATTRS_TO_SANITIZE.map { |x| "[#{x}]" }.join(', ')
+      XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze
+
+      TIMEOUT_MARKDOWN_MESSAGE =
+        <<~HTML
+          <p>Timeout while sanitizing links - rendering aborted. Please reduce the number of links if possible.</p>
+        HTML
+
+      def call
+        doc.xpath(self.class::XPATH).each do |el|
+          sanitize_unsafe_links({ node: el })
+        end
+
+        doc
+      end
+
+      private
+
+      def render_timeout
+        SANITIZATION_RENDER_TIMEOUT
+      end
+
+      # If sanitization times out, we can not return partial un-sanitized results.
+      # It's ok to allow any following filters to run since this is safe HTML.
+      def returned_timeout_value
+        HTML::Pipeline.parse(TIMEOUT_MARKDOWN_MESSAGE)
+      end
+    end
+  end
+end

lib/banzai/filter/spaced_link_filter.rb

@@ -17,8 +17,8 @@ module Filter
     # This is a small extension to the CommonMark spec.  If they start allowing
     # spaces in urls, we could then remove this filter.
     #
-    # Note: Filter::SanitizationFilter should always be run sometime after this filter
-    # to prevent XSS attacks
+    # Note: Filter::SanitizationFilter/Filter::SanitizeLinkFilter should always be run sometime
+    # after this filter to prevent XSS attacks
     #
     class SpacedLinkFilter < HTML::Pipeline::Filter
       prepend Concerns::PipelineTimingCheck

lib/banzai/pipeline/ascii_doc_pipeline.rb

@@ -6,6 +6,7 @@ class AsciiDocPipeline < BasePipeline
       def self.filters
         FilterArray[
           Filter::AsciiDocSanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::CodeLanguageFilter,
           Filter::GollumTagsFilter,
           Filter::WikiLinkGollumFilter,

lib/banzai/pipeline/broadcast_message_pipeline.rb

@@ -8,6 +8,7 @@ def self.filters
           Filter::BlockquoteFenceLegacyFilter,
           Filter::MarkdownFilter,
           Filter::BroadcastMessageSanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::EmojiFilter,
           Filter::ColorFilter,
           Filter::AutolinkFilter,

lib/banzai/pipeline/emoji_pipeline.rb

@@ -9,6 +9,7 @@ def self.filters
         @filters ||= FilterArray[
           Filter::HtmlEntityFilter,
           Filter::SanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::EmojiFilter
         ]
       end

lib/banzai/pipeline/gfm_pipeline.rb

@@ -13,9 +13,10 @@ def self.filters
         @filters ||= FilterArray[
           Filter::CodeLanguageFilter,
           Filter::PlantumlFilter,
-          # Must always be before the SanitizationFilter to prevent XSS attacks
+          # Must always be before the SanitizationFilter/SanitizeLinkFilter to prevent XSS attacks
           Filter::SpacedLinkFilter,
           Filter::SanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::EscapedCharFilter,
           Filter::KrokiFilter,
           Filter::GollumTagsFilter,

lib/banzai/pipeline/incident_management/timeline_event_pipeline.rb

@@ -12,6 +12,7 @@ def self.filters
           @filters ||= FilterArray[
             *super,
             Filter::SanitizationFilter,
+            Filter::SanitizeLinkFilter,
             *Banzai::Pipeline::GfmPipeline.reference_filters,
             Filter::EmojiFilter,
             Filter::ExternalLinkFilter,

lib/banzai/pipeline/label_pipeline.rb

@@ -6,6 +6,7 @@ class LabelPipeline < BasePipeline
       def self.filters
         @filters ||= FilterArray[
           Filter::SanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::References::LabelReferenceFilter
         ]
       end

lib/banzai/pipeline/markup_pipeline.rb

@@ -6,6 +6,7 @@ class MarkupPipeline < BasePipeline
       def self.filters
         @filters ||= FilterArray[
           Filter::SanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::CodeLanguageFilter,
           Filter::AssetProxyFilter,
           Filter::ExternalLinkFilter,

lib/banzai/pipeline/single_line_pipeline.rb

@@ -7,6 +7,7 @@ def self.filters
         @filters ||= FilterArray[
           Filter::HtmlEntityFilter,
           Filter::SanitizationFilter,
+          Filter::SanitizeLinkFilter,
           Filter::AssetProxyFilter,
           Filter::EmojiFilter,
           Filter::CustomEmojiFilter,

spec/lib/banzai/filter/broadcast_message_sanitization_filter_spec.rb

@@ -8,9 +8,6 @@
   it_behaves_like 'default allowlist'
 
   describe 'custom allowlist' do
-    it_behaves_like 'XSS prevention'
-    it_behaves_like 'sanitize link'
-
     subject { filter(exp).to_html }
 
     context 'allows `a` elements' do

spec/lib/banzai/filter/external_link_filter_spec.rb

@@ -210,10 +210,11 @@
     end
   end
 
-  it_behaves_like 'pipeline timing check'
+  it_behaves_like 'does not use pipeline timing check'
+
   it_behaves_like 'a filter timeout' do
     let(:text) { 'text' }
-    let(:expected_result) { described_class::COMPLEX_MARKDOWN_MESSAGE }
+    let(:expected_result) { Banzai::Filter::SanitizeLinkFilter::TIMEOUT_MARKDOWN_MESSAGE }
     let(:expected_timeout) { described_class::SANITIZATION_RENDER_TIMEOUT }
   end
 end

spec/lib/banzai/filter/math_filter_spec.rb

@@ -314,6 +314,7 @@ def legacy_pipeline_filter(text, context = {})
       doc = Banzai::Pipeline::PlainMarkdownPipeline.call(doc[:output], context)
       doc = Banzai::Filter::CodeLanguageFilter.call(doc[:output], context, nil)
       doc = Banzai::Filter::SanitizationFilter.call(doc, context, nil)
+      doc = Banzai::Filter::SanitizeLinkFilter.call(doc, context, nil)
 
       filter(doc, context)
     end

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -8,9 +8,6 @@
   it_behaves_like 'default allowlist'
 
   describe 'custom allowlist' do
-    it_behaves_like 'XSS prevention'
-    it_behaves_like 'sanitize link'
-
     it 'customizes the allowlist only once' do
       instance = described_class.new('Foo')
       control_count = instance.allowlist[:transformers].size
@@ -224,6 +221,8 @@
     end
   end
 
+  it_behaves_like 'does not use pipeline timing check'
+
   it_behaves_like 'a filter timeout' do
     let(:text) { 'text' }
     let(:expected_result) { described_class::COMPLEX_MARKDOWN_MESSAGE }

spec/lib/banzai/filter/sanitize_link_filter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Banzai::Filter::SanitizeLinkFilter, feature_category: :team_planning do
+  include FilterSpecHelper
+
+  it_behaves_like 'XSS prevention'
+  it_behaves_like 'sanitize link'
+  it_behaves_like 'does not use pipeline timing check'
+
+  it_behaves_like 'a filter timeout' do
+    let(:text) { 'text' }
+    let(:expected_result) { described_class::TIMEOUT_MARKDOWN_MESSAGE }
+    let(:expected_timeout) { described_class::SANITIZATION_RENDER_TIMEOUT }
+  end
+end

spec/lib/banzai/pipeline/broadcast_message_pipeline_spec.rb

@@ -12,6 +12,8 @@
 
   subject { described_class.to_html(exp, project: project) }
 
+  it_behaves_like 'sanitize pipeline'
+
   context "allows `a` elements" do
     let(:exp) { "<a>Link</a>" }