summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Release Tools Bot <robert+release-tools@gitlab.com>2019-06-03 12:34:07 +0000
committerGitLab Release Tools Bot <robert+release-tools@gitlab.com>2019-06-03 12:34:07 +0000
commit38e4977dc7931aea13f496cafd3ed7d15d5ec93e (patch)
treedea9ebb60dbdab61dd5933cc4405353704356306
parent5dc6c8f2d08534281b0e1adf404af0e8642eb407 (diff)
parentb70b43d07ec27c6410e4a8d7ad417662a8823f8f (diff)
downloadgitlab-ce-38e4977dc7931aea13f496cafd3ed7d15d5ec93e.tar.gz
Merge branch 'security-fix_milestones_search_api_leak' into 'master'
Resolve: Milestones leaked via search API Closes #2822 See merge request gitlab/gitlabhq!2997
-rw-r--r--app/models/project.rb12
-rw-r--r--changelogs/unreleased/security-fix_milestones_search_api_leak.yml5
-rw-r--r--lib/gitlab/project_search_results.rb6
-rw-r--r--lib/gitlab/search_results.rb28
-rw-r--r--spec/lib/gitlab/search_results_spec.rb24
-rw-r--r--spec/models/project_spec.rb17
-rw-r--r--spec/requests/api/search_spec.rb46
7 files changed, 131 insertions, 7 deletions
diff --git a/app/models/project.rb b/app/models/project.rb
index ab4da61dcf8..4ca14d1c2ac 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -406,6 +406,7 @@ class Project < ApplicationRecord
scope :with_builds_enabled, -> { with_feature_enabled(:builds) }
scope :with_issues_enabled, -> { with_feature_enabled(:issues) }
scope :with_issues_available_for_user, ->(current_user) { with_feature_available_for_user(:issues, current_user) }
+ scope :with_merge_requests_available_for_user, ->(current_user) { with_feature_available_for_user(:merge_requests, current_user) }
scope :with_merge_requests_enabled, -> { with_feature_enabled(:merge_requests) }
scope :with_remote_mirrors, -> { joins(:remote_mirrors).where(remote_mirrors: { enabled: true }).distinct }
@@ -596,6 +597,17 @@ class Project < ApplicationRecord
def group_ids
joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id)
end
+
+ # Returns ids of projects with milestones available for given user
+ #
+ # Used on queries to find milestones which user can see
+ # For example: Milestone.where(project_id: ids_with_milestone_available_for(user))
+ def ids_with_milestone_available_for(user)
+ with_issues_enabled = with_issues_available_for_user(user).select(:id)
+ with_merge_requests_enabled = with_merge_requests_available_for_user(user).select(:id)
+
+ from_union([with_issues_enabled, with_merge_requests_enabled]).select(:id)
+ end
end
def all_pipelines
diff --git a/changelogs/unreleased/security-fix_milestones_search_api_leak.yml b/changelogs/unreleased/security-fix_milestones_search_api_leak.yml
new file mode 100644
index 00000000000..5691550b602
--- /dev/null
+++ b/changelogs/unreleased/security-fix_milestones_search_api_leak.yml
@@ -0,0 +1,5 @@
+---
+title: 'Resolve: Milestones leaked via search API'
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb
index 78337518988..0f3b97e2317 100644
--- a/lib/gitlab/project_search_results.rb
+++ b/lib/gitlab/project_search_results.rb
@@ -138,6 +138,12 @@ module Gitlab
project
end
+ def filter_milestones_by_project(milestones)
+ return Milestone.none unless Ability.allowed?(@current_user, :read_milestone, @project)
+
+ milestones.where(project_id: project.id) # rubocop: disable CodeReuse/ActiveRecord
+ end
+
def repository_project_ref
@repository_project_ref ||= repository_ref || project.default_branch
end
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index 4a097a00101..7c1e6b1baff 100644
--- a/lib/gitlab/search_results.rb
+++ b/lib/gitlab/search_results.rb
@@ -103,9 +103,11 @@ module Gitlab
# rubocop: disable CodeReuse/ActiveRecord
def milestones
- milestones = Milestone.where(project_id: project_ids_relation)
- milestones = milestones.search(query)
- milestones.reorder('milestones.updated_at DESC')
+ milestones = Milestone.search(query)
+
+ milestones = filter_milestones_by_project(milestones)
+
+ milestones.reorder('updated_at DESC')
end
# rubocop: enable CodeReuse/ActiveRecord
@@ -123,6 +125,26 @@ module Gitlab
'projects'
end
+ # Filter milestones by authorized projects.
+ # For performance reasons project_id is being plucked
+ # to be used on a smaller query.
+ #
+ # rubocop: disable CodeReuse/ActiveRecord
+ def filter_milestones_by_project(milestones)
+ project_ids =
+ milestones.where(project_id: project_ids_relation)
+ .select(:project_id).distinct
+ .pluck(:project_id)
+
+ return Milestone.none if project_ids.nil?
+
+ authorized_project_ids_relation =
+ Project.where(id: project_ids).ids_with_milestone_available_for(current_user)
+
+ milestones.where(project_id: authorized_project_ids_relation)
+ end
+ # rubocop: enable CodeReuse/ActiveRecord
+
# rubocop: disable CodeReuse/ActiveRecord
def project_ids_relation
limit_projects.select(:id).reorder(nil)
diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb
index 312aa3be490..3d27156b356 100644
--- a/spec/lib/gitlab/search_results_spec.rb
+++ b/spec/lib/gitlab/search_results_spec.rb
@@ -256,4 +256,28 @@ describe Gitlab::SearchResults do
expect(results.objects('merge_requests')).not_to include merge_request
end
+
+ context 'milestones' do
+ it 'returns correct set of milestones' do
+ private_project_1 = create(:project, :private)
+ private_project_2 = create(:project, :private)
+ internal_project = create(:project, :internal)
+ public_project_1 = create(:project, :public)
+ public_project_2 = create(:project, :public, :issues_disabled, :merge_requests_disabled)
+ private_project_1.add_developer(user)
+ # milestones that should not be visible
+ create(:milestone, project: private_project_2, title: 'Private project without access milestone')
+ create(:milestone, project: public_project_2, title: 'Public project with milestones disabled milestone')
+ # milestones that should be visible
+ milestone_1 = create(:milestone, project: private_project_1, title: 'Private project with access milestone', state: 'closed')
+ milestone_2 = create(:milestone, project: internal_project, title: 'Internal project milestone')
+ milestone_3 = create(:milestone, project: public_project_1, title: 'Public project with milestones enabled milestone')
+ # Global search scope takes user authorized projects, internal projects and public projects.
+ limit_projects = ProjectsFinder.new(current_user: user).execute
+
+ milestones = described_class.new(user, limit_projects, 'milestone').objects('milestones')
+
+ expect(milestones).to match_array([milestone_1, milestone_2, milestone_3])
+ end
+ end
end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 08662231fdf..6401704367e 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3170,6 +3170,23 @@ describe Project do
end
end
+ describe '.ids_with_milestone_available_for' do
+ let!(:user) { create(:user) }
+
+ it 'returns project ids with milestones available for user' do
+ project_1 = create(:project, :public, :merge_requests_disabled, :issues_disabled)
+ project_2 = create(:project, :public, :merge_requests_disabled)
+ project_3 = create(:project, :public, :issues_disabled)
+ project_4 = create(:project, :public)
+ project_4.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE )
+
+ project_ids = described_class.ids_with_milestone_available_for(user).pluck(:id)
+
+ expect(project_ids).to include(project_2.id, project_3.id)
+ expect(project_ids).not_to include(project_1.id, project_4.id)
+ end
+ end
+
describe '.with_feature_available_for_user' do
let(:user) { create(:user) }
let(:feature) { MergeRequest }
diff --git a/spec/requests/api/search_spec.rb b/spec/requests/api/search_spec.rb
index 7d61ec9c4d8..3e0b478abb3 100644
--- a/spec/requests/api/search_spec.rb
+++ b/spec/requests/api/search_spec.rb
@@ -70,11 +70,30 @@ describe API::Search do
context 'for milestones scope' do
before do
create(:milestone, project: project, title: 'awesome milestone')
+ end
+
+ context 'when user can read project milestones' do
+ before do
+ get api('/search', user), params: { scope: 'milestones', search: 'awesome' }
+ end
- get api('/search', user), params: { scope: 'milestones', search: 'awesome' }
+ it_behaves_like 'response is correct', schema: 'public_api/v4/milestones'
end
- it_behaves_like 'response is correct', schema: 'public_api/v4/milestones'
+ context 'when user cannot read project milestones' do
+ before do
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+ project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+ end
+
+ it 'returns empty array' do
+ get api('/search', user), params: { scope: 'milestones', search: 'awesome' }
+
+ milestones = JSON.parse(response.body)
+
+ expect(milestones).to be_empty
+ end
+ end
end
context 'for users scope' do
@@ -318,11 +337,30 @@ describe API::Search do
context 'for milestones scope' do
before do
create(:milestone, project: project, title: 'awesome milestone')
+ end
+
+ context 'when user can read milestones' do
+ before do
+ get api("/projects/#{project.id}/search", user), params: { scope: 'milestones', search: 'awesome' }
+ end
- get api("/projects/#{project.id}/search", user), params: { scope: 'milestones', search: 'awesome' }
+ it_behaves_like 'response is correct', schema: 'public_api/v4/milestones'
end
- it_behaves_like 'response is correct', schema: 'public_api/v4/milestones'
+ context 'when user cannot read project milestones' do
+ before do
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+ project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+ end
+
+ it 'returns empty array' do
+ get api("/projects/#{project.id}/search", user), params: { scope: 'milestones', search: 'awesome' }
+
+ milestones = JSON.parse(response.body)
+
+ expect(milestones).to be_empty
+ end
+ end
end
context 'for users scope' do