Merge branch '26375-markdown-footnotes-not-working' into 'master'

Markdown footnotes not working

Closes #26375

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

Gemfile

@@ -125,9 +125,9 @@ gem 'wikicloth', '0.8.1'
 gem 'asciidoctor', '~> 1.5.8'
 gem 'asciidoctor-plantuml', '0.0.8'
 gem 'rouge', '~> 3.1'
-gem 'truncato', '~> 0.7.9'
+gem 'truncato', '~> 0.7.11'
 gem 'bootstrap_form', '~> 2.7.0'
-gem 'nokogiri', '~> 1.8.5'
+gem 'nokogiri', '~> 1.10.1'
 gem 'escape_utils', '~> 1.1'
 
 # Calendar rendering

Gemfile.lock

@@ -468,7 +468,7 @@ GEM
     mimemagic (0.3.2)
     mini_magick (4.8.0)
     mini_mime (1.0.1)
-    mini_portile2 (2.3.0)
+    mini_portile2 (2.4.0)
     minitest (5.11.3)
     msgpack (1.2.4)
     multi_json (1.13.1)
@@ -483,8 +483,8 @@ GEM
     net-ssh (5.0.1)
     netrc (0.11.0)
     nio4r (2.3.1)
-    nokogiri (1.8.5)
-      mini_portile2 (~> 2.3.0)
+    nokogiri (1.10.1)
+      mini_portile2 (~> 2.4.0)
     nokogumbo (1.5.0)
       nokogiri
     numerizer (0.1.1)
@@ -883,9 +883,9 @@ GEM
     toml-rb (1.0.0)
       citrus (~> 3.0, > 3.0)
     trollop (2.1.3)
-    truncato (0.7.10)
+    truncato (0.7.11)
       htmlentities (~> 4.3.1)
-      nokogiri (~> 1.8.0, >= 1.7.0)
+      nokogiri (>= 1.7.0, <= 2.0)
     tzinfo (1.2.5)
       thread_safe (~> 0.1)
     u2f (0.2.1)
@@ -1068,7 +1068,7 @@ DEPENDENCIES
   nakayoshi_fork (~> 0.0.4)
   net-ldap
   net-ssh (~> 5.0)
-  nokogiri (~> 1.8.5)
+  nokogiri (~> 1.10.1)
   oauth2 (~> 1.4)
   octokit (~> 4.9)
   omniauth (~> 1.8)
@@ -1165,7 +1165,7 @@ DEPENDENCIES
   thin (~> 1.7.0)
   timecop (~> 0.8.0)
   toml-rb (~> 1.0.0)
-  truncato (~> 0.7.9)
+  truncato (~> 0.7.11)
   u2f (~> 0.2.1)
   uglifier (~> 2.7.2)
   unf (~> 0.1.4)

app/models/concerns/cache_markdown_field.rb

@@ -15,7 +15,7 @@ module CacheMarkdownField
   # Increment this number every time the renderer changes its output
   CACHE_REDCARPET_VERSION         = 3
   CACHE_COMMONMARK_VERSION_START  = 10
-  CACHE_COMMONMARK_VERSION        = 12
+  CACHE_COMMONMARK_VERSION        = 13
 
   # changes to these attributes cause the cache to be invalidates
   INVALIDATED_BY = %w[author project].freeze

changelogs/unreleased/26375-markdown-footnotes-not-working.yml

@@ -0,0 +1,5 @@
+---
+title: Footnotes now render properly in markdown
+merge_request: 24168
+author:
+type: fixed

lib/banzai/filter/footnote_filter.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+module Banzai
+  module Filter
+    # HTML Filter for footnotes
+    #
+    # Footnotes are supported in CommonMark.  However we were stripping
+    # the ids during sanitization.  Those are now allowed.
+    #
+    # Footnotes are numbered the same - the first one has `id=fn1`, the
+    # second is `id=fn2`, etc.  In order to allow footnotes when rendering
+    # multiple markdown blocks on a page, we need to make each footnote
+    # reference unique.
+    #
+    # This filter adds a random number to each footnote (the same number
+    # can be used for a single render). So you get `id=fn1-4335` and `id=fn2-4335`.
+    #
+    class FootnoteFilter < HTML::Pipeline::Filter
+      INTEGER_PATTERN                 = /\A\d+\z/.freeze
+      FOOTNOTE_ID_PREFIX              = 'fn'.freeze
+      FOOTNOTE_LINK_ID_PREFIX         = 'fnref'.freeze
+      FOOTNOTE_LI_REFERENCE_PATTERN   = /\A#{FOOTNOTE_ID_PREFIX}\d+\z/.freeze
+      FOOTNOTE_LINK_REFERENCE_PATTERN = /\A#{FOOTNOTE_LINK_ID_PREFIX}\d+\z/.freeze
+      FOOTNOTE_START_NUMBER           = 1
+
+      def call
+        return doc unless first_footnote = doc.at_css("ol > li[id=#{fn_id(FOOTNOTE_START_NUMBER)}]")
+
+        # Sanitization stripped off the section wrapper - add it back in
+        first_footnote.parent.wrap('<section class="footnotes">')
+        rand_suffix = "-#{random_number}"
+
+        doc.css('sup > a[id]').each do |link_node|
+          ref_num       = link_node[:id].delete_prefix(FOOTNOTE_LINK_ID_PREFIX)
+          footnote_node = doc.at_css("li[id=#{fn_id(ref_num)}]")
+          backref_node  = footnote_node.at_css("a[href=\"##{fnref_id(ref_num)}\"]")
+
+          if ref_num =~ INTEGER_PATTERN && footnote_node && backref_node
+            link_node[:href]    += rand_suffix
+            link_node[:id]      += rand_suffix
+            footnote_node[:id]  += rand_suffix
+            backref_node[:href] += rand_suffix
+
+            # Sanitization stripped off class - add it back in
+            link_node.parent.append_class('footnote-ref')
+            backref_node.append_class('footnote-backref')
+          end
+        end
+
+        doc
+      end
+
+      private
+
+      def random_number
+        @random_number ||= rand(10000)
+      end
+
+      def fn_id(num)
+        "#{FOOTNOTE_ID_PREFIX}#{num}"
+      end
+
+      def fnref_id(num)
+        "#{FOOTNOTE_LINK_ID_PREFIX}#{num}"
+      end
+    end
+  end
+end

