Merge branch '31887-remove-images-from-todos' into 'master'

Resolve "Remove images from todos"

Closes #31887

See merge request gitlab-org/gitlab-ce!21704

3 files changed, 43 insertions, 7 deletions

diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb
index f2cd676bb1b..0d638b850b4 100644
--- a/app/helpers/markup_helper.rb
+++ b/app/helpers/markup_helper.rb
@@ -74,14 +74,21 @@ module MarkupHelper
   # the tag contents are truncated without removing the closing tag.
   def first_line_in_markdown(object, attribute, max_chars = nil, options = {})
     md = markdown_field(object, attribute, options)
+    return nil unless md.present?
 
-    text = truncate_visible(md, max_chars || md.length) if md.present?
+    tags = %w(a gl-emoji b pre code p span)
+    tags << 'img' if options[:allow_images]
 
-    sanitize(
+    text = truncate_visible(md, max_chars || md.length)
+    text = sanitize(
       text,
-      tags: %w(a img gl-emoji b pre code p span),
+      tags: tags,
       attributes: Rails::Html::WhiteListSanitizer.allowed_attributes + ['style', 'data-src', 'data-name', 'data-unicode-version']
     )
+
+    # since <img> tags are stripped, this can leave empty <a> tags hanging around
+    # (as our markdown wraps images in links)
+    options[:allow_images] ? text : strip_empty_link_tags(text).html_safe
   end
 
   def markdown(text, context = {})
@@ -235,6 +242,16 @@ module MarkupHelper
     end
   end
 
+  def strip_empty_link_tags(text)
+    scrubber = Loofah::Scrubber.new do |node|
+      node.remove if node.name == 'a' && node.content.blank?
+    end
+
+    # Use `Loofah` directly instead of `sanitize`
+    # as we still use the `rails-deprecated_sanitizer` gem
+    Loofah.fragment(text).scrub!(scrubber).to_s
+  end
+
   def markdown_toolbar_button(options = {})
     data = options[:data].merge({ container: 'body' })
     content_tag :button,
diff --git a/changelogs/unreleased/31887-remove-images-from-todos.yml b/changelogs/unreleased/31887-remove-images-from-todos.yml
new file mode 100644
index 00000000000..36388f66514
--- /dev/null
+++ b/changelogs/unreleased/31887-remove-images-from-todos.yml
@@ -0,0 +1,5 @@
+---
+title: Images are no longer displayed in Todo descriptions
+merge_request: 21704
+author:
+type: fixed
diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb
index 9a29ac26eff..a0c0af94fa5 100644
--- a/spec/helpers/markup_helper_spec.rb
+++ b/spec/helpers/markup_helper_spec.rb
@@ -339,11 +339,25 @@ describe MarkupHelper do
         expect(first_line_in_markdown(object, attribute, 150, project: project)).to eq(expected)
       end
 
-      it 'preserves data-src for lazy images' do
-        object = create_object("![ImageTest](/uploads/test.png)")
-        image_url = "data-src=\".*/uploads/test.png\""
+      context 'when images are allowed' do
+        it 'preserves data-src for lazy images' do
+          object    = create_object("![ImageTest](/uploads/test.png)")
+          image_url = "data-src=\".*/uploads/test.png\""
+          text      = first_line_in_markdown(object, attribute, 150, project: project, allow_images: true)
+
+          expect(text).to match(image_url)
+          expect(text).to match('<a')
+        end
+      end
 
-        expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(image_url)
+      context 'when images are not allowed' do
+        it 'removes any images' do
+          object = create_object("![ImageTest](/uploads/test.png)")
+          text   = first_line_in_markdown(object, attribute, 150, project: project)
+
+          expect(text).not_to match('<img')
+          expect(text).not_to match('<a')
+        end
       end
 
       context 'labels formatting' do