diff options
author | Tim Zallmann <tzallmann@gitlab.com> | 2017-08-30 09:02:08 +0200 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2017-09-05 11:54:10 -0400 |
commit | 654ea08b891e6742b7e97eb94931001906c6d445 (patch) | |
tree | bd02fb34457015f679824e7cb95ec876d3b998c7 | |
parent | 49d254e1e7cc5ce3127aa08b059b018e8ae2a268 (diff) | |
download | gitlab-ce-654ea08b891e6742b7e97eb94931001906c6d445.tar.gz |
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 | 6 | ||||
-rw-r--r-- | changelogs/unreleased/fix-escape-commit-block.yml | 5 | ||||
-rw-r--r-- | spec/helpers/commits_helper_spec.rb | 22 |
3 files changed, 30 insertions, 3 deletions
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index d08e346d605..c580751c48b 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -133,7 +133,7 @@ module CommitsHelper 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 @@ -144,9 +144,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 c245bb439db..a2a407bf1c2 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 |