Merge branch 'sh-fix-issue-20776' into 'master'

Fix Error 500 when viewing old merge requests with bad diff data

Customers running old versions of GitLab may have MergeRequestDiffs with the text ["--broken diff"] due to text generated by gitlab_git 1.0.3.
To avoid the Error 500, verify that each element is a type that gitlab_git will accept before attempting to create a DiffCollection.
    
Closes #20776

See merge request !6754

3 files changed, 24 insertions, 1 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 06dc2993b73..2095e4fc0fd 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -14,6 +14,7 @@ v 8.13.0 (unreleased)
   - AbstractReferenceFilter caches project_refs on RequestStore when active
   - Replaced the check sign to arrow in the show build view. !6501
   - Add a /wip slash command to toggle the Work In Progress status of a merge request. !6259 (tbalthazar)
+  - Fix Error 500 when viewing old merge requests with bad diff data
   - Speed-up group milestones show page
   - Fix inconsistent options dropdown caret on mobile viewports (ClemMakesApps)
   - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 36b8b70870b..3f7e96186a1 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -6,6 +6,9 @@ class MergeRequestDiff < ActiveRecord::Base
   # Prevent store of diff if commits amount more then 500
   COMMITS_SAFE_SIZE = 100
 
+  # Valid types of serialized diffs allowed by Gitlab::Git::Diff
+  VALID_CLASSES = [Hash, Rugged::Patch, Rugged::Diff::Delta]
+
   belongs_to :merge_request
 
   state_machine :state, initial: :empty do
@@ -170,6 +173,15 @@ class MergeRequestDiff < ActiveRecord::Base
 
   private
 
+  # Old GitLab implementations may have generated diffs as ["--broken-diff"].
+  # Avoid an error 500 by ignoring bad elements. See:
+  # https://gitlab.com/gitlab-org/gitlab-ce/issues/20776
+  def valid_raw_diff?(raw)
+    return false unless raw.respond_to?(:each)
+
+    raw.any? { |element| VALID_CLASSES.include?(element.class) }
+  end
+
   def dump_commits(commits)
     commits.map(&:to_hash)
   end
@@ -200,7 +212,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def load_diffs(raw, options)
-    if raw.respond_to?(:each)
+    if valid_raw_diff?(raw)
       if paths = options[:paths]
         raw = raw.select do |diff|
           paths.include?(diff[:old_path]) || paths.include?(diff[:new_path])
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 530a7def553..96f1f60dbc0 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -44,6 +44,16 @@ describe MergeRequestDiff, models: true do
       end
     end
 
+    context 'when the raw diffs have invalid content' do
+      before { mr_diff.update_attributes(st_diffs: ["--broken-diff"]) }
+
+      it 'returns an empty DiffCollection' do
+        expect(mr_diff.raw_diffs.to_a).to be_empty
+        expect(mr_diff.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
+        expect(mr_diff.raw_diffs).to be_empty
+      end
+    end
+
     context 'when the raw diffs exist' do
       it 'returns the diffs' do
         expect(mr_diff.raw_diffs).to be_a(Gitlab::Git::DiffCollection)