Speed up the overridden `link_to` helper

Only bothers to check the provided link's external status if it's a
String that doesn't begin with a path or anchor character.

2 files changed, 38 insertions, 32 deletions

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index b5b0015542c..6aef82354ab 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -236,33 +236,35 @@ module ApplicationHelper
     Gitlab::MarkdownHelper.gitlab_markdown?(filename)
   end
 
-  def link_to(name = nil, options = nil, html_options = nil, &block)
-    begin
-      uri = URI(options)
-      host = uri.host
-      absolute_uri = uri.absolute?
-    rescue URI::InvalidURIError, ArgumentError
-      host = nil
-      absolute_uri = nil
-    end
-
-    # Add 'nofollow' only to external links
-    if host && host != Gitlab.config.gitlab.host && absolute_uri
-      if html_options
-        if html_options[:rel]
-          html_options[:rel] << ' nofollow'
-        else
-          html_options.merge!(rel: 'nofollow')
-        end
-      else
-        html_options = Hash.new
-        html_options[:rel] = 'nofollow'
+  # Overrides ActionView::Helpers::UrlHelper#link_to to add `rel="nofollow"` to
+  # external links
+  def link_to(name = nil, options = nil, html_options = {})
+    if options.kind_of?(String)
+      if options[0] != '/' && options[0] != '#'
+        html_options = add_nofollow(options, html_options)
       end
     end
 
     super
   end
 
+  # Add `"rel=nofollow"` to external links
+  #
+  # link         - String link to check
+  # html_options - Hash of `html_options` passed to `link_to`
+  #
+  # Returns `html_options`, adding `rel: nofollow` for external links
+  def add_nofollow(link, html_options = {})
+    uri = URI(link)
+
+    if uri && uri.absolute? && uri.host != Gitlab.config.gitlab.host
+      rel = html_options.fetch(:rel, '')
+      html_options[:rel] = (rel + ' nofollow').strip
+    end
+
+    html_options
+  end
+
   def escaped_autolink(text)
     auto_link ERB::Util.html_escape(text), link: :urls
   end
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 4c11709ed6e..015a66f7fa0 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -225,25 +225,29 @@ describe ApplicationHelper do
   end
 
   describe 'link_to' do
-
     it 'should not include rel=nofollow for internal links' do
-      expect(link_to('Home', root_path)).to eq("<a href=\"/\">Home</a>")
+      expect(link_to('Home', root_path)).to eq('<a href="/">Home</a>')
     end
 
     it 'should include rel=nofollow for external links' do
-      expect(link_to('Example', 'http://www.example.com')).to eq("<a href=\"http://www.example.com\" rel=\"nofollow\">Example</a>")
+      expect(link_to('Example', 'http://www.example.com')).
+        to eq '<a href="http://www.example.com" rel="nofollow">Example</a>'
+    end
+
+    it 'should include rel=nofollow for external links and honor existing html_options' do
+      expect(link_to('Example', 'http://www.example.com', class: 'toggle', data: {toggle: 'dropdown'}))
+        .to eq '<a class="toggle" data-toggle="dropdown" href="http://www.example.com" rel="nofollow">Example</a>'
     end
 
-    it 'should include re=nofollow for external links and honor existing html_options' do
-      expect(
-        link_to('Example', 'http://www.example.com', class: 'toggle', data: {toggle: 'dropdown'})
-      ).to eq("<a class=\"toggle\" data-toggle=\"dropdown\" href=\"http://www.example.com\" rel=\"nofollow\">Example</a>")
+    it 'should include rel=nofollow for external links and preserve other rel values' do
+      expect(link_to('Example', 'http://www.example.com', rel: 'noreferrer'))
+        .to eq '<a href="http://www.example.com" rel="noreferrer nofollow">Example</a>'
     end
 
-    it 'should include rel=nofollow for external links and preserver other rel values' do
-      expect(
-        link_to('Example', 'http://www.example.com', rel: 'noreferrer')
-      ).to eq("<a href=\"http://www.example.com\" rel=\"noreferrer nofollow\">Example</a>")
+    it 'should not include rel=nofollow for external links on the same host as GitLab' do
+      expect(Gitlab.config.gitlab).to receive(:host).and_return('example.foo')
+      expect(link_to('Example', 'http://example.foo/bar')).
+        to eq '<a href="http://example.foo/bar">Example</a>'
     end
   end