Fix invalid Atom feeds when using emoji, horizontal rules, or images.

Fixes issues #880, #723, #1113: Markdown must be rendered to XHTML, not HTML, when generating summary content for Atom feeds. Otherwise, content-less tags like <img> and <hr>, generated when issue descriptions, merge request descriptions, comments, or commit messages use emoji, horizontal rules, or images, are not terminated and make the Atom XML invalid.

8 files changed, 53 insertions, 19 deletions

diff --git a/CHANGELOG b/CHANGELOG
index c4b5a847e12..92aadc05849 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,7 @@ v 7.9.0 (unreleased)
   - Move labels/milestones tabs to sidebar
   - Improve UI for commits, issues and merge request lists
   - Fix commit comments on first line of diff not rendering in Merge Request Discussion view.
+  - Fix invalid Atom feeds when using emoji, horizontal rules, or images (Christian Walther)
 
 v 7.8.0 (unreleased)
   - Fix access control and protection against XSS for note attachments and other uploads.
diff --git a/app/views/events/_event_issue.atom.haml b/app/views/events/_event_issue.atom.haml
index eba2b63797a..0edb61ea246 100644
--- a/app/views/events/_event_issue.atom.haml
+++ b/app/views/events/_event_issue.atom.haml
@@ -1,3 +1,3 @@
 %div{xmlns: "http://www.w3.org/1999/xhtml"}
   - if issue.description.present?
-    = markdown issue.description
+    = markdown(issue.description, xhtml: true)
diff --git a/app/views/events/_event_merge_request.atom.haml b/app/views/events/_event_merge_request.atom.haml
index 0aea2d17d65..1a8b62abeab 100644
--- a/app/views/events/_event_merge_request.atom.haml
+++ b/app/views/events/_event_merge_request.atom.haml
@@ -1,3 +1,3 @@
 %div{xmlns: "http://www.w3.org/1999/xhtml"}
   - if merge_request.description.present?
-    = markdown merge_request.description
+    = markdown(merge_request.description, xhtml: true)
diff --git a/app/views/events/_event_note.atom.haml b/app/views/events/_event_note.atom.haml
index be0e05481ed..b49c331ccf2 100644
--- a/app/views/events/_event_note.atom.haml
+++ b/app/views/events/_event_note.atom.haml
@@ -1,2 +1,2 @@
 %div{xmlns: "http://www.w3.org/1999/xhtml"}
-  = markdown note.note
+  = markdown(note.note, xhtml: true)
diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml
index 2b63519edac..2fb9f7ec246 100644
--- a/app/views/events/_event_push.atom.haml
+++ b/app/views/events/_event_push.atom.haml
@@ -6,7 +6,7 @@
       %i
         at
         = commit[:timestamp].to_time.to_s(:short)
-    %blockquote= markdown(escape_once(commit[:message]))
+    %blockquote= markdown(escape_once(commit[:message]), xhtml: true)
   - if event.commits_count > 15
     %p
       %i
diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index fb0218a2778..dceb2bc71f1 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -33,17 +33,23 @@ module Gitlab
 
     attr_reader :html_options
 
-    def gfm_with_tasks(text, project = @project, html_options = {})
-      text = gfm(text, project, html_options)
-      parse_tasks(text)
+    # Public: Parse the provided text with GitLab-Flavored Markdown
+    #
+    # text         - the source text
+    # project      - extra options for the reference links as given to link_to
+    # html_options - extra options for the reference links as given to link_to
+    def gfm(text, project = @project, html_options = {})
+      gfm_with_options(text, {}, project, html_options)
     end
 
     # Public: Parse the provided text with GitLab-Flavored Markdown
     #
     # text         - the source text
+    # options      - parse_tasks: true - render tasks
+    #              - xhtml: true       - output XHTML instead of HTML
     # project      - extra options for the reference links as given to link_to
     # html_options - extra options for the reference links as given to link_to
