diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-02-14 11:09:38 +0000 |
---|---|---|
committer | Filipa Lacerda <filipa@gitlab.com> | 2018-04-03 14:56:12 +0100 |
commit | 3760b3f13ea9bea18a5e74ad4fa743221288ceb6 (patch) | |
tree | 36356f79bcf05d7c0f99ab78c37ad1baafd2a5cd | |
parent | 6abc9ece9f5755c2d7a5481af655e3bcaea617a6 (diff) | |
download | gitlab-ce-3760b3f13ea9bea18a5e74ad4fa743221288ceb6.tar.gz |
Merge branch '42028-xss-diffs-10-4' into 'security-10-4'
Backport of "Fix XSS on commit diff view" for 10-4
See merge request gitlab/gitlabhq!2325
-rw-r--r-- | changelogs/unreleased/42028-xss-diffs.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/diff/inline_diff_marker.rb | 7 | ||||
-rw-r--r-- | spec/helpers/diff_helper_spec.rb | 30 |
3 files changed, 38 insertions, 4 deletions
diff --git a/changelogs/unreleased/42028-xss-diffs.yml b/changelogs/unreleased/42028-xss-diffs.yml new file mode 100644 index 00000000000..a05f9d3c78d --- /dev/null +++ b/changelogs/unreleased/42028-xss-diffs.yml @@ -0,0 +1,5 @@ +--- +title: Fix XSS on diff view stored on filenames +merge_request: +author: +type: security diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 010b4be7b40..81e91ea0ab7 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -1,11 +1,14 @@ module Gitlab module Diff class InlineDiffMarker < Gitlab::StringRangeMarker + def initialize(line, rich_line = nil) + super(line, rich_line || line) + end + def mark(line_inline_diffs, mode: nil) - mark = super(line_inline_diffs) do |text, left:, right:| + super(line_inline_diffs) do |text, left:, right:| %{<span class="#{html_class_names(left, right, mode)}">#{text}</span>} end - mark.html_safe end private diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 15cbe36ae76..53c010fa0db 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -135,11 +135,37 @@ describe DiffHelper do it "returns strings with marked inline diffs" do marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line) - expect(marked_old_line).to eq(%q{abc <span class="idiff left right deletion">'def'</span>}) + expect(marked_old_line).to eq(%q{abc <span class="idiff left right deletion">'def'</span>}) expect(marked_old_line).to be_html_safe - expect(marked_new_line).to eq(%q{abc <span class="idiff left right addition">"def"</span>}) + expect(marked_new_line).to eq(%q{abc <span class="idiff left right addition">"def"</span>}) expect(marked_new_line).to be_html_safe end + + context 'when given HTML' do + it 'sanitizes it' do + old_line = %{test.txt} + new_line = %{<img src=x onerror=alert(document.domain)>} + + marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line) + + expect(marked_old_line).to eq(%q{<span class="idiff left right deletion">test.txt</span>}) + expect(marked_old_line).to be_html_safe + expect(marked_new_line).to eq(%q{<span class="idiff left right addition"><img src=x onerror=alert(document.domain)></span>}) + expect(marked_new_line).to be_html_safe + end + + it 'sanitizes the entire line, not just the changes' do + old_line = %{<img src=x onerror=alert(document.domain)>} + new_line = %{<img src=y onerror=alert(document.domain)>} + + marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line) + + expect(marked_old_line).to eq(%q{<img src=<span class="idiff left right deletion">x</span> onerror=alert(document.domain)>}) + expect(marked_old_line).to be_html_safe + expect(marked_new_line).to eq(%q{<img src=<span class="idiff left right addition">y</span> onerror=alert(document.domain)>}) + expect(marked_new_line).to be_html_safe + end + end end describe '#parallel_diff_discussions' do |