Merge branch '30125-markdown-security'

Remove class from SanitizationFilter whitelist

See merge request !2079

10 files changed, 68 insertions, 36 deletions

diff --git a/changelogs/unreleased/30125-markdown-security.yml b/changelogs/unreleased/30125-markdown-security.yml
new file mode 100644
index 00000000000..b766caf7d08
--- /dev/null
+++ b/changelogs/unreleased/30125-markdown-security.yml
@@ -0,0 +1,4 @@
+---
+title: Remove the class attribute from the whitelist for HTML generated from Markdown.
+merge_request:
+author:
diff --git a/lib/banzai/filter/markdown_filter.rb b/lib/banzai/filter/markdown_filter.rb
index ff580ec68f8..ee73fa91589 100644
--- a/lib/banzai/filter/markdown_filter.rb
+++ b/lib/banzai/filter/markdown_filter.rb
@@ -14,7 +14,7 @@ module Banzai
 
       def self.renderer
         @renderer ||= begin
-          renderer = Redcarpet::Render::HTML.new
+          renderer = Banzai::Renderer::HTML.new
           Redcarpet::Markdown.new(renderer, redcarpet_options)
         end
       end
diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb
index d5f9e252f62..522217deae4 100644
--- a/lib/banzai/filter/sanitization_filter.rb
+++ b/lib/banzai/filter/sanitization_filter.rb
@@ -24,10 +24,6 @@ module Banzai
         # Only push these customizations once
         return if customized?(whitelist[:transformers])
 
-        # Allow code highlighting
-        whitelist[:attributes]['pre'] = %w(class v-pre)
-        whitelist[:attributes]['span'] = %w(class)
-
         # Allow table alignment
         whitelist[:attributes]['th'] = %w(style)
         whitelist[:attributes]['td'] = %w(style)
@@ -52,9 +48,6 @@ module Banzai
         # Remove `rel` attribute from `a` elements
         whitelist[:transformers].push(self.class.remove_rel)
 
-        # Remove `class` attribute from non-highlight spans
-        whitelist[:transformers].push(self.class.clean_spans)
-
         whitelist
       end
 
@@ -84,21 +77,6 @@ module Banzai
             end
           end
         end
-
-        def clean_spans
-          lambda do |env|
-            node = env[:node]
-
-            return unless node.name == 'span'
-            return unless node.has_attribute?('class')
-
-            unless node.ancestors.any? { |n| n.name.casecmp('pre').zero? }
-              node.remove_attribute('class')
-            end
-
-            { node_whitelist: [node] }
-          end
-        end
       end
     end
   end
diff --git a/lib/banzai/filter/syntax_highlight_filter.rb b/lib/banzai/filter/syntax_highlight_filter.rb
index 9f09ca90697..7da565043d1 100644
--- a/lib/banzai/filter/syntax_highlight_filter.rb
+++ b/lib/banzai/filter/syntax_highlight_filter.rb
@@ -14,7 +14,7 @@ module Banzai
       end
 
       def highlight_node(node)
-        language = node.attr('class')
+        language = node.attr('lang')
         code = node.text
         css_classes = "code highlight"
         lexer = lexer_for(language)
diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb
index fd4a6a107c2..bd4d1aa9ff8 100644
--- a/lib/banzai/pipeline/gfm_pipeline.rb
+++ b/lib/banzai/pipeline/gfm_pipeline.rb
@@ -9,9 +9,9 @@ module Banzai
       # The GFM-to-HTML-to-GFM cycle is tested in spec/features/copy_as_gfm_spec.rb.
       def self.filters
         @filters ||= FilterArray[
-          Filter::SyntaxHighlightFilter,
           Filter::PlantumlFilter,
           Filter::SanitizationFilter,
+          Filter::SyntaxHighlightFilter,
 
           Filter::MathFilter,
           Filter::UploadLinkFilter,
diff --git a/lib/banzai/renderer/html.rb b/lib/banzai/renderer/html.rb
new file mode 100644
index 00000000000..252caa35947
--- /dev/null
+++ b/lib/banzai/renderer/html.rb
@@ -0,0 +1,13 @@
+module Banzai
+  module Renderer
+    class HTML < Redcarpet::Render::HTML
+      def block_code(code, lang)
+        lang_attr = lang ? %Q{ lang="#{lang}"} : ''
+
+        "\n<pre>" \
+          "<code#{lang_attr}>#{html_escape(code)}</code>" \
+        "</pre>"
+      end
+    end
+  end
+end
diff --git a/spec/helpers/events_helper_spec.rb b/spec/helpers/events_helper_spec.rb
index 70443d27f33..a7c3c281083 100644
--- a/spec/helpers/events_helper_spec.rb
+++ b/spec/helpers/events_helper_spec.rb
@@ -2,8 +2,10 @@ require 'spec_helper'
 
 describe EventsHelper do
   describe '#event_note' do
+    let(:user) { build(:user) }
+
     before do
-      allow(helper).to receive(:current_user).and_return(double)
+      allow(helper).to receive(:current_user).and_return(user)
     end
 
     it 'displays one line of plain text without alteration' do
@@ -60,11 +62,26 @@ describe EventsHelper do
       expect(helper.event_note(input)).to eq(expected)
     end
 
-    it 'preserves style attribute within a tag' do
-      input = '<span class="" style="background-color: #44ad8e; color: #FFFFFF;"></span>'
-      expected = '<p><span style="background-color: #44ad8e; color: #FFFFFF;"></span></p>'
+    context 'labels formatting' do
+      let(:input) { 'this should be ~label_1' }
 
-      expect(helper.event_note(input)).to eq(expected)
+      def format_event_note(project)
+        create(:label, title: 'label_1', project: project)
+
+        helper.event_note(input, { project: project })
+      end
+
+      it 'preserves style attribute for a label that can be accessed by current_user' do
+        project = create(:empty_project, :public)
+
+        expect(format_event_note(project)).to match(/span class=.*style=.*/)
+      end
+
+      it 'does not style a label that can not be accessed by current_user' do
+        project = create(:empty_project, :private)
+
+        expect(format_event_note(project)).to eq("<p>#{input}</p>")
+      end
     end
   end
 
diff --git a/spec/lib/banzai/filter/markdown_filter_spec.rb b/spec/lib/banzai/filter/markdown_filter_spec.rb
new file mode 100644
index 00000000000..897288b8ad5
--- /dev/null
+++ b/spec/lib/banzai/filter/markdown_filter_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Banzai::Filter::MarkdownFilter, lib: true do
+  include FilterSpecHelper
+
+  context 'code block' do
+    it 'adds language to lang attribute when specified' do
+      result = filter("```html\nsome code\n```")
+
+      expect(result).to start_with("\n<pre><code lang=\"html\">")
+    end
+
+    it 'does not add language to lang attribute when not specified' do
+      result = filter("```\nsome code\n```")
+
+      expect(result).to start_with("\n<pre><code>")
+    end
+  end
+end
diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb
index b4cd5f63a15..fdbc65b5e00 100644
--- a/spec/lib/banzai/filter/sanitization_filter_spec.rb
+++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb
@@ -49,11 +49,12 @@ describe Banzai::Filter::SanitizationFilter, lib: true do
       instance = described_class.new('Foo')
       3.times { instance.whitelist }
 
-      expect(instance.whitelist[:transformers].size).to eq 5
+      expect(instance.whitelist[:transformers].size).to eq 4
     end
 
-    it 'allows syntax highlighting' do
-      exp = act = %q{<pre class="code highlight white c"><code><span class="k">def</span></code></pre>}
+    it 'sanitizes `class` attribute from all elements' do
+      act = %q{<pre class="code highlight white c"><code>&lt;span class="k"&gt;def&lt;/span&gt;</code></pre>}
+      exp = %q{<pre><code>&lt;span class="k"&gt;def&lt;/span&gt;</code></pre>}
       expect(filter(act).to_html).to eq exp
     end
 
diff --git a/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb b/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb
index 63fb1bb25c4..f61fc8ceb9e 100644
--- a/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb
+++ b/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb
@@ -12,14 +12,14 @@ describe Banzai::Filter::SyntaxHighlightFilter, lib: true do
 
   context "when a valid language is specified" do
     it "highlights as that language" do
-      result = filter('<pre><code class="ruby">def fun end</code></pre>')
+      result = filter('<pre><code lang="ruby">def fun end</code></pre>')
       expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight ruby" lang="ruby" v-pre="true"><code><span id="LC1" class="line" lang="ruby"><span class="k">def</span> <span class="nf">fun</span> <span class="k">end</span></span></code></pre>')
     end
   end
 
   context "when an invalid language is specified" do
     it "highlights as plaintext" do
-      result = filter('<pre><code class="gnuplot">This is a test</code></pre>')
+      result = filter('<pre><code lang="gnuplot">This is a test</code></pre>')
       expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight plaintext" lang="plaintext" v-pre="true"><code><span id="LC1" class="line" lang="plaintext">This is a test</span></code></pre>')
     end
   end
@@ -30,7 +30,7 @@ describe Banzai::Filter::SyntaxHighlightFilter, lib: true do
     end
 
     it "highlights as plaintext" do
-      result = filter('<pre><code class="ruby">This is a test</code></pre>')
+      result = filter('<pre><code lang="ruby">This is a test</code></pre>')
       expect(result.to_html).to eq('<pre class="code highlight" lang="" v-pre="true"><code>This is a test</code></pre>')
     end
   end