Merge branch 'osw-ensures-html-safety-of-new-diff-cache' into 'master'

Ensure HTML safety of new HSET/HMGET diffs cache

Closes #39104

See merge request gitlab-org/gitlab!21521

lib/gitlab/diff/highlight_cache.rb

@@ -21,7 +21,7 @@ module Gitlab
       def decorate(diff_file)
         if content = read_file(diff_file)
           diff_file.highlighted_diff_lines = content.map do |line|
-            Gitlab::Diff::Line.init_from_hash(line)
+            Gitlab::Diff::Line.safe_init_from_hash(line)
           end
         end
       end

lib/gitlab/diff/line.rb

@@ -34,6 +34,14 @@ module Gitlab
             rich_text: hash[:rich_text])
       end
 
+      def self.safe_init_from_hash(hash)
+        line = hash.with_indifferent_access
+        rich_text = line[:rich_text]
+        line[:rich_text] = rich_text&.html_safe
+
+        init_from_hash(line)
+      end
+
       def to_hash
         hash = {}
         SERIALIZE_KEYS.each { |key| hash[key] = send(key) } # rubocop:disable GitlabSecurity/PublicSend

lib/gitlab/discussions_diff/highlight_cache.rb

@@ -43,11 +43,7 @@ module Gitlab
             next unless lines
 
             JSON.parse(lines).map! do |line|
-              line = line.with_indifferent_access
-              rich_text = line[:rich_text]
-              line[:rich_text] = rich_text&.html_safe
-
-              Gitlab::Diff::Line.init_from_hash(line)
+              Gitlab::Diff::Line.safe_init_from_hash(line)
             end
           end
         end

spec/lib/gitlab/diff/highlight_cache_spec.rb

@@ -68,6 +68,15 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
 
       expect(diff_file.highlighted_diff_lines.size).to be > 5
     end
+
+    it 'assigns highlighted diff lines which rich_text are HTML-safe' do
+      cache.write_if_empty
+      cache.decorate(diff_file)
+
+      rich_texts = diff_file.highlighted_diff_lines.map(&:rich_text)
+
+      expect(rich_texts).to all(be_html_safe)
+    end
   end
 
   describe '#write_if_empty' do

spec/lib/gitlab/diff/line_spec.rb

 # frozen_string_literal: true
 
+require 'spec_helper'
+
 describe Gitlab::Diff::Line do
-  describe '.init_from_hash' do
+  shared_examples 'line object initialized by hash' do
     it 'round-trips correctly with to_hash' do
-      line = described_class.new('<input>', 'match', 0, 0, 1,
+      expect(described_class.safe_init_from_hash(line.to_hash).to_hash)
+        .to eq(line.to_hash)
+    end
+  end
+
+  let(:line) do
+    described_class.new('<input>', 'match', 0, 0, 1,
                         parent_file: double(:file),
                         line_code: double(:line_code),
-                                 rich_text: '&lt;input&gt;')
+                        rich_text: rich_text)
+  end
 
-      expect(described_class.init_from_hash(line.to_hash).to_hash)
-        .to eq(line.to_hash)
+  describe '.init_from_hash' do
+    let(:rich_text) { '&lt;input&gt;' }
+
+    it_behaves_like 'line object initialized by hash'
+  end
+
+  describe '.safe_init_from_hash' do
+    let(:rich_text) { '<input>' }
+
+    it_behaves_like 'line object initialized by hash'
+
+    it 'ensures rich_text is HTML-safe' do
+      expect(line.rich_text).not_to be_html_safe
+
+      new_line = described_class.safe_init_from_hash(line.to_hash)
+
+      expect(new_line.rich_text).to be_html_safe
+    end
+
+    context 'when given hash has no rich_text' do
+      it_behaves_like 'line object initialized by hash' do
+        let(:rich_text) { nil }
+      end
     end
   end
 

spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb

@@ -62,6 +62,15 @@ describe Gitlab::DiscussionsDiff::HighlightCache, :clean_gitlab_redis_cache do
       expect(found.second.size).to eq(2)
       expect(found.second).to all(be_a(Gitlab::Diff::Line))
     end
+
+    it 'returns lines which rich_text are HTML-safe' do
+      described_class.write_multiple(mapping)
+
+      found = described_class.read_multiple(mapping.keys)
+      rich_texts = found.flatten.map(&:rich_text)
+
+      expect(rich_texts).to all(be_html_safe)
+    end
   end
 
   describe '#clear_multiple' do