Merge branch...

Merge branch '346297-actionview-template-error-cannot-add-sibling-to-a-node-with-no-parent' into 'master'

Invalid Markdown footnote can cause a 500 error

See merge request gitlab-org/gitlab!75054

lib/banzai/filter/footnote_filter.rb

@@ -21,9 +21,9 @@ module Banzai
       FOOTNOTE_LI_REFERENCE_PATTERN   = /\A#{FOOTNOTE_ID_PREFIX}.+\z/.freeze
       FOOTNOTE_LINK_REFERENCE_PATTERN = /\A#{FOOTNOTE_LINK_ID_PREFIX}.+\z/.freeze
 
-      CSS_SECTION    = "ol > li a[href^=\"\##{FOOTNOTE_LINK_ID_PREFIX}\"]"
+      CSS_SECTION    = "section[data-footnotes]"
       XPATH_SECTION  = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_SECTION).freeze
-      CSS_FOOTNOTE   = 'sup > a[id]'
+      CSS_FOOTNOTE   = 'sup > a[data-footnote-ref]'
       XPATH_FOOTNOTE = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_FOOTNOTE).freeze
 
       # only needed when feature flag use_cmark_renderer is turned off
@@ -37,20 +37,28 @@ module Banzai
       XPATH_SECTION_OLD                   = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_SECTION_OLD).freeze
 
       def call
-        xpath_section = Feature.enabled?(:use_cmark_renderer) ? XPATH_SECTION : XPATH_SECTION_OLD
-        return doc unless first_footnote = doc.at_xpath(xpath_section)
-
-        # Sanitization stripped off the section wrapper - add it back in
         if Feature.enabled?(:use_cmark_renderer)
-          first_footnote.parent.parent.parent.wrap('<section class="footnotes" data-footnotes>')
+          # Sanitization stripped off the section class - add it back in
+          return doc unless section_node = doc.at_xpath(XPATH_SECTION)
+
+          section_node.append_class('footnotes')
         else
+          return doc unless first_footnote = doc.at_xpath(XPATH_SECTION_OLD)
+          return doc unless first_footnote.parent
+
           first_footnote.parent.wrap('<section class="footnotes">')
         end
 
         rand_suffix = "-#{random_number}"
         modified_footnotes = {}
 
-        doc.xpath(XPATH_FOOTNOTE).each do |link_node|
+        xpath_footnote = if Feature.enabled?(:use_cmark_renderer)
+                           XPATH_FOOTNOTE
+                         else
+                           Gitlab::Utils::Nokogiri.css_to_xpath('sup > a[id]')
+                         end
+
+        doc.xpath(xpath_footnote).each do |link_node|
           if Feature.enabled?(:use_cmark_renderer)
             ref_num = link_node[:id].delete_prefix(FOOTNOTE_LINK_ID_PREFIX)
             ref_num.gsub!(/[[:punct:]]/, '\\\\\&')
@@ -58,7 +66,8 @@ module Banzai
             ref_num = link_node[:id].delete_prefix(FOOTNOTE_LINK_ID_PREFIX_OLD)
           end
 
-          node_xpath = Gitlab::Utils::Nokogiri.css_to_xpath("li[id=#{fn_id(ref_num)}]")
+          css = Feature.enabled?(:use_cmark_renderer) ? "section[data-footnotes] li[id=#{fn_id(ref_num)}]" : "li[id=#{fn_id(ref_num)}]"
+          node_xpath = Gitlab::Utils::Nokogiri.css_to_xpath(css)
           footnote_node = doc.at_xpath(node_xpath)
 
           if footnote_node || modified_footnotes[ref_num]
@@ -69,7 +78,6 @@ module Banzai
 
             # Sanitization stripped off class - add it back in
             link_node.parent.append_class('footnote-ref')
-            link_node['data-footnote-ref'] = nil if Feature.enabled?(:use_cmark_renderer)
 
             unless modified_footnotes[ref_num]
               footnote_node[:id] += rand_suffix
@@ -78,7 +86,6 @@ module Banzai
               if backref_node
                 backref_node[:href] += rand_suffix
                 backref_node.append_class('footnote-backref')
-                backref_node['data-footnote-backref'] = nil if Feature.enabled?(:use_cmark_renderer)
               end
 
               modified_footnotes[ref_num] = true

lib/banzai/filter/sanitization_filter.rb

@@ -28,6 +28,13 @@ module Banzai
         allowlist[:attributes]['li'] = %w[id]
         allowlist[:transformers].push(self.class.remove_non_footnote_ids)
 
