Merge branch 'fix/id-claim-import-issue' into 'master'

Prevent claiming associated model IDs via import

On the import side, we should be careful not to use any IDs as part of the JSON file that could have been manipulated.

Part of https://gitlab.com/gitlab-org/gitlab-ce/issues/20821


Things we already do (__before__ this fix):

1. Remove all primary keys
1. **Always** reassign some of the foreign keys, such as ALL project IDs and user IDs (so it would be difficult to impersonate or try to gain access to another project)
1. Ignore/reject attributes that do not exist in the model
1. If someone reassigns a foreign key `submodel_id`, and that object has another json as the submodel, the new submodel will reassign the `submodel_id` to the newly created submodel ID.

Things we should do:

1. Remove/nullify any other foreign keys that we don't reassign (checked this, and there aren't many, fortunately. In fact, I don't think much harm can be done at all - at the moment).

See merge request !1985

5 files changed, 184 insertions, 5 deletions

diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb
new file mode 100644
index 00000000000..b9e4042220a
--- /dev/null
+++ b/lib/gitlab/import_export/attribute_cleaner.rb
@@ -0,0 +1,13 @@
+module Gitlab
+  module ImportExport
+    class AttributeCleaner
+      ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES
+
+      def self.clean!(relation_hash:)
+        relation_hash.reject! do |key, _value|
+          key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb
index 35ff134ea19..5a109f24f9f 100644
--- a/lib/gitlab/import_export/project_tree_restorer.rb
+++ b/lib/gitlab/import_export/project_tree_restorer.rb
@@ -110,9 +110,10 @@ module Gitlab
       def create_relation(relation, relation_hash_list)
         relation_array = [relation_hash_list].flatten.map do |relation_hash|
           Gitlab::ImportExport::RelationFactory.create(relation_sym: relation.to_sym,
-                                                       relation_hash: relation_hash.merge('project_id' => restored_project.id),
+                                                       relation_hash: relation_hash,
                                                        members_mapper: members_mapper,
-                                                       user: @user)
+                                                       user: @user,
+                                                       project_id: restored_project.id)
         end
 
         relation_hash_list.is_a?(Array) ? relation_array : relation_array.first
diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb
index 354ccd64696..9300f789e1b 100644
--- a/lib/gitlab/import_export/relation_factory.rb
+++ b/lib/gitlab/import_export/relation_factory.rb
@@ -13,6 +13,8 @@ module Gitlab
 
       USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id].freeze
 
+      PROJECT_REFERENCES = %w[project_id source_project_id gl_project_id target_project_id].freeze
+
       BUILD_MODELS = %w[Ci::Build commit_status].freeze
 
       IMPORTED_OBJECT_MAX_RETRIES = 5.freeze
@@ -25,9 +27,9 @@ module Gitlab
         new(*args).create
       end
 
-      def initialize(relation_sym:, relation_hash:, members_mapper:, user:)
+      def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project_id:)
         @relation_name = OVERRIDES[relation_sym] || relation_sym
-        @relation_hash = relation_hash.except('id', 'noteable_id')
+        @relation_hash = relation_hash.except('id', 'noteable_id').merge('project_id' => project_id)
         @members_mapper = members_mapper
         @user = user
         @imported_object_retries = 0
@@ -153,7 +155,11 @@ module Gitlab
       end
 
       def parsed_relation_hash
-        @parsed_relation_hash ||= @relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) }
+        @parsed_relation_hash ||= begin
+          Gitlab::ImportExport::AttributeCleaner.clean!(relation_hash: @relation_hash)
+
+          @relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) }
+        end
       end
 
       def set_st_diffs
