Correctly escape highlighted diff sections

lib/redmine/unified_diff.rb

@@ -191,42 +191,26 @@ def write_offsets
 
     def offsets(line_left, line_right)
       if line_left.present? && line_right.present? && line_left != line_right
-        line_left = escapeHTML(line_left)
-        line_right = escapeHTML(line_right)
         max = [line_left.size, line_right.size].min
-        starting = starting(line_left, line_right, max)
-        ending = ending(line_left, line_right, max, starting)
+        starting = 0
+        while starting < max && line_left[starting] == line_right[starting]
+          starting += 1
+        end
+        ending = -1
+        while ending >= -(max - starting) && (line_left[ending] == line_right[ending])
+          ending -= 1
+        end
         unless starting == 0 && ending == -1
           [starting, ending]
         end
       end
     end
-
-    def starting(line_left, line_right, max)
-      starting = 0
-      while starting < max && line_left[starting] == line_right[starting]
-        starting += 1
-      end
-      if starting.positive? && line_left[starting - 1] == '&'
-        starting -= 1
-      end
-      starting
-    end
-
-    def ending(line_left, line_right, max, starting)
-      ending = -1
-      while ending >= -(max - starting) && line_left[ending] == line_right[ending]
-        ending -= 1
-      end
-      if ending < -1 && line_left[ending + 1] == ';' && line_left[starting] == '&'
-        ending += 1
-      end
-      ending
-    end
   end
 
   # A line of diff
   class Diff
+    include ActionView::Helpers::TagHelper
+
     attr_accessor :nb_line_left
     attr_accessor :line_left
     attr_accessor :nb_line_right
@@ -253,35 +237,15 @@ def line
     end
 
     def html_line_left
-      if offsets
-        l = escapeHTML(line_left)
-        l.insert(offsets.first, '<span>').insert(offsets.last, '</span>').html_safe
-      else
-        line_left
-      end
+      line_to_html(line_left, offsets)
     end
 
     def html_line_right
-      if offsets
-        l = escapeHTML(line_right)
-        l.insert(offsets.first, '<span>').insert(offsets.last, '</span>').html_safe
-      else
-        line_right
-      end
-    end
-
-    # Escape the HTML for the diff
-    def escapeHTML(line)
-      CGI.escapeHTML(line)
+      line_to_html(line_right, offsets)
     end
 
     def html_line
-      if offsets
-        l = escapeHTML(line)
-        l.insert(offsets.first, '<span>').insert(offsets.last, '</span>').html_safe
-      else
-        line
-      end
+      line_to_html(line, offsets)
     end
 
     def inspect
@@ -291,5 +255,29 @@ def inspect
       puts nb_line_right
       puts line_right
     end
+
+    private
+
+    def line_to_html(line, offsets)
+      line_to_html_raw(line, offsets).tap do |html_str|
+        html_str.force_encoding('UTF-8')
+      end
+    end
+
+    def line_to_html_raw(line, offsets)
+      return line unless offsets
+
+      ActiveSupport::SafeBuffer.new.tap do |output|
+        if offsets.first != 0
+          output << line[0..offsets.first-1]
+        end
+
+        output << content_tag(:span, line[offsets.first..offsets.last])
+
+        unless offsets.last == -1
+          output << line[offsets.last+1..-1]
+        end
+      end.to_s
+    end
   end
 end

spec/lib/redmine/unified_diff_spec.rb

@@ -55,4 +55,33 @@ module Redmine
       expect(@diff.first.first.line_right).to eq('<script>someMethod();</script>')
     end
   end
+
+  describe 'unified diff html eescape' do
+    let(:diff) do
+      Redmine::UnifiedDiff.new(<<~DIFF
+        diff --git a/asdf b/asdf
+        index 7f6361d..3c52e50 100644
+        --- a/asdf
+        +++ b/asdf
+        @@ -1,4 +1,4 @@
+         Test 1
+        -Test 2 <_> pouet
+        +Test 2 >_> pouet
+         Test 3
+         Test 4
+      DIFF
+      )
+    end
+
+    subject do
+      [].tap do |lines|
+        diff.first.each_line { |_,l| lines << [l.html_line_left, l.html_line_right] }
+      end
+    end
+
+    it 'should correctly escape elements' do
+      expect(subject[1]).to eq(["Test 2 <span>&lt;</span>_&gt; pouet", "<span></span>"])
+      expect(subject[2]).to eq(["<span></span>", "Test 2 <span>&gt;</span>_&gt; pouet"])
+    end
+  end
 end