Commit 60942bf5 authored by Douwe Maan's avatar Douwe Maan

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
parents 0063194a 17a73017
...@@ -183,7 +183,7 @@ module CommitsHelper ...@@ -183,7 +183,7 @@ module CommitsHelper
options = { options = {
class: "commit-#{options[:source]}-link has-tooltip", class: "commit-#{options[:source]}-link has-tooltip",
data: { 'original-title'.to_sym => sanitize(source_email) } title: source_email
} }
if user.nil? if user.nil?
......
...@@ -52,7 +52,7 @@ module ProjectsHelper ...@@ -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 link_to(author_html, user_path(author), class: "author_link #{"#{opts[:mobile_classes]}" if opts[:mobile_classes]}").html_safe
else else
title = opts[:title].sub(":name", sanitize(author.name)) 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
end end
......
...@@ -23,5 +23,5 @@ ...@@ -23,5 +23,5 @@
- if assignee - if assignee
= link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }), = 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: '') - image_tag(avatar_icon(issuable.assignee, 16), class: "avatar s16", alt: '')
...@@ -29,8 +29,9 @@ class Spinach::Features::ProjectCommitsUserLookup < Spinach::FeatureSteps ...@@ -29,8 +29,9 @@ class Spinach::Features::ProjectCommitsUserLookup < Spinach::FeatureSteps
def check_author_link(email, user) def check_author_link(email, user)
author_link = find('.commit-author-link') author_link = find('.commit-author-link')
expect(author_link['href']).to eq user_path(user) 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 expect(find('.commit-author-name').text).to eq user.name
end end
......
...@@ -48,7 +48,7 @@ feature 'Multiple issue updating from issues#index', feature: true do ...@@ -48,7 +48,7 @@ feature 'Multiple issue updating from issues#index', feature: true do
click_update_issues_button click_update_issues_button
page.within('.issue .controls') do 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
end end
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment