From 33b9c811f1706f7b2882c2bf6fdbcb8b1cb522ff Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 31 Jan 2019 17:21:35 +1300 Subject: [PATCH 1/4] 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 004fd426062..5c92107d5a9 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -153,7 +153,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 @@ -258,7 +258,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 @@ -267,8 +267,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 633f24d3c9a..d5046489b7c 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 f5d21d8923f..f3611fe946c 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -180,7 +180,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) @@ -213,7 +214,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 @@ -275,7 +277,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 @@ -324,7 +327,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 @@ -338,7 +342,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 @@ -349,7 +353,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 @@ -362,7 +366,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 @@ -374,7 +378,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 @@ -410,7 +414,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 @@ -516,7 +520,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 e40db55cd20..fafe75bf886 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -89,6 +89,7 @@ describe API::Projects do end let!(:public_project) { create(:project, :public, name: 'public_project') } + before do project project2 @@ -886,8 +887,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}") @@ -895,8 +904,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 @@ -1046,6 +1081,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) -- GitLab From 49a7d52dcea5ad517b7c70cebb9ac7e12eb51c42 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 7 Feb 2019 12:04:09 +1300 Subject: [PATCH 2/4] 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 050706f97be..587e55c611f 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 -- GitLab From 12cb7ef83359b16d06f15dc806724da61a50b93f Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 7 Feb 2019 12:46:41 +1300 Subject: [PATCH 3/4] Add changelog entry --- .../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 -- GitLab From f44ab5b1e402cb05e968326ee9d77bcc9efb55c0 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 20 Feb 2019 13:08:56 +1300 Subject: [PATCH 4/4] Fix backported test for Rails 4 --- spec/requests/api/projects_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index fafe75bf886..25974d19e01 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1085,7 +1085,7 @@ describe API::Projects 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 } + get api("/projects/#{project.id}", user), statistics: true expect(response).to have_gitlab_http_status(200) expect(json_response).not_to include 'statistics' @@ -1094,7 +1094,7 @@ describe API::Projects do it "includes statistics if user is a member" do project.add_developer(user) - get api("/projects/#{project.id}", user), params: { statistics: true } + get api("/projects/#{project.id}", user), statistics: true expect(response).to have_gitlab_http_status(200) expect(json_response).to include 'statistics' -- GitLab