Merge branch 'panjan/gitlab-ce-reference_links_styling' into 'master'

Fix Markdown styling inside reference links

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/18096.

Based off https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6609.

See merge request !7169

CHANGELOG.md

@@ -7,6 +7,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Trim leading and trailing whitespace on project_path (Linus Thiel)
 - Prevent award emoji via notes for issues/MRs authored by user (barthc)
 - Adds an optional path parameter to the Commits API to filter commits by path (Luis HGO)
+- Fix Markdown styling inside reference links (Jan Zdráhal)
 - Fix extra space on Build sidebar on Firefox !7060
 - Fix mobile layout issues in admin user overview page !7087
 - Fix HipChat notifications rendering (airatshigapov, eisnerd)

lib/banzai/filter/abstract_reference_filter.rb

@@ -102,10 +102,10 @@ module Banzai
             end
 
           elsif element_node?(node)
-            yield_valid_link(node) do |link, text|
+            yield_valid_link(node) do |link, inner_html|
               if ref_pattern && link =~ /\A#{ref_pattern}\z/
                 replace_link_node_with_href(node, link) do
-                  object_link_filter(link, ref_pattern, link_text: text)
+                  object_link_filter(link, ref_pattern, link_content: inner_html)
                 end
 
                 next
@@ -113,9 +113,9 @@ module Banzai
 
               next unless link_pattern
 
-              if link == text && text =~ /\A#{link_pattern}/
+              if link == inner_html && inner_html =~ /\A#{link_pattern}/
                 replace_link_node_with_text(node, link) do
-                  object_link_filter(text, link_pattern)
+                  object_link_filter(inner_html, link_pattern)
                 end
 
                 next
@@ -123,7 +123,7 @@ module Banzai
 
               if link =~ /\A#{link_pattern}\z/
                 replace_link_node_with_href(node, link) do
-                  object_link_filter(link, link_pattern, link_text: text)
+                  object_link_filter(link, link_pattern, link_content: inner_html)
                 end
 
                 next
@@ -140,11 +140,11 @@ module Banzai
       #
       # text - String text to replace references in.
       # pattern - Reference pattern to match against.
-      # link_text - Original content of the link being replaced.
+      # link_content - Original content of the link being replaced.
       #
       # Returns a String with references replaced with links. All links
       # have `gfm` and `gfm-OBJECT_NAME` class names attached for styling.
-      def object_link_filter(text, pattern, link_text: nil)
+      def object_link_filter(text, pattern, link_content: nil)
         references_in(text, pattern) do |match, id, project_ref, matches|
           project = project_from_ref_cached(project_ref)
 
@@ -152,7 +152,7 @@ module Banzai
             title = object_link_title(object)
             klass = reference_class(object_sym)
 
-            data = data_attributes_for(link_text || match, project, object)
+            data = data_attributes_for(link_content || match, project, object)
 
             if matches.names.include?("url") && matches[:url]
               url = matches[:url]
@@ -160,11 +160,11 @@ module Banzai
               url = url_for_object_cached(object, project)
             end
 
