Merge branch 'html-safe-diff-line-content' into 'security'

Don't accidentally mark unsafe diff lines as HTML safe

Fixes potential XSS issue when a legacy diff note is created on a merge
request whose diff contained HTML

See https://gitlab.com/gitlab-org/gitlab-ce/issues/25249

See merge request !2040

3 files changed, 59 insertions, 10 deletions

diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index f489f9aa0d6..c35d6611ab0 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -55,7 +55,9 @@ module DiffHelper
     if line.blank?
       " ".html_safe
     else
-      line.sub(/^[\-+ ]/, '').html_safe
+      # We can't use `sub` because the HTML-safeness of `line` will not survive.
+      line[0] = '' if line.start_with?('+', '-', ' ')
+      line
     end
   end
 
diff --git a/changelogs/unreleased/html-safe-diff-line-content.yml b/changelogs/unreleased/html-safe-diff-line-content.yml
new file mode 100644
index 00000000000..8f8bbc51963
--- /dev/null
+++ b/changelogs/unreleased/html-safe-diff-line-content.yml
@@ -0,0 +1,4 @@
+---
+title: Don't accidentally mark unsafe diff lines as HTML safe
+merge_request: 
+author: 
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 837e7afa7e8..468bcc7badc 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -60,15 +60,58 @@ describe DiffHelper do
   end
 
   describe '#diff_line_content' do
-    it 'returns non breaking space when line is empty' do
-      expect(diff_line_content(nil)).to eq(' ')
-    end
-
-    it 'returns the line itself' do
-      expect(diff_line_content(diff_file.diff_lines.first.text)).
-        to eq('@@ -6,12 +6,18 @@ module Popen')
-      expect(diff_line_content(diff_file.diff_lines.first.type)).to eq('match')
-      expect(diff_file.diff_lines.first.new_pos).to eq(6)
+    context 'when the line is empty' do
+      it 'returns a non breaking space' do
+        expect(diff_line_content(nil)).to eq(' ')
+      end
+
+      it 'returns an HTML-safe string' do
+        expect(diff_line_content(nil)).to be_html_safe
+      end
+    end
+
+    context 'when the line is not empty' do
+      context 'when the line starts with +, -, or a space' do
+        it 'strips the first character' do
+          expect(diff_line_content('+new line')).to eq('new line')
+          expect(diff_line_content('-new line')).to eq('new line')
+          expect(diff_line_content(' new line')).to eq('new line')
+        end
+
+        context 'when the line is HTML-safe' do
+          it 'returns an HTML-safe string' do
+            expect(diff_line_content('+new line'.html_safe)).to be_html_safe
+            expect(diff_line_content('-new line'.html_safe)).to be_html_safe
+            expect(diff_line_content(' new line'.html_safe)).to be_html_safe
+          end
+        end
+
+        context 'when the line is not HTML-safe' do
+          it 'returns a non-HTML-safe string' do
+            expect(diff_line_content('+new line')).not_to be_html_safe
+            expect(diff_line_content('-new line')).not_to be_html_safe
+            expect(diff_line_content(' new line')).not_to be_html_safe
+          end
+        end
+      end
+
+      context 'when the line does not start with a +, -, or a space' do
+        it 'returns the string' do
+          expect(diff_line_content('@@ -6,12 +6,18 @@ module Popen')).to eq('@@ -6,12 +6,18 @@ module Popen')
+        end
+
+        context 'when the line is HTML-safe' do
+          it 'returns an HTML-safe string' do
+            expect(diff_line_content('@@ -6,12 +6,18 @@ module Popen'.html_safe)).to be_html_safe
+          end
+        end
+
+        context 'when the line is not HTML-safe' do
+          it 'returns a non-HTML-safe string' do
+            expect(diff_line_content('@@ -6,12 +6,18 @@ module Popen')).not_to be_html_safe
+          end
+        end
+      end
     end
   end