Commit d2db3649 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'rs-pick-security-fixes' into 'master'

Pick security fixes from 8.16.1 et al into master

Closes #26813, #26249, #26259, #26243, #26242

See merge request !8724
parents 2d3fcf90 76deb55f
......@@ -21,7 +21,7 @@ gem 'rugged', '~> 0.24.0'
# Authentication libraries
gem 'devise', '~> 4.2'
gem 'doorkeeper', '~> 4.2.0'
gem 'omniauth', '~> 1.3.1'
gem 'omniauth', '~> 1.3.2'
gem 'omniauth-auth0', '~> 1.4.1'
gem 'omniauth-azure-oauth2', '~> 0.0.6'
gem 'omniauth-cas3', '~> 1.1.2'
......
......@@ -449,7 +449,7 @@ GEM
octokit (4.6.2)
sawyer (~> 0.8.0, >= 0.5.3)
oj (2.17.4)
omniauth (1.3.1)
omniauth (1.3.2)
hashie (>= 1.2, < 4)
rack (>= 1.0, < 3)
omniauth-auth0 (1.4.1)
......@@ -925,7 +925,7 @@ DEPENDENCIES
oauth2 (~> 1.2.0)
octokit (~> 4.6.2)
oj (~> 2.17.4)
omniauth (~> 1.3.1)
omniauth (~> 1.3.2)
omniauth-auth0 (~> 1.4.1)
omniauth-authentiq (~> 0.2.0)
omniauth-azure-oauth2 (~> 0.0.6)
......
......@@ -130,6 +130,8 @@ class Namespace < ActiveRecord::Base
Gitlab::UploadsTransfer.new.rename_namespace(path_was, path)
remove_exports!
# If repositories moved successfully we need to
# send update instructions to users.
# However we cannot allow rollback since we moved namespace dir
......@@ -214,6 +216,8 @@ class Namespace < ActiveRecord::Base
GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path)
end
end
remove_exports!
end
def refresh_access_of_projects_invited_groups
......@@ -226,4 +230,20 @@ class Namespace < ActiveRecord::Base
def full_path_changed?
path_changed? || parent_id_changed?
end
def remove_exports!
Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
end
def export_path
File.join(Gitlab::ImportExport.storage_path, full_path_was)
end
def full_path_was
if parent
parent.full_path + '/' + path_was
else
path_was
end
end
end
---
title: Ensure export files are removed after a namespace is deleted
merge_request:
author:
---
title: Don't allow project guests to subscribe to merge requests through the API
merge_request:
author: Robert Schilling
---
title: Prevent users from creating notes on resources they can't access
merge_request:
author:
---
title: Prevent users from deleting system deploy keys via the project deploy key API
merge_request:
author:
---
title: Upgrade omniauth gem to 1.3.2
merge_request:
author:
......@@ -105,15 +105,19 @@ module API
present key.deploy_key, with: Entities::SSHKey
end
desc 'Delete existing deploy key of currently authenticated user' do
desc 'Delete deploy key for a project' do
success Key
end
params do
requires :key_id, type: Integer, desc: 'The ID of the deploy key'
end
delete ":id/#{path}/:key_id" do
key = user_project.deploy_keys.find(params[:key_id])
key.destroy
key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
if key
key.destroy
else
not_found!('Deploy Key')
end
end
end
end
......
......@@ -90,6 +90,12 @@ module API
MergeRequestsFinder.new(current_user, project_id: user_project.id).find(id)
end
def find_merge_request_with_access(id, access_level = :read_merge_request)
merge_request = user_project.merge_requests.find(id)
authorize! access_level, merge_request
merge_request
end
def authenticate!
unauthorized! unless current_user
end
......
......@@ -15,10 +15,8 @@ module API
end
get ":id/merge_requests/:merge_request_id/versions" do
merge_request = user_project.merge_requests.
find(params[:merge_request_id])
merge_request = find_merge_request_with_access(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff
end
......@@ -34,10 +32,8 @@ module API
end
get ":id/merge_requests/:merge_request_id/versions/:version_id" do
merge_request = user_project.merge_requests.
find(params[:merge_request_id])
merge_request = find_merge_request_with_access(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull
end
end
......
......@@ -118,8 +118,8 @@ module API
success Entities::MergeRequest
end
get path do
merge_request = find_project_merge_request(params[:merge_request_id])
authorize! :read_merge_request, merge_request
merge_request = find_merge_request_with_access(params[:merge_request_id])
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
end
......@@ -127,8 +127,8 @@ module API
success Entities::RepoCommit
end
get "#{path}/commits" do
merge_request = find_project_merge_request(params[:merge_request_id])
authorize! :read_merge_request, merge_request
merge_request = find_merge_request_with_access(params[:merge_request_id])
present merge_request.commits, with: Entities::RepoCommit
end
......@@ -136,8 +136,8 @@ module API
success Entities::MergeRequestChanges
end
get "#{path}/changes" do
merge_request = find_project_merge_request(params[:merge_request_id])
authorize! :read_merge_request, merge_request
merge_request = find_merge_request_with_access(params[:merge_request_id])
present merge_request, with: Entities::MergeRequestChanges, current_user: current_user
end
......@@ -155,8 +155,7 @@ module API
:remove_source_branch
end
put path do
merge_request = find_project_merge_request(params.delete(:merge_request_id))
authorize! :update_merge_request, merge_request
merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request)
mr_params = declared_params(include_missing: false)
mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present?
......@@ -235,10 +234,7 @@ module API
use :pagination
end
get "#{path}/comments" do
merge_request = find_project_merge_request(params[:merge_request_id])
authorize! :read_merge_request, merge_request
merge_request = find_merge_request_with_access(params[:merge_request_id])
present paginate(merge_request.notes.fresh), with: Entities::MRNote
end
......@@ -250,8 +246,7 @@ module API
requires :note, type: String, desc: 'The text of the comment'
end
post "#{path}/comments" do
merge_request = find_project_merge_request(params[:merge_request_id])
authorize! :create_note, merge_request
merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note)
opts = {
note: params[:note],
......@@ -275,7 +270,7 @@ module API
use :pagination
end
get "#{path}/closes_issues" do
merge_request = find_project_merge_request(params[:merge_request_id])
merge_request = find_merge_request_with_access(params[:merge_request_id])
issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user))
present paginate(issues), with: issue_entity(user_project), current_user: current_user
end
......
......@@ -70,21 +70,27 @@ module API
end
post ":id/#{noteables_str}/:noteable_id/notes" do
opts = {
note: params[:body],
noteable_type: noteables_str.classify,
noteable_id: params[:noteable_id]
note: params[:body],
noteable_type: noteables_str.classify,
noteable_id: params[:noteable_id]
}
if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user)
opts[:created_at] = params[:created_at]
end
noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
if can?(current_user, noteable_read_ability_name(noteable), noteable)
if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user)
opts[:created_at] = params[:created_at]
end
note = ::Notes::CreateService.new(user_project, current_user, opts).execute
note = ::Notes::CreateService.new(user_project, current_user, opts).execute
if note.valid?
present note, with: Entities::const_get(note.class.name)
if note.valid?
present note, with: Entities::const_get(note.class.name)
else
not_found!("Note #{note.errors.messages}")
end
else
not_found!("Note #{note.errors.messages}")
not_found!("Note")
end
end
......
......@@ -3,8 +3,8 @@ module API
before { authenticate! }
subscribable_types = {
'merge_request' => proc { |id| user_project.merge_requests.find(id) },
'merge_requests' => proc { |id| user_project.merge_requests.find(id) },
'merge_request' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
'issues' => proc { |id| find_project_issue(id) },
'labels' => proc { |id| find_project_label(id) },
}
......
......@@ -5,7 +5,7 @@ module API
before { authenticate! }
ISSUABLE_TYPES = {
'merge_requests' => ->(id) { user_project.merge_requests.find(id) },
'merge_requests' => ->(id) { find_merge_request_with_access(id) },
'issues' => ->(id) { find_project_issue(id) }
}
......
require 'spec_helper'
feature 'Import/Export - Namespace export file cleanup', feature: true, js: true do
let(:export_path) { "#{Dir::tmpdir}/import_file_spec" }
let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
let(:project) { create(:empty_project) }
background do
allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
end
after do
FileUtils.rm_rf(export_path, secure: true)
end
context 'admin user' do
before do
login_as(:admin)
end
context 'moving the namespace' do
scenario 'removes the export file' do
setup_export_project
old_export_path = project.export_path.dup
expect(File).to exist(old_export_path)
project.namespace.update(path: 'new_path')
expect(File).not_to exist(old_export_path)
end
end
context 'deleting the namespace' do
scenario 'removes the export file' do
setup_export_project
old_export_path = project.export_path.dup
expect(File).to exist(old_export_path)
project.namespace.destroy
expect(File).not_to exist(old_export_path)
end
end
def setup_export_project
visit edit_namespace_project_path(project.namespace, project)
expect(page).to have_content('Export project')
click_link 'Export project'
visit edit_namespace_project_path(project.namespace, project)
expect(page).to have_content('Download export')
end
end
end
......@@ -117,6 +117,7 @@ describe Namespace, models: true do
new_path = @namespace.path + "_new"
allow(@namespace).to receive(:path_was).and_return(@namespace.path)
allow(@namespace).to receive(:path).and_return(new_path)
expect(@namespace).to receive(:remove_exports!)
expect(@namespace.move_dir).to be_truthy
end
......@@ -139,11 +140,17 @@ describe Namespace, models: true do
let!(:project) { create(:project, namespace: namespace) }
let!(:path) { File.join(Gitlab.config.repositories.storages.default, namespace.path) }
before { namespace.destroy }
it "removes its dirs when deleted" do
namespace.destroy
expect(File.exist?(path)).to be(false)
end
it 'removes the exports folder' do
expect(namespace).to receive(:remove_exports!)
namespace.destroy
end
end
describe '.find_by_path_or_name' do
......
......@@ -627,6 +627,17 @@ describe API::MergeRequests, api: true do
expect(json_response.first['title']).to eq(issue.title)
expect(json_response.first['id']).to eq(issue.id)
end
it 'returns 403 if the user has no access to the merge request' do
project = create(:empty_project, :private)
merge_request = create(:merge_request, :simple, source_project: project)
guest = create(:user)
project.team << [guest, :guest]
get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/closes_issues", guest)
expect(response).to have_http_status(403)
end
end
describe 'POST :id/merge_requests/:merge_request_id/subscription' do
......@@ -648,6 +659,15 @@ describe API::MergeRequests, api: true do
expect(response).to have_http_status(404)
end
it 'returns 403 if user has no access to read code' do
guest = create(:user)
project.team << [guest, :guest]
post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest)
expect(response).to have_http_status(403)
end
end
describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do
......@@ -669,6 +689,15 @@ describe API::MergeRequests, api: true do
expect(response).to have_http_status(404)
end
it 'returns 403 if user has no access to read code' do
guest = create(:user)
project.team << [guest, :guest]
delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest)
expect(response).to have_http_status(403)
end
end
describe 'Time tracking' do
......
......@@ -264,6 +264,18 @@ describe API::Notes, api: true do
end
end
context 'when user does not have access to read the noteable' do
it 'responds with 404' do
project = create(:empty_project, :private) { |p| p.add_guest(user) }
issue = create(:issue, :confidential, project: project)
post api("/projects/#{project.id}/issues/#{issue.id}/notes", user),
body: 'Foo'
expect(response).to have_http_status(404)
end
end
context 'when user does not have access to create noteable' do
let(:private_issue) { create(:issue, project: create(:empty_project, :private)) }
......
......@@ -183,12 +183,25 @@ describe API::Todos, api: true do
expect(response.status).to eq(404)
end
it 'returns an error if the issuable is not accessible' do
guest = create(:user)
project_1.team << [guest, :guest]
post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.id}/todo", guest)
if issuable_type == 'merge_requests'
expect(response).to have_http_status(403)
else
expect(response).to have_http_status(404)
end
end
end
describe 'POST :id/issuable_type/:issueable_id/todo' do
context 'for an issue' do
it_behaves_like 'an issuable', 'issues' do
let(:issuable) { create(:issue, author: author_1, project: project_1) }
let(:issuable) { create(:issue, :confidential, author: author_1, project: project_1) }
end
end
......
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