Commit 12970a8a authored by Stan Hu's avatar Stan Hu

Merge branch 'strong-validate-import-export-references' into 'master'

Strong validate import export references

Closes #35339

See merge request gitlab-org/gitlab!19682
parents 5a6ebe09 28acc1c0
---
title: Strong validate import export references
merge_request: 19682
author:
type: added
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module Gitlab module Gitlab
module ImportExport module ImportExport
class AttributeCleaner class AttributeCleaner
ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + %w[group_id commit_id] ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + %w[group_id commit_id discussion_id]
PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_ids\Z/, /_html\Z/).freeze PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_ids\Z/, /_html\Z/).freeze
def self.clean(*args) def self.clean(*args)
......
...@@ -172,24 +172,36 @@ excluded_attributes: ...@@ -172,24 +172,36 @@ excluded_attributes:
- :external_diff - :external_diff
- :stored_externally - :stored_externally
- :external_diff_store - :external_diff_store
- :merge_request_id
merge_request_diff_commits:
- :merge_request_diff_id
merge_request_diff_files: merge_request_diff_files:
- :diff - :diff
- :external_diff_offset - :external_diff_offset
- :external_diff_size - :external_diff_size
- :merge_request_diff_id
issues: issues:
- :milestone_id - :milestone_id
- :moved_to_id
- :state_id
- :duplicated_to_id
- :promoted_to_epic_id
merge_request: merge_request:
- :milestone_id - :milestone_id
- :ref_fetched - :ref_fetched
- :merge_jid - :merge_jid
- :rebase_jid - :rebase_jid
- :latest_merge_request_diff_id - :latest_merge_request_diff_id
- :head_pipeline_id
- :state_id
merge_requests: merge_requests:
- :milestone_id - :milestone_id
- :ref_fetched - :ref_fetched
- :merge_jid - :merge_jid
- :rebase_jid - :rebase_jid
- :latest_merge_request_diff_id - :latest_merge_request_diff_id
- :head_pipeline_id
- :state_id
award_emoji: award_emoji:
- :awardable_id - :awardable_id
statuses: statuses:
...@@ -203,6 +215,16 @@ excluded_attributes: ...@@ -203,6 +215,16 @@ excluded_attributes:
- :artifacts_metadata_store - :artifacts_metadata_store
- :artifacts_size - :artifacts_size
- :commands - :commands
- :runner_id
- :trigger_request_id
- :erased_by_id
- :auto_canceled_by_id
- :stage_id
- :upstream_pipeline_id
- :resource_group_id
- :waiting_for_resource_at
sentry_issue:
- :issue_id
push_event_payload: push_event_payload:
- :event_id - :event_id
project_badges: project_badges:
...@@ -211,6 +233,9 @@ excluded_attributes: ...@@ -211,6 +233,9 @@ excluded_attributes:
- :reference - :reference
- :reference_html - :reference_html
- :epic_id - :epic_id
- :issue_id
- :merge_request_id
- :label_id
runners: runners:
- :token - :token
- :token_encrypted - :token_encrypted
...@@ -222,7 +247,64 @@ excluded_attributes: ...@@ -222,7 +247,64 @@ excluded_attributes:
- :enabled - :enabled
service_desk_setting: service_desk_setting:
- :outgoing_name - :outgoing_name
priorities:
- :label_id
events:
- :target_id
timelogs:
- :issue_id
- :merge_request_id
notes:
- :noteable_id
- :review_id
label_links:
- :label_id
- :target_id
issue_assignees:
- :issue_id
zoom_meetings:
- :issue_id
design:
- :issue_id
designs:
- :issue_id
design_versions:
- :issue_id
actions:
- :design_id
- :version_id
links:
- :release_id
project_members:
- :source_id
metrics:
- :merge_request_id
- :pipeline_id
suggestions:
- :note_id
ci_pipelines:
- :auto_canceled_by_id
- :pipeline_schedule_id
- :merge_request_id
- :external_pull_request_id
stages:
- :pipeline_id
merge_access_levels:
- :protected_branch_id
push_access_levels:
- :protected_branch_id
unprotect_access_levels:
- :protected_branch_id
create_access_levels:
- :protected_tag_id
deploy_access_levels:
- :protected_environment_id
boards:
- :milestone_id
lists:
- :board_id
- :label_id
- :milestone_id
methods: methods:
notes: notes:
- :type - :type
......
...@@ -12,8 +12,6 @@ describe 'Import/Export - project export integration test', :js do ...@@ -12,8 +12,6 @@ describe 'Import/Export - project export integration test', :js do
let(:user) { create(:admin) } let(:user) { create(:admin) }
let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } let(:export_path) { "#{Dir.tmpdir}/import_file_spec" }
let(:config_hash) { Gitlab::ImportExport::Config.new.to_h.deep_stringify_keys }
let(:sensitive_words) { %w[pass secret token key encrypted html] } let(:sensitive_words) { %w[pass secret token key encrypted html] }
let(:safe_list) do let(:safe_list) do
{ {
......
...@@ -12,21 +12,11 @@ require 'spec_helper' ...@@ -12,21 +12,11 @@ require 'spec_helper'
describe 'Import/Export attribute configuration' do describe 'Import/Export attribute configuration' do
include ConfigurationHelper include ConfigurationHelper
let(:config_hash) { Gitlab::ImportExport::Config.new.to_h.deep_stringify_keys }
let(:relation_names) do
names = names_from_tree(config_hash.dig('tree', 'project'))
# Remove duplicated or add missing models
# - project is not part of the tree, so it has to be added manually.
# - milestone, labels have both singular and plural versions in the tree, so remove the duplicates.
names.flatten.uniq - %w(milestones labels) + ['project']
end
let(:safe_attributes_file) { 'spec/lib/gitlab/import_export/safe_model_attributes.yml' } let(:safe_attributes_file) { 'spec/lib/gitlab/import_export/safe_model_attributes.yml' }
let(:safe_model_attributes) { YAML.load_file(safe_attributes_file) } let(:safe_model_attributes) { YAML.load_file(safe_attributes_file) }
it 'has no new columns' do it 'has no new columns' do
relation_names.each do |relation_name| relation_names_for(:project).each do |relation_name|
relation_class = relation_class_for_name(relation_name) relation_class = relation_class_for_name(relation_name)
relation_attributes = relation_class.new.attributes.keys - relation_class.encrypted_attributes.keys.map(&:to_s) relation_attributes = relation_class.new.attributes.keys - relation_class.encrypted_attributes.keys.map(&:to_s)
......
...@@ -8,19 +8,10 @@ require 'spec_helper' ...@@ -8,19 +8,10 @@ require 'spec_helper'
describe 'Import/Export model configuration' do describe 'Import/Export model configuration' do
include ConfigurationHelper include ConfigurationHelper
let(:config_hash) { Gitlab::ImportExport::Config.new.to_h.deep_stringify_keys }
let(:model_names) do
names = names_from_tree(config_hash.dig('tree', 'project'))
# Remove duplicated or add missing models
# - project is not part of the tree, so it has to be added manually.
# - milestone, labels, merge_request have both singular and plural versions in the tree, so remove the duplicates.
# - User, Author... Models we do not care about for checking models
names.flatten.uniq - %w(milestones labels user author merge_request) + ['project']
end
let(:all_models_yml) { 'spec/lib/gitlab/import_export/all_models.yml' } let(:all_models_yml) { 'spec/lib/gitlab/import_export/all_models.yml' }
let(:all_models_hash) { YAML.load_file(all_models_yml) } let(:all_models_hash) { YAML.load_file(all_models_yml) }
let(:current_models) { setup_models } let(:current_models) { setup_models }
let(:model_names) { relation_names_for(:project) }
it 'has no new models' do it 'has no new models' do
model_names.each do |model_name| model_names.each do |model_name|
......
# frozen_string_literal: true
require 'spec_helper'
# Part of the test security suite for the Import/Export feature
# Checks whether there are new reference attributes ending with _id in models that are currently being exported as part of the
# project Import/Export feature.
# If there are new references (foreign keys), these will have to either be replaced with actual relation
# or to be blacklisted by using the import_export.yml configuration file.
# Likewise, new models added to import_export.yml, will need to be added with their correspondent relations
# to this spec.
describe 'Import/Export Project configuration' do
include ConfigurationHelper
where(:relation_path, :relation_name) do
relation_paths_for(:project).map do |relation_names|
next if relation_names.last == :author
[relation_names.join("."), relation_names.last]
end.compact
end
with_them do
context "where relation #{params[:relation_path]}" do
it 'does not have prohibited keys' do
relation_class = relation_class_for_name(relation_name)
relation_attributes = relation_class.new.attributes.keys - relation_class.encrypted_attributes.keys.map(&:to_s)
current_attributes = parsed_attributes(relation_name, relation_attributes)
prohibited_keys = current_attributes.select do |attribute|
prohibited_key?(attribute) || !relation_class.attribute_method?(attribute)
end
expect(prohibited_keys).to be_empty, failure_message(relation_class.to_s, prohibited_keys)
end
end
end
def failure_message(relation_class, prohibited_keys)
<<-MSG
It looks like #{relation_class}, which is exported using the project Import/Export, has references: #{prohibited_keys.join(',')}
Please replace it with actual relation in IMPORT_EXPORT_CONFIG if you consider this can be exported.
Please blacklist the attribute(s) in IMPORT_EXPORT_CONFIG by adding it to its correspondent
model in the +excluded_attributes+ section.
IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file}
MSG
end
end
...@@ -33,7 +33,6 @@ Issue: ...@@ -33,7 +33,6 @@ Issue:
Event: Event:
- id - id
- target_type - target_type
- target_id
- project_id - project_id
- group_id - group_id
- created_at - created_at
...@@ -60,7 +59,6 @@ Note: ...@@ -60,7 +59,6 @@ Note:
- attachment - attachment
- line_code - line_code
- commit_id - commit_id
- noteable_id
- system - system
- st_diff - st_diff
- updated_by_id - updated_by_id
...@@ -73,11 +71,8 @@ Note: ...@@ -73,11 +71,8 @@ Note:
- resolved_by_push - resolved_by_push
- discussion_id - discussion_id
- original_discussion_id - original_discussion_id
- review_id
LabelLink: LabelLink:
- id - id
- label_id
- target_id
- target_type - target_type
- created_at - created_at
- updated_at - updated_at
...@@ -130,13 +125,11 @@ Release: ...@@ -130,13 +125,11 @@ Release:
- released_at - released_at
Evidence: Evidence:
- id - id
- release_id
- summary - summary
- created_at - created_at
- updated_at - updated_at
Releases::Link: Releases::Link:
- id - id
- release_id
- url - url
- name - name
- created_at - created_at
...@@ -144,7 +137,6 @@ Releases::Link: ...@@ -144,7 +137,6 @@ Releases::Link:
ProjectMember: ProjectMember:
- id - id
- access_level - access_level
- source_id
- source_type - source_type
- user_id - user_id
- notification_level - notification_level
...@@ -600,7 +592,6 @@ AwardEmoji: ...@@ -600,7 +592,6 @@ AwardEmoji:
LabelPriority: LabelPriority:
- id - id
- project_id - project_id
- label_id
- priority - priority
- created_at - created_at
- updated_at - updated_at
...@@ -608,7 +599,6 @@ Timelog: ...@@ -608,7 +599,6 @@ Timelog:
- id - id
- time_spent - time_spent
- merge_request_id - merge_request_id
- issue_id
- user_id - user_id
- spent_at - spent_at
- created_at - created_at
...@@ -623,7 +613,6 @@ ProjectAutoDevops: ...@@ -623,7 +613,6 @@ ProjectAutoDevops:
- updated_at - updated_at
IssueAssignee: IssueAssignee:
- user_id - user_id
- issue_id
ProjectCustomAttribute: ProjectCustomAttribute:
- id - id
- created_at - created_at
...@@ -679,7 +668,6 @@ ProtectedEnvironment::DeployAccessLevel: ...@@ -679,7 +668,6 @@ ProtectedEnvironment::DeployAccessLevel:
ResourceLabelEvent: ResourceLabelEvent:
- id - id
- action - action
- issue_id
- merge_request_id - merge_request_id
- label_id - label_id
- user_id - user_id
...@@ -691,11 +679,9 @@ ErrorTracking::ProjectErrorTrackingSetting: ...@@ -691,11 +679,9 @@ ErrorTracking::ProjectErrorTrackingSetting:
- organization_name - organization_name
SentryIssue: SentryIssue:
- id - id
- issue_id
- sentry_issue_identifier - sentry_issue_identifier
Suggestion: Suggestion:
- id - id
- note_id
- relative_order - relative_order
- applied - applied
- commit_id - commit_id
...@@ -750,21 +736,16 @@ ExternalPullRequest: ...@@ -750,21 +736,16 @@ ExternalPullRequest:
DesignManagement::Design: DesignManagement::Design:
- id - id
- project_id - project_id
- issue_id
- filename - filename
DesignManagement::Action: DesignManagement::Action:
- design_id
- event - event
- version_id
DesignManagement::Version: DesignManagement::Version:
- id - id
- created_at - created_at
- sha - sha
- issue_id
- author_id - author_id
ZoomMeeting: ZoomMeeting:
- id - id
- issue_id
- project_id - project_id
- issue_status - issue_status
- url - url
......
...@@ -10,21 +10,54 @@ module ConfigurationHelper ...@@ -10,21 +10,54 @@ module ConfigurationHelper
end end
end end
def all_relations(tree, tree_path = [])
tree.flat_map do |relation_name, relations|
relation_path = tree_path + [relation_name]
[relation_path] + all_relations(relations, relation_path)
end
end
def config_hash(config = Gitlab::ImportExport.config_file)
Gitlab::ImportExport::Config.new(config: config).to_h
end
def relation_paths_for(key, config: Gitlab::ImportExport.config_file)
# - project is not part of the tree, so it has to be added manually.
all_relations({ project: config_hash(config).dig(:tree, key) })
end
def relation_names_for(key, config: Gitlab::ImportExport.config_file)
names = names_from_tree(config_hash(config).dig(:tree, key))
# Remove duplicated or add missing models
# - project is not part of the tree, so it has to be added manually.
# - milestone, labels, merge_request have both singular and plural versions in the tree, so remove the duplicates.
# - User, Author... Models we do not care about for checking models
names.flatten.uniq - %w(milestones labels user author merge_request design) + [key.to_s]
end
def relation_class_for_name(relation_name) def relation_class_for_name(relation_name)
relation_name = Gitlab::ImportExport::RelationFactory.overrides[relation_name.to_sym] || relation_name relation_name = Gitlab::ImportExport::RelationFactory.overrides[relation_name.to_sym] || relation_name
Gitlab::ImportExport::RelationFactory.relation_class(relation_name) Gitlab::ImportExport::RelationFactory.relation_class(relation_name)
end end
def parsed_attributes(relation_name, attributes) def parsed_attributes(relation_name, attributes, config: Gitlab::ImportExport.config_file)
excluded_attributes = config_hash['excluded_attributes'][relation_name] import_export_config = config_hash(config)
included_attributes = config_hash['included_attributes'][relation_name] excluded_attributes = import_export_config[:excluded_attributes][relation_name.to_sym]
included_attributes = import_export_config[:included_attributes][relation_name.to_sym]
attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes
attributes = attributes & JSON[included_attributes.to_json] if included_attributes attributes = attributes & JSON[included_attributes.to_json] if included_attributes
attributes attributes
end end
def prohibited_key?(key)
key =~ Gitlab::ImportExport::AttributeCleaner::PROHIBITED_REFERENCES && !permitted_key?(key)
end
def permitted_key?(key)
Gitlab::ImportExport::AttributeCleaner::ALLOWED_REFERENCES.include?(key)
end
def associations_for(safe_model) def associations_for(safe_model)
safe_model.reflect_on_all_associations.map { |assoc| assoc.name.to_s } safe_model.reflect_on_all_associations.map { |assoc| assoc.name.to_s }
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment