diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 6e2adc76ec6..a8c9e54f00c 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -15,7 +15,7 @@ module CacheMarkdownField # Increment this number every time the renderer changes its output CACHE_REDCARPET_VERSION = 3 CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 11 + CACHE_COMMONMARK_VERSION = 12 # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze diff --git a/changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml b/changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml new file mode 100644 index 00000000000..3bd8123a346 --- /dev/null +++ b/changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml @@ -0,0 +1,5 @@ +--- +title: Fix possible XSS attack in Markdown urls with spaces +merge_request: 2599 +author: +type: security diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb index a27f1d46863..c6a3a763c23 100644 --- a/lib/banzai/filter/spaced_link_filter.rb +++ b/lib/banzai/filter/spaced_link_filter.rb @@ -17,6 +17,9 @@ module Banzai # 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 + # class SpacedLinkFilter < HTML::Pipeline::Filter include ActionView::Helpers::TagHelper diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index be75e34a673..96bea7ca935 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -12,13 +12,16 @@ module Banzai def self.filters @filters ||= FilterArray[ Filter::PlantumlFilter, + + # Must always be before the SanitizationFilter to prevent XSS attacks + Filter::SpacedLinkFilter, + Filter::SanitizationFilter, Filter::SyntaxHighlightFilter, Filter::MathFilter, Filter::ColorFilter, Filter::MermaidFilter, - Filter::SpacedLinkFilter, Filter::VideoLinkFilter, Filter::ImageLazyLoadFilter, Filter::ImageLinkFilter, diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb index df24cef0b8b..91b0499375d 100644 --- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb @@ -104,5 +104,17 @@ describe Banzai::Pipeline::GfmPipeline do expect(output).to include("src=\"test%20image.png\"") end + + it 'sanitizes the fixed link' do + markdown_xss = "[xss](javascript: alert%28document.domain%29)" + output = described_class.to_html(markdown_xss, project: project) + + expect(output).not_to include("javascript") + + markdown_xss = "\n[xss](javascript:alert%28document.domain%29)" + output = described_class.to_html(markdown_xss, project: project) + + expect(output).not_to include("javascript") + end end end