From 08bfec57c3e17225a93a33e464a898a14741d5c4 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Tue, 11 Dec 2018 17:20:06 +0100
Subject: Set URL rel attribute for broken URLs

It's possible that URI fails to parse a link, but browsers
still recognize given URL as a link, we should make sure
that 'rel' attribute is set also in this case.
---
 changelogs/unreleased/security-master-url-rel.yml   |  5 +++++
 lib/banzai/filter/external_link_filter.rb           | 12 ++++++------
 spec/lib/banzai/filter/external_link_filter_spec.rb |  8 ++++----
 3 files changed, 15 insertions(+), 10 deletions(-)
 create mode 100644 changelogs/unreleased/security-master-url-rel.yml

diff --git a/changelogs/unreleased/security-master-url-rel.yml b/changelogs/unreleased/security-master-url-rel.yml
new file mode 100644
index 00000000000..75f599f6bcd
--- /dev/null
+++ b/changelogs/unreleased/security-master-url-rel.yml
@@ -0,0 +1,5 @@
+---
+title: Set URL rel attribute for broken URLs.
+merge_request:
+author:
+type: security
diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb
index 2e6d742de27..4f60b6f84c6 100644
--- a/lib/banzai/filter/external_link_filter.rb
+++ b/lib/banzai/filter/external_link_filter.rb
@@ -9,11 +9,10 @@ module Banzai
       def call
         links.each do |node|
           uri = uri(node['href'].to_s)
-          next unless uri
 
-          node.set_attribute('href', uri.to_s)
+          node.set_attribute('href', uri.to_s) if uri
 
-          if SCHEMES.include?(uri.scheme) && external_url?(uri)
+          if SCHEMES.include?(uri&.scheme) && !internal_url?(uri)
             node.set_attribute('rel', 'nofollow noreferrer noopener')
             node.set_attribute('target', '_blank')
           end
@@ -35,11 +34,12 @@ module Banzai
         doc.xpath(query)
       end
 
-      def external_url?(uri)
+      def internal_url?(uri)
+        return false if uri.nil?
         # Relative URLs miss a hostname
-        return false unless uri.hostname
+        return true unless uri.hostname
 
-        uri.hostname != internal_url.hostname
+        uri.hostname == internal_url.hostname
       end
 
       def internal_url
diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb
index 2a3c0cd78b8..e6dae8d5382 100644
--- a/spec/lib/banzai/filter/external_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/external_link_filter_spec.rb
@@ -49,16 +49,16 @@ describe Banzai::Filter::ExternalLinkFilter do
   end
 
   context 'for invalid urls' do
-    it 'skips broken hrefs' do
+    it 'adds rel and target attributes to broken hrefs' do
       doc = filter %q(<p><a href="don't crash on broken urls">Google</a></p>)
-      expected = %q(<p><a href="don't%20crash%20on%20broken%20urls">Google</a></p>)
+      expected = %q(<p><a href="don't%20crash%20on%20broken%20urls" rel="nofollow noreferrer noopener" target="_blank">Google</a></p>)
 
       expect(doc.to_html).to eq(expected)
     end
 
-    it 'skips improperly formatted mailtos' do
+    it 'adds rel and target to improperly formatted mailtos' do
       doc = filter %q(<p><a href="mailto://jblogs@example.com">Email</a></p>)
-      expected = %q(<p><a href="mailto://jblogs@example.com">Email</a></p>)
+      expected = %q(<p><a href="mailto://jblogs@example.com" rel="nofollow noreferrer noopener" target="_blank">Email</a></p>)
 
       expect(doc.to_html).to eq(expected)
     end
-- 
cgit v1.2.1