Merge branch 'security-cross-reference-fix-ce' into 'master'

Filter not accessible label events

See merge request gitlab/gitlabhq!3439

app/finders/resource_label_event_finder.rb

+# frozen_string_literal: true
+
+class ResourceLabelEventFinder
+  include FinderMethods
+
+  MAX_PER_PAGE = 100
+
+  attr_reader :params, :current_user, :eventable
+
+  def initialize(current_user, eventable, params = {})
+    @current_user = current_user
+    @eventable = eventable
+    @params = params
+  end
+
+  def execute
+    events = eventable.resource_label_events.inc_relations
+    events = events.page(page).per(per_page)
+    events = visible_to_user(events)
+
+    Kaminari.paginate_array(events)
+  end
+
+  private
+
+  def visible_to_user(events)
+    ResourceLabelEvent.preload_label_subjects(events)
+
+    events.select do |event|
+      Ability.allowed?(current_user, :read_label, event)
+    end
+  end
+
+  def per_page
+    [params[:per_page], MAX_PER_PAGE].compact.min
+  end
+
+  def page
+    params[:page] || 1
+  end
+end

app/models/resource_label_event.rb

@@ -13,6 +13,7 @@ class ResourceLabelEvent < ApplicationRecord
   belongs_to :label
 
   scope :created_after, ->(time) { where('created_at > ?', time) }
+  scope :inc_relations, -> { includes(:label, :user) }
 
   validates :user, presence: { unless: :importing? }, on: :create
   validates :label, presence: { unless: :importing? }, on: :create
@@ -30,6 +31,15 @@ class ResourceLabelEvent < ApplicationRecord
     %i(issue merge_request).freeze
   end
 
+  def self.preload_label_subjects(events)
+    labels = events.map(&:label).compact
+    project_labels, group_labels = labels.partition { |label| label.is_a? ProjectLabel }
+
+    preloader = ActiveRecord::Associations::Preloader.new
+    preloader.preload(project_labels, { project: :project_feature })
+    preloader.preload(group_labels, :group)
+  end
+
   def issuable
     issue || merge_request
   end

app/policies/resource_label_event_policy.rb

+# frozen_string_literal: true
+
+class ResourceLabelEventPolicy < BasePolicy
+  condition(:can_read_label) { @subject.label_id.nil? || can?(:read_label, @subject.label) }
+  condition(:can_read_issuable) { can?(:"read_#{@subject.issuable.to_ability_name}", @subject.issuable) }
+
+  rule { can_read_label }.policy do
+    enable :read_label
+  end
+
+  rule { can_read_label & can_read_issuable }.policy do
+    enable :read_resource_label_event
+  end
+end

changelogs/unreleased/security-cross-reference-fix.yml

+---
+title: Do not show resource label events referencing not accessible labels.
+merge_request:
+author:
+type: security

lib/api/resource_label_events.rb

@@ -24,14 +24,14 @@ module API
           use :pagination
         end
 
-        # rubocop: disable CodeReuse/ActiveRecord
         get ":id/#{eventables_str}/:eventable_id/resource_label_events" do
           eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id])
-          events = eventable.resource_label_events.includes(:label, :user)
+
+          opts = { page: params[:page], per_page: params[:per_page] }
+          events = ResourceLabelEventFinder.new(current_user, eventable, opts).execute
 
           present paginate(events), with: Entities::ResourceLabelEvent
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         desc "Get a single #{eventable_type.to_s.downcase} resource label event" do
           success Entities::ResourceLabelEvent
@@ -45,6 +45,8 @@ module API
           eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id])
           event = eventable.resource_label_events.find(params[:event_id])
 
+          not_found!('ResourceLabelEvent') unless can?(current_user, :read_resource_label_event, event)
+
           present event, with: Entities::ResourceLabelEvent
         end
       end

spec/finders/resource_label_event_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ResourceLabelEventFinder do
+  set(:user) { create(:user) }
+  set(:issue_project) { create(:project) }
+  set(:issue) { create(:issue, project: issue_project) }
+
+  describe '#execute' do
+    subject { described_class.new(user, issue).execute }
+
+    it 'returns events with labels accessible by user' do
+      label = create(:label, project: issue_project)
+      event = create_event(label)
+      issue_project.add_guest(user)
+
+      expect(subject).to eq [event]
+    end
+
+    it 'filters events with public project labels if issues and MRs are private' do
+      project = create(:project, :public, :issues_private, :merge_requests_private)
+      label = create(:label, project: project)
+      create_event(label)
+
+      expect(subject).to be_empty
+    end
+
+    it 'filters events with project labels not accessible by user' do
+      project = create(:project, :private)
+      label = create(:label, project: project)
+      create_event(label)
+
+      expect(subject).to be_empty
+    end
+
+    it 'filters events with group labels not accessible by user' do
+      group = create(:group, :private)
+      label = create(:group_label, group: group)
+      create_event(label)
+
+      expect(subject).to be_empty
+    end
+
+    it 'paginates results' do
+      label = create(:label, project: issue_project)
+      create_event(label)
+      create_event(label)
+      issue_project.add_guest(user)
+
+      paginated = described_class.new(user, issue, per_page: 1).execute
+
+      expect(subject.count).to eq 2
+      expect(paginated.count).to eq 1
+    end
+
+    def create_event(label)
+      create(:resource_label_event, issue: issue, label: label)
+    end
+  end
+end

