diff --git a/spec/features/projects/import_export/export_file_spec.rb b/spec/features/projects/import_export/export_file_spec.rb index 1a04c25012b..0311edfa133 100644 --- a/spec/features/projects/import_export/export_file_spec.rb +++ b/spec/features/projects/import_export/export_file_spec.rb @@ -1,18 +1,25 @@ require 'spec_helper' +# Integration test that exports a file using the Import/EXport feature +# It looks up for any sensitive word inside the JSON, so if a sensitive word is found +# we''l have to either include it adding the model containing it to the +safe_list+ +# or make sure the attribute is blacklisted in the +import_export.yml+ configuration feature 'project export', feature: true, js: true do include Select2Helper include ExportFileHelper let(:user) { create(:admin) } let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } let(:sensitive_words) { %w[pass secret token key] } - let(:safe_models) do + let(:safe_list) do { - token: [ProjectHook, Ci::Trigger] + token: [ProjectHook, Ci::Trigger], + key: [Project, Ci::Variable, :yaml_variables] } end + let(:safe_hashes) { { yaml_variables: %w[key value public] } } let(:project) { setup_project } @@ -49,7 +56,9 @@ feature 'project export', feature: true, js: true do project_hash = JSON.parse(IO.read(project_json_path)) sensitive_words.each do |sensitive_word| - expect(has_sensitive_attributes?(sensitive_word, project_hash)).to be false + found = find_sensitive_attributes(sensitive_word, project_hash) + + expect(found).to be_nil, "Found a new sensitive word <#{found.try(:key_found)}>, which is part of the hash #{found.try(:parent)}" end end end diff --git a/spec/lib/gitlab/import_export/all_models.json b/spec/lib/gitlab/import_export/all_models.json deleted file mode 100644 index 81ebeeb64f9..00000000000 --- a/spec/lib/gitlab/import_export/all_models.json +++ /dev/null @@ -1,196 +0,0 @@ -{ - "issues": [ - "subscriptions", - "award_emoji", - "author", - "assignee", - "updated_by", - "milestone", - "notes", - "label_links", - "labels", - "todos", - "user_agent_detail", - "project", - "moved_to", - "events" - ], - "events": [ - "author", - "project", - "target" - ], - "notes": [ - "award_emoji", - "project", - "noteable", - "author", - "updated_by", - "resolved_by", - "todos", - "events" - ], - "label_links": [ - "target", - "label" - ], - "label": [ - "subscriptions", - "project", - "lists", - "label_links", - "issues", - "merge_requests" - ], - "milestone": [ - "project", - "issues", - "labels", - "merge_requests", - "participants", - "events" - ], - "snippets": [ - "author", - "project", - "notes" - ], - "releases": [ - "project" - ], - "project_members": [ - "created_by", - "user", - "source", - "project" - ], - "merge_requests": [ - "subscriptions", - "award_emoji", - "author", - "assignee", - "updated_by", - "milestone", - "notes", - "label_links", - "labels", - "todos", - "target_project", - "source_project", - "merge_user", - "merge_request_diffs", - "merge_request_diff", - "events" - ], - "merge_request_diff": [ - "merge_request" - ], - "pipelines": [ - "project", - "user", - "statuses", - "builds", - "trigger_requests" - ], - "statuses": [ - "project", - "pipeline", - "user" - ], - "variables": [ - "project" - ], - "triggers": [ - "project", - "trigger_requests" - ], - "deploy_keys": [ - "user", - "deploy_keys_projects", - "projects" - ], - "services": [ - "project", - "service_hook" - ], - "hooks": [ - "project" - ], - "protected_branches": [ - "project", - "merge_access_levels", - "push_access_levels" - ], - "project": [ - "taggings", - "base_tags", - "tag_taggings", - "tags", - "creator", - "group", - "namespace", - "board", - "last_event", - "services", - "campfire_service", - "drone_ci_service", - "emails_on_push_service", - "builds_email_service", - "irker_service", - "pivotaltracker_service", - "hipchat_service", - "flowdock_service", - "assembla_service", - "asana_service", - "gemnasium_service", - "slack_service", - "buildkite_service", - "bamboo_service", - "teamcity_service", - "pushover_service", - "jira_service", - "redmine_service", - "custom_issue_tracker_service", - "bugzilla_service", - "gitlab_issue_tracker_service", - "external_wiki_service", - "forked_project_link", - "forked_from_project", - "forked_project_links", - "forks", - "merge_requests", - "fork_merge_requests", - "issues", - "labels", - "events", - "milestones", - "notes", - "snippets", - "hooks", - "protected_branches", - "project_members", - "users", - "requesters", - "deploy_keys_projects", - "deploy_keys", - "users_star_projects", - "starrers", - "releases", - "lfs_objects_projects", - "lfs_objects", - "project_group_links", - "invited_groups", - "todos", - "notification_settings", - "import_data", - "commit_statuses", - "pipelines", - "builds", - "runner_projects", - "runners", - "variables", - "triggers", - "environments", - "deployments" - ] -} \ No newline at end of file diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml new file mode 100644 index 00000000000..16ad3e1e599 --- /dev/null +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -0,0 +1,175 @@ +--- +issues: +- subscriptions +- award_emoji +- author +- assignee +- updated_by +- milestone +- notes +- label_links +- labels +- todos +- user_agent_detail +- project +- moved_to +- events +events: +- author +- project +- target +notes: +- award_emoji +- project +- noteable +- author +- updated_by +- resolved_by +- todos +- events +label_links: +- target +- label +label: +- subscriptions +- project +- lists +- label_links +- issues +- merge_requests +milestone: +- project +- issues +- labels +- merge_requests +- participants +- events +snippets: +- author +- project +- notes +releases: +- project +project_members: +- created_by +- user +- source +- project +merge_requests: +- subscriptions +- award_emoji +- author +- assignee +- updated_by +- milestone +- notes +- label_links +- labels +- todos +- target_project +- source_project +- merge_user +- merge_request_diffs +- merge_request_diff +- events +merge_request_diff: +- merge_request +pipelines: +- project +- user +- statuses +- builds +- trigger_requests +statuses: +- project +- pipeline +- user +variables: +- project +triggers: +- project +- trigger_requests +deploy_keys: +- user +- deploy_keys_projects +- projects +services: +- project +- service_hook +hooks: +- project +protected_branches: +- project +- merge_access_levels +- push_access_levels +project: +- taggings +- base_tags +- tag_taggings +- tags +- creator +- group +- namespace +- board +- last_event +- services +- campfire_service +- drone_ci_service +- emails_on_push_service +- builds_email_service +- irker_service +- pivotaltracker_service +- hipchat_service +- flowdock_service +- assembla_service +- asana_service +- gemnasium_service +- slack_service +- buildkite_service +- bamboo_service +- teamcity_service +- pushover_service +- jira_service +- redmine_service +- custom_issue_tracker_service +- bugzilla_service +- gitlab_issue_tracker_service +- external_wiki_service +- forked_project_link +- forked_from_project +- forked_project_links +- forks +- merge_requests +- fork_merge_requests +- issues +- labels +- events +- milestones +- notes +- snippets +- hooks +- protected_branches +- project_members +- users +- requesters +- deploy_keys_projects +- deploy_keys +- users_star_projects +- starrers +- releases +- lfs_objects_projects +- lfs_objects +- project_group_links +- invited_groups +- todos +- notification_settings +- import_data +- commit_statuses +- pipelines +- builds +- runner_projects +- runners +- variables +- triggers +- environments +- deployments diff --git a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb index 931b27ffb24..ba923a24cdd 100644 --- a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb +++ b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb @@ -1,5 +1,7 @@ require 'spec_helper' + +# Part of the Import/Export feature security testing # Checks whether there are new attributes in models that are currently being exported as part of the # project Import/Export feature. # If there are new attributes, these will have to either be added to this spec in case we want them @@ -73,16 +75,6 @@ describe 'Attribute configuration', lib: true do MSG end - def parsed_attributes(relation_name, attributes) - excluded_attributes = config_hash['excluded_attributes'][relation_name] - included_attributes = config_hash['included_attributes'][relation_name] - - attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes - attributes = attributes & JSON[included_attributes.to_json] if included_attributes - - attributes - end - class Author < User end end diff --git a/spec/lib/gitlab/import_export/model_configuration_spec.rb b/spec/lib/gitlab/import_export/model_configuration_spec.rb index 82bab7ac164..1805a9b3324 100644 --- a/spec/lib/gitlab/import_export/model_configuration_spec.rb +++ b/spec/lib/gitlab/import_export/model_configuration_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' +# Part of the Import/Export feature security testing # Finds if a new model has been added that can potentially be part of the Import/Export # If it finds a new model, it will show a +failure_message+ with the options available. describe 'Model configuration', lib: true do @@ -12,12 +13,12 @@ describe 'Model configuration', lib: true do # 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. - # - User, Author... Models we don not care about for checking relations + # - User, Author... Models we do not care about for checking relations names.flatten.uniq - ['milestones', 'labels', 'user', 'author'] + ['project'] end - let(:all_models_json) { 'spec/lib/gitlab/import_export/all_models.json' } - let(:all_models) { JSON.parse(File.read(all_models_json)) } + let(:all_models_yml) { 'spec/lib/gitlab/import_export/all_models.yml' } + let(:all_models) { YAML.load_file(all_models_yml) } let(:current_models) { setup_models } it 'has no new models' do @@ -43,15 +44,15 @@ describe 'Model configuration', lib: true do all_models_hash end - def failure_message(relation_name, new_models) + def failure_message(parent_model_name, new_models) <<-MSG - New model(s) <#{new_models.join(',')}> have been added, related to #{relation_name}, which is exported by + New model(s) <#{new_models.join(',')}> have been added, related to #{parent_model_name}, which is exported by the Import/Export feature. - If you don't think this should be exported, please add it to MODELS_JSON, inside the #{relation_name} hash. - If you think we should export this new model, please add it to IMPORT_EXPORT_CONFIG. + If you don't think this should be exported, please add it to MODELS_JSON, inside the #{parent_model_name} hash. + If you think we should export this new model, please add it to IMPORT_EXPORT_CONFIG and to MODELS_JSON. - MODELS_JSON: #{File.expand_path(all_models_json)} + MODELS_JSON: #{File.expand_path(all_models_yml)} IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file} MSG end diff --git a/spec/support/import_export/configuration_helper.rb b/spec/support/import_export/configuration_helper.rb index fec83765253..d7582057c83 100644 --- a/spec/support/import_export/configuration_helper.rb +++ b/spec/support/import_export/configuration_helper.rb @@ -12,4 +12,14 @@ module ConfigurationHelper relation_name = Gitlab::ImportExport::RelationFactory::OVERRIDES[relation_name.to_sym] || relation_name relation_name.to_s.classify.constantize end + + def parsed_attributes(relation_name, attributes) + excluded_attributes = config_hash['excluded_attributes'][relation_name] + included_attributes = config_hash['included_attributes'][relation_name] + + attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes + attributes = attributes & JSON[included_attributes.to_json] if included_attributes + + attributes + end end diff --git a/spec/support/import_export/export_file_helper.rb b/spec/support/import_export/export_file_helper.rb index 8e56bca21a3..de066f1cd66 100644 --- a/spec/support/import_export/export_file_helper.rb +++ b/spec/support/import_export/export_file_helper.rb @@ -1,4 +1,8 @@ module ExportFileHelper + include ConfigurationHelper + + ObjectWithParent = Struct.new(:object, :parent, :key_found) + def setup_project project = create(:project, :public) @@ -54,48 +58,78 @@ module ExportFileHelper # Recursively finds key/values including +key+ as part of the key, inside a nested hash def deep_find_with_parent(sensitive_key_word, object, found = nil) + sensitive_key_found = object_contains_key?(object, sensitive_key_word) + # Returns the parent object and the object found containing a sensitive word as part of the key - if object_contains_key?(object, sensitive_key_word) - [object[sensitive_key_word], object] if object[sensitive_key_word] + if sensitive_key_found && object[sensitive_key_found] + ObjectWithParent.new(object[sensitive_key_found], object, sensitive_key_found) elsif object.is_a?(Enumerable) # Recursively lookup for keys containing sensitive words in a Hash or Array - object.find { |*hash_or_array| found, object = deep_find_with_parent(sensitive_key_word, hash_or_array.last, found) } - [found, object] if found + object_with_parent = nil + + object.find do |*hash_or_array| + object_with_parent = deep_find_with_parent(sensitive_key_word, hash_or_array.last, found) + end + + object_with_parent end end # Return true if the hash has a key containing a sensitive word def object_contains_key?(object, sensitive_key_word) - object.is_a?(Hash) && object.keys.any? { |key| key.include?(sensitive_key_word) } + return false unless object.is_a?(Hash) + + object.keys.find { |key| key.include?(sensitive_key_word) } end - # Returns true if a sensitive word is found inside a hash, excluding safe hashes - def has_sensitive_attributes?(sensitive_word, project_hash) + # Returns the offended ObjectWithParent object if a sensitive word is found inside a hash, + # excluding the whitelisted safe hashes. + def find_sensitive_attributes(sensitive_word, project_hash) loop do - object, parent = deep_find_with_parent(sensitive_word, project_hash) + object_with_parent = deep_find_with_parent(sensitive_word, project_hash) - if object && is_safe_hash?(parent, sensitive_word) + return nil unless object_with_parent && object_with_parent.object + + if is_safe_hash?(object_with_parent.parent, sensitive_word) # It's in the safe list, remove hash and keep looking - parent.delete(object) - elsif object - return true + object_with_parent.parent.delete(object_with_parent.key_found) else - return false + return object_with_parent end + + nil end end - # Returns true if it's one of the excluded models in +safe_models+ + # Returns true if it's one of the excluded models in +safe_list+ def is_safe_hash?(parent, sensitive_word) - return false unless parent + return false unless parent && safe_list[sensitive_word.to_sym] # Extra attributes that appear in a model but not in the exported hash. excluded_attributes = ['type'] - safe_models[sensitive_word.to_sym].each do |safe_model| - return true if (safe_model.attribute_names - parent.keys - excluded_attributes).empty? + safe_list[sensitive_word.to_sym].each do |model| + # Check whether this is a hash attribute inside a model + if model.is_a?(Symbol) + return true if (safe_hashes[model] - parent.keys).empty? + else + return true if safe_model?(model, excluded_attributes, parent) + end end false end + + def associations_for(safe_model) + safe_model.reflect_on_all_associations.map { |assoc| assoc.name.to_s } + end + + # Compares model attributes with those those found in the hash + # and returns true if there is a match, ignoring some excluded attributes. + def safe_model?(model, excluded_attributes, parent) + excluded_attributes += associations_for(model) + parsed_model_attributes = parsed_attributes(model.name.underscore, model.attribute_names) + + (parsed_model_attributes - parent.keys - excluded_attributes).empty? + end end