Banzai::Filter::ExternalLinkFilter use XPath18582-banzai-filter-external-link-filter

3 files changed, 26 insertions, 22 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 39532e88138..b886668d89d 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -119,6 +119,7 @@ v 8.8.5
   - Forbid scripting for wiki files
   - Only show notes through JSON on confidential issues that the user has access to
   - Banzai::Filter::UploadLinkFilter use XPath instead CSS expressions
+  - Banzai::Filter::ExternalLinkFilter use XPath instead CSS expressions
 
 v 8.8.4
   - Fix LDAP-based login for users with 2FA enabled. !4493
diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb
index f73ecfc9418..0a29c547a4d 100644
--- a/lib/banzai/filter/external_link_filter.rb
+++ b/lib/banzai/filter/external_link_filter.rb
@@ -3,17 +3,8 @@ module Banzai
     # HTML Filter to modify the attributes of external links
     class ExternalLinkFilter < HTML::Pipeline::Filter
       def call
-        doc.search('a').each do |node|
-          link = node.attr('href')
-
-          next unless link
-
-          # Skip non-HTTP(S) links
-          next unless link.start_with?('http')
-
-          # Skip internal links
-          next if link.start_with?(internal_url)
-
+        # Skip non-HTTP(S) links and internal links
+        doc.xpath("descendant-or-self::a[starts-with(@href, 'http') and not(starts-with(@href, '#{internal_url}'))]").each do |node|
           node.set_attribute('rel', 'nofollow noreferrer')
           node.set_attribute('target', '_blank')
         end
diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb
index f4c5c621bd0..695a5bc6fd4 100644
--- a/spec/lib/banzai/filter/external_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/external_link_filter_spec.rb
@@ -19,19 +19,31 @@ describe Banzai::Filter::ExternalLinkFilter, lib: true do
     expect(filter(act).to_html).to eq exp
   end
 
-  it 'adds rel="nofollow" to external links' do
-    act = %q(<a href="https://google.com/">Google</a>)
-    doc = filter(act)
-
-    expect(doc.at_css('a')).to have_attribute('rel')
-    expect(doc.at_css('a')['rel']).to include 'nofollow'
+  context 'for root links on document' do
+    let(:doc) { filter %q(<a href="https://google.com/">Google</a>) }
+
+    it 'adds rel="nofollow" to external links' do
+      expect(doc.at_css('a')).to have_attribute('rel')
+      expect(doc.at_css('a')['rel']).to include 'nofollow'
+    end
+
+    it 'adds rel="noreferrer" to external links' do
+      expect(doc.at_css('a')).to have_attribute('rel')
+      expect(doc.at_css('a')['rel']).to include 'noreferrer'
+    end
   end
 
-  it 'adds rel="noreferrer" to external links' do
-    act = %q(<a href="https://google.com/">Google</a>)
-    doc = filter(act)
+  context 'for nested links on document' do
+    let(:doc) { filter %q(<p><a href="https://google.com/">Google</a></p>) }
+
+    it 'adds rel="nofollow" to external links' do
+      expect(doc.at_css('a')).to have_attribute('rel')
+      expect(doc.at_css('a')['rel']).to include 'nofollow'
+    end
 
-    expect(doc.at_css('a')).to have_attribute('rel')
-    expect(doc.at_css('a')['rel']).to include 'noreferrer'
+    it 'adds rel="noreferrer" to external links' do
+      expect(doc.at_css('a')).to have_attribute('rel')
+      expect(doc.at_css('a')['rel']).to include 'noreferrer'
+    end
   end
 end