Merge branch 'security-ee_fix_milestones_search_api_leak' into 'master'

Resolve: Milestones leaked via search API

See merge request gitlab/gitlab-ee!865

app/models/project.rb

@@ -407,6 +407,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 }
 
@@ -597,6 +598,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

changelogs/unreleased/security-fix_milestones_search_api_leak.yml

+---
+title: 'Resolve: Milestones leaked via search API'
+merge_request:
+author:
+type: security

ee/app/models/concerns/elastic/application_search.rb

@@ -371,7 +371,7 @@ module Elastic
           options[:current_user],
           options[:project_ids],
           options[:public_and_internal_projects],
-          options[:feature]
+          options[:features]
         )
 
         query_hash[:query][:bool][:filter] ||= []
@@ -390,10 +390,10 @@ module Elastic
       # Builds an elasticsearch query that will select projects the user is
       # granted access to.
       #
-      # If a project feature is specified, it indicates interest in child
+      # If a project feature(s) is specified, it indicates interest in child
       # documents gated by that project feature - e.g., "issues". The feature's
       # visibility level must be taken into account.
-      def project_ids_query(user, project_ids, public_and_internal_projects, feature = nil)
+      def project_ids_query(user, project_ids, public_and_internal_projects, features = nil)
         # When reading cross project is not allowed, only allow searching a
         # a single project, so the `:read_*` ability is only checked once.
         unless Ability.allowed?(user, :read_cross_project)
@@ -404,18 +404,18 @@ module Elastic
         # anonymous users.
         # Pick private, internal and public projects the user is a member of.
         # Pick all private projects for admins & auditors.
-        conditions = [pick_projects_by_membership(project_ids, feature)]
+        conditions = [pick_projects_by_membership(project_ids, features)]
 
         if public_and_internal_projects
           # Skip internal projects for anonymous and external users.
           # Others are given access to all internal projects. Admins & auditors
           # get access to internal projects where the feature is private.
-          conditions << pick_projects_by_visibility(Project::INTERNAL, user, feature) if user && !user.external?
+          conditions << pick_projects_by_visibility(Project::INTERNAL, user, features) if user && !user.external?
 
           # All users, including anonymous, can access public projects.
           # Admins & auditors get access to public projects where the feature is
           # private.
-          conditions << pick_projects_by_visibility(Project::PUBLIC, user, feature)
+          conditions << pick_projects_by_visibility(Project::PUBLIC, user, features)
         end
 
         { should: conditions }
@@ -430,7 +430,7 @@ module Elastic
       # Admins & auditors are given access to all private projects. Access to
       # internal or public projects where the project feature is private is not
       # granted here.
-      def pick_projects_by_membership(project_ids, feature = nil)
+      def pick_projects_by_membership(project_ids, features = nil)
         condition =
           if project_ids == :any
             { term: { visibility_level: Project::PRIVATE } }
@@ -438,37 +438,43 @@ module Elastic
             { terms: { id: project_ids } }
           end
 
-        limit_by_feature(condition, feature, include_members_only: true)
+        limit_by_feature(condition, features, include_members_only: true)
       end
 
       # Grant access to projects of the specified visibility level to the user.
       #
       # If a project feature is specified, access is only granted if the feature
       # is enabled or, for admins & auditors, private.
-      def pick_projects_by_visibility(visibility, user, feature)
+      def pick_projects_by_visibility(visibility, user, features)
         condition = { term: { visibility_level: visibility } }
 
-        limit_by_feature(condition, feature, include_members_only: user&.full_private_access?)
+        limit_by_feature(condition, features, include_members_only: user&.full_private_access?)
       end
 
-      # If a project feature is specified, access is dependent on its visibility
+      # If a project feature(s) is specified, access is dependent on its visibility
       # level being enabled (or private if `include_members_only: true`).
       #
       # This method is a no-op if no project feature is specified.
+      # It accepts an array of features or a single feature, when an array is provided
+      # it queries if any of the features is enabled.
       #
-      # Always denies access to projects when the feature is disabled - even to
+      # Always denies access to projects when the features are disabled - even to
       # admins & auditors - as stale child documents may be present.
