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

2 files changed, 30 insertions, 1 deletions

diff --git a/spec/features/issues/update_issues_spec.rb b/spec/features/issues/update_issues_spec.rb
index 3eb903a93fe..b03dd0f666d 100644
--- a/spec/features/issues/update_issues_spec.rb
+++ b/spec/features/issues/update_issues_spec.rb
@@ -48,7 +48,7 @@ feature 'Multiple issue updating from issues#index', feature: true do
       click_update_issues_button
 
       page.within('.issue .controls') do
-        expect(find('.author_link')["data-original-title"]).to have_content(user.name)
+        expect(find('.author_link')["title"]).to have_content(user.name)
       end
     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