diff options
author | Robert Speicher <rspeicher@gmail.com> | 2017-09-01 19:21:50 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2017-09-01 19:25:46 +0300 |
commit | bbe836b2042079b4728c8b25a474399682af466f (patch) | |
tree | 74f7ea058364b95e72a0c2d14d5dee300e899cbe | |
parent | 65649b9fac234902b6994f01360bb05fcce9e240 (diff) |
Unmark the commit author/committer link as HTML-safe
We now make use of the `content_tag` helper so that the untrusted input
is escaped and the trusted output is then automatically safe. When we
don't need to wrap the name in a `span` tag (when `avatar` is falsey),
it's treated as unsafe by default, so no further sanitization/escaping
is necessary.
-rw-r--r-- | app/helpers/commits_helper.rb | 8 | ||||
-rw-r--r-- | changelogs/unreleased/fix-escape-commit-block.yml | 5 | ||||
-rw-r--r-- | spec/helpers/commits_helper_spec.rb | 22 |
3 files changed, 31 insertions, 4 deletions
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 2c1e731df0d..ea0771ea6c5 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -133,11 +133,11 @@ module CommitsHelper source_name = clean(commit.send "#{options[:source]}_name".to_sym) source_email = clean(commit.send "#{options[:source]}_email".to_sym) - person_name = sanitize(user.try(:name) || source_name) + person_name = user.try(:name) || source_name text = if options[:avatar] - %Q{<span class="commit-#{options[:source]}-name">#{person_name}</span>} + content_tag(:span, person_name, class: "commit-#{options[:source]}-name") else person_name end @@ -148,9 +148,9 @@ module CommitsHelper } if user.nil? - mail_to(source_email, text.html_safe, options) + mail_to(source_email, text, options) else - link_to(text.html_safe, user_path(user), options) + link_to(text, user_path(user), options) end end diff --git a/changelogs/unreleased/fix-escape-commit-block.yml b/changelogs/unreleased/fix-escape-commit-block.yml new file mode 100644 index 00000000000..0665c8e5db2 --- /dev/null +++ b/changelogs/unreleased/fix-escape-commit-block.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a persistent XSS in the commit author block +merge_request: +author: +type: security diff --git a/spec/helpers/commits_helper_spec.rb b/spec/helpers/commits_helper_spec.rb index 7179185285c..4b6c7c33e5b 100644 --- a/spec/helpers/commits_helper_spec.rb +++ b/spec/helpers/commits_helper_spec.rb @@ -12,6 +12,17 @@ describe CommitsHelper do expect(helper.commit_author_link(commit)) .not_to include('onmouseover="alert(1)"') end + + it 'escapes the author name' do + user = build_stubbed(:user, name: 'Foo <script>alert("XSS")</script>') + + commit = double(author: user, author_name: '', author_email: '') + + expect(helper.commit_author_link(commit)) + .to include('Foo <script>') + expect(helper.commit_author_link(commit, avatar: true)) + .to include('commit-author-name', 'Foo <script>') + end end describe 'commit_committer_link' do @@ -25,6 +36,17 @@ describe CommitsHelper do expect(helper.commit_committer_link(commit)) .not_to include('onmouseover="alert(1)"') end + + it 'escapes the commiter name' do + user = build_stubbed(:user, name: 'Foo <script>alert("XSS")</script>') + + commit = double(committer: user, committer_name: '', committer_email: '') + + expect(helper.commit_committer_link(commit)) + .to include('Foo <script>') + expect(helper.commit_committer_link(commit, avatar: true)) + .to include('commit-committer-name', 'Foo <script>') + end end describe '#view_on_environment_button' do |