Merge branch 'render-hipchat-notification-descriptions' into 'master'

Render HipChat notification descriptions as HTML instead of raw markdown

## What does this MR do?

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/1621 for issues, merge requests and comments.

## Are there points in the code the reviewer needs to double check?

This MR is small enough to double check everything.

## Why was this MR needed?

Because seeing raw markdown in HipChat notifications is not nice :)

See merge request !6371

3 files changed, 45 insertions, 30 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4f163f323e6..0bfdf23b959 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 ## 8.14.0 (2016-11-22)
   - Adds user project membership expired event to clarify why user was removed (Callum Dryden)
+  - Fix HipChat notifications rendering (airatshigapov, eisnerd)
   - Simpler arguments passed to named_route on toggle_award_url helper method
 
 ## 8.13.0 (2016-10-22)
diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb
index afebd3b6a12..660a8ae3421 100644
--- a/app/models/project_services/hipchat_service.rb
+++ b/app/models/project_services/hipchat_service.rb
@@ -1,5 +1,12 @@
 class HipchatService < Service
+  include ActionView::Helpers::SanitizeHelper
+
   MAX_COMMITS = 3
+  HIPCHAT_ALLOWED_TAGS = %w[
+    a b i strong em br img pre code
+    table th tr td caption colgroup col thead tbody tfoot
+    ul ol li dl dt dd
+  ]
 
   prop_accessor :token, :room, :server, :notify, :color, :api_version
   boolean_accessor :notify_only_broken_builds
@@ -88,6 +95,10 @@ class HipchatService < Service
     end
   end
 
+  def render_line(text)
+    markdown(text.lines.first.chomp, pipeline: :single_line) if text
+  end
+
   def create_push_message(push)
     ref_type = Gitlab::Git.tag_ref?(push[:ref]) ? 'tag' : 'branch'
     ref = Gitlab::Git.ref_name(push[:ref])
@@ -110,7 +121,7 @@ class HipchatService < Service
       message << "(<a href=\"#{project.web_url}/compare/#{before}...#{after}\">Compare changes</a>)"
 
       push[:commits].take(MAX_COMMITS).each do |commit|
-        message << "<br /> - #{commit[:message].lines.first} (<a href=\"#{commit[:url]}\">#{commit[:id][0..5]}</a>)"
+        message << "<br /> - #{render_line(commit[:message])} (<a href=\"#{commit[:url]}\">#{commit[:id][0..5]}</a>)"
       end
 
       if push[:commits].count > MAX_COMMITS
@@ -121,12 +132,22 @@ class HipchatService < Service
     message
   end
 
-  def format_body(body)
-    if body
-      body = body.truncate(200, separator: ' ', omission: '...')
-    end
+  def markdown(text, options = {})
+    return "" unless text
+
+    context = {
+      project: project,
+      pipeline: :email
+    }
+
+    Banzai.render(text, context)
 
-    "<pre>#{body}</pre>"
+    context.merge!(options)
+
+    html = Banzai.post_process(Banzai.render(text, context), context)
+    sanitized_html = sanitize(html, tags: HIPCHAT_ALLOWED_TAGS, attributes: %w[href title alt])
+
+    sanitized_html.truncate(200, separator: ' ', omission: '...')
   end
 
   def create_issue_message(data)
@@ -134,7 +155,7 @@ class HipchatService < Service
 
     obj_attr = data[:object_attributes]
     obj_attr = HashWithIndifferentAccess.new(obj_attr)
-    title = obj_attr[:title]
+    title = render_line(obj_attr[:title])
     state = obj_attr[:state]
     issue_iid = obj_attr[:iid]
     issue_url = obj_attr[:url]
@@ -143,10 +164,7 @@ class HipchatService < Service
     issue_link = "<a href=\"#{issue_url}\">issue ##{issue_iid}</a>"
     message = "#{user_name} #{state} #{issue_link} in #{project_link}: <b>#{title}</b>"
 
-    if description
-      description = format_body(description)
-      message << description
-    end
+    message << "<pre>#{markdown(description)}</pre>"
 
     message
   end
@@ -159,23 +177,20 @@ class HipchatService < Service
     merge_request_id = obj_attr[:iid]
     state = obj_attr[:state]
     description = obj_attr[:description]
-    title = obj_attr[:title]
+    title = render_line(obj_attr[:title])
 
     merge_request_url = "#{project_url}/merge_requests/#{merge_request_id}"
     merge_request_link = "<a href=\"#{merge_request_url}\">merge request !#{merge_request_id}</a>"
     message = "#{user_name} #{state} #{merge_request_link} in " \
       "#{project_link}: <b>#{title}</b>"
 
