Merge branch 'rs-issue-15126' into 'master'

Remove persistent XSS vulnerability in `commit_person_link` helper

Because we were incorrectly supplying the tooltip title as
`data-original-title` (which Bootstrap's Tooltip JS automatically
applies based on the `title` attribute; we should never be setting it
directly), the value was being passed through as-is.

Instead, we should be supplying the normal `title` attribute and letting
Rails escape the value, which also negates the need for us to call
`sanitize` on it.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15126

See merge request !1948

Signed-off-by: Rémy Coutable <remy@rymai.me>

6 files changed, 35 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index b831eee859c..564b89838c7 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.6.7
+  - Fix persistent XSS vulnerability in `commit_person_link` helper
   - Fix vulnerability that made it possible to enumerate private projects belonging to group
 
 v 8.6.6
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index f994c9e6170..3f233eb77b3 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -183,7 +183,7 @@ module CommitsHelper
 
     options = {
       class: "commit-#{options[:source]}-link has_tooltip",
-      data: { 'original-title'.to_sym => sanitize(source_email) }
+      title: source_email
     }
 
     if user.nil?
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 5473419ef24..f9d2757cf82 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -52,7 +52,7 @@ module ProjectsHelper
       link_to(author_html, user_path(author), class: "author_link #{"#{opts[:mobile_classes]}" if opts[:mobile_classes]}").html_safe
     else
       title = opts[:title].sub(":name", sanitize(author.name))
-      link_to(author_html, user_path(author), class: "author_link has_tooltip", data: { 'original-title'.to_sym => title, container: 'body' } ).html_safe
+      link_to(author_html, user_path(author), class: "author_link has_tooltip", title: title, data: { container: 'body' } ).html_safe
     end
   end
 
diff --git a/app/views/shared/milestones/_issuable.html.haml b/app/views/shared/milestones/_issuable.html.haml
index 85888096722..818d0928fdf 100644
--- a/app/views/shared/milestones/_issuable.html.haml
+++ b/app/views/shared/milestones/_issuable.html.haml
@@ -23,5 +23,5 @@
 
     - if assignee
       = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }),
-                class: 'has_tooltip', data: { 'original-title' => "Assigned to #{sanitize(assignee.name)}", container: 'body' } do
+                class: 'has_tooltip', title: "Assigned to #{assignee.name}", data: { container: 'body' } do
         - image_tag(avatar_icon(issuable.assignee, 16), class: "avatar s16", alt: '')
diff --git a/features/steps/project/commits/user_lookup.rb b/features/steps/project/commits/user_lookup.rb
index 40cada6da45..2d43be5a386 100644
--- a/features/steps/project/commits/user_lookup.rb
+++ b/features/steps/project/commits/user_lookup.rb
@@ -29,8 +29,9 @@ class Spinach::Features::ProjectCommitsUserLookup < Spinach::FeatureSteps
 
   def check_author_link(email, user)
     author_link = find('.commit-author-link')
+
     expect(author_link['href']).to eq user_path(user)
-    expect(author_link['data-original-title']).to eq email
+    expect(author_link['title']).to eq email
     expect(find('.commit-author-name').text).to eq user.name
   end
 
diff --git a/spec/helpers/commits_helper_spec.rb b/spec/helpers/commits_helper_spec.rb
new file mode 100644
index 00000000000..727c25ff529
--- /dev/null
+++ b/spec/helpers/commits_helper_spec.rb
@@ -0,0 +1,29 @@
+require 'rails_helper'
+
+describe CommitsHelper do
+  describe 'commit_author_link' do
+    it 'escapes the author email' do
+      commit = double(
+        author: nil,
+        author_name: 'Persistent XSS',
+        author_email: 'my@email.com" onmouseover="alert(1)'
+      )
+
+      expect(helper.commit_author_link(commit)).
+        not_to include('onmouseover="alert(1)"')
+    end
+  end
+
+  describe 'commit_committer_link' do
+    it 'escapes the committer email' do
+      commit = double(
+        committer: nil,
+        committer_name: 'Persistent XSS',
+        committer_email: 'my@email.com" onmouseover="alert(1)'
+      )
+
+      expect(helper.commit_committer_link(commit)).
+        not_to include('onmouseover="alert(1)"')
+    end
+  end
+end