diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2016-04-22 06:51:40 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-04-26 11:41:12 +0200 |
commit | 3e84dc5e1415ef6ebaf88ae6e61356cf7b8640da (patch) | |
tree | 83377ededfc34b5348a204a36caf77d323926658 | |
parent | 1e495e7633ee8fa3acee68f4e389129a294efd91 (diff) | |
download | gitlab-ce-3e84dc5e1415ef6ebaf88ae6e61356cf7b8640da.tar.gz |
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
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 28 | ||||
-rw-r--r-- | spec/services/issues/bulk_update_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/issues/create_service_spec.rb | 58 | ||||
-rw-r--r-- | spec/services/issues/update_service_spec.rb | 12 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 12 |
6 files changed, 95 insertions, 18 deletions
diff --git a/CHANGELOG b/CHANGELOG index ac57eca4c6d..ee850a1c92e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.3.9 - Fix a window.opener bug that could lead to XSS and open redirects - Prevent XSS via custom issue tracker URL + - Fix vulnerability that leaks private labels and milestones - Prevent privilege escalation via "impersonate" feature - Prevent users from deleting Webhooks via API they do not own - Prevent information disclosure via snippet API diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 2556f06e2d3..d819c476068 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 6a7ea4b2f44..e91906d0d49 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 2148d091a57..5d29db64077 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -4,20 +4,60 @@ describe Issues::CreateService, services: true do let(:project) { create(:empty_project) } let(:user) { 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(:label, title: 'foo', project: project), create(:label, title: 'bar', project: project)] } + before do project.team << [user, :master] - opts = { - title: 'Awesome issue', - description: 'please fix' - } + project.team << [assignee, :master] + end - @issue = Issues::CreateService.new(project, user, opts).execute + 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).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 } + + 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 87da0e9618b..d603666252f 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -4,9 +4,15 @@ 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(:label2) { create(:label) } + + 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 2e9e6e0870d..4f43d5d59ad 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -1,12 +1,18 @@ 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(:label2) { create(:label) } + + 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] |