Merge branch 'security-aw-secure-trace-downloads-14-10' into '14-10-stable-ee'

Secure debug trace artifact download

See merge request gitlab-org/security/gitlab!2376

app/controllers/projects/application_controller.rb

@@ -32,6 +32,21 @@ class Projects::ApplicationController < ApplicationController
     ->(project) { !project.pending_delete? }
   end
 
+  def authorize_read_build_trace!
+    return if can?(current_user, :read_build_trace, build)
+
+    if build.debug_mode?
+      access_denied!(
+        _('You must have developer or higher permissions in the associated project to view job logs when debug trace ' \
+          "is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' variable to 'false' in your pipeline " \
+          'configuration or CI/CD settings. If you need to view this job log, a project maintainer must add you to ' \
+          'the project with developer permissions or higher.')
+      )
+    else
+      access_denied!(_('The current user is not authorized to access the job log.'))
+    end
+  end
+
   def build_canonical_path(project)
     params[:namespace_id] = project.namespace.to_param
     params[:project_id] = project.to_param

app/controllers/projects/artifacts_controller.rb

@@ -9,6 +9,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
   layout 'project'
   before_action :authorize_read_build!
+  before_action :authorize_read_build_trace!, only: [:download]
   before_action :authorize_update_build!, only: [:keep]
   before_action :authorize_destroy_artifacts!, only: [:destroy]
   before_action :extract_ref_name_and_path
@@ -164,4 +165,10 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
     render_404 unless @entry.exists?
   end
+
+  def authorize_read_build_trace!
+    return unless params[:file_type] == 'trace'
+
+    super
+  end
 end

app/controllers/projects/jobs_controller.rb

@@ -177,17 +177,7 @@ class Projects::JobsController < Projects::ApplicationController
 
   private
 
-  def authorize_read_build_trace!
-    return if can?(current_user, :read_build_trace, @build)
-
-    msg = _(
-      "You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' variable to 'false' in your pipeline configuration or CI/CD settings. " \
-      "If you need to view this job log, a project maintainer must add you to the project with developer permissions or higher."
-    )
-    return access_denied!(msg) if @build.debug_mode?
-
-    access_denied!(_('The current user is not authorized to access the job log.'))
-  end
+  attr_reader :build
 
   def authorize_update_build!
     return access_denied! unless can?(current_user, :update_build, @build)

spec/controllers/projects/artifacts_controller_spec.rb

@@ -204,6 +204,44 @@ RSpec.describe Projects::ArtifactsController do
         end
       end
     end
+
+    context 'when downloading a debug trace' do
+      let(:file_type) { 'trace' }
+      let(:job) { create(:ci_build, :success, :trace_artifact, pipeline: pipeline) }
+
+      before do
+        create(:ci_job_variable, key: 'CI_DEBUG_TRACE', value: 'true', job: job)
+      end
+
+      context 'when the user does not have update_build permissions' do
+        let(:user) { create(:user) }
+
+        before do
+          project.add_guest(user)
+        end
+
+        render_views
+
+        it 'denies the user access' do
+          download_artifact(file_type: file_type)
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+          expect(response.body).to include(
+            'You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. ' \
+            'To disable debug trace, set the 'CI_DEBUG_TRACE' variable to 'false' in your pipeline configuration or CI/CD settings. ' \
+            'If you need to view this job log, a project maintainer must add you to the project with developer permissions or higher.'
+          )
+        end
+      end
+
+      context 'when the user has update_build permissions' do
+        it 'sends the trace' do
+          download_artifact(file_type: file_type)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+    end
   end
 
   describe 'GET browse' do