Merge branch 'morefice/fix-artifacts-content-type-endpoint' into 'master'

Set content-type as plain/text for raw artifact endpoint

See merge request gitlab-org/gitlab!83826

app/controllers/projects/artifacts_controller.rb

@@ -81,7 +81,11 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
     path = Gitlab::Ci::Build::Artifacts::Path.new(params[:path])
 
-    send_artifacts_entry(artifacts_file, path)
+    if ::Feature.enabled?(:ci_safe_artifact_content_type, project, default_enabled: :yaml)
+      send_artifacts_entry(artifacts_file, path)
+    else
+      legacy_send_artifacts_entry(artifacts_file, path)
+    end
   end
 
   def keep

app/helpers/workhorse_helper.rb

@@ -36,8 +36,16 @@ module WorkhorseHelper
   end
 
   # Send an entry from artifacts through Workhorse
+  def legacy_send_artifacts_entry(file, entry)
+    headers.store(*Gitlab::Workhorse.send_artifacts_entry(file, entry))
+    head :ok
+  end
+
+  # Send an entry from artifacts through Workhorse and set safe content type
   def send_artifacts_entry(file, entry)
     headers.store(*Gitlab::Workhorse.send_artifacts_entry(file, entry))
+    headers.store(*Gitlab::Workhorse.detect_content_type)
+
     head :ok
   end
 

config/feature_flags/development/ci_safe_artifact_content_type.yml

+---
+name: ci_safe_artifact_content_type
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83826
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/357664
+milestone: '14.10'
+type: development
+group: group::pipeline insights
+default_enabled: false

lib/api/ci/job_artifacts.rb

@@ -60,7 +60,11 @@ module API
 
             bad_request! unless path.valid?
 
-            send_artifacts_entry(build.artifacts_file, path)
+            if ::Feature.enabled?(:ci_safe_artifact_content_type, build&.project, default_enabled: :yaml)
+              send_artifacts_entry(build.artifacts_file, path)
+            else
+              legacy_send_artifacts_entry(build.artifacts_file, path)
+            end
           end
 
         desc 'Download the artifacts archive from a job' do
@@ -100,7 +104,11 @@ module API
 
           bad_request! unless path.valid?
 
-          send_artifacts_entry(build.artifacts_file, path)
+          # This endpoint is being used for Artifact Browser feature that renders the content via pages.
+          # Since Content-Type is controlled by Rails and Workhorse, if a wrong
+          # content-type is sent, it could cause a regression on pages rendering.
+          # See https://gitlab.com/gitlab-org/gitlab/-/issues/357078 for more information.
+          legacy_send_artifacts_entry(build.artifacts_file, path)
         end
 
         desc 'Keep the artifacts to prevent them from being deleted' do

lib/api/helpers.rb

@@ -705,8 +705,16 @@ module API
       body ''
     end
 
+    # Deprecated. Use `send_artifacts_entry` instead.
+    def legacy_send_artifacts_entry(file, entry)
+      header(*Gitlab::Workhorse.send_artifacts_entry(file, entry))
+
+      body ''
+    end
+
     def send_artifacts_entry(file, entry)
       header(*Gitlab::Workhorse.send_artifacts_entry(file, entry))
+      header(*Gitlab::Workhorse.detect_content_type)
 
       body ''
     end

lib/gitlab/workhorse.rb

@@ -226,6 +226,13 @@ module Gitlab
         end
       end
 
+      def detect_content_type
+        [
+          Gitlab::Workhorse::DETECT_HEADER,
+          'true'
+        ]
+      end
+
       protected
 
       # This is the outermost encoding of a senddata: header. It is safe for

spec/controllers/projects/artifacts_controller_spec.rb

@@ -323,6 +323,7 @@ RSpec.describe Projects::ArtifactsController do
           subject
 
           expect(response).to have_gitlab_http_status(:ok)
+          expect(response.headers['Gitlab-Workhorse-Detect-Content-Type']).to eq('true')
           expect(send_data).to start_with('artifacts-entry:')
 
           expect(params.keys).to eq(%w(Archive Entry))
@@ -380,6 +381,18 @@ RSpec.describe Projects::ArtifactsController do
             let(:store) { ObjectStorage::Store::LOCAL }
             let(:archive_path) { JobArtifactUploader.root }
           end
+
+          context 'when ci_safe_artifact_content_type is disabled' do
+            before do
+              stub_feature_flags(ci_safe_artifact_content_type: false)
+            end
+
+            it 'does not let workhorse set content type' do
+              subject
+
+              expect(response.headers).not_to include('Gitlab-Workhorse-Detect-Content-Type')
+            end
+          end
         end
 
         context 'when the artifact is not zip' do

spec/lib/gitlab/workhorse_spec.rb

@@ -448,6 +448,14 @@ RSpec.describe Gitlab::Workhorse do
     end
   end
 
+  describe '.detect_content_type' do
+    subject { described_class.detect_content_type }
+
+    it 'returns array setting detect content type in workhorse' do
+      expect(subject).to eq(%w[Gitlab-Workhorse-Detect-Content-Type true])
+    end
+  end
+
   describe '.send_git_blob' do
     include FakeBlobHelpers
 

spec/requests/api/ci/job_artifacts_spec.rb

@@ -224,6 +224,8 @@ RSpec.describe API::Ci::JobArtifacts do
           expect(response.headers.to_h)
             .to include('Content-Type' => 'application/json',
                         'Gitlab-Workhorse-Send-Data' => /artifacts-entry/)
+          expect(response.headers.to_h)
+            .not_to include('Gitlab-Workhorse-Detect-Content-Type' => 'true')
           expect(response.parsed_body).to be_empty
         end
 
@@ -556,7 +558,18 @@ RSpec.describe API::Ci::JobArtifacts do
             expect(response).to have_gitlab_http_status(:ok)
             expect(response.headers.to_h)
               .to include('Content-Type' => 'application/json',
-                          'Gitlab-Workhorse-Send-Data' => /artifacts-entry/)
+                          'Gitlab-Workhorse-Send-Data' => /artifacts-entry/,
+                          'Gitlab-Workhorse-Detect-Content-Type' => 'true')
+          end
+
+          context 'when ci_safe_artifact_content_type is disabled' do
+            before do
+              stub_feature_flags(ci_safe_artifact_content_type: false)
+            end
+
+            it 'does not let workhorse set content type' do
+              expect(response.headers).not_to include('Gitlab-Workhorse-Detect-Content-Type')
+            end
           end
         end
 
@@ -626,7 +639,8 @@ RSpec.describe API::Ci::JobArtifacts do
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.headers.to_h)
             .to include('Content-Type' => 'application/json',
-                        'Gitlab-Workhorse-Send-Data' => /artifacts-entry/)
+                        'Gitlab-Workhorse-Send-Data' => /artifacts-entry/,
+                        'Gitlab-Workhorse-Detect-Content-Type' => 'true')
           expect(response.parsed_body).to be_empty
         end
       end
@@ -644,7 +658,8 @@ RSpec.describe API::Ci::JobArtifacts do
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.headers.to_h)
             .to include('Content-Type' => 'application/json',
-                        'Gitlab-Workhorse-Send-Data' => /artifacts-entry/)
+                        'Gitlab-Workhorse-Send-Data' => /artifacts-entry/,
+                        'Gitlab-Workhorse-Detect-Content-Type' => 'true')
         end
       end