Merge branch 'atom-xhtml-squashed' into 'master'

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

This is a fix for 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* are not terminated and make the Atom XML invalid. Such tags are generated when issue descriptions, merge request descriptions, comments, or commit messages use emoji, horizontal rules, or images.

To pass this option through from the relevant Haml templates to the proper place in the `gfm()` method, a new method `gfm_with_options()` is introduced. It reuses the options dictionary passed to `markdown()` and interprets options `xhtml` and `parse_tasks` from it (the latter was a convenient replacement for `gfm_with_tasks()`). `xhtml` is already interpreted by Redcarpet::Render::HTML, but that alone was not sufficient, because the post-processing in `gfm()` would convert its XHTML tags back to HTML.

I found no way of passing additional optional options to the existing `gfm()` method without requiring updates to existing callers and without getting in the way of the existing optional arguments, but maybe someone who knows more about Ruby than I can think of one.

Thorough review appreciated since this is the first time I have used Ruby.

See merge request !344

8 files changed, 55 insertions, 19 deletions

diff --git a/CHANGELOG b/CHANGELOG
index bd66a92933d..ec30b09b90b 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -101,6 +101,9 @@ v 7.8.1
   - Fix urls for the issues when relative url was enabled
 
 v 7.8.0
+  - 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.
   - Replace highlight.js with rouge-fork rugments (Stefan Tatschner)
   - Make project search case insensitive (Hannes Rosenögger)
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 0ffd2aa0b98..5d14def8f75 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 c3a8d90ef54..e02e5b9fc3d 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -34,17 +34,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
@@ -87,14 +93,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 1cd3933e4b7..10efff2ae9f 100644
--- a/lib/redcarpet/render/gitlab_html.rb
+++ b/lib/redcarpet/render/gitlab_html.rb
@@ -67,10 +67,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