From 8dc82011f5bb17c0792e432c86d75e3bd294fc30 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 22 Apr 2016 06:51:40 +0000 Subject: [PATCH] Merge branch 'fix/private-labels-permissions' into 'master' Fix vulnerability that leaks private labels and milestones This fixes vulnerability that leaks information about private labels and milestones because of insecure direct object reference in issueable create service. This affects merge requests and issues. See https://gitlab.com/gitlab-org/gitlab-ce/issues/15439 This MR introduces additional check that rejects labels and milestone that does not belong to the same project issue/merg request does. `IssuableBaseService` may benefit from encapsulating filters in separate class/module, which then may improve coherency in this class. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15439 See merge request !1954 --- app/services/issuable_base_service.rb | 28 +++++++- .../issues/bulk_update_service_spec.rb | 2 +- spec/services/issues/create_service_spec.rb | 67 ++++++++++++++----- spec/services/issues/update_service_spec.rb | 10 ++- .../merge_requests/update_service_spec.rb | 11 ++- 5 files changed, 93 insertions(+), 25 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index ca87dca4a7..4a27ff9990 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -34,8 +34,9 @@ class IssuableBaseService < BaseService end def filter_params(issuable_ability_name = :issue) - params[:assignee_id] = "" if params[:assignee_id] == IssuableFinder::NONE - params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE + filter_assignee + filter_milestone + filter_labels ability = :"admin_#{issuable_ability_name}" @@ -46,6 +47,29 @@ class IssuableBaseService < BaseService end end + def filter_assignee + if params[:assignee_id] == IssuableFinder::NONE + params[:assignee_id] = '' + end + end + + def filter_milestone + milestone_id = params[:milestone_id] + return unless milestone_id + + if milestone_id == IssuableFinder::NONE || + project.milestones.find_by(id: milestone_id).nil? + params[:milestone_id] = '' + end + end + + def filter_labels + return if params[:label_ids].to_a.empty? + + params[:label_ids] = + project.labels.where(id: params[:label_ids]).pluck(:id) + end + def update(issuable) change_state(issuable) filter_params diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issues/bulk_update_service_spec.rb index 6a7ea4b2f4..e91906d0d4 100644 --- a/spec/services/issues/bulk_update_service_spec.rb +++ b/spec/services/issues/bulk_update_service_spec.rb @@ -100,7 +100,7 @@ describe Issues::BulkUpdateService, services: true do describe :update_milestone do before do - @milestone = create :milestone + @milestone = create(:milestone, project: @project) @params = { issues_ids: [issue.id], milestone_id: @milestone.id diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 5e7915db7e..ac28b6f71f 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -3,40 +3,75 @@ require 'spec_helper' describe Issues::CreateService, services: true do let(:project) { create(:empty_project) } let(:user) { create(:user) } - let(:assignee) { create(:user) } - describe :execute do - context 'valid params' do + describe '#execute' do + let(:issue) { described_class.new(project, user, opts).execute } + + context 'when params are valid' do + let(:assignee) { create(:user) } + let(:milestone) { create(:milestone, project: project) } + let(:labels) { create_pair(:label, project: project) } + before do project.team << [user, :master] project.team << [assignee, :master] - - opts = { - title: 'Awesome issue', - description: 'please fix', - assignee: assignee - } - - @issue = Issues::CreateService.new(project, user, opts).execute end - it { expect(@issue).to be_valid } - it { expect(@issue.title).to eq('Awesome issue') } - it { expect(@issue.assignee).to eq assignee } + let(:opts) do + { title: 'Awesome issue', + description: 'please fix', + assignee: assignee, + label_ids: labels.map(&:id), + milestone_id: milestone.id } + end + + it { expect(issue).to be_valid } + it { expect(issue.title).to eq('Awesome issue') } + it { expect(issue.assignee).to eq assignee } + it { expect(issue.labels).to match_array labels } + it { expect(issue.milestone).to eq milestone } it 'creates a pending todo for new assignee' do attributes = { project: project, author: user, user: assignee, - target_id: @issue.id, - target_type: @issue.class.name, + target_id: issue.id, + target_type: issue.class.name, action: Todo::ASSIGNED, state: :pending } expect(Todo.where(attributes).count).to eq 1 end + + context 'when label belongs to different project' do + let(:label) { create(:label) } + + let(:opts) do + { title: 'Title', + description: 'Description', + label_ids: [label.id] } + end + + it 'does not assign label'do + expect(issue.labels).to_not include label + end + end + + context 'when milestone belongs to different project' do + let(:milestone) { create(:milestone) } + + let(:opts) do + { title: 'Title', + description: 'Description', + milestone_id: milestone.id } + end + + it 'does not assign milestone' do + expect(issue.milestone).to_not eq milestone + end + end end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index e579e49dfa..da62605bef 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -4,9 +4,13 @@ describe Issues::UpdateService, services: true do let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } - let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) } - let(:label) { create(:label) } - let(:project) { issue.project } + let(:project) { create(:empty_project) } + let(:label) { create(:label, project: project) } + let(:issue) do + create(:issue, title: 'Old title', + assignee_id: user3.id, + project: project) + end before do project.team << [user, :master] diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 99703c7a8e..1827f9b75c 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -1,12 +1,17 @@ require 'spec_helper' describe MergeRequests::UpdateService, services: true do + let(:project) { create(:project) } let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } - let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) } - let(:project) { merge_request.project } - let(:label) { create(:label) } + let(:label) { create(:label, project: project) } + + let(:merge_request) do + create(:merge_request, :simple, title: 'Old title', + assignee_id: user3.id, + source_project: project) + end before do project.team << [user, :master]