Arbitrary file read via MergeRequestDiff

app/models/merge_request.rb

@@ -71,7 +71,7 @@ class MergeRequest < ActiveRecord::Base
   serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
-  after_create :ensure_merge_request_diff, unless: :importing?
+  after_create :ensure_merge_request_diff
   after_update :clear_memoized_shas
   after_update :reload_diff_if_branch_changed
   after_save :ensure_metrics

app/models/merge_request_diff.rb

@@ -25,6 +25,8 @@ class MergeRequestDiff < ActiveRecord::Base
   has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) }
+  validates :base_commit_sha, :head_commit_sha, :start_commit_sha, sha: true
+
   state_machine :state, initial: :empty do
     event :clean do
       transition any => :without_files

app/validators/sha_validator.rb

+# frozen_string_literal: true
+
+class ShaValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    return if value.blank? || value.match(/\A\h{40}\z/)
+
+    record.errors.add(attribute, 'is not a valid SHA')
+  end
+end

changelogs/unreleased/security-fj-diff-import-file-read-fix.yml

+---
+title: Fix arbitrary file read via diffs during import
+merge_request:
+author:
+type: security

lib/gitlab/import_export/merge_request_parser.rb

@@ -20,6 +20,17 @@ module Gitlab
           create_target_branch unless branch_exists?(@merge_request.target_branch)
         end
+        # The merge_request_diff associated with the current @merge_request might
+        # be invalid. Than means, when the @merge_request object is saved, the
+        # @merge_request.merge_request_diff won't. This can leave the merge request
+        # in an invalid state, because a merge request must have an associated
+        # merge request diff.
+        # In this change, if the associated merge request diff is invalid, we set
+        # it to nil. This change, in association with the after callback
+        # :ensure_merge_request_diff in the MergeRequest class, makes that
+        # when the merge request is going to be created and it doesn't have
+        # one, a default one will be generated.
+        @merge_request.merge_request_diff = nil unless @merge_request.merge_request_diff&.valid?
         @merge_request
       end

spec/features/merge_request/user_sees_versions_spec.rb

 require 'rails_helper'
 describe 'Merge request > User sees versions', :js do
-  let(:merge_request) { create(:merge_request, importing: true) }
+  let(:merge_request) do
+    create(:merge_request).tap do |mr|
+      mr.merge_request_diff.destroy
+    end
+  end
   let(:project) { merge_request.source_project }
   let(:user) { project.creator }
   let!(:merge_request_diff1) { merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }

spec/lib/gitlab/import_export/merge_request_parser_spec.rb

@@ -41,4 +41,20 @@ describe Gitlab::ImportExport::MergeRequestParser do
     expect(parsed_merge_request).to eq(merge_request)
   end
+
+  context 'when the merge request has diffs' do
+    let(:merge_request) do
+      build(:merge_request, source_project: forked_project, target_project: project)
+    end
+
+    context 'when the diff is invalid' do
+      let(:merge_request_diff) { build(:merge_request_diff, merge_request: merge_request, base_commit_sha: 'foobar') }
+
+      it 'sets the diff to nil' do
+        expect(merge_request_diff).to be_invalid
+        expect(merge_request_diff.merge_request).to eq merge_request
+        expect(parsed_merge_request.merge_request_diff).to be_nil
+      end
+    end
+  end
 end

spec/models/merge_request_diff_spec.rb

@@ -3,6 +3,18 @@ require 'spec_helper'
 describe MergeRequestDiff do
   let(:diff_with_commits) { create(:merge_request).merge_request_diff }
+  describe 'validations' do
+    subject { diff_with_commits }
+
+    it 'checks sha format of base_commit_sha, head_commit_sha and start_commit_sha' do
+      subject.base_commit_sha = subject.head_commit_sha = subject.start_commit_sha = 'foobar'
+
+      expect(subject.valid?).to be false
+      expect(subject.errors.count).to eq 3
+      expect(subject.errors).to all(include('is not a valid SHA'))
+    end
+  end
+
   describe 'create new record' do
     subject { diff_with_commits }
@@ -78,7 +90,7 @@ describe MergeRequestDiff do
       it 'returns persisted diffs if cannot compare with diff refs' do
         expect(diff).to receive(:load_diffs).and_call_original
-        diff.update!(head_commit_sha: 'invalid-sha')
+        diff.update!(head_commit_sha: Digest::SHA1.hexdigest(SecureRandom.hex))
         diff.diffs.diff_files
       end

spec/validators/sha_validator_spec.rb

+require 'spec_helper'
+
+describe ShaValidator do
+  let(:validator) { described_class.new(attributes: [:base_commit_sha]) }
+  let(:merge_diff) { build(:merge_request_diff) }
+
+  subject { validator.validate_each(merge_diff, :base_commit_sha, value) }
+
+  context 'with empty value' do
+    let(:value) { nil }
+
+    it 'does not add any error if value is empty' do
+      subject
+
+      expect(merge_diff.errors).to be_empty
+    end
+  end
+
+  context 'with valid sha' do
+    let(:value) { Digest::SHA1.hexdigest(SecureRandom.hex) }
+
+    it 'does not add any error if value is empty' do
+      subject
+
+      expect(merge_diff.errors).to be_empty
+    end
+  end
+
+  context 'with invalid sha' do
+    let(:value) { 'foo' }
+
+    it 'adds error to the record' do
+      expect(merge_diff.errors).to be_empty
+
+      subject
+
+      expect(merge_diff.errors).not_to be_empty
+    end
+  end
+end

spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb

@@ -18,7 +18,7 @@ describe UpdateHeadPipelineForMergeRequestWorker do
       context 'when merge request sha does not equal pipeline sha' do
         before do
-          merge_request.merge_request_diff.update(head_commit_sha: 'different_sha')
+          merge_request.merge_request_diff.update(head_commit_sha: Digest::SHA1.hexdigest(SecureRandom.hex))
         end
         it 'does not update head pipeline' do