Commit 79eb7e4e authored by Robert Speicher's avatar Robert Speicher

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
parents 2ef90053 d4fab17d
...@@ -14,6 +14,7 @@ v 8.13.0 (unreleased) ...@@ -14,6 +14,7 @@ v 8.13.0 (unreleased)
- AbstractReferenceFilter caches project_refs on RequestStore when active - AbstractReferenceFilter caches project_refs on RequestStore when active
- Replaced the check sign to arrow in the show build view. !6501 - 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) - 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 - Speed-up group milestones show page
- Fix inconsistent options dropdown caret on mobile viewports (ClemMakesApps) - Fix inconsistent options dropdown caret on mobile viewports (ClemMakesApps)
- Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs) - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
......
...@@ -6,6 +6,9 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -6,6 +6,9 @@ class MergeRequestDiff < ActiveRecord::Base
# Prevent store of diff if commits amount more then 500 # Prevent store of diff if commits amount more then 500
COMMITS_SAFE_SIZE = 100 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 belongs_to :merge_request
state_machine :state, initial: :empty do state_machine :state, initial: :empty do
...@@ -170,6 +173,15 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -170,6 +173,15 @@ class MergeRequestDiff < ActiveRecord::Base
private 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) def dump_commits(commits)
commits.map(&:to_hash) commits.map(&:to_hash)
end end
...@@ -200,7 +212,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -200,7 +212,7 @@ class MergeRequestDiff < ActiveRecord::Base
end end
def load_diffs(raw, options) def load_diffs(raw, options)
if raw.respond_to?(:each) if valid_raw_diff?(raw)
if paths = options[:paths] if paths = options[:paths]
raw = raw.select do |diff| raw = raw.select do |diff|
paths.include?(diff[:old_path]) || paths.include?(diff[:new_path]) paths.include?(diff[:old_path]) || paths.include?(diff[:new_path])
......
...@@ -44,6 +44,16 @@ describe MergeRequestDiff, models: true do ...@@ -44,6 +44,16 @@ describe MergeRequestDiff, models: true do
end end
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 context 'when the raw diffs exist' do
it 'returns the diffs' do it 'returns the diffs' do
expect(mr_diff.raw_diffs).to be_a(Gitlab::Git::DiffCollection) expect(mr_diff.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment