kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch 'security-protect-private-repo-information' into 'master'

Browse source 
Fix leaking private repository information in API

See merge request gitlab/gitlabhq!2881
This commit is contained in:
Yorick Peterse 2019-03-04 18:37:03 +00:00
commit 9faf957bd3
6 changed files with 85 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
app/graphql/types/project_type.rb
View file

@ -16,7 +16,6 @@ module Types
field :description, GraphQL::STRING_TYPE, null: true
field :default_branch, GraphQL::STRING_TYPE, null: true
field :tag_list, GraphQL::STRING_TYPE, null: true
field :ssh_url_to_repo, GraphQL::STRING_TYPE, null: true
@ -59,7 +58,6 @@ module Types
end
field :import_status, GraphQL::STRING_TYPE, null: true
field :ci_config_path, GraphQL::STRING_TYPE, null: true
field :only_allow_merge_if_pipeline_succeeds, GraphQL::BOOLEAN_TYPE, null: true
field :request_access_enabled, GraphQL::BOOLEAN_TYPE, null: true

5
changelogs/unreleased/security-protect-private-repo-information.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Fix leaking private repository information in API
merge_request:
author:
type: security

9
lib/api/entities.rb
View file

@ -156,7 +156,7 @@ module API
class BasicProjectDetails < ProjectIdentity
include ::API::ProjectsRelationBuilder
expose :default_branch
expose :default_branch, if: -> (project, options) { Ability.allowed?(options[:current_user], :download_code, project) }
# Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770
expose :tag_list do |project|
# project.tags.order(:name).pluck(:name) is the most suitable option
@ -261,7 +261,7 @@ module API
expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) }
expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
expose :public_builds, as: :public_jobs
expose :ci_config_path
expose :ci_config_path, if: -> (project, options) { Ability.allowed?(options[:current_user], :download_code, project) }
expose :shared_with_groups do |project, options|
SharedGroup.represent(project.project_group_links, options)
end
@ -270,8 +270,9 @@ module API
expose :only_allow_merge_if_all_discussions_are_resolved
expose :printing_merge_request_link_enabled
expose :merge_method
expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics
expose :statistics, using: 'API::Entities::ProjectStatistics', if: -> (project, options) {
options[:statistics] && Ability.allowed?(options[:current_user], :download_code, project)
}
# rubocop: disable CodeReuse/ActiveRecord
def self.preload_relation(projects_relation, options = {})

8
lib/api/environments.rb
View file

@ -22,7 +22,7 @@ module API
get ':id/environments' do
authorize! :read_environment, user_project
present paginate(user_project.environments), with: Entities::Environment
present paginate(user_project.environments), with: Entities::Environment, current_user: current_user
end
desc 'Creates a new environment' do
@ -40,7 +40,7 @@ module API
environment = user_project.environments.create(declared_params)
if environment.persisted?
present environment, with: Entities::Environment
present environment, with: Entities::Environment, current_user: current_user
else
render_validation_error!(environment)
end
@ -63,7 +63,7 @@ module API
update_params = declared_params(include_missing: false).extract!(:name, :external_url)
if environment.update(update_params)
present environment, with: Entities::Environment
present environment, with: Entities::Environment, current_user: current_user
else
render_validation_error!(environment)
end
@ -99,7 +99,7 @@ module API
environment.stop_with_action!(current_user)
status 200
present environment, with: Entities::Environment
present environment, with: Entities::Environment, current_user: current_user
end
end
end

24
lib/api/projects.rb
View file

@ -184,7 +184,8 @@ module API
if project.saved?
present project, with: Entities::Project,
user_can_admin_project: can?(current_user, :admin_project, project)
user_can_admin_project: can?(current_user, :admin_project, project),
current_user: current_user
else
if project.errors[:limit_reached].present?
error!(project.errors[:limit_reached], 403)
@ -217,7 +218,8 @@ module API
if project.saved?
present project, with: Entities::Project,
user_can_admin_project: can?(current_user, :admin_project, project)
user_can_admin_project: can?(current_user, :admin_project, project),
current_user: current_user
else
render_validation_error!(project)
end
@ -281,7 +283,8 @@ module API
conflict!(forked_project.errors.messages)
else
present forked_project, with: Entities::Project,
user_can_admin_project: can?(current_user, :admin_project, forked_project)
user_can_admin_project: can?(current_user, :admin_project, forked_project),
current_user: current_user
end
end
@ -330,7 +333,8 @@ module API
if result[:status] == :success
present user_project, with: Entities::Project,
user_can_admin_project: can?(current_user, :admin_project, user_project)
user_can_admin_project: can?(current_user, :admin_project, user_project),
current_user: current_user
else
render_validation_error!(user_project)
end
@ -344,7 +348,7 @@ module API
::Projects::UpdateService.new(user_project, current_user, archived: true).execute
present user_project, with: Entities::Project
present user_project, with: Entities::Project, current_user: current_user
end
desc 'Unarchive a project' do
@ -355,7 +359,7 @@ module API
::Projects::UpdateService.new(@project, current_user, archived: false).execute
present user_project, with: Entities::Project
present user_project, with: Entities::Project, current_user: current_user
end
desc 'Star a project' do
@ -368,7 +372,7 @@ module API
current_user.toggle_star(user_project)
user_project.reload
present user_project, with: Entities::Project
present user_project, with: Entities::Project, current_user: current_user
end
end
@ -380,7 +384,7 @@ module API
current_user.toggle_star(user_project)
user_project.reload
present user_project, with: Entities::Project
present user_project, with: Entities::Project, current_user: current_user
else
not_modified!
end
@ -420,7 +424,7 @@ module API
result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project)
if result
present user_project.reload, with: Entities::Project
present user_project.reload, with: Entities::Project, current_user: current_user
else
render_api_error!("Project already forked", 409) if user_project.forked?
end
@ -523,7 +527,7 @@ module API
result = ::Projects::TransferService.new(user_project, current_user).execute(namespace)
if result
present user_project, with: Entities::Project
present user_project, with: Entities::Project, current_user: current_user
else
render_api_error!("Failed to transfer project #{user_project.errors.messages}", 400)
end

59
spec/requests/api/projects_spec.rb
View file

@ -136,6 +136,7 @@ describe API::Projects do
end
let!(:public_project) { create(:project, :public, name: 'public_project') }
before do
project
project2
@ -968,8 +969,16 @@ describe API::Projects do
describe 'GET /projects/:id' do
context 'when unauthenticated' do
it 'returns the public projects' do
public_project = create(:project, :public)
it 'does not return private projects' do
private_project = create(:project, :private)
get api("/projects/#{private_project.id}")
expect(response).to have_gitlab_http_status(404)
end
it 'returns public projects' do
public_project = create(:project, :repository, :public)
get api("/projects/#{public_project.id}")
@ -977,8 +986,34 @@ describe API::Projects do
expect(json_response['id']).to eq(public_project.id)
expect(json_response['description']).to eq(public_project.description)
expect(json_response['default_branch']).to eq(public_project.default_branch)
expect(json_response['ci_config_path']).to eq(public_project.ci_config_path)
expect(json_response.keys).not_to include('permissions')
end
context 'and the project has a private repository' do
let(:project) { create(:project, :repository, :public, :repository_private) }
let(:protected_attributes) { %w(default_branch ci_config_path) }
it 'hides protected attributes of private repositories if user is not a member' do
get api("/projects/#{project.id}", user)
expect(response).to have_gitlab_http_status(200)
protected_attributes.each do |attribute|
expect(json_response.keys).not_to include(attribute)
end
end
it 'exposes protected attributes of private repositories if user is a member' do
project.add_developer(user)
get api("/projects/#{project.id}", user)
expect(response).to have_gitlab_http_status(200)
protected_attributes.each do |attribute|
expect(json_response.keys).to include(attribute)
end
end
end
end
context 'when authenticated' do
@ -1130,6 +1165,26 @@ describe API::Projects do
expect(json_response).to include 'statistics'
end
context "and the project has a private repository" do
let(:project) { create(:project, :public, :repository, :repository_private) }
it "does not include statistics if user is not a member" do
get api("/projects/#{project.id}", user), params: { statistics: true }
expect(response).to have_gitlab_http_status(200)
expect(json_response).not_to include 'statistics'
end
it "includes statistics if user is a member" do
project.add_developer(user)
get api("/projects/#{project.id}", user), params: { statistics: true }
expect(response).to have_gitlab_http_status(200)
expect(json_response).to include 'statistics'
end
end
it "includes import_error if user can admin project" do
get api("/projects/#{project.id}", user)