Commit ae54a129 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-protect-private-repo-information-11-8' into '11-8-stable'

Fix leaking private repository information in API

See merge request gitlab/gitlabhq!2948
parents 5eb62fb9 cf8449bf
...@@ -16,7 +16,6 @@ module Types ...@@ -16,7 +16,6 @@ module Types
field :description, GraphQL::STRING_TYPE, null: true field :description, GraphQL::STRING_TYPE, null: true
field :default_branch, GraphQL::STRING_TYPE, null: true
field :tag_list, GraphQL::STRING_TYPE, null: true field :tag_list, GraphQL::STRING_TYPE, null: true
field :ssh_url_to_repo, GraphQL::STRING_TYPE, null: true field :ssh_url_to_repo, GraphQL::STRING_TYPE, null: true
...@@ -59,7 +58,6 @@ module Types ...@@ -59,7 +58,6 @@ module Types
end end
field :import_status, GraphQL::STRING_TYPE, null: true 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 :only_allow_merge_if_pipeline_succeeds, GraphQL::BOOLEAN_TYPE, null: true
field :request_access_enabled, GraphQL::BOOLEAN_TYPE, null: true field :request_access_enabled, GraphQL::BOOLEAN_TYPE, null: true
......
---
title: Fix leaking private repository information in API
merge_request:
author:
type: security
...@@ -156,7 +156,7 @@ module API ...@@ -156,7 +156,7 @@ module API
class BasicProjectDetails < ProjectIdentity class BasicProjectDetails < ProjectIdentity
include ::API::ProjectsRelationBuilder 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 # Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770
expose :tag_list do |project| expose :tag_list do |project|
# project.tags.order(:name).pluck(:name) is the most suitable option # project.tags.order(:name).pluck(:name) is the most suitable option
...@@ -261,7 +261,7 @@ module API ...@@ -261,7 +261,7 @@ module API
expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) } 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 :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
expose :public_builds, as: :public_jobs 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| expose :shared_with_groups do |project, options|
SharedGroup.represent(project.project_group_links, options) SharedGroup.represent(project.project_group_links, options)
end end
...@@ -270,8 +270,9 @@ module API ...@@ -270,8 +270,9 @@ module API
expose :only_allow_merge_if_all_discussions_are_resolved expose :only_allow_merge_if_all_discussions_are_resolved
expose :printing_merge_request_link_enabled expose :printing_merge_request_link_enabled
expose :merge_method expose :merge_method
expose :statistics, using: 'API::Entities::ProjectStatistics', if: -> (project, options) {
expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics options[:statistics] && Ability.allowed?(options[:current_user], :download_code, project)
}
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def self.preload_relation(projects_relation, options = {}) def self.preload_relation(projects_relation, options = {})
......
...@@ -22,7 +22,7 @@ module API ...@@ -22,7 +22,7 @@ module API
get ':id/environments' do get ':id/environments' do
authorize! :read_environment, user_project 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 end
desc 'Creates a new environment' do desc 'Creates a new environment' do
...@@ -40,7 +40,7 @@ module API ...@@ -40,7 +40,7 @@ module API
environment = user_project.environments.create(declared_params) environment = user_project.environments.create(declared_params)
if environment.persisted? if environment.persisted?
present environment, with: Entities::Environment present environment, with: Entities::Environment, current_user: current_user
else else
render_validation_error!(environment) render_validation_error!(environment)
end end
...@@ -63,7 +63,7 @@ module API ...@@ -63,7 +63,7 @@ module API
update_params = declared_params(include_missing: false).extract!(:name, :external_url) update_params = declared_params(include_missing: false).extract!(:name, :external_url)
if environment.update(update_params) if environment.update(update_params)
present environment, with: Entities::Environment present environment, with: Entities::Environment, current_user: current_user
else else
render_validation_error!(environment) render_validation_error!(environment)
end end
...@@ -99,7 +99,7 @@ module API ...@@ -99,7 +99,7 @@ module API
environment.stop_with_action!(current_user) environment.stop_with_action!(current_user)
status 200 status 200
present environment, with: Entities::Environment present environment, with: Entities::Environment, current_user: current_user
end end
end end
end end
......
...@@ -184,7 +184,8 @@ module API ...@@ -184,7 +184,8 @@ module API
if project.saved? if project.saved?
present project, with: Entities::Project, 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 else
if project.errors[:limit_reached].present? if project.errors[:limit_reached].present?
error!(project.errors[:limit_reached], 403) error!(project.errors[:limit_reached], 403)
...@@ -217,7 +218,8 @@ module API ...@@ -217,7 +218,8 @@ module API
if project.saved? if project.saved?
present project, with: Entities::Project, 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 else
render_validation_error!(project) render_validation_error!(project)
end end
...@@ -279,7 +281,8 @@ module API ...@@ -279,7 +281,8 @@ module API
conflict!(forked_project.errors.messages) conflict!(forked_project.errors.messages)
else else
present forked_project, with: Entities::Project, 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
end end
...@@ -328,7 +331,8 @@ module API ...@@ -328,7 +331,8 @@ module API
if result[:status] == :success if result[:status] == :success
present user_project, with: Entities::Project, 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 else
render_validation_error!(user_project) render_validation_error!(user_project)
end end
...@@ -342,7 +346,7 @@ module API ...@@ -342,7 +346,7 @@ module API
::Projects::UpdateService.new(user_project, current_user, archived: true).execute ::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 end
desc 'Unarchive a project' do desc 'Unarchive a project' do
...@@ -353,7 +357,7 @@ module API ...@@ -353,7 +357,7 @@ module API
::Projects::UpdateService.new(@project, current_user, archived: false).execute ::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 end
desc 'Star a project' do desc 'Star a project' do
...@@ -366,7 +370,7 @@ module API ...@@ -366,7 +370,7 @@ module API
current_user.toggle_star(user_project) current_user.toggle_star(user_project)
user_project.reload user_project.reload
present user_project, with: Entities::Project present user_project, with: Entities::Project, current_user: current_user
end end
end end
...@@ -378,7 +382,7 @@ module API ...@@ -378,7 +382,7 @@ module API
current_user.toggle_star(user_project) current_user.toggle_star(user_project)
user_project.reload user_project.reload
present user_project, with: Entities::Project present user_project, with: Entities::Project, current_user: current_user
else else
not_modified! not_modified!
end end
...@@ -414,7 +418,7 @@ module API ...@@ -414,7 +418,7 @@ module API
result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project) result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project)
if result if result
present user_project.reload, with: Entities::Project present user_project.reload, with: Entities::Project, current_user: current_user
else else
render_api_error!("Project already forked", 409) if user_project.forked? render_api_error!("Project already forked", 409) if user_project.forked?
end end
...@@ -517,7 +521,7 @@ module API ...@@ -517,7 +521,7 @@ module API
result = ::Projects::TransferService.new(user_project, current_user).execute(namespace) result = ::Projects::TransferService.new(user_project, current_user).execute(namespace)
if result if result
present user_project, with: Entities::Project present user_project, with: Entities::Project, current_user: current_user
else else
render_api_error!("Failed to transfer project #{user_project.errors.messages}", 400) render_api_error!("Failed to transfer project #{user_project.errors.messages}", 400)
end end
......
...@@ -110,6 +110,7 @@ describe API::Projects do ...@@ -110,6 +110,7 @@ describe API::Projects do
end end
let!(:public_project) { create(:project, :public, name: 'public_project') } let!(:public_project) { create(:project, :public, name: 'public_project') }
before do before do
project project
project2 project2
...@@ -942,8 +943,16 @@ describe API::Projects do ...@@ -942,8 +943,16 @@ describe API::Projects do
describe 'GET /projects/:id' do describe 'GET /projects/:id' do
context 'when unauthenticated' do context 'when unauthenticated' do
it 'returns the public projects' do it 'does not return private projects' do
public_project = create(:project, :public) 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}") get api("/projects/#{public_project.id}")
...@@ -951,8 +960,34 @@ describe API::Projects do ...@@ -951,8 +960,34 @@ describe API::Projects do
expect(json_response['id']).to eq(public_project.id) expect(json_response['id']).to eq(public_project.id)
expect(json_response['description']).to eq(public_project.description) expect(json_response['description']).to eq(public_project.description)
expect(json_response['default_branch']).to eq(public_project.default_branch) 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') expect(json_response.keys).not_to include('permissions')
end 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 end
context 'when authenticated' do context 'when authenticated' do
...@@ -1104,6 +1139,26 @@ describe API::Projects do ...@@ -1104,6 +1139,26 @@ describe API::Projects do
expect(json_response).to include 'statistics' expect(json_response).to include 'statistics'
end 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 it "includes import_error if user can admin project" do
get api("/projects/#{project.id}", user) get api("/projects/#{project.id}", user)
......
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