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>

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

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

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

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]

spec/services/merge_requests/update_service_spec.rb

 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]