-    def gfm(text, project = @project, html_options = {})
+    def gfm_with_options(text, options = {}, project = @project, html_options = {})
       return text if text.nil?
 
       # Duplicate the string so we don't alter the original, then call to_str
@@ -86,14 +92,22 @@ module Gitlab
       markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline
 
       result = markdown_pipeline.call(text, markdown_context)
-      text = result[:output].to_html(save_with: 0)
+      saveoptions = 0
+      if options[:xhtml]
+        saveoptions |= Nokogiri::XML::Node::SaveOptions::AS_XHTML
+      end
+      text = result[:output].to_html(save_with: saveoptions)
 
       allowed_attributes = ActionView::Base.sanitized_allowed_attributes
       allowed_tags = ActionView::Base.sanitized_allowed_tags
 
-      sanitize text.html_safe,
-               attributes: allowed_attributes + %w(id class style),
-               tags: allowed_tags + %w(table tr td th)
+      text = sanitize text.html_safe,
+                      attributes: allowed_attributes + %w(id class style),
+                      tags: allowed_tags + %w(table tr td th)
+      if options[:parse_tasks]
+        text = parse_tasks(text)
+      end
+      text
     end
 
     private
diff --git a/lib/redcarpet/render/gitlab_html.rb b/lib/redcarpet/render/gitlab_html.rb
index 714261f815c..8b0c193f3db 100644
--- a/lib/redcarpet/render/gitlab_html.rb
+++ b/lib/redcarpet/render/gitlab_html.rb
@@ -58,10 +58,6 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML
     unless @template.instance_variable_get("@project_wiki") || @project.nil?
       full_document = h.create_relative_links(full_document)
     end
-    if @options[:parse_tasks]
-      h.gfm_with_tasks(full_document)
-    else
-      h.gfm(full_document)
-    end
+    h.gfm_with_options(full_document, @options)
   end
 end
diff --git a/spec/features/atom/users_spec.rb b/spec/features/atom/users_spec.rb
index c0316b073ad..770ac04c2c5 100644
--- a/spec/features/atom/users_spec.rb
+++ b/spec/features/atom/users_spec.rb
@@ -15,17 +15,24 @@ describe "User Feed", feature: true  do
       let(:project) { create(:project) }
       let(:issue) do
         create(:issue, project: project,
-               author: user, description: '')
+               author: user, description: "Houston, we have a bug!\n\n***\n\nI guess.")
       end
       let(:note) do
         create(:note, noteable: issue, author: user,
-               note: 'Bug confirmed', project: project)
+               note: 'Bug confirmed :+1:', project: project)
+      end
+      let(:merge_request) do
+        create(:merge_request,
+               title: 'Fix bug', author: user,
+               source_project: project, target_project: project,
+               description: "Here is the fix: ![an image](image.png)")
       end
 
       before do
         project.team << [user, :master]
         issue_event(issue, user)
         note_event(note, user)
+        merge_request_event(merge_request, user)
         visit user_path(user, :atom, private_token: user.private_token)
       end
 
@@ -37,6 +44,18 @@ describe "User Feed", feature: true  do
         expect(body).
           to have_content("#{safe_name} commented on issue ##{issue.iid}")
       end
+
+      it 'should have XHTML summaries in issue descriptions' do
+        expect(body).to match /we have a bug!<\/p>\n\n<hr ?\/>\n\n<p>I guess/
+      end
+
+      it 'should have XHTML summaries in notes' do
+        expect(body).to match /Bug confirmed <img[^>]*\/>/
+      end
+
+      it 'should have XHTML summaries in merge request descriptions' do
+        expect(body).to match /Here is the fix: <img[^>]*\/>/
+      end
     end
   end
 
@@ -48,6 +67,10 @@ describe "User Feed", feature: true  do
     EventCreateService.new.leave_note(note, user)
   end
 
+  def merge_request_event(request, user)
+    EventCreateService.new.open_mr(request, user)
+  end
+
   def safe_name
     html_escape(user.name)
   end