lib/banzai/filter/sanitization_filter.rb

@@ -8,8 +8,8 @@ module Banzai
     class SanitizationFilter < HTML::Pipeline::SanitizationFilter
       include Gitlab::Utils::StrongMemoize
 
-      UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze
-      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/
+      UNSAFE_PROTOCOLS        = %w(data javascript vbscript).freeze
+      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/.freeze
 
       def whitelist
         strong_memoize(:whitelist) do
@@ -45,10 +45,9 @@ module Banzai
         whitelist[:attributes][:all].delete('name')
         whitelist[:attributes]['a'].push('name')
 
-        # Allow any protocol in `a` elements...
+        # Allow any protocol in `a` elements
+        # and then remove links with unsafe protocols
         whitelist[:protocols].delete('a')
-
-        # ...but then remove links with unsafe protocols
         whitelist[:transformers].push(self.class.remove_unsafe_links)
 
         # Remove `rel` attribute from `a` elements
@@ -57,6 +56,12 @@ module Banzai
         # Remove any `style` properties not required for table alignment
         whitelist[:transformers].push(self.class.remove_unsafe_table_style)
 
+        # Allow `id` in a and li elements for footnotes
+        # and remove any `id` properties not matching for footnotes
+        whitelist[:attributes]['a'].push('id')
+        whitelist[:attributes]['li'] = %w(id)
+        whitelist[:transformers].push(self.class.remove_non_footnote_ids)
+
         whitelist
       end
 
@@ -112,6 +117,20 @@ module Banzai
             end
           end
         end
+
+        def remove_non_footnote_ids
+          lambda do |env|
+            node = env[:node]
+
+            return unless node.name == 'a' || node.name == 'li'
+            return unless node.has_attribute?('id')
+
+            return if node.name == 'a' && node['id'] =~ Banzai::Filter::FootnoteFilter::FOOTNOTE_LINK_REFERENCE_PATTERN
+            return if node.name == 'li' && node['id'] =~ Banzai::Filter::FootnoteFilter::FOOTNOTE_LI_REFERENCE_PATTERN
+
+            node.remove_attribute('id')
+          end
+        end
       end
     end
   end

lib/banzai/pipeline/gfm_pipeline.rb

@@ -30,6 +30,7 @@ module Banzai
           Filter::AutolinkFilter,
           Filter::ExternalLinkFilter,
           Filter::SuggestionFilter,
+          Filter::FootnoteFilter,
 
           *reference_filters,
 

qa/Gemfile

@@ -7,4 +7,4 @@ gem 'rake', '~> 12.3.0'
 gem 'rspec', '~> 3.7'
 gem 'selenium-webdriver', '~> 3.12'
 gem 'airborne', '~> 0.2.13'
-gem 'nokogiri', '~> 1.8.5'
+gem 'nokogiri', '~> 1.10.1'

qa/Gemfile.lock

@@ -44,11 +44,11 @@ GEM
       mime-types-data (~> 3.2015)
     mime-types-data (3.2016.0521)
     mini_mime (1.0.0)
-    mini_portile2 (2.3.0)
+    mini_portile2 (2.4.0)
     minitest (5.11.1)
     netrc (0.11.0)
-    nokogiri (1.8.5)
-      mini_portile2 (~> 2.3.0)
+    nokogiri (1.10.1)
+      mini_portile2 (~> 2.4.0)
     pry (0.11.3)
       coderay (~> 1.1.0)
       method_source (~> 0.9.0)
@@ -97,7 +97,7 @@ DEPENDENCIES
   airborne (~> 0.2.13)
   capybara (~> 2.16.1)
   capybara-screenshot (~> 1.0.18)
-  nokogiri (~> 1.8.5)
+  nokogiri (~> 1.10.0)
   pry-byebug (~> 3.5.1)
   rake (~> 12.3.0)
   rspec (~> 3.7)

