Commit 8dc82011 authored by Grzegorz Bizon's avatar Grzegorz Bizon Committed by Robert Speicher

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
parent 5c7de435
......@@ -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
......
......@@ -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
......
......@@ -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]
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
......@@ -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]
......
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]
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment