Merge branch 'bvl-security-9-1-markup-pipeline' into 'security-9-1'

(security-9-1) Render asciidoc & other markup using banzai in a pipeline

See merge request !2098

7 files changed, 52 insertions, 32 deletions

diff --git a/changelogs/unreleased/bvl-markup-pipeline.yml b/changelogs/unreleased/bvl-markup-pipeline.yml
new file mode 100644
index 00000000000..d73bad03340
--- /dev/null
+++ b/changelogs/unreleased/bvl-markup-pipeline.yml
@@ -0,0 +1,4 @@
+---
+title: Make Asciidoc & other markup go through pipeline to prevent XSS
+merge_request:
+author:
diff --git a/lib/banzai/pipeline/markup_pipeline.rb b/lib/banzai/pipeline/markup_pipeline.rb
new file mode 100644
index 00000000000..c56d908009f
--- /dev/null
+++ b/lib/banzai/pipeline/markup_pipeline.rb
@@ -0,0 +1,13 @@
+module Banzai
+  module Pipeline
+    class MarkupPipeline < BasePipeline
+      def self.filters
+        @filters ||= FilterArray[
+          Filter::SanitizationFilter,
+          Filter::ExternalLinkFilter,
+          Filter::PlantumlFilter
+        ]
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/asciidoc.rb b/lib/gitlab/asciidoc.rb
index d575367d81a..9ebe79a640e 100644
--- a/lib/gitlab/asciidoc.rb
+++ b/lib/gitlab/asciidoc.rb
@@ -30,15 +30,14 @@ module Gitlab
       )
       asciidoc_opts[:attributes].unshift(*DEFAULT_ADOC_ATTRS)
 
+      context[:pipeline] = :markup
+
       plantuml_setup
 
       html = ::Asciidoctor.convert(input, asciidoc_opts)
-
+      html = Banzai.render(html, context)
       html = Banzai.post_process(html, context)
 
-      filter = Banzai::Filter::SanitizationFilter.new(html)
-      html = filter.call.to_s
-
       html.html_safe
     end
 
diff --git a/lib/gitlab/other_markup.rb b/lib/gitlab/other_markup.rb
index e67acf28c94..2003ee615e5 100644
--- a/lib/gitlab/other_markup.rb
+++ b/lib/gitlab/other_markup.rb
@@ -14,12 +14,11 @@ module Gitlab
     def self.render(file_name, input, context)
       html = GitHub::Markup.render(file_name, input).
         force_encoding(input.encoding)
+      context[:pipeline] = :markup
 
+      html = Banzai.render(html, context)
       html = Banzai.post_process(html, context)
 
-      filter = Banzai::Filter::SanitizationFilter.new(html)
-      html = filter.call.to_s
-
       html.html_safe
     end
   end
diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb
index 858ca61de77..0f069cc306e 100644
--- a/spec/lib/gitlab/asciidoc_spec.rb
+++ b/spec/lib/gitlab/asciidoc_spec.rb
@@ -50,7 +50,7 @@ module Gitlab
           },
           'images' => {
             input: 'image:https://localhost.com/image.png[Alt text" onerror="alert(7)]',
-            output: "<div>\n<p><span><img src=\"https://localhost.com/image.png\" alt=\"Alt text\"></span></p>\n</div>"
+            output: "<img src=\"https://localhost.com/image.png\" alt=\"Alt text\">"
           },
           'pre' => {
             input: '```mypre"><script>alert(3)</script>',
@@ -60,7 +60,7 @@ module Gitlab
 
         links.each do |name, data|
           it "does not convert dangerous #{name} into HTML" do
-            expect(render(data[:input], context)).to eql data[:output]
+            expect(render(data[:input], context)).to include(data[:output])
           end
         end
       end
@@ -69,7 +69,7 @@ module Gitlab
         it 'adds the `rel` attribute to the link' do
           output = render('link:https://google.com[Google]', context)
 
-          expect(output).to include('rel="nofollow noreferrer noopener"')
+          expect(output).to include('rel="nofollow noreferrer"')
         end
       end
     end
diff --git a/spec/lib/gitlab/other_markup.rb b/spec/lib/gitlab/other_markup.rb
deleted file mode 100644
index 8f5a353b381..00000000000
--- a/spec/lib/gitlab/other_markup.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::OtherMarkup, lib: true do
-  context "XSS Checks" do
-    links = {
-      'links' => {
-        file: 'file.rdoc',
-        input: 'XSS[JaVaScriPt:alert(1)]',
-        output: '<p><a>XSS</a></p>'
-      }
-    }
-    links.each do |name, data|
-      it "does not convert dangerous #{name} into HTML" do
-        expect(render(data[:file], data[:input], context)).to eql data[:output]
-      end
-    end
-  end
-
-  def render(*args)
-    described_class.render(*args)
-  end
-end
diff --git a/spec/lib/gitlab/other_markup_spec.rb b/spec/lib/gitlab/other_markup_spec.rb
new file mode 100644
index 00000000000..d6babd01971
--- /dev/null
+++ b/spec/lib/gitlab/other_markup_spec.rb
@@ -0,0 +1,27 @@
+require 'spec_helper'
+
+describe Gitlab::OtherMarkup, lib: true do
+  context "XSS Checks" do
+    it "does not convert dangerous #{name} into HTML" do
+      context = {}
+      filename = 'file.rdoc'
+      input = 'XSS[JaVaScriPt:alert(1)]'
+      output = '<p><a>XSS</a></p>'
+
+      expect(render(filename, input, context)).to eql output
+    end
+  end
+
+  context 'external links' do
+    it 'adds the `rel` attribute to the link' do
+      context = {}
+      output = render('file.rdoc', '{Google}[https://google.com]', context)
+
+      expect(output).to include('rel="nofollow noreferrer"')
+    end
+  end
+
+  def render(*args)
+    described_class.render(*args).strip
+  end
+end