+        if Feature.enabled?(:use_cmark_renderer)
+          # Allow section elements with data-footnotes attribute
+          allowlist[:elements].push('section')
+          allowlist[:attributes]['section'] = %w(data-footnotes)
+          allowlist[:attributes]['a'].push('data-footnote-ref', 'data-footnote-backref')
+        end
+
         allowlist
       end
 

spec/lib/banzai/filter/footnote_filter_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Banzai::Filter::FootnoteFilter do
   include FilterSpecHelper
+  using RSpec::Parameterized::TableSyntax
 
   # rubocop:disable Style/AsciiComments
   # first[^1] and second[^second] and third[^_😄_]
@@ -13,16 +14,16 @@ RSpec.describe Banzai::Filter::FootnoteFilter do
   # rubocop:enable Style/AsciiComments
   let(:footnote) do
     <<~EOF.strip_heredoc
-      <p>first<sup><a href="#fn-1" id="fnref-1">1</a></sup> and second<sup><a href="#fn-second" id="fnref-second">2</a></sup> and third<sup><a href="#fn-_%F0%9F%98%84_" id="fnref-_%F0%9F%98%84_">3</a></sup></p>
-
+      <p>first<sup><a href="#fn-1" id="fnref-1" data-footnote-ref>1</a></sup> and second<sup><a href="#fn-second" id="fnref-second" data-footnote-ref>2</a></sup> and third<sup><a href="#fn-_%F0%9F%98%84_" id="fnref-_%F0%9F%98%84_" data-footnote-ref>3</a></sup></p>
+      <section data-footnotes>
       <ol>
       <li id="fn-1">
-      <p>one <a href="#fnref-1" aria-label="Back to content">↩</a></p>
+      <p>one <a href="#fnref-1" aria-label="Back to content" data-footnote-backref>↩</a></p>
       </li>
       <li id="fn-second">
-      <p>two <a href="#fnref-second" aria-label="Back to content">↩</a></p>
+      <p>two <a href="#fnref-second" aria-label="Back to content" data-footnote-backref>↩</a></p>
       </li>\n<li id="fn-_%F0%9F%98%84_">
-      <p>three <a href="#fnref-_%F0%9F%98%84_" aria-label="Back to content">↩</a></p>
+      <p>three <a href="#fnref-_%F0%9F%98%84_" aria-label="Back to content" data-footnote-backref>↩</a></p>
       </li>
       </ol>
     EOF
@@ -30,19 +31,20 @@ RSpec.describe Banzai::Filter::FootnoteFilter do
 
   let(:filtered_footnote) do
     <<~EOF.strip_heredoc
-      <p>first<sup class="footnote-ref"><a href="#fn-1-#{identifier}" id="fnref-1-#{identifier}" data-footnote-ref="">1</a></sup> and second<sup class="footnote-ref"><a href="#fn-second-#{identifier}" id="fnref-second-#{identifier}" data-footnote-ref="">2</a></sup> and third<sup class="footnote-ref"><a href="#fn-_%F0%9F%98%84_-#{identifier}" id="fnref-_%F0%9F%98%84_-#{identifier}" data-footnote-ref="">3</a></sup></p>
-
-      <section class=\"footnotes\" data-footnotes><ol>
+      <p>first<sup class="footnote-ref"><a href="#fn-1-#{identifier}" id="fnref-1-#{identifier}" data-footnote-ref>1</a></sup> and second<sup class="footnote-ref"><a href="#fn-second-#{identifier}" id="fnref-second-#{identifier}" data-footnote-ref>2</a></sup> and third<sup class="footnote-ref"><a href="#fn-_%F0%9F%98%84_-#{identifier}" id="fnref-_%F0%9F%98%84_-#{identifier}" data-footnote-ref>3</a></sup></p>
+      <section data-footnotes class=\"footnotes\">
+      <ol>
       <li id="fn-1-#{identifier}">
-      <p>one <a href="#fnref-1-#{identifier}" aria-label="Back to content" class="footnote-backref" data-footnote-backref="">↩</a></p>
+      <p>one <a href="#fnref-1-#{identifier}" aria-label="Back to content" data-footnote-backref class="footnote-backref">↩</a></p>
       </li>
       <li id="fn-second-#{identifier}">
-      <p>two <a href="#fnref-second-#{identifier}" aria-label="Back to content" class="footnote-backref" data-footnote-backref="">↩</a></p>
+      <p>two <a href="#fnref-second-#{identifier}" aria-label="Back to content" data-footnote-backref class="footnote-backref">↩</a></p>
       </li>
       <li id="fn-_%F0%9F%98%84_-#{identifier}">
-      <p>three <a href="#fnref-_%F0%9F%98%84_-#{identifier}" aria-label="Back to content" class="footnote-backref" data-footnote-backref="">↩</a></p>
+      <p>three <a href="#fnref-_%F0%9F%98%84_-#{identifier}" aria-label="Back to content" data-footnote-backref class="footnote-backref">↩</a></p>
       </li>
-      </ol></section>
+      </ol>
+      </section>
     EOF
   end
 
@@ -52,7 +54,7 @@ RSpec.describe Banzai::Filter::FootnoteFilter do
     let(:identifier) { link_node[:id].delete_prefix('fnref-1-') }
 
     it 'properly adds the necessary ids and classes' do
-      expect(doc.to_html).to eq filtered_footnote
+      expect(doc.to_html).to eq filtered_footnote.strip
     end
 
     context 'using ruby-based HTML renderer' do
@@ -101,4 +103,21 @@ RSpec.describe Banzai::Filter::FootnoteFilter do
       end
     end
   end
+
+  context 'when detecting footnotes' do
+    where(:valid, :markdown) do
+      true   | "1. one[^1]\n[^1]: AbC"
+      true   | "1. one[^abc]\n[^abc]: AbC"
+      false  | '1. [one](#fnref-abc)'
+      false  | "1. one[^1]\n[^abc]: AbC"
+    end
+
+    with_them do
+      it 'detects valid footnotes' do
+        result = Banzai::Pipeline::FullPipeline.call(markdown, project: nil)
+
+        expect(result[:output].at_css('section.footnotes').present?).to eq(valid)
+      end
+    end
+  end
 end

spec/lib/banzai/pipeline/full_pipeline_spec.rb

@@ -43,26 +43,27 @@ RSpec.describe Banzai::Pipeline::FullPipeline do
 
     let(:filtered_footnote) do
       <<~EOF.strip_heredoc
-        <p dir="auto">first<sup class="footnote-ref"><a href="#fn-1-#{identifier}" id="fnref-1-#{identifier}" data-footnote-ref="">1</a></sup> and second<sup class="footnote-ref"><a href="#fn-%F0%9F%98%84second-#{identifier}" id="fnref-%F0%9F%98%84second-#{identifier}" data-footnote-ref="">2</a></sup> and twenty<sup class="footnote-ref"><a href="#fn-_twenty-#{identifier}" id="fnref-_twenty-#{identifier}" data-footnote-ref="">3</a></sup></p>
-
-        <section class="footnotes" data-footnotes><ol>
+        <p dir="auto">first<sup class="footnote-ref"><a href="#fn-1-#{identifier}" id="fnref-1-#{identifier}" data-footnote-ref>1</a></sup> and second<sup class="footnote-ref"><a href="#fn-%F0%9F%98%84second-#{identifier}" id="fnref-%F0%9F%98%84second-#{identifier}" data-footnote-ref>2</a></sup> and twenty<sup class="footnote-ref"><a href="#fn-_twenty-#{identifier}" id="fnref-_twenty-#{identifier}" data-footnote-ref>3</a></sup></p>
+        <section data-footnotes class="footnotes">
+        <ol>
         <li id="fn-1-#{identifier}">
-        <p>one <a href="#fnref-1-#{identifier}" aria-label="Back to content" class="footnote-backref" data-footnote-backref=""><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        <p>one <a href="#fnref-1-#{identifier}" data-footnote-backref aria-label="Back to content" 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="fn-%F0%9F%98%84second-#{identifier}">
-        <p>two <a href="#fnref-%F0%9F%98%84second-#{identifier}" aria-label="Back to content" class="footnote-backref" data-footnote-backref=""><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        <p>two <a href="#fnref-%F0%9F%98%84second-#{identifier}" data-footnote-backref aria-label="Back to content" 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="fn-_twenty-#{identifier}">
-        <p>twenty <a href="#fnref-_twenty-#{identifier}" aria-label="Back to content" class="footnote-backref" data-footnote-backref=""><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        <p>twenty <a href="#fnref-_twenty-#{identifier}" data-footnote-backref aria-label="Back to content" 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>
+        </ol>
+        </section>
       EOF
     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
+      expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote.strip
     end
 
     context 'using ruby-based HTML renderer' do