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.

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)