Merge branch 'bw-enable-sourcepos' into 'master'

Enable CommonMark source line position information

See merge request gitlab-org/gitlab-ce!23971

21 files changed, 113 insertions, 17 deletions

diff --git a/lib/banzai/filter/markdown_engines/common_mark.rb b/lib/banzai/filter/markdown_engines/common_mark.rb
index e52c0d15b31..d3af776db05 100644
--- a/lib/banzai/filter/markdown_engines/common_mark.rb
+++ b/lib/banzai/filter/markdown_engines/common_mark.rb
@@ -32,8 +32,13 @@ module Banzai
           :DEFAULT # default rendering system. Nothing special.
         ].freeze
 
-        def initialize
-          @renderer = Banzai::Renderer::CommonMark::HTML.new(options: RENDER_OPTIONS)
+        RENDER_OPTIONS_SOURCEPOS = RENDER_OPTIONS + [
+          :SOURCEPOS # enable embedding of source position information
+        ].freeze
+
+        def initialize(context)
+          @context  = context
+          @renderer = Banzai::Renderer::CommonMark::HTML.new(options: render_options)
         end
 
         def render(text)
@@ -41,6 +46,12 @@ module Banzai
 
           @renderer.render(doc)
         end
+
+        private
+
+        def render_options
+          @context[:no_sourcepos] ? RENDER_OPTIONS : RENDER_OPTIONS_SOURCEPOS
+        end
       end
     end
   end
diff --git a/lib/banzai/filter/markdown_engines/redcarpet.rb b/lib/banzai/filter/markdown_engines/redcarpet.rb
index ec150d041ff..5b3f75096b1 100644
--- a/lib/banzai/filter/markdown_engines/redcarpet.rb
+++ b/lib/banzai/filter/markdown_engines/redcarpet.rb
@@ -20,7 +20,7 @@ module Banzai
           tables:              true
         }.freeze
 
-        def initialize
+        def initialize(context = nil)
           html_renderer = Banzai::Renderer::Redcarpet::HTML.new
           @renderer = ::Redcarpet::Markdown.new(html_renderer, OPTIONS)
         end
diff --git a/lib/banzai/filter/markdown_filter.rb b/lib/banzai/filter/markdown_filter.rb
index cdf758472c1..242e39f5495 100644
--- a/lib/banzai/filter/markdown_filter.rb
+++ b/lib/banzai/filter/markdown_filter.rb
@@ -6,7 +6,7 @@ module Banzai
       def initialize(text, context = nil, result = nil)
         super(text, context, result)
 
-        @renderer = renderer(context[:markdown_engine]).new
+        @renderer = renderer(context[:markdown_engine]).new(context)
         @text = @text.delete("\r")
       end
 
diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb
index edc053638a8..a4a06eae7b7 100644
--- a/lib/banzai/filter/sanitization_filter.rb
+++ b/lib/banzai/filter/sanitization_filter.rb
@@ -41,6 +41,9 @@ module Banzai
         whitelist[:elements].push('abbr')
         whitelist[:attributes]['abbr'] = %w(title)
 
+        # Allow the 'data-sourcepos' from CommonMark on all elements
+        whitelist[:attributes][:all].push('data-sourcepos')
+
         # Disallow `name` attribute globally, allow on `a`
         whitelist[:attributes][:all].delete('name')
         whitelist[:attributes]['a'].push('name')
diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb
index c6a3a763c23..00dbf2d3130 100644
--- a/lib/banzai/filter/spaced_link_filter.rb
+++ b/lib/banzai/filter/spaced_link_filter.rb
@@ -73,7 +73,8 @@ module Banzai
         html = Banzai::Filter::MarkdownFilter.call(transform_markdown(match), context)
 
         # link is wrapped in a <p>, so strip that off
-        html.sub('<p>', '').chomp('</p>')
+        p_node = Nokogiri::HTML.fragment(html).at_css('p')
+        p_node ? p_node.children.to_html : html
       end
 
       def spaced_link_filter(text)
diff --git a/lib/banzai/pipeline/atom_pipeline.rb b/lib/banzai/pipeline/atom_pipeline.rb
index 13a342351b6..c632910585d 100644
--- a/lib/banzai/pipeline/atom_pipeline.rb
+++ b/lib/banzai/pipeline/atom_pipeline.rb
@@ -6,7 +6,8 @@ module Banzai
       def self.transform_context(context)
         super(context).merge(
           only_path: false,
-          xhtml: true
+          xhtml: true,
+          no_sourcepos: true
         )
       end
     end
diff --git a/lib/banzai/pipeline/broadcast_message_pipeline.rb b/lib/banzai/pipeline/broadcast_message_pipeline.rb
index a3d63e0aaf5..580b5b72474 100644
--- a/lib/banzai/pipeline/broadcast_message_pipeline.rb
+++ b/lib/banzai/pipeline/broadcast_message_pipeline.rb
@@ -14,6 +14,12 @@ module Banzai
           Filter::ExternalLinkFilter
         ]
       end
+
+      def self.transform_context(context)
+        super(context).merge(
+          no_sourcepos: true
+        )
+      end
     end
   end
 end
