Commit d4fab17d authored by Stan Hu's avatar Stan Hu

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
parent a5cd9c9a
...@@ -12,6 +12,7 @@ v 8.13.0 (unreleased) ...@@ -12,6 +12,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
- Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs) - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
- Add tag shortcut from the Commit page. !6543 - Add tag shortcut from the Commit page. !6543
......
...@@ -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