From c4185c13396c7fb38b87fa5852ec3369c6eed0fc Mon Sep 17 00:00:00 2001
From: Khadija Sidhpuri <59063821+squarebat@users.noreply.github.com>
Date: Tue, 20 Jul 2021 19:40:34 +0530
Subject: [PATCH] Fix shorten_urls to not remove formatting and image tags
 (#14262)

* fix: shorten_urls to only shorten URLs

* revert: changes to schema.rb

* modify: test to aggregate failures

Co-authored-by: rhymes <github@rhymes.dev>

* fix: shorten_urls to only strip urls

* improve: regex and variable assignments

* remove: unnecessary var assignment

Co-authored-by: rhymes <github@rhymes.dev>
---
 app/models/comment.rb       | 17 +++++++++++++++--
 spec/models/comment_spec.rb | 23 +++++++++++++++++++----
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/app/models/comment.rb b/app/models/comment.rb
index c3c460815..b9d0992c2 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -12,6 +12,14 @@ class Comment < ApplicationRecord
   TITLE_DELETED = "[deleted]".freeze
   TITLE_HIDDEN = "[hidden by post author]".freeze
 
+  URI_REGEXP = %r{
+    \A
+    (?:https?://)?  # optional scheme
+    .+?             # host
+    (?::\d+)?       # optional port
+    \z
+  }x.freeze
+
   # The date that we began limiting the number of user mentions in a comment.
   MAX_USER_MENTION_LIVE_AT = Time.utc(2021, 3, 12).freeze
 
@@ -202,9 +210,14 @@ class Comment < ApplicationRecord
   def shorten_urls!
     doc = Nokogiri::HTML.fragment(processed_html)
     doc.css("a").each do |anchor|
-      unless anchor.to_s.include?("<img") || anchor.to_s.include?("<del") || anchor.attr("class")&.include?("ltag")
-        anchor.content = strip_url(anchor.content) unless anchor.to_s.include?("<img") # rubocop:disable Style/SoleNestedConditional
+      next if anchor.inner_html.include?("<img")
+
+      urls = anchor.content.scan(URI_REGEXP).flatten
+      anchor_content = anchor.content
+      urls.each do |url|
+        anchor_content.sub!(/#{Regexp.escape(url)}/, strip_url(url))
       end
+      anchor.inner_html = anchor.inner_html.sub!(/#{Regexp.escape(anchor.content)}/, anchor_content)
     end
     self.processed_html = doc.to_html.html_safe # rubocop:disable Rails/OutputSafety
   end
diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb
index 84508991f..3a849fa53 100644
--- a/spec/models/comment_spec.rb
+++ b/spec/models/comment_spec.rb
@@ -184,14 +184,29 @@ RSpec.describe Comment, type: :model do
         expect(comment.processed_html.include?("Hello <a")).to be(true)
       end
 
-      it "shortens long urls" do
-        comment.body_markdown = "Hello https://longurl.com/#{'x' * 100}?#{'y' * 100}"
+      # rubocop:disable RSpec/ExampleLength
+      it "shortens long urls without removing formatting", :aggregate_failures do
+        long_url = "https://longurl.com/#{'x' * 100}?#{'y' * 100}"
+        comment.body_markdown = "Hello #{long_url}"
         comment.validate!
-        expect(comment.processed_html.include?("...</a>")).to be(true)
+        expect(comment.processed_html.include?("...")).to be(true)
         expect(comment.processed_html.size < 450).to be(true)
+
+        comment.body_markdown = "Hello this is [**#{long_url}**](#{long_url})"
+        comment.validate!
+        expect(comment.processed_html.include?("...</strong>")).to be(true)
+
+        long_text = "Does not strip out text without urls #{'x' * 200}#{'y' * 200}"
+        comment.body_markdown = "[**#{long_text}**](#{long_url})"
+        comment.validate!
+        expect(comment.processed_html.include?("...")).to be(false)
+
+        image_url = "https://i.picsum.photos/id/126/500/500.jpg?hmac=jNnQC44a_UR01TNuazfKROio0T_HaZVg0ikfR0d_xWY"
+        comment.body_markdown = "Hello ![Alt-text](#{image_url})"
+        comment.validate!
+        expect(comment.processed_html.include?("<img src=\"#{image_url}\"")).to be(true)
       end
 
-      # rubocop:disable RSpec/ExampleLength
       it "adds timestamp url if commentable has video and timestamp", :aggregate_failures do
         article.video = "https://example.com"