Merge branch 'security-fix_milestones_search_api_leak' into 'master'

Resolve: Milestones leaked via search API

Closes #2822

See merge request gitlab/gitlabhq!2997

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