FIX: Make sure rel attributes are correctly set. (#10645)

FIX: Make sure rel attributes are correctly set. (#10645)

We must guarantee that “rel=noopener” was set if “target=_blank” is present, which is not always the case for trusted users. Also, if the link contains the “nofollow” attribute, it has to have the “ugc” attribute as well.

diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
index f255161..6f11b38 100644
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@@ -656,9 +656,8 @@ class CookedPostProcessor
   end
 
   def enforce_nofollow
-    if !@omit_nofollow && SiteSetting.add_rel_nofollow_to_user_content
-      PrettyText.add_rel_nofollow_to_user_content(@doc)
-    end
+    add_nofollow = !@omit_nofollow && SiteSetting.add_rel_nofollow_to_user_content
+    PrettyText.add_rel_attributes_to_user_content(@doc, add_nofollow)
   end
 
   def pull_hotlinked_images
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index 743b28d..ec0936e 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -269,9 +269,8 @@ module PrettyText
 
     doc = Nokogiri::HTML5.fragment(sanitized)
 
-    if !options[:omit_nofollow] && SiteSetting.add_rel_nofollow_to_user_content
-      add_rel_nofollow_to_user_content(doc)
-    end
+    add_nofollow = !options[:omit_nofollow] && SiteSetting.add_rel_nofollow_to_user_content
+    add_rel_attributes_to_user_content(doc, add_nofollow)
 
     if SiteSetting.enable_mentions
       add_mentions(doc, user_id: opts[:user_id])
@@ -284,7 +283,7 @@ module PrettyText
     loofah_fragment.scrub!(scrubber).to_html
   end
 
-  def self.add_rel_nofollow_to_user_content(doc)
+  def self.add_rel_attributes_to_user_content(doc, add_nofollow)
     allowlist = []
 
     domains = SiteSetting.exclude_rel_nofollow_domains
@@ -293,22 +292,21 @@ module PrettyText
     site_uri = nil
     doc.css("a").each do |l|
       href = l["href"].to_s
+      l["rel"] = "noopener" if l["target"] == "_blank"
+
       begin
         uri = URI(UrlHelper.encode_component(href))
         site_uri ||= URI(Discourse.base_url)
 
-        if !uri.host.present? ||
-           uri.host == site_uri.host ||
-           uri.host.ends_with?(".#{site_uri.host}") ||
-           allowlist.any? { |u| uri.host == u || uri.host.ends_with?(".#{u}") }
-          # we are good no need for nofollow
-          l.remove_attribute("rel")
-        else
-          l["rel"] = "nofollow noopener"
-        end
+        same_domain = !uri.host.present? ||
+          uri.host == site_uri.host ||
+          uri.host.ends_with?(".#{site_uri.host}") ||
+          allowlist.any? { |u| uri.host == u || uri.host.ends_with?(".#{u}") }
+
+        l["rel"] = "noopener nofollow ugc" if add_nofollow && !same_domain
       rescue URI::Error
         # add a nofollow anyway
-        l["rel"] = "nofollow noopener"
+        l["rel"] = "noopener nofollow ugc"
       end
     end
   end
diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb
index 424ec99..143536b 100644
--- a/spec/components/cooked_post_processor_spec.rb
+++ b/spec/components/cooked_post_processor_spec.rb
@@ -237,13 +237,13 @@ describe CookedPostProcessor do
             count: 1
           )
 
-          expect(html).to have_tag("a[rel='nofollow noopener']")
+          expect(html).to have_tag("a[rel='noopener nofollow ugc']")
         end
 
         it 'removes nofollow if user is staff/tl3' do
           cpp = CookedPostProcessor.new(staff_post, invalidate_oneboxes: true)
           cpp.post_process
-          expect(cpp.html).to_not have_tag("a[rel='nofollow noopener']")
+          expect(cpp.html).to_not have_tag("a[rel='noopener nofollow ugc']")
         end
       end
     end
@@ -1102,7 +1102,7 @@ describe CookedPostProcessor do
       SiteSetting.add_rel_nofollow_to_user_content = false
       Oneboxer.expects(:onebox)
         .with("http://www.youtube.com/watch?v=9bZkp7q19f0", invalidate_oneboxes: true, user_id: nil, category_id: post.topic.category_id)
-        .returns('<aside class="onebox"><a href="https://www.youtube.com/watch?v=9bZkp7q19f0" rel="nofollow noopener ugc">GANGNAM STYLE</a></aside>')
+        .returns('<aside class="onebox"><a href="https://www.youtube.com/watch?v=9bZkp7q19f0" rel="noopener nofollow ugc">GANGNAM STYLE</a></aside>')
       cpp.post_process_oneboxes
     end
 
@@ -1123,7 +1123,7 @@ describe CookedPostProcessor do
       SiteSetting.tl3_links_no_follow = false
       Oneboxer.expects(:onebox)
         .with("http://www.youtube.com/watch?v=9bZkp7q19f0", invalidate_oneboxes: true, user_id: nil, category_id: post.topic.category_id)
-        .returns('<aside class="onebox"><a href="https://www.youtube.com/watch?v=9bZkp7q19f0" rel="nofollow ugc noopener">GANGNAM STYLE</a></aside>')
+        .returns('<aside class="onebox"><a href="https://www.youtube.com/watch?v=9bZkp7q19f0" rel="noopener nofollow ugc">GANGNAM STYLE</a></aside>')
       cpp.post_process_oneboxes
     end
 
@@ -1204,7 +1204,7 @@ describe CookedPostProcessor do
       expect(cpp.html).to match_html <<~HTML.rstrip
         <p><a href="//test.localhost/#{upload_path}/original/2X/2345678901234567.jpg">Link</a><br>
         <img src="//test.localhost/#{upload_path}/original/1X/1234567890123456.jpg"><br>
-        <a href="http://www.google.com" rel="nofollow noopener">Google</a><br>
+        <a href="http://www.google.com" rel="noopener nofollow ugc">Google</a><br>
         <img src="http://foo.bar/image.png"><br>
         <a class="attachment" href="//test.localhost/#{upload_path}/original/1X/af2c2618032c679333bebf745e75f9088748d737.txt">text.txt</a> (20 Bytes)<br>
         <img src="//test.localhost/images/emoji/twitter/smile.png?v=#{Emoji::EMOJI_VERSION}" title=":smile:" class="emoji only-emoji" alt=":smile:"></p>
@@ -1219,7 +1219,7 @@ describe CookedPostProcessor do
         expect(cpp.html).to match_html <<~HTML.rstrip
           <p><a href="//my.cdn.com/#{upload_path}/original/2X/2345678901234567.jpg">Link</a><br>
           <img src="//my.cdn.com/#{upload_path}/original/1X/1234567890123456.jpg"><br>
-          <a href="http://www.google.com" rel="nofollow noopener">Google</a><br>
+          <a href="http://www.google.com" rel="noopener nofollow ugc">Google</a><br>
           <img src="http://foo.bar/image.png"><br>
           <a class="attachment" href="//my.cdn.com/#{upload_path}/original/1X/af2c2618032c679333bebf745e75f9088748d737.txt">text.txt</a> (20 Bytes)<br>
           <img src="//my.cdn.com/images/emoji/twitter/smile.png?v=#{Emoji::EMOJI_VERSION}" title=":smile:" class="emoji only-emoji" alt=":smile:"></p>
@@ -1232,7 +1232,7 @@ describe CookedPostProcessor do
         expect(cpp.html).to match_html <<~HTML.rstrip
           <p><a href="https://my.cdn.com/#{upload_path}/original/2X/2345678901234567.jpg">Link</a><br>
           <img src="https://my.cdn.com/#{upload_path}/original/1X/1234567890123456.jpg"><br>
-          <a href="http://www.google.com" rel="nofollow noopener">Google</a><br>
+          <a href="http://www.google.com" rel="noopener nofollow ugc">Google</a><br>
           <img src="http://foo.bar/image.png"><br>
           <a class="attachment" href="https://my.cdn.com/#{upload_path}/original/1X/af2c2618032c679333bebf745e75f9088748d737.txt">text.txt</a> (20 Bytes)<br>
           <img src="https://my.cdn.com/images/emoji/twitter/smile.png?v=#{Emoji::EMOJI_VERSION}" title=":smile:" class="emoji only-emoji" alt=":smile:"></p>
@@ -1246,7 +1246,7 @@ describe CookedPostProcessor do
         expect(cpp.html).to match_html <<~HTML.rstrip
           <p><a href="//my.cdn.com/#{upload_path}/original/2X/2345678901234567.jpg">Link</a><br>
           <img src="//my.cdn.com/#{upload_path}/original/1X/1234567890123456.jpg"><br>
-          <a href="http://www.google.com" rel="nofollow noopener">Google</a><br>
+          <a href="http://www.google.com" rel="noopener nofollow ugc">Google</a><br>
           <img src="http://foo.bar/image.png"><br>
           <a class="attachment" href="//test.localhost/#{upload_path}/original/1X/af2c2618032c679333bebf745e75f9088748d737.txt">text.txt</a> (20 Bytes)<br>
           <img src="//my.cdn.com/images/emoji/twitter/smile.png?v=#{Emoji::EMOJI_VERSION}" title=":smile:" class="emoji only-emoji" alt=":smile:"></p>

[... diff too long, it was truncated ...]

GitHub sha: efb9fd6a

1 Like

This commit appears in #10645 which was approved by eviltrout. It was merged by romanrizzi.