Refactoring and addressing review comments

and additional spec

2 files changed, 25 insertions, 16 deletions

diff --git a/lib/banzai/filter/footnote_filter.rb b/lib/banzai/filter/footnote_filter.rb
index dc00ca73147..97527976437 100644
--- a/lib/banzai/filter/footnote_filter.rb
+++ b/lib/banzai/filter/footnote_filter.rb
@@ -16,19 +16,24 @@ module Banzai
     # 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
+      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=fn1]')
+        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('fnref')
-          footnote_node = doc.at_css("li[id=fn#{ref_num}]")
-          backref_node  = footnote_node.at_css("a[href=\"#fnref#{ref_num}\"]")
+          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
@@ -50,6 +55,14 @@ module Banzai
       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
diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb
index 16accefa850..edc053638a8 100644
--- a/lib/banzai/filter/sanitization_filter.rb
+++ b/lib/banzai/filter/sanitization_filter.rb
@@ -8,10 +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)/.freeze
-      FOOTNOTE_LINK_REFERENCE_PATTERN = /\Afnref\d+\z/.freeze
-      FOOTNOTE_LI_REFERENCE_PATTERN   = /\Afn\d+\z/.freeze
+      UNSAFE_PROTOCOLS        = %w(data javascript vbscript).freeze
+      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/.freeze
 
       def whitelist
         strong_memoize(:whitelist) do
@@ -47,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
@@ -60,10 +57,9 @@ module Banzai
         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)
-
-        # ...but remove any `id` properties not matching for footnotes
         whitelist[:transformers].push(self.class.remove_non_footnote_ids)
 
         whitelist
@@ -129,8 +125,8 @@ module Banzai
             return unless node.name == 'a' || node.name == 'li'
             return unless node.has_attribute?('id')
 
-            return if node.name == 'a' && node['id'] =~ FOOTNOTE_LINK_REFERENCE_PATTERN
-            return if node.name == 'li' && node['id'] =~ FOOTNOTE_LI_REFERENCE_PATTERN
+            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