-      def limit_by_feature(condition, feature, include_members_only:)
-        return condition unless feature
+      def limit_by_feature(condition, features, include_members_only:)
+        return condition unless features
 
-        limit =
-          if include_members_only
-            { terms: { "#{feature}_access_level" => [::ProjectFeature::ENABLED, ::ProjectFeature::PRIVATE] } }
-          else
-            { term: { "#{feature}_access_level" => ::ProjectFeature::ENABLED } }
-          end
+        features = Array(features)
 
-        { bool: { filter: [condition, limit] } }
+        features.map do |feature|
+          limit =
+            if include_members_only
+              { terms: { "#{feature}_access_level" => [::ProjectFeature::ENABLED, ::ProjectFeature::PRIVATE] } }
+            else
+              { term: { "#{feature}_access_level" => ::ProjectFeature::ENABLED } }
+            end
+
+          { bool: { filter: [condition, limit] } }
+        end
       end
     end
   end

ee/app/models/concerns/elastic/issues_search.rb

@@ -33,7 +33,7 @@ module Elastic
             basic_query_hash(%w(title^2 description), query)
           end
 
-        options[:feature] = 'issues'
+        options[:features] = 'issues'
         query_hash = project_ids_filter(query_hash, options)
         query_hash = confidentiality_filter(query_hash, options[:current_user])
 

ee/app/models/concerns/elastic/merge_requests_search.rb

@@ -49,7 +49,7 @@ module Elastic
             basic_query_hash(%w(title^2 description), query)
           end
 
-        options[:feature] = 'merge_requests'
+        options[:features] = 'merge_requests'
         query_hash = project_ids_filter(query_hash, options)
 
         self.__elasticsearch__.search(query_hash)

ee/lib/gitlab/elastic/search_results.rb

@@ -150,7 +150,14 @@ module Gitlab
       end
 
       def milestones
-        Milestone.elastic_search(query, options: base_options)
+        # Must pass 'issues' and 'merge_requests' to check
+        # if any of the features is available for projects in Elastic::ApplicationSearch#project_ids_query
+        # Otherwise it will ignore project_ids and return milestones
+        # from projects with milestones disabled.
+        options = base_options
+        options[:features] = [:issues, :merge_requests]
+
+        Milestone.elastic_search(query, options: options)
       end
 
       def merge_requests

ee/spec/lib/gitlab/elastic/search_results_spec.rb

@@ -379,6 +379,7 @@ describe Gitlab::Elastic::SearchResults do
   describe 'project scoping' do
     it "returns items for project" do
       project = create :project, :repository, name: "term"
+      project.add_developer(user)
 
       # Create issue
       create :issue, title: 'bla-bla term', project: project
@@ -686,29 +687,118 @@ describe Gitlab::Elastic::SearchResults do
     end
 
     context 'Milestones' do
-      it 'finds right set of milestine' do
-        milestone_1 = create :milestone, project: internal_project, title: "Internal project"
-        create :milestone, project: private_project1, title: "Private project"
-        milestone_3 = create :milestone, project: private_project2, title: "Private project where I'm a member"
-        milestone_4 = create :milestone, project: public_project, title: "Public project"
+      let!(:milestone_1) { create(:milestone, project: internal_project, title: "Internal project") }
+      let!(:milestone_2) { create(:milestone, project: private_project1, title: "Private project") }
+      let!(:milestone_3) { create(:milestone, project: private_project2, title: "Private project which user is member") }
+      let!(:milestone_4) { create(:milestone, project: public_project, title: "Public project") }
 
+      before do
         Gitlab::Elastic::Helper.refresh_index
+      end
 
-        # Authenticated search
-        results = described_class.new(user, 'project', limit_project_ids)
-        milestones = results.objects('milestones')
+      context 'when project ids are present' do
+        context 'when authenticated' do
+          context 'when user and merge requests are disabled in a project' do
+            it 'returns right set of milestones' do
+              private_project2.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+              private_project2.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+              public_project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+              public_project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+              internal_project.project_feature.update!(issues_access_level: ProjectFeature::DISABLED)
+              Gitlab::Elastic::Helper.refresh_index
+
+              project_ids = user.authorized_projects.pluck(:id)
+              results = described_class.new(user, 'project', project_ids)
+              milestones = results.objects('milestones')
+
+              expect(milestones).to match_array([milestone_1, milestone_3])
+            end
+          end
+
+          context 'when user is admin' do
+            it 'returns right set of milestones' do
+              user.update(admin: true)
+              public_project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+              public_project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+              internal_project.project_feature.update!(issues_access_level: ProjectFeature::DISABLED)
+              internal_project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
+              Gitlab::Elastic::Helper.refresh_index
+
+              results = described_class.new(user, 'project', :any)
+              milestones = results.objects('milestones')
+
+              expect(milestones).to match_array([milestone_2, milestone_3, milestone_4])
+            end
+          end
+
+          context 'when user can read milestones' do
+            it 'returns right set of milestones' do
+              # Authenticated search
+              project_ids = user.authorized_projects.pluck(:id)
+              results = described_class.new(user, 'project', project_ids)
+              milestones = results.objects('milestones')
+
+              expect(milestones).to match_array([milestone_1, milestone_3, milestone_4])
+            end
+          end
+        end
+      end
 
-        expect(milestones).to include milestone_1
-        expect(milestones).to include milestone_3
-        expect(milestones).to include milestone_4
-        expect(results.milestones_count).to eq 3
+      context 'when not authenticated' do
+        it 'returns right set of milestones' do
+          results = described_class.new(nil, 'project', [])
+          milestones = results.objects('milestones')
 
-        # Unauthenticated search
-        results = described_class.new(nil, 'project', [])
-        milestones = results.objects('milestones')
+          expect(milestones).to include milestone_4
+          expect(results.milestones_count).to eq 1
+        end
+      end
+
+      context 'when project_ids is not present' do
+        context 'when project_ids is :any' do
+          it 'returns all milestones' do
+            results = described_class.new(user, 'project', :any)
+            milestones = results.objects('milestones')
+
+            expect(milestones).to include(milestone_1)
+            expect(milestones).to include(milestone_2)
+            expect(milestones).to include(milestone_3)
+            expect(milestones).to include(milestone_4)
+            expect(results.milestones_count).to eq(4)
+          end
+        end
 
-        expect(milestones).to include milestone_4
-        expect(results.milestones_count).to eq 1
+        context 'when authenticated' do
+          it 'returns right set of milestones' do
+            results = described_class.new(user, 'project', [])
+            milestones = results.objects('milestones')
+
+            expect(milestones).to include(milestone_1)
+            expect(milestones).to include(milestone_4)
+            expect(results.milestones_count).to eq(2)
+          end
+        end
+
+        context 'when not authenticated' do
+          it 'returns right set of milestones' do
+            # Should not be returned because issues and merge requests feature are disabled
+            other_public_project = create(:project, :public)
+            create(:milestone, project: other_public_project, title: 'Public project milestone 1')
+            other_public_project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+            other_public_project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+            # Should be returned because only issues is disabled
+            other_public_project_1 = create(:project, :public)
+            milestone_5 = create(:milestone, project: other_public_project_1, title: 'Public project milestone 2')
+            other_public_project_1.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+            Gitlab::Elastic::Helper.refresh_index
+
+            results = described_class.new(nil, 'project', [])
+            milestones = results.objects('milestones')
+
+            expect(milestones).to match_array([milestone_4, milestone_5])
+            expect(results.milestones_count).to eq(2)
+          end
+        end
       end
     end
 

lib/gitlab/project_search_results.rb

@@ -134,8 +134,16 @@ module Gitlab
       project.repository.commit(key) if Commit.valid_hash?(key)
     end
 
+    # rubocop: disable CodeReuse/ActiveRecord
     def project_ids_relation
-      project
+      Project.where(id: project).select(:id).reorder(nil)
+    end
+    # rubocop: enabled CodeReuse/ActiveRecord
+
+    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

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)

spec/lib/gitlab/search_results_spec.rb

@@ -256,4 +256,27 @@ 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')
+      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

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 }

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