spec/policies/resource_label_event_policy_spec.rb

+require 'spec_helper'
+
+describe ResourceLabelEventPolicy do
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :private) }
+  set(:issue) { create(:issue, project: project) }
+  set(:private_project) { create(:project, :private) }
+
+  describe '#read_resource_label_event' do
+    context 'with non-member user' do
+      it 'does not allow to read event' do
+        event = build_event(project)
+
+        expect(permissions(user, event)).to be_disallowed(:read_resource_label_event)
+      end
+    end
+
+    context 'with member user' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'allows to read event for accessible label' do
+        event = build_event(project)
+
+        expect(permissions(user, event)).to be_allowed(:read_resource_label_event)
+      end
+
+      it 'does not allow to read event for not accessible label' do
+        event = build_event(private_project)
+
+        expect(permissions(user, event)).to be_disallowed(:read_resource_label_event)
+      end
+    end
+  end
+
+  describe '#read_label' do
+    it 'allows to read deleted label' do
+      event = build(:resource_label_event, issue: issue, label: nil)
+
+      expect(permissions(user, event)).to be_allowed(:read_label)
+    end
+
+    it 'allows to read accessible label' do
+      project.add_guest(user)
+      event = build_event(project)
+
+      expect(permissions(user, event)).to be_allowed(:read_label)
+    end
+
+    it 'does not allow to read not accessible label' do
+      event = build_event(private_project)
+
+      expect(permissions(user, event)).to be_disallowed(:read_label)
+    end
+  end
+
+  def build_event(label_project)
+    label = create(:label, project: label_project)
+
+    build(:resource_label_event, issue: issue, label: label)
+  end
+
+  def permissions(user, issue)
+    described_class.new(user, issue)
+  end
+end

spec/requests/api/resource_label_events_spec.rb

@@ -5,28 +5,23 @@ require 'spec_helper'
 describe API::ResourceLabelEvents do
   set(:user) { create(:user) }
   set(:project) { create(:project, :public, namespace: user.namespace) }
+  set(:label) { create(:label, project: project) }
 
   before do
     project.add_developer(user)
   end
 
   context 'when eventable is an Issue' do
-    let(:issue) { create(:issue, project: project, author: user) }
-
     it_behaves_like 'resource_label_events API', 'projects', 'issues', 'iid' do
       let(:parent) { project }
-      let(:eventable) { issue }
-      let!(:event) { create(:resource_label_event, issue: issue) }
+      let(:eventable) { create(:issue, project: project, author: user) }
     end
   end
 
   context 'when eventable is a Merge Request' do
-    let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
-
     it_behaves_like 'resource_label_events API', 'projects', 'merge_requests', 'iid' do
       let(:parent) { project }
-      let(:eventable) { merge_request }
-      let!(:event) { create(:resource_label_event, merge_request: merge_request) }
+      let(:eventable) { create(:merge_request, source_project: project, target_project: project, author: user) }
     end
   end
 end

spec/support/shared_examples/resource_label_events_api.rb

@@ -2,6 +2,9 @@
 
 shared_examples 'resource_label_events API' do |parent_type, eventable_type, id_name|
   describe "GET /#{parent_type}/:id/#{eventable_type}/:noteable_id/resource_label_events" do
+    context "with local label reference" do
+      let!(:event) { create_event(label) }
+
       it "returns an array of resource label events" do
         get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", user)
 
@@ -27,7 +30,33 @@ shared_examples 'resource_label_events API' do |parent_type, eventable_type, id_
       end
     end
 
+    context "with cross-project label reference" do
+      let(:private_project) { create(:project, :private) }
+      let(:project_label) { create(:label, project: private_project) }
+      let!(:event) { create_event(project_label) }
+
+      it "returns cross references accessible by user" do
+        private_project.add_guest(user)
+
+        get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", user)
+
+        expect(json_response).to be_an Array
+        expect(json_response.first['id']).to eq(event.id)
+      end
+
+      it "does not return cross references not accessible by user" do
+        get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", user)
+
+        expect(json_response).to be_an Array
+        expect(json_response).to eq []
+      end
+    end
+  end
+
   describe "GET /#{parent_type}/:id/#{eventable_type}/:noteable_id/resource_label_events/:event_id" do
+    context "with local label reference" do
+      let!(:event) { create_event(label) }
+
       it "returns a resource label event by id" do
         get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", user)
 
@@ -35,10 +64,36 @@ shared_examples 'resource_label_events API' do |parent_type, eventable_type, id_
         expect(json_response['id']).to eq(event.id)
       end
 
+      it "returns 404 when not authorized" do
+        parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        private_user = create(:user)
+
+        get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", private_user)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
       it "returns a 404 error if resource label event not found" do
         get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/12345", user)
 
         expect(response).to have_gitlab_http_status(404)
       end
     end
+
+    context "with cross-project label reference" do
+      let(:private_project) { create(:project, :private) }
+      let(:project_label) { create(:label, project: private_project) }
+      let!(:event) { create_event(project_label) }
+
+      it "returns a 404 error if cross-reference project is not accessible" do
+        get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", user)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
+  def create_event(label)
+    create(:resource_label_event, eventable.class.name.underscore => eventable, label: label)
+  end
 end