summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/projects/feature_flags_controller.rb6
-rw-r--r--app/models/operations/feature_flags/user_list.rb5
-rw-r--r--app/services/feature_flags/update_service.rb11
-rw-r--r--changelogs/unreleased/security-idor-ff-user-list.yml5
-rw-r--r--spec/controllers/projects/feature_flags_controller_spec.rb34
-rw-r--r--[-rwxr-xr-x]vendor/gitignore/C++.gitignore0
-rw-r--r--[-rwxr-xr-x]vendor/gitignore/Java.gitignore0
7 files changed, 58 insertions, 3 deletions
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
index 259148fa18f..259148fa18f 100755..100644
--- a/vendor/gitignore/C++.gitignore
+++ b/vendor/gitignore/C++.gitignore
diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore
index a1c2a238a96..a1c2a238a96 100755..100644
--- a/vendor/gitignore/Java.gitignore
+++ b/vendor/gitignore/Java.gitignore