Arbitrary file read via MergeRequestDiff

5 files changed, 75 insertions, 3 deletions

diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb
index 63d8decc2d2..70ad10c830c 100644
--- a/spec/features/merge_request/user_sees_versions_spec.rb
+++ b/spec/features/merge_request/user_sees_versions_spec.rb
@@ -1,7 +1,11 @@
 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') }
diff --git a/spec/lib/gitlab/import_export/merge_request_parser_spec.rb b/spec/lib/gitlab/import_export/merge_request_parser_spec.rb
index 68eaa70e6b6..4b234411a44 100644
--- a/spec/lib/gitlab/import_export/merge_request_parser_spec.rb
+++ b/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
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 33e984dc399..42ad2811b22 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/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
diff --git a/spec/validators/sha_validator_spec.rb b/spec/validators/sha_validator_spec.rb
new file mode 100644
index 00000000000..b9242ef931e
--- /dev/null
+++ b/spec/validators/sha_validator_spec.rb
@@ -0,0 +1,40 @@
+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
diff --git a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
index 963237ceadf..f29e49f202a 100644
--- a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
+++ b/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