From d00f14d73f41129f9d986d4bec32f1f927b525a6 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 4 Dec 2020 16:47:02 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee --- .../projects/feature_flags_controller.rb | 6 ++-- app/models/operations/feature_flags/user_list.rb | 5 ++++ app/services/feature_flags/update_service.rb | 11 +++++++ .../unreleased/security-idor-ff-user-list.yml | 5 ++++ .../projects/feature_flags_controller_spec.rb | 34 ++++++++++++++++++++++ vendor/gitignore/C++.gitignore | 0 vendor/gitignore/Java.gitignore | 0 7 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/security-idor-ff-user-list.yml mode change 100755 => 100644 vendor/gitignore/C++.gitignore mode change 100755 => 100644 vendor/gitignore/Java.gitignore diff --git a/app/controllers/projects/feature_flags_controller.rb b/app/controllers/projects/feature_flags_controller.rb index e9d450a6ce3..9142f769b28 100644 --- a/app/controllers/projects/feature_flags_controller.rb +++ b/app/controllers/projects/feature_flags_controller.rb @@ -77,7 +77,7 @@ class Projects::FeatureFlagsController < Projects::ApplicationController end else respond_to do |format| - format.json { render_error_json(result[:message]) } + format.json { render_error_json(result[:message], result[:http_status]) } end end end @@ -167,8 +167,8 @@ class Projects::FeatureFlagsController < Projects::ApplicationController render json: feature_flag_json(feature_flag), status: :ok end - def render_error_json(messages) + def render_error_json(messages, status = :bad_request) render json: { message: messages }, - status: :bad_request + status: status end end diff --git a/app/models/operations/feature_flags/user_list.rb b/app/models/operations/feature_flags/user_list.rb index 3e492eaa892..ec109bde0eb 100644 --- a/app/models/operations/feature_flags/user_list.rb +++ b/app/models/operations/feature_flags/user_list.rb @@ -28,6 +28,11 @@ module Operations fuzzy_search(query, [:name], use_minimum_char_limit: false) end + def self.belongs_to?(project_id, user_list_ids) + uniq_ids = user_list_ids.uniq + where(id: uniq_ids, project_id: project_id).count == uniq_ids.count + end + private def ensure_no_associated_strategies diff --git a/app/services/feature_flags/update_service.rb b/app/services/feature_flags/update_service.rb index ed5e2e794b4..d956d4b3357 100644 --- a/app/services/feature_flags/update_service.rb +++ b/app/services/feature_flags/update_service.rb @@ -10,6 +10,7 @@ module FeatureFlags def execute(feature_flag) return error('Access Denied', 403) unless can_update?(feature_flag) + return error('Not Found', 404) unless valid_user_list_ids?(feature_flag, user_list_ids(params)) ActiveRecord::Base.transaction do feature_flag.assign_attributes(params) @@ -87,5 +88,15 @@ module FeatureFlags def can_update?(feature_flag) Ability.allowed?(current_user, :update_feature_flag, feature_flag) end + + def user_list_ids(params) + params.fetch(:strategies_attributes, []) + .select { |s| s[:user_list_id].present? } + .map { |s| s[:user_list_id] } + end + + def valid_user_list_ids?(feature_flag, user_list_ids) + user_list_ids.empty? || ::Operations::FeatureFlags::UserList.belongs_to?(feature_flag.project_id, user_list_ids) + end end end diff --git a/changelogs/unreleased/security-idor-ff-user-list.yml b/changelogs/unreleased/security-idor-ff-user-list.yml new file mode 100644 index 00000000000..6d17f9af11d --- /dev/null +++ b/changelogs/unreleased/security-idor-ff-user-list.yml @@ -0,0 +1,5 @@ +--- +title: Forbid setting a gitlabUserList strategy to a list from another project +merge_request: +author: +type: security diff --git a/spec/controllers/projects/feature_flags_controller_spec.rb b/spec/controllers/projects/feature_flags_controller_spec.rb index 96eeb6f239f..1473ec95192 100644 --- a/spec/controllers/projects/feature_flags_controller_spec.rb +++ b/spec/controllers/projects/feature_flags_controller_spec.rb @@ -1511,6 +1511,40 @@ RSpec.describe Projects::FeatureFlagsController do expect(response).to have_gitlab_http_status(:not_found) end + it 'returns not found when trying to update a gitlabUserList strategy with a user list from another project' do + user_list = create(:operations_feature_flag_user_list, project: project, name: 'My List', user_xids: 'user1,user2') + strategy = create(:operations_strategy, feature_flag: new_version_flag, name: 'gitlabUserList', parameters: {}, user_list: user_list) + other_project = create(:project) + other_user_list = create(:operations_feature_flag_user_list, project: other_project, name: 'Other List', user_xids: 'some,one') + + put_request(new_version_flag, strategies_attributes: [{ + id: strategy.id, + user_list_id: other_user_list.id + }]) + + expect(response).to have_gitlab_http_status(:not_found) + expect(strategy.reload.user_list).to eq(user_list) + end + + it 'allows setting multiple gitlabUserList strategies to the same user list' do + user_list_a = create(:operations_feature_flag_user_list, project: project, name: 'My List A', user_xids: 'user1,user2') + user_list_b = create(:operations_feature_flag_user_list, project: project, name: 'My List B', user_xids: 'user3,user4') + strategy_a = create(:operations_strategy, feature_flag: new_version_flag, name: 'gitlabUserList', parameters: {}, user_list: user_list_a) + strategy_b = create(:operations_strategy, feature_flag: new_version_flag, name: 'gitlabUserList', parameters: {}, user_list: user_list_a) + + put_request(new_version_flag, strategies_attributes: [{ + id: strategy_a.id, + user_list_id: user_list_b.id + }, { + id: strategy_b.id, + user_list_id: user_list_b.id + }]) + + expect(response).to have_gitlab_http_status(:ok) + expect(strategy_a.reload.user_list).to eq(user_list_b) + expect(strategy_b.reload.user_list).to eq(user_list_b) + end + it 'updates an existing strategy' do strategy = create(:operations_strategy, feature_flag: new_version_flag, name: 'default', parameters: {}) diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore old mode 100755 new mode 100644 diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore old mode 100755 new mode 100644 -- cgit v1.2.3