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

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

See merge request !2098

7 files changed, 42 insertions, 17 deletions

diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb
index b241a14740b..b636233c426 100644
--- a/app/helpers/markup_helper.rb
+++ b/app/helpers/markup_helper.rb
@@ -116,13 +116,13 @@ module MarkupHelper
     if gitlab_markdown?(file_name)
       markdown_unsafe(text, context)
     elsif asciidoc?(file_name)
-      asciidoc_unsafe(text)
+      asciidoc_unsafe(text, context)
     elsif plain?(file_name)
       content_tag :pre, class: 'plain-readme' do
         text
       end
     else
-      other_markup_unsafe(file_name, text)
+      other_markup_unsafe(file_name, text, context)
     end
   rescue RuntimeError
     simple_format(text)
@@ -217,12 +217,12 @@ module MarkupHelper
     Banzai.render(text, context)
   end
 
-  def asciidoc_unsafe(text)
-    Gitlab::Asciidoc.render(text)
+  def asciidoc_unsafe(text, context = {})
+    Gitlab::Asciidoc.render(text, context)
   end
 
-  def other_markup_unsafe(file_name, text)
-    Gitlab::OtherMarkup.render(file_name, text)
+  def other_markup_unsafe(file_name, text, context = {})
+    Gitlab::OtherMarkup.render(file_name, text, context)
   end
 
   def prepare_for_rendering(html, context = {})
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 fba80c7132e..96d38f6daa0 100644
--- a/lib/gitlab/asciidoc.rb
+++ b/lib/gitlab/asciidoc.rb
@@ -15,17 +15,17 @@ module Gitlab
     #
     # input         - the source text in Asciidoc format
     #
-    def self.render(input)
+    def self.render(input, context)
       asciidoc_opts = { safe: :secure,
                         backend: :gitlab_html5,
                         attributes: DEFAULT_ADOC_ATTRS }
 
+      context[:pipeline] = :markup
+
       plantuml_setup
 
       html = ::Asciidoctor.convert(input, asciidoc_opts)
-
-      filter = Banzai::Filter::SanitizationFilter.new(html)
-      html = filter.call.to_s
+      html = Banzai.render(html, context)
 
       html.html_safe
     end
diff --git a/lib/gitlab/other_markup.rb b/lib/gitlab/other_markup.rb
index c2adc9aa10b..31a24460f0f 100644
--- a/lib/gitlab/other_markup.rb
+++ b/lib/gitlab/other_markup.rb
@@ -5,12 +5,12 @@ module Gitlab
     #
     # input         - the source text in a markup format
     #
-    def self.render(file_name, input)
+    def self.render(file_name, input, context)
       html = GitHub::Markup.render(file_name, input).
         force_encoding(input.encoding)
+      context[:pipeline] = :markup
 
-      filter = Banzai::Filter::SanitizationFilter.new(html)
-      html = filter.call.to_s
+      html = Banzai.render(html, context)
 
       html.html_safe
     end
diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb
index 0f47fb2fbd9..f284dd14cec 100644
--- a/spec/lib/gitlab/asciidoc_spec.rb
+++ b/spec/lib/gitlab/asciidoc_spec.rb
@@ -22,7 +22,7 @@ module Gitlab
         expect(Asciidoctor).to receive(:convert)
           .with(input, expected_asciidoc_opts).and_return(html)
 
-        expect(render(input)).to eq(html)
+        expect(render(input, context)).to eq(html)
       end
 
       context "XSS" do
@@ -33,7 +33,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>',
@@ -43,10 +43,18 @@ module Gitlab
 
         links.each do |name, data|
           it "does not convert dangerous #{name} into HTML" do
-            expect(render(data[:input])).to eq(data[:output])
+            expect(render(data[:input], context)).to include(data[:output])
           end
         end
       end
+
+      context 'external links' do
+        it 'adds the `rel` attribute to the link' do
+          output = render('link:https://google.com[Google]', context)
+
+          expect(output).to include('rel="nofollow noreferrer"')
+        end
+      end
     end
 
     def render(*args)
diff --git a/spec/lib/gitlab/other_markup_spec.rb b/spec/lib/gitlab/other_markup_spec.rb
index d6d53e8586c..c0f5fa9dc1f 100644
--- a/spec/lib/gitlab/other_markup_spec.rb
+++ b/spec/lib/gitlab/other_markup_spec.rb
@@ -13,7 +13,7 @@ describe Gitlab::OtherMarkup, lib: true do
     }
     links.each do |name, data|
       it "does not convert dangerous #{name} into HTML" do
-        expect(render(data[:file], data[:input])).to eq(data[:output])
+        expect(render(data[:file], data[:input], context)).to eq(data[:output])
       end
     end
   end