Merge branch '42028-xss-diffs' into 'security-10-5'

Fix XSS on commit diff view

See merge request gitlab/gitlabhq!2323

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">&#39;def&#39;</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">&quot;def&quot;</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">&lt;img src=x onerror=alert(document.domain)&gt;</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{&lt;img src=<span class="idiff left right deletion">x</span> onerror=alert(document.domain)&gt;})
+        expect(marked_old_line).to be_html_safe
+        expect(marked_new_line).to eq(%q{&lt;img src=<span class="idiff left right addition">y</span> onerror=alert(document.domain)&gt;})
+        expect(marked_new_line).to be_html_safe
+      end
+    end
   end
 
   describe '#parallel_diff_discussions' do