diff options
author | Stan Hu <stanhu@gmail.com> | 2016-10-07 22:48:23 -0700 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2016-10-10 13:05:09 -0700 |
commit | d4fab17d7c8c2b233248295755a6277fdee09c9f (patch) | |
tree | 4ee583bb9ef541d5e6975d9e36b5b22ffec8b763 | |
parent | a5cd9c9a596c4160bbdc7645f57266655488386c (diff) | |
download | gitlab-ce-sh-fix-issue-20776.tar.gz |
Fix Error 500 when viewing old merge requests with bad diff datash-fix-issue-20776
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 14 | ||||
-rw-r--r-- | spec/models/merge_request_diff_spec.rb | 10 |
3 files changed, 24 insertions, 1 deletions
diff --git a/CHANGELOG b/CHANGELOG index 24f77442f1a..dfb953fabd9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -12,6 +12,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 - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs) - Add tag shortcut from the Commit page. !6543 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) |