Merge branch 'security-fj-import-export-assignment' into 'master'
[master] Arbitrary assignment of Project fields using "Import project" from a Gitlab tar.gz Closes #2678 See merge request gitlab/gitlabhq!2378
This commit is contained in:
commit
0033e57269
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Fixed bug that allowed importing arbitrary project attributes
|
||||
merge_request:
|
||||
author:
|
||||
type: security
|
|
@ -7,14 +7,15 @@ module Gitlab
|
|||
new(*args).clean
|
||||
end
|
||||
|
||||
def initialize(relation_hash:, relation_class:)
|
||||
def initialize(relation_hash:, relation_class:, excluded_keys: [])
|
||||
@relation_hash = relation_hash
|
||||
@relation_class = relation_class
|
||||
@excluded_keys = excluded_keys
|
||||
end
|
||||
|
||||
def clean
|
||||
@relation_hash.reject do |key, _value|
|
||||
prohibited_key?(key) || !@relation_class.attribute_method?(key)
|
||||
prohibited_key?(key) || !@relation_class.attribute_method?(key) || excluded_key?(key)
|
||||
end.except('id')
|
||||
end
|
||||
|
||||
|
@ -23,6 +24,12 @@ module Gitlab
|
|||
def prohibited_key?(key)
|
||||
key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
|
||||
end
|
||||
|
||||
def excluded_key?(key)
|
||||
return false if @excluded_keys.empty?
|
||||
|
||||
@excluded_keys.include?(key)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -32,6 +32,10 @@ module Gitlab
|
|||
@methods[key].nil? ? {} : { methods: @methods[key] }
|
||||
end
|
||||
|
||||
def find_excluded_keys(klass_name)
|
||||
@excluded_attributes[klass_name.to_sym]&.map(&:to_s) || []
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def find_attributes_only(value)
|
||||
|
|
|
@ -98,8 +98,6 @@ excluded_attributes:
|
|||
- :import_jid
|
||||
- :created_at
|
||||
- :updated_at
|
||||
- :import_jid
|
||||
- :import_jid
|
||||
- :id
|
||||
- :star_count
|
||||
- :last_activity_at
|
||||
|
|
|
@ -88,16 +88,18 @@ module Gitlab
|
|||
end
|
||||
|
||||
def project_params
|
||||
@project_params ||= json_params.merge(override_params)
|
||||
@project_params ||= begin
|
||||
attrs = json_params.merge(override_params)
|
||||
|
||||
# Cleaning all imported and overridden params
|
||||
Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: attrs,
|
||||
relation_class: Project,
|
||||
excluded_keys: excluded_keys_for_relation(:project))
|
||||
end
|
||||
end
|
||||
|
||||
def override_params
|
||||
return {} unless params = @project.import_data&.data&.fetch('override_params', nil)
|
||||
|
||||
@override_params ||= params.select do |key, _value|
|
||||
Project.column_names.include?(key.to_s) &&
|
||||
!reader.project_tree[:except].include?(key.to_sym)
|
||||
end
|
||||
@override_params ||= @project.import_data&.data&.fetch('override_params', nil) || {}
|
||||
end
|
||||
|
||||
def json_params
|
||||
|
@ -171,7 +173,8 @@ module Gitlab
|
|||
relation_hash: parsed_relation_hash(relation_hash, relation.to_sym),
|
||||
members_mapper: members_mapper,
|
||||
user: @user,
|
||||
project: @restored_project)
|
||||
project: @restored_project,
|
||||
excluded_keys: excluded_keys_for_relation(relation))
|
||||
end.compact
|
||||
|
||||
relation_hash_list.is_a?(Array) ? relation_array : relation_array.first
|
||||
|
@ -192,6 +195,10 @@ module Gitlab
|
|||
def reader
|
||||
@reader ||= Gitlab::ImportExport::Reader.new(shared: @shared)
|
||||
end
|
||||
|
||||
def excluded_keys_for_relation(relation)
|
||||
@reader.attributes_finder.find_excluded_keys(relation)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
module Gitlab
|
||||
module ImportExport
|
||||
class Reader
|
||||
attr_reader :tree
|
||||
attr_reader :tree, :attributes_finder
|
||||
|
||||
def initialize(shared:)
|
||||
@shared = shared
|
||||
|
|
|
@ -36,13 +36,21 @@ module Gitlab
|
|||
new(*args).create
|
||||
end
|
||||
|
||||
def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:)
|
||||
def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:, excluded_keys: [])
|
||||
@relation_name = OVERRIDES[relation_sym] || relation_sym
|
||||
@relation_hash = relation_hash.except('noteable_id')
|
||||
@members_mapper = members_mapper
|
||||
@user = user
|
||||
@project = project
|
||||
@imported_object_retries = 0
|
||||
|
||||
# Remove excluded keys from relation_hash
|
||||
# We don't do this in the parsed_relation_hash because of the 'transformed attributes'
|
||||
# For example, MergeRequestDiffFiles exports its diff attribute as utf8_diff. Then,
|
||||
# in the create method that attribute is renamed to diff. And because diff is an excluded key,
|
||||
# if we clean the excluded keys in the parsed_relation_hash, it will be removed
|
||||
# from the object attributes and the export will fail.
|
||||
@relation_hash.except!(*excluded_keys)
|
||||
end
|
||||
|
||||
# Creates an object from an actual model with name "relation_sym" with params from
|
||||
|
|
|
@ -15,7 +15,10 @@ describe Gitlab::ImportExport::AttributeCleaner do
|
|||
'project_id' => 99,
|
||||
'user_id' => 99,
|
||||
'random_id_in_the_middle' => 99,
|
||||
'notid' => 99
|
||||
'notid' => 99,
|
||||
'import_source' => 'whatever',
|
||||
'import_type' => 'whatever',
|
||||
'non_existent_attr' => 'whatever'
|
||||
}
|
||||
end
|
||||
|
||||
|
@ -28,10 +31,30 @@ describe Gitlab::ImportExport::AttributeCleaner do
|
|||
}
|
||||
end
|
||||
|
||||
let(:excluded_keys) { %w[import_source import_type] }
|
||||
|
||||
subject { described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class, excluded_keys: excluded_keys) }
|
||||
|
||||
before do
|
||||
allow(relation_class).to receive(:attribute_method?).and_return(true)
|
||||
allow(relation_class).to receive(:attribute_method?).with('non_existent_attr').and_return(false)
|
||||
end
|
||||
|
||||
it 'removes unwanted attributes from the hash' do
|
||||
# allow(relation_class).to receive(:attribute_method?).and_return(true)
|
||||
expect(subject).to eq(post_safe_hash)
|
||||
end
|
||||
|
||||
it 'removes attributes not present in relation_class' do
|
||||
expect(subject.keys).not_to include 'non_existent_attr'
|
||||
end
|
||||
|
||||
it 'removes excluded keys from the hash' do
|
||||
expect(subject.keys).not_to include excluded_keys
|
||||
end
|
||||
|
||||
it 'does not remove excluded key if not listed' do
|
||||
parsed_hash = described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class)
|
||||
|
||||
expect(parsed_hash).to eq(post_safe_hash)
|
||||
expect(parsed_hash.keys).to eq post_safe_hash.keys + excluded_keys
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,5 +1,7 @@
|
|||
{
|
||||
"description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
|
||||
"import_type": "gitlab_project",
|
||||
"creator_id": 123,
|
||||
"visibility_level": 10,
|
||||
"archived": false,
|
||||
"labels": [
|
||||
|
|
|
@ -1,5 +1,7 @@
|
|||
{
|
||||
"description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
|
||||
"import_type": "gitlab_project",
|
||||
"creator_id": 123,
|
||||
"visibility_level": 10,
|
||||
"archived": false,
|
||||
"milestones": [
|
||||
|
|
|
@ -23,6 +23,10 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
|
|||
allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch)
|
||||
|
||||
project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project)
|
||||
|
||||
expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: ['whatever'])).and_call_original.at_least(:once)
|
||||
allow(project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever'])
|
||||
|
||||
@restored_project_json = project_tree_restorer.restore
|
||||
end
|
||||
end
|
||||
|
@ -248,6 +252,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
|
|||
expect(labels.where(type: "ProjectLabel").count).to eq(results.fetch(:first_issue_labels, 0))
|
||||
expect(labels.where(type: "ProjectLabel").where.not(group_id: nil).count).to eq(0)
|
||||
end
|
||||
|
||||
it 'does not set params that are excluded from import_export settings' do
|
||||
expect(project.import_type).to be_nil
|
||||
expect(project.creator_id).not_to eq 123
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'restores group correctly' do |**results|
|
||||
|
|
|
@ -4,12 +4,14 @@ describe Gitlab::ImportExport::RelationFactory do
|
|||
let(:project) { create(:project) }
|
||||
let(:members_mapper) { double('members_mapper').as_null_object }
|
||||
let(:user) { create(:admin) }
|
||||
let(:excluded_keys) { [] }
|
||||
let(:created_object) do
|
||||
described_class.create(relation_sym: relation_sym,
|
||||
relation_hash: relation_hash,
|
||||
members_mapper: members_mapper,
|
||||
user: user,
|
||||
project: project)
|
||||
project: project,
|
||||
excluded_keys: excluded_keys)
|
||||
end
|
||||
|
||||
context 'hook object' do
|
||||
|
@ -67,6 +69,14 @@ describe Gitlab::ImportExport::RelationFactory do
|
|||
expect(created_object.service_id).not_to eq(service_id)
|
||||
end
|
||||
end
|
||||
|
||||
context 'excluded attributes' do
|
||||
let(:excluded_keys) { %w[url] }
|
||||
|
||||
it 'are removed from the imported object' do
|
||||
expect(created_object.url).to be_nil
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# Mocks an ActiveRecordish object with the dodgy columns
|
||||
|
|
Loading…
Reference in New Issue