From 618b87448e9167f39d8216d1100733cc0fbf020b Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 31 Jan 2019 17:21:35 +1300 Subject: Prevent leaking of private repo data through API default_branch, statistics and config_ci_path are now only exposed if the user has permissions to the repository. --- lib/api/entities.rb | 9 +++--- lib/api/environments.rb | 8 +++--- lib/api/projects.rb | 24 +++++++++------- spec/requests/api/projects_spec.rb | 59 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 80 insertions(+), 20 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 27da2c2e5ed..46cd4841e2d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -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 = {}) diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 0278c6c54a5..5b0f3b914cb 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -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 diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 6a93ef9f3ad..2325fc96a67 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -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 @@ -279,7 +281,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 @@ -328,7 +331,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 @@ -342,7 +346,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 @@ -353,7 +357,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 @@ -366,7 +370,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 @@ -378,7 +382,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 @@ -414,7 +418,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 @@ -520,7 +524,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 diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index cfa7a1a31a3..1c05be8de70 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -110,6 +110,7 @@ describe API::Projects do end let!(:public_project) { create(:project, :public, name: 'public_project') } + before do project project2 @@ -942,8 +943,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}") @@ -951,8 +960,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 @@ -1104,6 +1139,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) -- cgit v1.2.3 From 19cb1985c76ae756194a05f21af0d02caedb60c2 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 7 Feb 2019 12:04:09 +1300 Subject: Removing sensitive properties from ProjectType defaultBranch and ciConfigPath should only be available to users with the :download_code permission for the Project, as the respository might be private. When implementing the authorize check on these properties, it was found that our current Graphql::Authorize::Instrumentation class does not work with fields that resolve to subclasses of GraphQL::Schema::Scalar, like GraphQL::STRING_TYPE. After discussion with other Create Team members, it has been decided that because the GraphQL API is not GA, to remove these properties from ProjectType, and instead implement them as part of epic https://gitlab.com/groups/gitlab-org/-/epics/711 Issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/55316 --- app/graphql/types/project_type.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index d25c8c8bd90..8a6ccb8c5c1 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -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 -- cgit v1.2.3 From b11d018bd59a3c94086dbeb11f38db81e441a1e2 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 7 Feb 2019 12:46:41 +1300 Subject: Add changelog entry --- changelogs/unreleased/security-protect-private-repo-information.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-protect-private-repo-information.yml diff --git a/changelogs/unreleased/security-protect-private-repo-information.yml b/changelogs/unreleased/security-protect-private-repo-information.yml new file mode 100644 index 00000000000..8b1a528206d --- /dev/null +++ b/changelogs/unreleased/security-protect-private-repo-information.yml @@ -0,0 +1,5 @@ +--- +title: Fix leaking private repository information in API +merge_request: +author: +type: security -- cgit v1.2.3