diff --git a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
new file mode 100644
index 00000000000..b8e7932eb4a
--- /dev/null
+++ b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe Gitlab::ImportExport::AttributeCleaner, lib: true do
+  let(:unsafe_hash) do
+    {
+      'service_id' => 99,
+      'moved_to_id' => 99,
+      'namespace_id' => 99,
+      'ci_id' => 99,
+      'random_project_id' => 99,
+      'random_id' => 99,
+      'milestone_id' => 99,
+      'project_id' => 99,
+      'user_id' => 99,
+      'random_id_in_the_middle' => 99,
+      'notid' => 99
+    }
+  end
+
+  let(:post_safe_hash) do
+    {
+      'project_id' => 99,
+      'user_id' => 99,
+      'random_id_in_the_middle' => 99,
+      'notid' => 99
+    }
+  end
+
+  it 'removes unwanted attributes from the hash' do
+    described_class.clean!(relation_hash: unsafe_hash)
+
+    expect(unsafe_hash).to eq(post_safe_hash)
+  end
+end
diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb
new file mode 100644
index 00000000000..3aa492a8ab1
--- /dev/null
+++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb
@@ -0,0 +1,125 @@
+require 'spec_helper'
+
+describe Gitlab::ImportExport::RelationFactory, lib: true do
+  let(:project) { create(:empty_project) }
+  let(:members_mapper) { double('members_mapper').as_null_object }
+  let(:user) { create(:user) }
+  let(:created_object) do
+    described_class.create(relation_sym: relation_sym,
+                           relation_hash: relation_hash,
+                           members_mapper: members_mapper,
+                           user: user,
+                           project_id: project.id)
+  end
+
+  context 'hook object' do
+    let(:relation_sym) { :hooks }
+    let(:id) { 999 }
+    let(:service_id) { 99 }
+    let(:original_project_id) { 8 }
+    let(:token) { 'secret' }
+
+    let(:relation_hash) do
+      {
+        'id' => id,
+        'url' => 'https://example.json',
+        'project_id' => original_project_id,
+        'created_at' => '2016-08-12T09:41:03.462Z',
+        'updated_at' => '2016-08-12T09:41:03.462Z',
+        'service_id' => service_id,
+        'push_events' => true,
+        'issues_events' => false,
+        'merge_requests_events' => true,
+        'tag_push_events' => false,
+        'note_events' => true,
+        'enable_ssl_verification' => true,
+        'build_events' => false,
+        'wiki_page_events' => true,
+        'token' => token
+      }
+    end
+
+    it 'does not have the original ID' do
+      expect(created_object.id).not_to eq(id)
+    end
+
+    it 'does not have the original service_id' do
+      expect(created_object.service_id).not_to eq(service_id)
+    end
+
+    it 'does not have the original project_id' do
+      expect(created_object.project_id).not_to eq(original_project_id)
+    end
+
+    it 'has the new project_id' do
+      expect(created_object.project_id).to eq(project.id)
+    end
+
+    it 'has a token' do
+      expect(created_object.token).to eq(token)
+    end
+
+    context 'original service exists' do
+      let(:service_id) { Service.create(project: project).id }
+
+      it 'does not have the original service_id' do
+        expect(created_object.service_id).not_to eq(service_id)
+      end
+    end
+  end
+
+  # Mocks an ActiveRecordish object with the dodgy columns
+  class FooModel
+    include ActiveModel::Model
+
+    def initialize(params)
+      params.each { |key, value| send("#{key}=", value) }
+    end
+
+    def values
+      instance_variables.map { |ivar| instance_variable_get(ivar) }
+    end
+  end
+
+  # `project_id`, `described_class.USER_REFERENCES`, noteable_id, target_id, and some project IDs are already
+  # re-assigned by described_class.
+  context 'Potentially hazardous foreign keys' do
+    let(:relation_sym) { :hazardous_foo_model }
+    let(:relation_hash) do
+      {
+        'service_id' => 99,
+        'moved_to_id' => 99,
+        'namespace_id' => 99,
+        'ci_id' => 99,
+        'random_project_id' => 99,
+        'random_id' => 99,
+        'milestone_id' => 99,
+        'project_id' => 99,
+        'user_id' => 99,
+      }
+    end
+
+    class HazardousFooModel < FooModel
+      attr_accessor :service_id, :moved_to_id, :namespace_id, :ci_id, :random_project_id, :random_id, :milestone_id, :project_id
+    end
+
+    it 'does not preserve any foreign key IDs' do
+      expect(created_object.values).not_to include(99)
+    end
+  end
+
+  context 'Project references' do
+    let(:relation_sym) { :project_foo_model }
+    let(:relation_hash) do
+      Gitlab::ImportExport::RelationFactory::PROJECT_REFERENCES.map { |ref| { ref => 99 } }.inject(:merge)
+    end
+
+    class ProjectFooModel < FooModel
+      attr_accessor(*Gitlab::ImportExport::RelationFactory::PROJECT_REFERENCES)
+    end
+
+    it 'does not preserve any project foreign key IDs' do
+      expect(created_object.values).not_to include(99)
+    end
+  end
+end