-    if description
-      description = format_body(description)
-      message << description
-    end
+    message << "<pre>#{markdown(description)}</pre>"
 
     message
   end
 
   def format_title(title)
-    "<b>" + title.lines.first.chomp + "</b>"
+    "<b>#{render_line(title)}</b>"
   end
 
   def create_note_message(data)
@@ -186,11 +201,13 @@ class HipchatService < Service
     note = obj_attr[:note]
     note_url = obj_attr[:url]
     noteable_type = obj_attr[:noteable_type]
+    commit_id = nil
 
     case noteable_type
     when "Commit"
       commit_attr = HashWithIndifferentAccess.new(data[:commit])
-      subject_desc = commit_attr[:id]
+      commit_id = commit_attr[:id]
+      subject_desc = commit_id
       subject_desc = Commit.truncate_sha(subject_desc)
       subject_type = "commit"
       title = format_title(commit_attr[:message])
@@ -218,10 +235,7 @@ class HipchatService < Service
     message = "#{user_name} commented on #{subject_html} in #{project_link}: "
     message << title
 
-    if note
-      note = format_body(note)
-      message << note
-    end
+    message << "<pre>#{markdown(note, ref: commit_id)}</pre>"
 
     message
   end
diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb
index 26dd95bdfec..2da3a9cb09f 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/spec/models/project_services/hipchat_service_spec.rb
@@ -117,7 +117,7 @@ describe HipchatService, models: true do
     end
 
     context 'issue events' do
-      let(:issue) { create(:issue, title: 'Awesome issue', description: 'please fix') }
+      let(:issue) { create(:issue, title: 'Awesome issue', description: '**please** fix') }
       let(:issue_service) { Issues::CreateService.new(project, user) }
       let(:issues_sample_data) { issue_service.hook_data(issue, 'open') }
 
@@ -135,12 +135,12 @@ describe HipchatService, models: true do
             "<a href=\"#{obj_attr[:url]}\">issue ##{obj_attr["iid"]}</a> in " \
             "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
             "<b>Awesome issue</b>" \
-            "<pre>please fix</pre>")
+            "<pre><strong>please</strong> fix</pre>")
       end
     end
 
     context 'merge request events' do
-      let(:merge_request) { create(:merge_request, description: 'please fix', title: 'Awesome merge request', target_project: project, source_project: project) }
+      let(:merge_request) { create(:merge_request, description: '**please** fix', title: 'Awesome merge request', target_project: project, source_project: project) }
       let(:merge_service) { MergeRequests::CreateService.new(project, user) }
       let(:merge_sample_data) { merge_service.hook_data(merge_request, 'open') }
 
@@ -159,7 +159,7 @@ describe HipchatService, models: true do
             "<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr["iid"]}</a> in " \
             "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
             "<b>Awesome merge request</b>" \
-            "<pre>please fix</pre>")
+            "<pre><strong>please</strong> fix</pre>")
       end
     end
 
@@ -203,7 +203,7 @@ describe HipchatService, models: true do
         let(:merge_request_note) do
           create(:note_on_merge_request, noteable: merge_request,
                                          project: project,
-                                         note: "merge request note")
+                                         note: "merge request **note**")
         end
 
         it "calls Hipchat API for merge request comment events" do
@@ -222,7 +222,7 @@ describe HipchatService, models: true do
               "<a href=\"#{obj_attr[:url]}\">merge request !#{merge_id}</a> in " \
               "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
               "<b>#{title}</b>" \
-              "<pre>merge request note</pre>")
+              "<pre>merge request <strong>note</strong></pre>")
         end
       end
 
@@ -230,7 +230,7 @@ describe HipchatService, models: true do
         let(:issue) { create(:issue, project: project) }
         let(:issue_note) do
           create(:note_on_issue, noteable: issue, project: project,
-                                 note: "issue note")
+                                 note: "issue **note**")
         end
 
         it "calls Hipchat API for issue comment events" do
@@ -247,7 +247,7 @@ describe HipchatService, models: true do
               "<a href=\"#{obj_attr[:url]}\">issue ##{issue_id}</a> in " \
               "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
               "<b>#{title}</b>" \
-              "<pre>issue note</pre>")
+              "<pre>issue <strong>note</strong></pre>")
         end
       end