Avoid string manipulation when autolinking.

Instead, let's use a Loofah scrubber which will create DOM nodes directly. This should be faster and is a tiny bit simpler, as well as removing a potential HTML injection vector. Also, add "noreferrer" to all `mailto:` links (already present on URLs).
2025-12-29 10:35:28 -05:00
parent 55257c83f1
commit 4579f7cd61
3 changed files with 90 additions and 61 deletions
@@ -1,64 +1,5 @@
 module HtmlHelper
-  include ERB::Util
-
-  EXCLUDE_PUNCTUATION = %(.?,:!;"'<>)
-  EXCLUDE_PUNCTUATION_REGEX = /[#{Regexp.escape(EXCLUDE_PUNCTUATION)}]+\z/
-
  def format_html(html)
-    fragment = Nokogiri::HTML5.fragment(html)
-
-    auto_link(fragment)
-
-    fragment.to_html.html_safe
+    Loofah::HTML5::DocumentFragment.parse(html).scrub!(AutoLinkScrubber.new).to_html.html_safe
  end
-
-  private
-    EXCLUDED_ELEMENTS = %w[ a figcaption pre code ]
-    EMAIL_AUTOLINK_REGEXP = /\b[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\b/
-    URL_REGEXP = URI::DEFAULT_PARSER.make_regexp(%w[http https])
-
-    def auto_link(fragment)
-      fragment.traverse do |node|
-        next unless auto_linkable_node?(node)
-
-        # Take care to escape the html text node, so that the subsequent Nokogiri re-parse doesn't
-        # create tags where there aren't any.
-        content = h(node.text)
-        linked_content = content.dup
-
-        auto_link_urls(linked_content)
-        auto_link_emails(linked_content)
-
-        if linked_content != content
-          node.replace(Nokogiri::HTML5.fragment(linked_content))
-        end
-      end
-    end
-
-    def auto_linkable_node?(node)
-      node.text? && node.ancestors.none? { |ancestor| EXCLUDED_ELEMENTS.include?(ancestor.name) }
-    end
-
-    def auto_link_urls(linked_content)
-      linked_content.gsub!(URL_REGEXP) do |match|
-        url, trailing_punct = extract_url_and_punctuation(match)
-        %(<a href="#{url}" rel="noreferrer">#{url}</a>#{trailing_punct})
-      end
-    end
-
-    def extract_url_and_punctuation(url_match)
-      url_match = CGI.unescapeHTML(url_match)
-      if match = url_match.match(EXCLUDE_PUNCTUATION_REGEX)
-        len = match[0].length
-        [ url_match[..-(len+1)], url_match[-len..] ]
-      else
-        [ url_match, "" ]
-      end
-    end
-
-    def auto_link_emails(text)
-      text.gsub!(EMAIL_AUTOLINK_REGEXP) do |match|
-        %(<a href="mailto:#{match}">#{match}</a>)
-      end
-    end
 end
@@ -0,0 +1,79 @@
+# Loofah scrubber to auto-link URLs and email addresses in HTML text nodes.
+#
+# This scrubber does not perform HTML sanitization; it's assumed that the input is already sanitized
+# (for example, ActionText rich text).
+class AutoLinkScrubber < Loofah::Scrubber
+  EXCLUDED_ELEMENTS = %w[a figcaption pre code].freeze
+
+  # This regexp is similar to URI::MailTo::EMAIL_REGEXP but uses \b word boundaries instead of \A/\z
+  # anchors, allowing it to match email addresses embedded within longer strings.
+  #
+  # It's named EMAIL_AUTOLINK_REGEXP (not EMAIL_REGEXP) to avoid confusing Brakeman's imprecise
+  # constant lookup, which otherwise assumes Identity's email validation uses this \b-anchored pattern.
+  # See https://github.com/presidentbeef/brakeman/pull/1981
+  EMAIL_AUTOLINK_REGEXP = /\b[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\b/
+  URL_REGEXP = URI::DEFAULT_PARSER.make_regexp(%w[http https])
+  AUTOLINK_REGEXP = /(?<url>#{URL_REGEXP})|(?<email>#{EMAIL_AUTOLINK_REGEXP})/
+
+  TRAILING_PUNCTUATION = %(.?,:!;"'<>)
+  TRAILING_PUNCTUATION_REGEXP = /[#{Regexp.escape(TRAILING_PUNCTUATION)}]+\z/
+
+  def initialize
+    @direction = :top_down
+  end
+
+  def scrub(node)
+    return Loofah::Scrubber::STOP if EXCLUDED_ELEMENTS.include?(node.name)
+
+    if node.text?
+      replacement = autolink_text_node(node)
+      node.replace(replacement) if replacement
+    end
+
+    Loofah::Scrubber::CONTINUE
+  end
+
+  private
+    def autolink_text_node(node)
+      text = node.text
+      links = find_links(text)
+
+      return nil if links.empty?
+
+      doc = node.document
+      nodes = Nokogiri::XML::NodeSet.new(doc)
+      pos = 0
+
+      links.each do |link|
+        nodes << doc.create_text_node(text[pos...link[:start]]) if link[:start] > pos
+        nodes << doc.create_element("a", link[:text], href: link[:href], rel: "noreferrer")
+        pos = link[:start] + link[:length]
+      end
+      nodes << doc.create_text_node(text[pos..]) if pos < text.length
+
+      nodes
+    end
+
+    def find_links(text)
+      links = []
+
+      text.scan(AUTOLINK_REGEXP) do
+        match_data = Regexp.last_match
+        start_pos = match_data.begin(0)
+
+        if match_data[:url]
+          url = clean_url(match_data[:url])
+          links << { start: start_pos, length: url.length, text: url, href: url }
+        else
+          email = match_data[:email]
+          links << { start: start_pos, length: email.length, text: email, href: "mailto:#{email}" }
+        end
+      end
+
+      links
+    end
+
+    def clean_url(url)
+      url.sub(TRAILING_PUNCTUATION_REGEXP, "")
+    end
+end
@@ -60,6 +60,15 @@ class HtmlHelperTest < ActionView::TestCase
      format_html(%(&lt;img src=&quot;https://example.com/&quot;!&gt;))
  end

+  test "make sure the linked content is properly sanitized" do
+    # https://hackerone.com/reports/3481093
+    result = format_html(%(https://google.com/\"&gt;test&lt;/a&gt;&lt;input&gt;&lt;/input&gt;))
+    assert_no_match(/<input>/i, result, "should not create an input element")
+
+    result = format_html(%(https://google.com/\"&gt;&lt;script&gt;alert('xss')&lt;/script&gt;))
+    assert_no_match(/<script>/i, result, "should not create a script element")
+  end
+
  test "handle URLs with query parameters" do
    # use assert_equal and not assert_equal_html to make sure we're getting entities correct
    assert_equal \
@@ -79,7 +88,7 @@ class HtmlHelperTest < ActionView::TestCase

  test "convert email addresses into mailto links" do
    assert_equal_html \
-      %(<p>Contact us at <a href="mailto:support@example.com">support@example.com</a></p>),
+      %(<p>Contact us at <a href="mailto:support@example.com" rel="noreferrer">support@example.com</a></p>),
      format_html("<p>Contact us at support@example.com</p>")
  end