Merge pull request #1512 from tsigo/escape_gfm

Better escaping of text passed into GFM

11 files changed, 23 insertions, 14 deletions

diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb
index 511d4628e12..111982e9147 100644
--- a/app/helpers/gitlab_markdown_helper.rb
+++ b/app/helpers/gitlab_markdown_helper.rb
@@ -12,8 +12,8 @@ module GitlabMarkdownHelper
   # "<a>outer text </a><a>gfm ref</a><a> more outer text</a>").
   def link_to_gfm(body, url, html_options = {})
     return "" if body.blank?
-    
-    gfm_body = gfm(body, html_options)
+
+    gfm_body = gfm(escape_once(body), html_options)
 
     gfm_body.gsub!(%r{<a.*?>.*?</a>}m) do |match|
       "</a>#{match}#{link_to("", url, html_options)[0..-5]}" # "</a>".length +1
diff --git a/app/views/commits/_commit_box.html.haml b/app/views/commits/_commit_box.html.haml
index 506f4e092a1..572337debc8 100644
--- a/app/views/commits/_commit_box.html.haml
+++ b/app/views/commits/_commit_box.html.haml
@@ -11,10 +11,10 @@
       = link_to tree_project_ref_path(@project, @commit.id), class: "browse-button primary grouped" do
         %strong Browse Code »
     %h3.commit-title.page_title
-      = gfm @commit.title
+      = gfm escape_once(@commit.title)
     - if @commit.description.present?
       %pre.commit-description
-        = gfm @commit.description
+        = gfm escape_once(@commit.description)
   .commit-info
     .row
       .span4
diff --git a/app/views/events/_commit.html.haml b/app/views/events/_commit.html.haml
index 1e5c00cb4f4..cb25d831bca 100644
--- a/app/views/events/_commit.html.haml
+++ b/app/views/events/_commit.html.haml
@@ -5,4 +5,4 @@
     %strong.cdark= commit.author_name
     &ndash;
     = image_tag gravatar_icon(commit.author_email), class: "avatar", width: 16
-    = gfm truncate(commit.title, length: 50) rescue "--broken encoding"
+    = gfm escape_once(truncate(commit.title, length: 50)) rescue "--broken encoding"
diff --git a/app/views/issues/show.html.haml b/app/views/issues/show.html.haml
index e7365e10eeb..da2aeac433d 100644
--- a/app/views/issues/show.html.haml
+++ b/app/views/issues/show.html.haml
@@ -31,7 +31,7 @@
         .alert-message.error.status_info Closed
       - else
         .alert-message.success.status_info Open
-      = gfm @issue.title
+      = gfm escape_once(@issue.title)
 
   .middle_box_content
     %cite.cgray Created by
diff --git a/app/views/merge_requests/show/_mr_box.html.haml b/app/views/merge_requests/show/_mr_box.html.haml
index 81ab83f3436..89c3110bddf 100644
--- a/app/views/merge_requests/show/_mr_box.html.haml
+++ b/app/views/merge_requests/show/_mr_box.html.haml
@@ -5,7 +5,7 @@
         .alert-message.error.status_info Closed
       - else
         .alert-message.success.status_info Open
-      = gfm @merge_request.title
+      = gfm escape_once(@merge_request.title)
 
   .middle_box_content
     %div
diff --git a/app/views/milestones/show.html.haml b/app/views/milestones/show.html.haml
index 0d6cb2a0f64..ba71ead7311 100644
--- a/app/views/milestones/show.html.haml
+++ b/app/views/milestones/show.html.haml
@@ -21,7 +21,7 @@
         .alert-message.error.status_info Closed
       - else
         .alert-message.success.status_info Open
-      = gfm @milestone.title
+      = gfm escape_once(@milestone.title)
       %small.right= @milestone.expires_at
 
   .middle_box_content