spec/lib/banzai/filter/footnote_filter_spec.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Banzai::Filter::FootnoteFilter do
+  include FilterSpecHelper
+
+  # first[^1] and second[^second]
+  # [^1]: one
+  # [^second]: two
+  let(:footnote) do
+    <<~EOF
+      <p>first<sup><a href="#fn1" id="fnref1">1</a></sup> and second<sup><a href="#fn2" id="fnref2">2</a></sup></p>
+      <ol>
+      <li id="fn1">
+      <p>one <a href="#fnref1">↩</a></p>
+      </li>
+      <li id="fn2">
+      <p>two <a href="#fnref2">↩</a></p>
+      </li>
+      </ol>
+    EOF
+  end
+
+  let(:filtered_footnote) do
+    <<~EOF
+      <p>first<sup class="footnote-ref"><a href="#fn1-#{identifier}" id="fnref1-#{identifier}">1</a></sup> and second<sup class="footnote-ref"><a href="#fn2-#{identifier}" id="fnref2-#{identifier}">2</a></sup></p>
+      <section class="footnotes"><ol>
+      <li id="fn1-#{identifier}">
+      <p>one <a href="#fnref1-#{identifier}" class="footnote-backref">↩</a></p>
+      </li>
+      <li id="fn2-#{identifier}">
+      <p>two <a href="#fnref2-#{identifier}" class="footnote-backref">↩</a></p>
+      </li>
+      </ol></section>
+    EOF
+  end
+
+  context 'when footnotes exist' do
+    let(:doc)        { filter(footnote) }
+    let(:link_node)  { doc.css('sup > a').first }
+    let(:identifier) { link_node[:id].delete_prefix('fnref1-') }
+
+    it 'properly adds the necessary ids and classes' do
+      expect(doc.to_html).to eq filtered_footnote
+    end
+  end
+end

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -246,7 +246,7 @@ describe Banzai::Filter::SanitizationFilter do
 
       'protocol-based JS injection: spaces and entities' => {
         input:  '<a href=" &#14;  javascript:alert(\'XSS\');">foo</a>',
-        output: '<a href>foo</a>'
+        output: '<a href="">foo</a>'
       },
 
       'protocol whitespace' => {
@@ -300,5 +300,48 @@ describe Banzai::Filter::SanitizationFilter do
 
       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>}
+        act = filter(exp)
+
+        expect(act.to_html).to eq exp
+      end
+
+      it 'allows correct footnote id property on li element' do
+        exp = %q{<ol><li id="fn1">footnote</li></ol>}
+        act = filter(exp)
+
+        expect(act.to_html).to eq exp
+      end
+
+      it 'removes invalid id for footnote links' do
+        exp = %q{<a href="#fn1">link</a>}
+
+        %w[fnrefx test xfnref1].each do |id|
+          act = filter(%Q{<a href="#fn1" id="#{id}">link</a>})
+
+          expect(act.to_html).to eq exp
+        end
+      end
+
+      it 'removes invalid id for footnote li' do
+        exp = %q{<ol><li>footnote</li></ol>}
+
+        %w[fnx test xfn1].each do |id|
+          act = filter(%Q{<ol><li id="#{id}">footnote</li></ol>})
+
+          expect(act.to_html).to eq exp
+        end
+      end
+
+      it 'allows footnotes numbered higher than 9' do
+        exp = %q{<a href="#fn15" id="fnref15">link</a><ol><li id="fn15">footnote</li></ol>}
+        act = filter(exp)
+
+        expect(act.to_html).to eq exp
+      end
+    end
   end
 end

spec/lib/banzai/pipeline/full_pipeline_spec.rb

@@ -25,4 +25,36 @@ describe Banzai::Pipeline::FullPipeline do
       expect(result).to include(%{data-original='\">bad things'})
     end
   end
+
+  describe 'footnotes' do
+    let(:project)    { create(:project, :public) }
+    let(:html)       { described_class.to_html(footnote_markdown, project: project) }
+    let(:identifier) { html[/.*fnref1-(\d+).*/, 1] }
+    let(:footnote_markdown) do
+      <<~EOF
+        first[^1] and second[^second]
+        [^1]: one
+        [^second]: two
+      EOF
+    end
+
+    let(:filtered_footnote) do
+      <<~EOF
+        <p dir="auto">first<sup class="footnote-ref"><a href="#fn1-#{identifier}" id="fnref1-#{identifier}">1</a></sup> and second<sup class="footnote-ref"><a href="#fn2-#{identifier}" id="fnref2-#{identifier}">2</a></sup></p>
+
+        <section class="footnotes"><ol>
+        <li id="fn1-#{identifier}">
+        <p>one <a href="#fnref1-#{identifier}" class="footnote-backref"><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        </li>
+        <li id="fn2-#{identifier}">
+        <p>two <a href="#fnref2-#{identifier}" class="footnote-backref"><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        </li>
+        </ol></section>
+      EOF
+    end
+
+    it 'properly adds the necessary ids and classes' do
+      expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote
+    end
+  end
 end