diff --git a/lib/banzai/pipeline/email_pipeline.rb b/lib/banzai/pipeline/email_pipeline.rb
index 2c08581ce0d..0f4dd9d143d 100644
--- a/lib/banzai/pipeline/email_pipeline.rb
+++ b/lib/banzai/pipeline/email_pipeline.rb
@@ -11,7 +11,8 @@ module Banzai
 
       def self.transform_context(context)
         super(context).merge(
-          only_path: false
+          only_path: false,
+          no_sourcepos: true
         )
       end
     end
diff --git a/lib/banzai/pipeline/single_line_pipeline.rb b/lib/banzai/pipeline/single_line_pipeline.rb
index 61ff7b0bcce..72374207a8f 100644
--- a/lib/banzai/pipeline/single_line_pipeline.rb
+++ b/lib/banzai/pipeline/single_line_pipeline.rb
@@ -27,6 +27,12 @@ module Banzai
           Filter::CommitReferenceFilter
         ]
       end
+
+      def self.transform_context(context)
+        super(context).merge(
+          no_sourcepos: true
+        )
+      end
     end
   end
 end
diff --git a/lib/gitlab/gfm/reference_rewriter.rb b/lib/gitlab/gfm/reference_rewriter.rb
index 08d7db49ad7..4d82acd9d87 100644
--- a/lib/gitlab/gfm/reference_rewriter.rb
+++ b/lib/gitlab/gfm/reference_rewriter.rb
@@ -93,7 +93,7 @@ module Gitlab
       end
 
       def markdown(text)
-        Banzai.render(text, project: @source_parent, no_original_data: true)
+        Banzai.render(text, project: @source_parent, no_original_data: true, no_sourcepos: true)
       end
     end
   end
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 412ec910f3a..03e3a72a82f 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -173,6 +173,7 @@ describe IssuablesHelper do
     before do
       allow(helper).to receive(:current_user).and_return(user)
       allow(helper).to receive(:can?).and_return(true)
+      stub_commonmark_sourcepos_disabled
     end
 
     it 'returns the correct json for an issue' do
diff --git a/spec/lib/banzai/filter/markdown_filter_spec.rb b/spec/lib/banzai/filter/markdown_filter_spec.rb
index cf49249756a..4c4e821deab 100644
--- a/spec/lib/banzai/filter/markdown_filter_spec.rb
+++ b/spec/lib/banzai/filter/markdown_filter_spec.rb
@@ -30,21 +30,21 @@ describe Banzai::Filter::MarkdownFilter do
       end
 
       it 'adds language to lang attribute when specified' do
-        result = filter("```html\nsome code\n```")
+        result = filter("```html\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with("<pre><code lang=\"html\">")
+        expect(result).to start_with('<pre><code lang="html">')
       end
 
       it 'does not add language to lang attribute when not specified' do
-        result = filter("```\nsome code\n```")
+        result = filter("```\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with("<pre><code>")
+        expect(result).to start_with('<pre><code>')
       end
 
       it 'works with utf8 chars in language' do
-        result = filter("```日\nsome code\n```")
+        result = filter("```日\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with("<pre><code lang=\"日\">")
+        expect(result).to start_with('<pre><code lang="日">')
       end
     end
 
@@ -67,6 +67,38 @@ describe Banzai::Filter::MarkdownFilter do
     end
   end
 
+  describe 'source line position' do
+    context 'using CommonMark' do
+      before do
+        stub_const('Banzai::Filter::MarkdownFilter::DEFAULT_ENGINE', :common_mark)
+      end
+
+      it 'defaults to add data-sourcepos' do
+        result = filter('test')
+
+        expect(result).to eq '<p data-sourcepos="1:1-1:4">test</p>'
+      end
+
+      it 'disables data-sourcepos' do
+        result = filter('test', no_sourcepos: true)
+
+        expect(result).to eq '<p>test</p>'
+      end
+    end
+
+    context 'using Redcarpet' do
+      before do
+        stub_const('Banzai::Filter::MarkdownFilter::DEFAULT_ENGINE', :redcarpet)
+      end
+
+      it 'does not support data-sourcepos' do
+        result = filter('test')
+
+        expect(result).to eq '<p>test</p>'
+      end
+    end
+  end
+
   describe 'footnotes in tables' do
     it 'processes footnotes in table cells' do
       text = <<-MD.strip_heredoc
@@ -77,7 +109,7 @@ describe Banzai::Filter::MarkdownFilter do
       [^1]: a footnote
       MD
 
-      result = filter(text)
+      result = filter(text, no_sourcepos: true)
 
       expect(result).to include('<td>foot <sup')
       expect(result).to include('<section class="footnotes">')
diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb
index 836af18e0b6..f2a5d7b2c9f 100644
--- a/spec/lib/banzai/filter/sanitization_filter_spec.rb
+++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb
@@ -301,6 +301,13 @@ describe Banzai::Filter::SanitizationFilter do
       expect(act.to_html).to eq exp
     end
 
+    it 'allows the `data-sourcepos` attribute globally' do
+      exp = %q{<p data-sourcepos="1:1-1:10">foo/bar.md</p>}
+      act = filter(exp)
+
+      expect(act.to_html).to eq exp
+    end
+
     describe 'footnotes' do
       it 'allows correct footnote id property on links' do
         exp = %q{<a href="#fn1" id="fnref1">foo/bar.md</a>}
diff --git a/spec/lib/banzai/pipeline/description_pipeline_spec.rb b/spec/lib/banzai/pipeline/description_pipeline_spec.rb
index 8cce1b96698..77cb1954ea3 100644
--- a/spec/lib/banzai/pipeline/description_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/description_pipeline_spec.rb
@@ -13,6 +13,10 @@ describe Banzai::Pipeline::DescriptionPipeline do
     output
   end
 
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   it 'uses a limited whitelist' do
     doc = parse('# Description')
 
diff --git a/spec/lib/banzai/pipeline/full_pipeline_spec.rb b/spec/lib/banzai/pipeline/full_pipeline_spec.rb
index 3634655c6a5..aa503b6e1d5 100644
--- a/spec/lib/banzai/pipeline/full_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/full_pipeline_spec.rb
@@ -54,6 +54,8 @@ describe Banzai::Pipeline::FullPipeline do
     end
 
     it 'properly adds the necessary ids and classes' do
+      stub_commonmark_sourcepos_disabled
+
       expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote
     end
   end
diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb
index f8d50e89d40..ef6af232999 100644
--- a/spec/models/concerns/cache_markdown_field_spec.rb
+++ b/spec/models/concerns/cache_markdown_field_spec.rb
@@ -67,13 +67,17 @@ describe CacheMarkdownField do
   end
 
   let(:markdown) { '`Foo`' }
-  let(:html) { '<p dir="auto"><code>Foo</code></p>' }
+  let(:html)     { '<p dir="auto"><code>Foo</code></p>' }
 
   let(:updated_markdown) { '`Bar`' }
-  let(:updated_html) { '<p dir="auto"><code>Bar</code></p>' }
+  let(:updated_html)     { '<p dir="auto"><code>Bar</code></p>' }
 
   let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
 
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   describe '.attributes' do
     it 'excludes cache attributes' do
       expect(thing.attributes.keys.sort).to eq(%w[bar baz foo])
diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb
index 689e7d3058f..43a544cfe26 100644
--- a/spec/models/concerns/cacheable_attributes_spec.rb
+++ b/spec/models/concerns/cacheable_attributes_spec.rb
@@ -159,6 +159,10 @@ describe CacheableAttributes do
 
     describe 'edge cases' do
       describe 'caching behavior', :use_clean_rails_memory_store_caching do
+        before do
+          stub_commonmark_sourcepos_disabled
+        end
+
         it 'retrieves upload fields properly' do
           ar_record = create(:appearance, :with_logo)
           ar_record.cache!
diff --git a/spec/models/concerns/redactable_spec.rb b/spec/models/concerns/redactable_spec.rb
index 7d320edd492..7feeaa54069 100644
--- a/spec/models/concerns/redactable_spec.rb
+++ b/spec/models/concerns/redactable_spec.rb
@@ -1,6 +1,10 @@
 require 'spec_helper'
 
 describe Redactable do
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   shared_examples 'model with redactable field' do
     it 'redacts unsubscribe token' do
       model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
diff --git a/spec/requests/api/markdown_spec.rb b/spec/requests/api/markdown_spec.rb
index e82ef002d32..0cf5c5677b9 100644
--- a/spec/requests/api/markdown_spec.rb
+++ b/spec/requests/api/markdown_spec.rb
@@ -7,6 +7,8 @@ describe API::Markdown do
     let(:user) {} # No-op. It gets overwritten in the contexts below.
 
     before do
+      stub_commonmark_sourcepos_disabled
+
       post api("/markdown", user), params: params
     end
 
diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb
index dbc40bddc30..d02b4c554b1 100644
--- a/spec/serializers/group_child_entity_spec.rb
+++ b/spec/serializers/group_child_entity_spec.rb
@@ -10,6 +10,7 @@ describe GroupChildEntity do
 
   before do
     allow(request).to receive(:current_user).and_return(user)
+    stub_commonmark_sourcepos_disabled
   end
 
   shared_examples 'group child json' do
diff --git a/spec/support/helpers/stub_gitlab_calls.rb b/spec/support/helpers/stub_gitlab_calls.rb
index 370d20df444..4cb3b18df85 100644
--- a/spec/support/helpers/stub_gitlab_calls.rb
+++ b/spec/support/helpers/stub_gitlab_calls.rb
@@ -62,6 +62,12 @@ module StubGitlabCalls
     end
   end
 
+  def stub_commonmark_sourcepos_disabled
+    allow_any_instance_of(Banzai::Filter::MarkdownEngines::CommonMark)
+      .to receive(:render_options)
+      .and_return(Banzai::Filter::MarkdownEngines::CommonMark::RENDER_OPTIONS)
+  end
+
   private
 
   def stub_container_registry_tag_manifest_content