diff --git a/app/views/repositories/_branch.html.haml b/app/views/repositories/_branch.html.haml
index cf8558ec33e..64a633be8ac 100644
--- a/app/views/repositories/_branch.html.haml
+++ b/app/views/repositories/_branch.html.haml
@@ -11,7 +11,7 @@
       %code= commit.short_id
 
     = image_tag gravatar_icon(commit.author_email), class: "", width: 16
-    = gfm truncate(commit.title, length: 40)
+    = gfm escape_once(truncate(commit.title, length: 40))
     %span.update-author.right
       = time_ago_in_words(commit.committed_date)
       ago
diff --git a/app/views/repositories/_feed.html.haml b/app/views/repositories/_feed.html.haml
index ac4eb483945..0c13551d607 100644
--- a/app/views/repositories/_feed.html.haml
+++ b/app/views/repositories/_feed.html.haml
@@ -13,7 +13,7 @@
       = link_to project_commits_path(@project, commit.id) do
         %code= commit.short_id
       = image_tag gravatar_icon(commit.author_email), class: "", width: 16
-      = gfm truncate(commit.title, length: 40)
+      = gfm escape_once(truncate(commit.title, length: 40))
   %td
     %span.right.cgray
       = time_ago_in_words(commit.committed_date)
diff --git a/app/views/repositories/tags.html.haml b/app/views/repositories/tags.html.haml
index 0e870c80bbc..a41145867e9 100644
--- a/app/views/repositories/tags.html.haml
+++ b/app/views/repositories/tags.html.haml
@@ -17,7 +17,7 @@
           = link_to project_commit_path(@project, commit.id) do
             %code= commit.short_id
           = image_tag gravatar_icon(commit.author_email), class: "", width: 16
-          = gfm truncate(commit.title, length: 40)
+          = gfm escape_once(truncate(commit.title, length: 40))
         %td
           %span.update-author.right
             = time_ago_in_words(commit.committed_date)
diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index 0a467a8d9c9..9201003e2e1 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -48,8 +48,10 @@ module Gitlab
     def gfm(text, html_options = {})
       return text if text.nil?
 
-      # prevents the string supplied through the _text_ argument to be altered
-      text = text.dup
+      # Duplicate the string so we don't alter the original, then call to_str
+      # to cast it back to a String instead of a SafeBuffer. This is required
+      # for gsub calls to work as we need them to.
+      text = text.dup.to_str
 
       @html_options = html_options
 
diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb
index 0af331424f5..a6708a7acd6 100644
--- a/spec/helpers/gitlab_markdown_helper_spec.rb
+++ b/spec/helpers/gitlab_markdown_helper_spec.rb
@@ -292,11 +292,18 @@ describe GitlabMarkdownHelper do
       actual = link_to_gfm("Fixed in #{commit.id}", commit_path, class: 'foo')
       actual.should have_selector 'a.gfm.gfm-commit.foo'
     end
+
+    it "escapes HTML passed in as the body" do
+      actual = "This is a <h1>test</h1> - see ##{issues[0].id}"
+      link_to_gfm(actual, commit_path).should match('&lt;h1&gt;test&lt;/h1&gt;')
+    end
   end
 
   describe "#markdown" do
     it "should handle references in paragraphs" do
-      markdown("\n\nLorem ipsum dolor sit amet, consectetur adipiscing elit. #{commit.id} Nam pulvinar sapien eget odio adipiscing at faucibus orci vestibulum.\n").should == "<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. #{link_to commit.id, project_commit_path(project, commit), title: commit.link_title, class: "gfm gfm-commit "} Nam pulvinar sapien eget odio adipiscing at faucibus orci vestibulum.</p>\n"
+      actual = "\n\nLorem ipsum dolor sit amet. #{commit.id} Nam pulvinar sapien eget.\n"
+      expected = project_commit_path(project, commit)
+      markdown(actual).should match(expected)
     end
 
     it "should handle references in headers" do