-            text = link_text || object_link_text(object, matches)
+            content = link_content || object_link_text(object, matches)
 
             %(<a href="#{url}" #{data}
                  title="#{escape_once(title)}"
-                 class="#{klass}">#{escape_once(text)}</a>)
+                 class="#{klass}">#{content}</a>)
           else
             match
           end

lib/banzai/filter/external_issue_reference_filter.rb

@@ -37,10 +37,10 @@ module Banzai
             end
 
           elsif element_node?(node)
-            yield_valid_link(node) do |link, text|
+            yield_valid_link(node) do |link, inner_html|
               if link =~ ref_start_pattern
                 replace_link_node_with_href(node, link) do
-                  issue_link_filter(link, link_text: text)
+                  issue_link_filter(link, link_content: inner_html)
                 end
               end
             end
@@ -54,10 +54,11 @@ module Banzai
       # issue's details page.
       #
       # text - String text to replace references in.
+      # link_content - Original content of the link being replaced.
       #
       # Returns a String with `JIRA-123` references replaced with links. All
       # links have `gfm` and `gfm-issue` class names attached for styling.
-      def issue_link_filter(text, link_text: nil)
+      def issue_link_filter(text, link_content: nil)
         project = context[:project]
 
         self.class.references_in(text, issue_reference_pattern) do |match, id|
@@ -69,11 +70,11 @@ module Banzai
           klass = reference_class(:issue)
           data  = data_attribute(project: project.id, external_issue: id)
 
-          text = link_text || match
+          content = link_content || match
 
           %(<a href="#{url}" #{data}
                title="#{escape_once(title)}"
-               class="#{klass}">#{escape_once(text)}</a>)
+               class="#{klass}">#{content}</a>)
         end
       end
 

lib/banzai/filter/reference_filter.rb

@@ -85,14 +85,14 @@ module Banzai
         @nodes ||= each_node.to_a
       end
 
-      # Yields the link's URL and text whenever the node is a valid <a> tag.
+      # Yields the link's URL and inner HTML whenever the node is a valid <a> tag.
       def yield_valid_link(node)
         link = CGI.unescape(node.attr('href').to_s)
-        text = node.text
+        inner_html = node.inner_html
 
         return unless link.force_encoding('UTF-8').valid_encoding?
 
-        yield link, text
+        yield link, inner_html
       end
 
       def replace_text_when_pattern_matches(node, pattern)

lib/banzai/filter/user_reference_filter.rb

@@ -35,10 +35,10 @@ module Banzai
               user_link_filter(content)
             end
           elsif element_node?(node)
-            yield_valid_link(node) do |link, text|
+            yield_valid_link(node) do |link, inner_html|
               if link =~ ref_pattern_start
                 replace_link_node_with_href(node, link) do
-                  user_link_filter(link, link_text: text)
+                  user_link_filter(link, link_content: inner_html)
                 end
               end
             end
@@ -52,15 +52,16 @@ module Banzai
       # user's profile page.
       #
       # text - String text to replace references in.
+      # link_content - Original content of the link being replaced.
       #
       # Returns a String with `@user` references replaced with links. All links
       # have `gfm` and `gfm-project_member` class names attached for styling.
-      def user_link_filter(text, link_text: nil)
+      def user_link_filter(text, link_content: nil)
         self.class.references_in(text) do |match, username|
           if username == 'all'
-            link_to_all(link_text: link_text)
+            link_to_all(link_content: link_content)
           elsif namespace = namespaces[username]
-            link_to_namespace(namespace, link_text: link_text) || match
+            link_to_namespace(namespace, link_content: link_content) || match
           else
             match
           end
@@ -102,49 +103,49 @@ module Banzai
         reference_class(:project_member)
       end
 
-      def link_to_all(link_text: nil)
+      def link_to_all(link_content: nil)
         project = context[:project]
         author = context[:author]
 
         if author && !project.team.member?(author)
-          link_text
+          link_content
         else
           url = urls.namespace_project_url(project.namespace, project,
                                            only_path: context[:only_path])
 
           data = data_attribute(project: project.id, author: author.try(:id))
-          text = link_text || User.reference_prefix + 'all'
+          content = link_content || User.reference_prefix + 'all'
 
-          link_tag(url, data, text, 'All Project and Group Members')
+          link_tag(url, data, content, 'All Project and Group Members')
         end
       end
 
-      def link_to_namespace(namespace, link_text: nil)
+      def link_to_namespace(namespace, link_content: nil)
         if namespace.is_a?(Group)
-          link_to_group(namespace.path, namespace, link_text: link_text)
+          link_to_group(namespace.path, namespace, link_content: link_content)
         else
-          link_to_user(namespace.path, namespace, link_text: link_text)
+          link_to_user(namespace.path, namespace, link_content: link_content)
         end
       end
 
-      def link_to_group(group, namespace, link_text: nil)
+      def link_to_group(group, namespace, link_content: nil)
         url = urls.group_url(group, only_path: context[:only_path])
         data = data_attribute(group: namespace.id)
-        text = link_text || Group.reference_prefix + group
+        content = link_content || Group.reference_prefix + group
 
-        link_tag(url, data, text, namespace.name)
+        link_tag(url, data, content, namespace.name)
       end
 
-      def link_to_user(user, namespace, link_text: nil)
+      def link_to_user(user, namespace, link_content: nil)
         url = urls.user_url(user, only_path: context[:only_path])
         data = data_attribute(user: namespace.owner_id)
-        text = link_text || User.reference_prefix + user
+        content = link_content || User.reference_prefix + user
 
-        link_tag(url, data, text, namespace.owner_name)
+        link_tag(url, data, content, namespace.owner_name)
       end
 
-      def link_tag(url, data, text, title)
-        %(<a href="#{url}" #{data} class="#{link_class}" title="#{escape_once(title)}">#{escape_once(text)}</a>)
+      def link_tag(url, data, link_content, title)
+        %(<a href="#{url}" #{data} class="#{link_class}" title="#{escape_once(title)}">#{link_content}</a>)
       end
     end
   end

lib/banzai/redactor.rb

@@ -41,10 +41,10 @@ module Banzai
           next if visible.include?(node)
 
           doc_data[:visible_reference_count] -= 1
-          # The reference should be replaced by the original text,
-          # which is not always the same as the rendered text.
-          text = node.attr('data-original') || node.text
-          node.replace(text)
+          # The reference should be replaced by the original link's content,
+          # which is not always the same as the rendered one.
+          content = node.attr('data-original') || node.inner_html
+          node.replace(content)
         end
       end
 

spec/lib/banzai/filter/external_issue_reference_filter_spec.rb

@@ -8,6 +8,8 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
   end
 
   shared_examples_for "external issue tracker" do
+    it_behaves_like 'a reference containing an element node'
+
     it 'requires project context' do
       expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
     end

spec/lib/banzai/filter/issue_reference_filter_spec.rb

@@ -22,6 +22,8 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
   end
 
   context 'internal reference' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:reference) { issue.to_reference }
 
     it 'ignores valid references when using non-default tracker' do
@@ -83,6 +85,20 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
       expect(link.attr('data-issue')).to eq issue.id.to_s
     end
 
+    it 'includes a data-original attribute' do
+      doc = reference_filter("See #{reference}")
+      link = doc.css('a').first
+
+      expect(link).to have_attribute('data-original')
+      expect(link.attr('data-original')).to eq reference
+    end
+
+    it 'does not escape the data-original attribute' do
+      inner_html = 'element <code>node</code> inside'
+      doc = reference_filter(%{<a href="#{reference}">#{inner_html}</a>})
+      expect(doc.children.first.attr('data-original')).to eq inner_html
+    end
+
     it 'supports an :only_path context' do
       doc = reference_filter("Issue #{reference}", only_path: true)
       link = doc.css('a').first.attr('href')
@@ -101,6 +117,8 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
   end
 
   context 'cross-project reference' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:namespace) { create(:namespace, name: 'cross-reference') }
     let(:project2)  { create(:empty_project, :public, namespace: namespace) }
     let(:issue)     { create(:issue, project: project2) }
@@ -141,6 +159,8 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
   end
 
   context 'cross-project URL reference' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:namespace) { create(:namespace, name: 'cross-reference') }
     let(:project2)  { create(:empty_project, :public, namespace: namespace) }
     let(:issue)     { create(:issue, project: project2) }
@@ -160,39 +180,45 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
   end
 
   context 'cross-project reference in link href' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:namespace) { create(:namespace, name: 'cross-reference') }
     let(:project2)  { create(:empty_project, :public, namespace: namespace) }
     let(:issue)     { create(:issue, project: project2) }
-    let(:reference) { %Q{<a href="#{issue.to_reference(project)}">Reference</a>} }
+    let(:reference) { issue.to_reference(project) }
+    let(:reference_link) { %{<a href="#{reference}">Reference</a>} }
 
     it 'links to a valid reference' do
-      doc = reference_filter("See #{reference}")
+      doc = reference_filter("See #{reference_link}")
 
       expect(doc.css('a').first.attr('href')).
         to eq helper.url_for_issue(issue.iid, project2)
     end
 
     it 'links with adjacent text' do
-      doc = reference_filter("Fixed (#{reference}.)")
+      doc = reference_filter("Fixed (#{reference_link}.)")
       expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/)
     end
   end
 
   context 'cross-project URL in link href' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:namespace) { create(:namespace, name: 'cross-reference') }
     let(:project2)  { create(:empty_project, :public, namespace: namespace) }
     let(:issue)     { create(:issue, project: project2) }
