Commit fcc2bc3b authored by Felipe Artur's avatar Felipe Artur

Resolve: Milestones leaked via search API

Fix milestone titles being leaked using search API
when users cannot read milestones
parent 406fe0e9
......@@ -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
......
---
title: 'Resolve: Milestones leaked via search API'
merge_request:
author:
type: security
......@@ -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
......
......@@ -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)
......
......@@ -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
......@@ -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 }
......
......@@ -70,13 +70,32 @@ 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
it_behaves_like 'response is correct', schema: 'public_api/v4/milestones'
end
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
before do
create(:user, name: 'billy')
......@@ -318,13 +337,32 @@ 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
it_behaves_like 'response is correct', schema: 'public_api/v4/milestones'
end
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
before do
user1 = create(:user, name: 'billy')
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment