diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-04-19 18:16:00 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-04-19 18:35:32 +0300 |
commit | 2c9cd67f78ea5cebc8fa4c57027990589bea0d2d (patch) | |
tree | a3f91abb3461fe02049d387ffa494388934c4f16 | |
parent | 93e923fc044f4d686b72a6db882620c538727f9b (diff) |
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>
-rw-r--r-- | CHANGELOG | 2 | ||||
-rw-r--r-- | app/helpers/commits_helper.rb | 2 | ||||
-rw-r--r-- | app/helpers/projects_helper.rb | 2 | ||||
-rw-r--r-- | features/steps/project/commits/user_lookup.rb | 3 | ||||
-rw-r--r-- | spec/helpers/commits_helper_spec.rb | 29 |
5 files changed, 34 insertions, 4 deletions
diff --git a/CHANGELOG b/CHANGELOG index eb2df0e39c8..abd6b4ad3e7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,7 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.5.11 - - No changes + - Fix persistent XSS vulnerability in `commit_person_link` helper v 8.5.10 - Fix a 2FA authentication spoofing vulnerability. diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index a09e91578b6..f45a3050cb6 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 d6fb629b0c2..b5d5b6785ca 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -46,7 +46,7 @@ module ProjectsHelper link_to(author_html, user_path(author), class: "author_link").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/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 |