-    let(:reference) { %Q{<a href="#{helper.url_for_issue(issue.iid, project2) + "#note_123"}">Reference</a>} }
+    let(:reference) { "#{helper.url_for_issue(issue.iid, project2) + "#note_123"}" }
+    let(:reference_link) { %{<a href="#{reference}">Reference</a>} }
 
     it 'links to a valid reference' do
-      doc = reference_filter("See #{reference}")
+      doc = reference_filter("See #{reference_link}")
 
       expect(doc.css('a').first.attr('href')).
         to eq helper.url_for_issue(issue.iid, project2) + "#note_123"
     end
 
     it 'links with adjacent text' do
-      doc = reference_filter("Fixed (#{reference}.)")
+      doc = reference_filter("Fixed (#{reference_link}.)")
       expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/)
     end
   end

spec/lib/banzai/filter/user_reference_filter_spec.rb

@@ -24,6 +24,8 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
   end
 
   context 'mentioning @all' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:reference) { User.reference_prefix + 'all' }
 
     before do
@@ -60,6 +62,8 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
   end
 
   context 'mentioning a user' do
+    it_behaves_like 'a reference containing an element node'
+
     it 'links to a User' do
       doc = reference_filter("Hey #{reference}")
       expect(doc.css('a').first.attr('href')).to eq urls.user_url(user)
@@ -89,6 +93,8 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
   end
 
   context 'mentioning a group' do
+    it_behaves_like 'a reference containing an element node'
+
     let(:group)     { create(:group) }
     let(:reference) { group.to_reference }
 

spec/lib/banzai/pipeline/full_pipeline_spec.rb

+require 'rails_helper'
+
+describe Banzai::Pipeline::FullPipeline do
+  describe 'References' do
+    let(:project) { create(:empty_project, :public) }
+    let(:issue)   { create(:issue, project: project) }
+
+    it 'handles markdown inside a reference' do
+      markdown = "[some `code` inside](#{issue.to_reference})"
+      result = described_class.call(markdown, project: project)
+      link_content = result[:output].css('a').inner_html
+      expect(link_content).to eq('some <code>code</code> inside')
+    end
+
+    it 'sanitizes reference HTML' do
+      link_label = '<script>bad things</script>'
+      markdown = "[#{link_label}](#{issue.to_reference})"
+      result = described_class.to_html(markdown, project: project)
+      expect(result).not_to include(link_label)
+    end
+
+    it 'escapes the data-original attribute on a reference' do
+      markdown = %Q{[">bad things](#{issue.to_reference})}
+      result = described_class.to_html(markdown, project: project)
+      expect(result).to include(%{data-original='\"&gt;bad things'})
+    end
+  end
+end

spec/lib/banzai/redactor_spec.rb

@@ -6,39 +6,60 @@ describe Banzai::Redactor do
   let(:redactor) { described_class.new(project, user) }
 
   describe '#redact' do
-    it 'redacts an Array of documents' do
-      doc1 = Nokogiri::HTML.
-        fragment('<a class="gfm" data-reference-type="issue">foo</a>')
-
-      doc2 = Nokogiri::HTML.
-        fragment('<a class="gfm" data-reference-type="issue">bar</a>')
-
-      expect(redactor).to receive(:nodes_visible_to_user).and_return([])
-
-      redacted_data = redactor.redact([doc1, doc2])
-
-      expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
-      expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([0, 0])
-      expect(doc1.to_html).to eq('foo')
-      expect(doc2.to_html).to eq('bar')
+    context 'when reference not visible to user' do
+      before do
+        expect(redactor).to receive(:nodes_visible_to_user).and_return([])
+      end
+
+      it 'redacts an array of documents' do
+        doc1 = Nokogiri::HTML.
+               fragment('<a class="gfm" data-reference-type="issue">foo</a>')
+
+        doc2 = Nokogiri::HTML.
+               fragment('<a class="gfm" data-reference-type="issue">bar</a>')
+
+        redacted_data = redactor.redact([doc1, doc2])
+
+        expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
+        expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([0, 0])
+        expect(doc1.to_html).to eq('foo')
+        expect(doc2.to_html).to eq('bar')
+      end
+
+      it 'replaces redacted reference with inner HTML' do
+        doc = Nokogiri::HTML.fragment("<a class='gfm' data-reference-type='issue'>foo</a>")
+        redactor.redact([doc])
+        expect(doc.to_html).to eq('foo')
+      end
+
+      context 'when data-original attribute provided' do
+        let(:original_content) { '<code>foo</code>' }
+        it 'replaces redacted reference with original content' do
+          doc = Nokogiri::HTML.fragment("<a class='gfm' data-reference-type='issue' data-original='#{original_content}'>bar</a>")
+          redactor.redact([doc])
+          expect(doc.to_html).to eq(original_content)
+        end
+      end
     end
 
-    it 'does not redact an Array of documents' do
-      doc1_html = '<a class="gfm" data-reference-type="issue">foo</a>'
-      doc1 = Nokogiri::HTML.fragment(doc1_html)
+    context 'when reference visible to user' do
+      it 'does not redact an array of documents' do
+        doc1_html = '<a class="gfm" data-reference-type="issue">foo</a>'
+        doc1 = Nokogiri::HTML.fragment(doc1_html)
 
-      doc2_html = '<a class="gfm" data-reference-type="issue">bar</a>'
-      doc2 = Nokogiri::HTML.fragment(doc2_html)
+        doc2_html = '<a class="gfm" data-reference-type="issue">bar</a>'
+        doc2 = Nokogiri::HTML.fragment(doc2_html)
 
-      nodes = redactor.document_nodes([doc1, doc2]).map { |x| x[:nodes] }
-      expect(redactor).to receive(:nodes_visible_to_user).and_return(nodes.flatten)
+        nodes = redactor.document_nodes([doc1, doc2]).map { |x| x[:nodes] }
+        expect(redactor).to receive(:nodes_visible_to_user).and_return(nodes.flatten)
 
-      redacted_data = redactor.redact([doc1, doc2])
+        redacted_data = redactor.redact([doc1, doc2])
 
-      expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
-      expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([1, 1])
-      expect(doc1.to_html).to eq(doc1_html)
-      expect(doc2.to_html).to eq(doc2_html)
+        expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
+        expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([1, 1])
+        expect(doc1.to_html).to eq(doc1_html)
+        expect(doc2.to_html).to eq(doc2_html)
+      end
     end
   end
 

spec/support/banzai/reference_filter_shared_examples.rb

+# Specs for reference links containing HTML.
+#
+# Requires a reference:
+#   let(:reference) { '#42' }
+shared_examples 'a reference containing an element node' do
+  let(:inner_html) { 'element <code>node</code> inside' }
+  let(:reference_with_element) { %{<a href="#{reference}">#{inner_html}</a>} }
+
+  it 'does not escape inner html' do
+    doc = reference_filter(reference_with_element)
+    expect(doc.children.first.inner_html).to eq(inner_html)
+  end
+end