Merge branch 'fix/private-labels-permissions' into 'master'

Fix vulnerability that leaks private labels and milestones

## Summary

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

## Fix

This MR introduces additional check that rejects labels and milestone that does not belong to the same project issue/merg request does.

## Further work

`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

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
+v 8.8.0 (unreleased)
+
+v 8.7.0 (unreleased)
+  - Gitlab::GitAccess and Gitlab::GitAccessWiki are now instrumented
+  - Fix vulnerability that made it possible to gain access to private labels and milestones
+  - The number of InfluxDB points stored per UDP packet can now be configured
+  - Fix error when cross-project label reference used with non-existent project
+  - Transactions for /internal/allowed now have an "action" tag set
+  - Method instrumentation now uses Module#prepend instead of aliasing methods
+  - Repository.clean_old_archives is now instrumented
+  - Add support for environment variables on a job level in CI configuration file
+  - SQL query counts are now tracked per transaction
+  - The Projects::HousekeepingService class has extra instrumentation
+  - All service classes (those residing in app/services) are now instrumented
+  - Developers can now add custom tags to transactions
+  - Loading of an issue's referenced merge requests and related branches is now done asynchronously
+  - Enable gzip for assets, makes the page size significantly smaller. !3544 / !3632 (Connor Shea)
+  - Add support to cherry-pick any commit into any branch in the web interface (Minqi Pan)
+  - Project switcher uses new dropdown styling
+  - Load award emoji images separately unless opening the full picker. Saves several hundred KBs of data for most pages. (Connor Shea)
+  - Do not include award_emojis in issue and merge_request comment_count !3610 (Lucas Charles)
+  - Restrict user profiles when public visibility level is restricted.
+  - Add ability set due date to issues, sort and filter issues by due date (Mehmet Beydogan)
+  - All images in discussions and wikis now link to their source files !3464 (Connor Shea).
+  - Return status code 303 after a branch DELETE operation to avoid project deletion (Stan Hu)
+  - Add setting for customizing the list of trusted proxies !3524
+  - Allow projects to be transfered to a lower visibility level group
+  - Fix `signed_in_ip` being set to 127.0.0.1 when using a reverse proxy !3524
+  - Improved Markdown rendering performance !3389
+  - Make shared runners text in box configurable
+  - Don't attempt to look up an avatar in repo if repo directory does not exist (Stan Hu)
+  - API: Ability to subscribe and unsubscribe from issues and merge requests (Robert Schilling)
+  - Expose project badges in project settings
+  - Make /profile/keys/new redirect to /profile/keys for back-compat. !3717
+  - Preserve time notes/comments have been updated at when moving issue
+  - Make HTTP(s) label consistent on clone bar (Stan Hu)
+  - Add support for `after_script`, requires Runner 1.2 (Kamil Trzciński)
+  - Expose label description in API (Mariusz Jachimowicz)
+  - API: Ability to update a group (Robert Schilling)
+  - API: Ability to move issues (Robert Schilling)
+  - Fix Error 500 after renaming a project path (Stan Hu)
+  - Fix a bug whith trailing slash in teamcity_url (Charles May)
+  - Allow back dating on issues when created or updated through the API
+  - Allow back dating on issue notes when created through the API
+  - Propose license template when creating a new LICENSE file
+  - API: Expose /licenses and /licenses/:key
+  - Fix avatar stretching by providing a cropping feature
+  - API: Expose `subscribed` for issues and merge requests (Robert Schilling)
+  - Allow SAML to handle external users based on user's information !3530
+  - Allow Omniauth providers to be marked as `external` !3657
+  - Add endpoints to archive or unarchive a project !3372
+  - Fix a bug whith trailing slash in bamboo_url
+  - Add links to CI setup documentation from project settings and builds pages
+  - Display project members page to all members
+  - Handle nil descriptions in Slack issue messages (Stan Hu)
+  - Add automated repository integrity checks (OFF by default)
+  - API: Expose open_issues_count, closed_issues_count, open_merge_requests_count for labels (Robert Schilling)
+  - API: Ability to star and unstar a project (Robert Schilling)
+  - Add default scope to projects to exclude projects pending deletion
+  - Allow to close merge requests which source projects(forks) are deleted.
+  - Ensure empty recipients are rejected in BuildsEmailService
+  - Use rugged to change HEAD in Project#change_head (P.S.V.R)
+  - API: Ability to filter milestones by state `active` and `closed` (Robert Schilling)
+  - API: Fix milestone filtering by `iid` (Robert Schilling)
+  - Make before_script and after_script overridable on per-job (Kamil Trzciński)
+  - API: Delete notes of issues, snippets, and merge requests (Robert Schilling)
+  - Implement 'Groups View' as an option for dashboard preferences !3379 (Elias W.)
+  - Better errors handling when creating milestones inside groups
+  - Fix high CPU usage when PostReceive receives refs/merge-requests/<id>
+  - Hide `Create a group` help block when creating a new project in a group
+  - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.)
+  - Allow issues and merge requests to be assigned to the author !2765
+  - Make Ci::Commit to group only similar builds and make it stateful (ref, tag)
+  - Gracefully handle notes on deleted commits in merge requests (Stan Hu)
+  - Decouple membership and notifications
+  - Fix creation of merge requests for orphaned branches (Stan Hu)
+  - API: Ability to retrieve a single tag (Robert Schilling)
+  - While signing up, don't persist the user password across form redisplays
+  - Fall back to `In-Reply-To` and `References` headers when sub-addressing is not available (David Padilla)
+  - Remove "Congratulations!" tweet button on newly-created project. (Connor Shea)
+  - Fix admin/projects when using visibility levels on search (PotHix)
+  - Build status notifications
+  - API: Expose user location (Robert Schilling)
+  - API: Do not leak group existence via return code (Robert Schilling)
+  - ClosingIssueExtractor regex now also works with colons. e.g. "Fixes: #1234" !3591
+  - Update number of Todos in the sidebar when it's marked as "Done". !3600
+  - Sanitize branch names created for confidential issues
+  - API: Expose 'updated_at' for issue, snippet, and merge request notes (Robert Schilling)
+  - API: User can leave a project through the API when not master or owner. !3613
+  - Fix repository cache invalidation issue when project is recreated with an empty repo (Stan Hu)
+  - Fix: Allow empty recipients list for builds emails service when pushed is added (Frank Groeneveld)
+  - Improved markdown forms
+  - Diff design updates (colors, button styles, etc)
+  - Copying and pasting a diff no longer pastes the line numbers or +/-
+  - Add null check to formData when updating profile content to fix Firefox bug
+  - Disable spellcheck and autocorrect for username field in admin page
+  - Delete tags using Rugged for performance reasons (Robert Schilling)
+  - Add Slack notifications when Wiki is edited (Sebastian Klier)
+  - Diffs load at the correct point when linking from from number
+  - Selected diff rows highlight
+  - Fix emoji categories in the emoji picker
+  - API: Properly display annotated tags for GET /projects/:id/repository/tags (Robert Schilling)
+  - Add encrypted credentials for imported projects and migrate old ones
+  - Properly format all merge request references with ! rather than # !3740 (Ben Bodenmiller)
+  - Author and participants are displayed first on users autocompletion
+  - Show number sign on external issue reference text (Florent Baldino)
+  - Updated print style for issues
+  - Use GitHub Issue/PR number as iid to keep references
+  - Import GitHub labels
+  - Import GitHub milestones
+  - Fix emoji catgories in the emoji picker
+  - Execute system web hooks on push to the project
+  - Allow enable/disable push events for system hooks
+  - Fix GitHub project's link in the import page when provider has a custom URL
+  - Add RAW build trace output and button on build page
+  - Add incremental build trace update into CI API
+
 v 8.6.7
   - Fix persistent XSS vulnerability in `commit_person_link` helper
   - Fix persistent XSS vulnerability in Label and Milestone dropdowns

app/services/issuable_base_service.rb

@@ -37,8 +37,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}"
 
@@ -49,6 +50,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

@@ -3,40 +3,75 @@ require 'spec_helper'
 describe Issues::CreateService, services: true do
   let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
+
+  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) }
 
-  describe :execute do
-    context 'valid params' do
       before do
         project.team << [user, :master]
         project.team << [assignee, :master]
+      end
 
-        opts = {
-          title: 'Awesome issue',
+      let(:opts) do
+        { title: 'Awesome issue',
           description: 'please fix',
-          assignee: assignee
-        }
-
-        @issue = Issues::CreateService.new(project, user, opts).execute
+          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).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

spec/services/issues/update_service_spec.rb

@@ -4,10 +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) { create(:empty_project) }
+  let(:label) { create(:label, project: project) }
   let(:label2) { create(:label) }
-  let(:project) { issue.project }
+
+  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]
     project.team << [user2, :developer]