Merge branch 'fj-clean-content-headers' into 'master'

Add feature flag for workhorse content type calculation

See merge request gitlab-org/gitlab-ce!22667

GITLAB_WORKHORSE_VERSION

-7.4.0
+7.5.0

Gemfile

@@ -263,6 +263,9 @@ gem 'ace-rails-ap', '~> 4.1.0'
 # Detect and convert string character encoding
 gem 'charlock_holmes', '~> 0.7.5'
 
+# Detect mime content type from content
+gem 'mimemagic', '~> 0.3.2'
+
 # Faster blank
 gem 'fast_blank'
 

Gemfile.lock

@@ -459,7 +459,7 @@ GEM
     mime-types (3.2.2)
       mime-types-data (~> 3.2015)
     mime-types-data (3.2018.0812)
-    mimemagic (0.3.0)
+    mimemagic (0.3.2)
     mini_magick (4.8.0)
     mini_mime (1.0.1)
     mini_portile2 (2.3.0)
@@ -1051,6 +1051,7 @@ DEPENDENCIES
   loofah (~> 2.2)
   mail_room (~> 0.9.1)
   method_source (~> 0.8)
+  mimemagic (~> 0.3.2)
   mini_magick
   minitest (~> 5.7.0)
   mysql2 (~> 0.4.10)

Gemfile.rails4.lock

@@ -456,7 +456,7 @@ GEM
     mime-types (3.2.2)
       mime-types-data (~> 3.2015)
     mime-types-data (3.2018.0812)
-    mimemagic (0.3.0)
+    mimemagic (0.3.2)
     mini_magick (4.8.0)
     mini_mime (1.0.1)
     mini_portile2 (2.3.0)
@@ -1042,6 +1042,7 @@ DEPENDENCIES
   loofah (~> 2.2)
   mail_room (~> 0.9.1)
   method_source (~> 0.8)
+  mimemagic (~> 0.3.2)
   mini_magick
   minitest (~> 5.7.0)
   mysql2 (~> 0.4.10)

app/controllers/concerns/snippets_actions.rb

@@ -10,6 +10,8 @@ module SnippetsActions
   def raw
     disposition = params[:inline] == 'false' ? 'attachment' : 'inline'
 
+    workhorse_set_content_type!
+
     send_data(
       convert_line_endings(@snippet.content),
       type: 'text/plain; charset=utf-8',

app/controllers/concerns/uploads_actions.rb

@@ -38,6 +38,7 @@ module UploadsActions
 
     return render_404 unless uploader
 
+    workhorse_set_content_type!
     send_upload(uploader, attachment: uploader.filename, disposition: disposition)
   end
 

app/controllers/projects/jobs_controller.rb

@@ -140,15 +140,22 @@ class Projects::JobsController < Projects::ApplicationController
 
   def raw
     if trace_artifact_file
+      workhorse_set_content_type!
       send_upload(trace_artifact_file,
                   send_params: raw_send_params,
                   redirect_params: raw_redirect_params)
     else
       build.trace.read do |stream|
         if stream.file?
+          workhorse_set_content_type!
           send_file stream.path, type: 'text/plain; charset=utf-8', disposition: 'inline'
         else
-          send_data stream.raw, type: 'text/plain; charset=utf-8', disposition: 'inline', filename: 'job.log'
+          # In this case we can't use workhorse_set_content_type! and let
+          # Workhorse handle the response because the data is streamed directly
+          # to the user but, because we have the trace content, we can calculate
+          # the proper content type and disposition here.
+          raw_data = stream.raw
+          send_data raw_data, type: 'text/plain; charset=utf-8', disposition: raw_trace_content_disposition(raw_data), filename: 'job.log'
         end
       end
     end
@@ -201,4 +208,13 @@ class Projects::JobsController < Projects::ApplicationController
   def build_path(build)
     project_job_path(build.project, build)
   end
+
+  def raw_trace_content_disposition(raw_data)
+    mime_type = MimeMagic.by_magic(raw_data)
+
+    # if mime_type is nil can also represent 'text/plain'
+    return 'inline' if mime_type.nil? || mime_type.type == 'text/plain'
+
+    'attachment'
+  end
 end

app/helpers/blob_helper.rb

@@ -140,6 +140,8 @@ module BlobHelper
     Gitlab::Sanitizers::SVG.clean(data)
   end
 
+  # Remove once https://gitlab.com/gitlab-org/gitlab-ce/issues/36103 is closed
+  # and :workhorse_set_content_type flag is removed
   # If we blindly set the 'real' content type when serving a Git blob we
   # are enabling XSS attacks. An attacker could upload e.g. a Javascript
   # file to a Git repository, trick the browser of a victim into
@@ -161,6 +163,8 @@ module BlobHelper
   end
 
   def content_disposition(blob, inline)
+    # Remove the following line when https://gitlab.com/gitlab-org/gitlab-ce/issues/36103
+    # is closed and :workhorse_set_content_type flag is removed
     return 'attachment' if blob.extension == 'svg'
 
     inline ? 'inline' : 'attachment'

app/helpers/workhorse_helper.rb

@@ -6,8 +6,13 @@ module WorkhorseHelper
   # Send a Git blob through Workhorse
   def send_git_blob(repository, blob, inline: true)
     headers.store(*Gitlab::Workhorse.send_git_blob(repository, blob))
+
     headers['Content-Disposition'] = content_disposition(blob, inline)
     headers['Content-Type'] = safe_content_type(blob)
+
+    # If enabled, this will override the values set above
+    workhorse_set_content_type!
+
     render plain: ""
   end
 
@@ -40,4 +45,8 @@ module WorkhorseHelper
   def set_workhorse_internal_api_content_type
     headers['Content-Type'] = Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
   end
+
+  def workhorse_set_content_type!
+    headers[Gitlab::Workhorse::DETECT_HEADER] = "true" if Feature.enabled?(:workhorse_set_content_type)
+  end
 end

changelogs/unreleased/fj-clean-content-headers.yml

+---
+title: Added feature flag to signal content headers detection by Workhorse
+merge_request: 22667
+author:
+type: added

lib/gitlab/workhorse.rb

@@ -13,6 +13,7 @@ module Gitlab
     INTERNAL_API_REQUEST_HEADER = 'Gitlab-Workhorse-Api-Request'.freeze
     NOTIFICATION_CHANNEL = 'workhorse:notifications'.freeze
     ALLOWED_GIT_HTTP_ACTIONS = %w[git_receive_pack git_upload_pack info_refs].freeze
+    DETECT_HEADER = 'Gitlab-Workhorse-Detect-Content-Type'.freeze
 
     # Supposedly the effective key size for HMAC-SHA256 is 256 bits, i.e. 32
     # bytes https://tools.ietf.org/html/rfc4868#section-2.6

spec/controllers/projects/avatars_controller_spec.rb

@@ -26,12 +26,37 @@ describe Projects::AvatarsController do
       context 'when the avatar is stored in the repository' do
         let(:filepath) { 'files/images/logo-white.png' }
 
-        it 'sends the avatar' do
-          subject
+        context 'when feature flag workhorse_set_content_type is' do
+          before do
+            stub_feature_flags(workhorse_set_content_type: flag_value)
+          end
 
-          expect(response).to have_gitlab_http_status(200)
-          expect(response.header['Content-Type']).to eq('image/png')
-          expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+          context 'enabled' do
+            let(:flag_value) { true }
+
+            it 'sends the avatar' do
+              subject
+
+              expect(response).to have_gitlab_http_status(200)
+              expect(response.header['Content-Disposition']).to eq('inline')
+              expect(response.header['Content-Type']).to eq 'image/png'
+              expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+              expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+            end
+          end
+
+          context 'disabled' do
+            let(:flag_value) { false }
+
+            it 'sends the avatar' do
+              subject
+
+              expect(response).to have_gitlab_http_status(200)
+              expect(response.header['Content-Type']).to eq('image/png')
+              expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+              expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
+            end
+          end
         end
       end
 

spec/controllers/projects/jobs_controller_spec.rb

@@ -838,23 +838,48 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
     context "when job has a trace artifact" do
       let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
 
-      it 'returns a trace' do
-        response = subject
+      context 'when feature flag workhorse_set_content_type is' do
+        before do
+          stub_feature_flags(workhorse_set_content_type: flag_value)
+        end
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
-        expect(response.body).to eq(job.job_artifacts_trace.open.read)
+        context 'enabled' do
+          let(:flag_value) { true }
+
+          it "sets #{Gitlab::Workhorse::DETECT_HEADER} header" do
+            response = subject
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
+            expect(response.body).to eq(job.job_artifacts_trace.open.read)
+            expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+          end
+        end
+
+        context 'disabled' do
+          let(:flag_value) { false }
+
+          it 'returns a trace' do
+            response = subject
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
+            expect(response.body).to eq(job.job_artifacts_trace.open.read)
+            expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to be nil
+          end
+        end
       end
     end
 
     context "when job has a trace file" do
       let(:job) { create(:ci_build, :trace_live, pipeline: pipeline) }
 
-      it "send a trace file" do
+      it 'sends a trace file' do
         response = subject
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
+        expect(response.headers["Content-Disposition"]).to match(/^inline/)
         expect(response.body).to eq("BUILD TRACE")
       end
     end
@@ -866,12 +891,27 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         job.update_column(:trace, "Sample trace")
       end
 
-      it "send a trace file" do
+      it 'sends a trace file' do
         response = subject
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
-        expect(response.body).to eq("Sample trace")
+        expect(response.headers['Content-Type']).to eq('text/plain; charset=utf-8')
+        expect(response.headers['Content-Disposition']).to match(/^inline/)
+        expect(response.body).to eq('Sample trace')
+      end
+
+      context 'when trace format is not text/plain' do
+        before do
+          job.update_column(:trace, '<html></html>')
+        end
+
+        it 'sets content disposition to attachment' do
+          response = subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response.headers['Content-Type']).to eq('text/plain; charset=utf-8')
+          expect(response.headers['Content-Disposition']).to match(/^attachment/)
+        end
       end
     end
 

spec/controllers/projects/raw_controller_spec.rb

@@ -14,26 +14,74 @@ describe Projects::RawController do
     context 'regular filename' do
       let(:filepath) { 'master/README.md' }
 
-      it 'delivers ASCII file' do
-        subject
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
-        expect(response.header['Content-Disposition'])
-            .to eq('inline')
-        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+      context 'when feature flag workhorse_set_content_type is' do
+        before do
+          stub_feature_flags(workhorse_set_content_type: flag_value)
+
+          subject
+        end
+
+        context 'enabled' do
+          let(:flag_value) { true }
+
+          it 'delivers ASCII file' do
+            expect(response).to have_gitlab_http_status(200)
+            expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
+            expect(response.header['Content-Disposition']).to eq('inline')
+            expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+            expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+          end
+        end
+
+        context 'disabled' do
+          let(:flag_value) { false }
+
+          it 'delivers ASCII file' do
+            expect(response).to have_gitlab_http_status(200)
+            expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
+            expect(response.header['Content-Disposition']).to eq('inline')
+            expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
+            expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+          end
+        end
       end
     end
 
     context 'image header' do
       let(:filepath) { 'master/files/images/6049019_460s.jpg' }
 
-      it 'sets image content type header' do
-        subject
+      context 'when feature flag workhorse_set_content_type is' do
+        before do
+          stub_feature_flags(workhorse_set_content_type: flag_value)
+        end
+
+        context 'enabled' do
+          let(:flag_value) { true }
+
+          it 'leaves image content disposition' do
+            subject
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(response.header['Content-Type']).to eq('image/jpeg')
+            expect(response.header['Content-Disposition']).to eq('inline')
+            expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+            expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+          end
+        end
+
+        context 'disabled' do
+          let(:flag_value) { false }
+
+          it 'sets image content type header' do
+            subject
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.header['Content-Type']).to eq('image/jpeg')
-        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+            expect(response).to have_gitlab_http_status(200)
+            expect(response.header['Content-Type']).to eq('image/jpeg')
+            expect(response.header['Content-Disposition']).to eq('inline')
+            expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
+            expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+          end
+        end
       end
     end
 

spec/controllers/projects/wikis_controller_spec.rb

@@ -52,24 +52,56 @@ describe Projects::WikisController do
 
       let(:path) { upload_file_to_wiki(project, user, file_name) }
 
-      before do
-        subject
-      end
-
       subject { get :show, namespace_id: project.namespace, project_id: project, id: path }
 
       context 'when file is an image' do
         let(:file_name) { 'dk.png' }
 
-        it 'renders the content inline' do
-          expect(response.headers['Content-Disposition']).to match(/^inline/)
-        end
+        context 'when feature flag workhorse_set_content_type is' do
+          before do
+            stub_feature_flags(workhorse_set_content_type: flag_value)
+
+            subject
+          end
 
-        context 'when file is a svg' do
-          let(:file_name) { 'unsanitized.svg' }
+          context 'enabled' do
+            let(:flag_value) { true }
 
-          it 'renders the content as an attachment' do
-            expect(response.headers['Content-Disposition']).to match(/^attachment/)
+            it 'delivers the image' do
+              expect(response.headers['Content-Type']).to eq('image/png')
+              expect(response.headers['Content-Disposition']).to match(/^inline/)
+              expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+            end
+
+            context 'when file is a svg' do
+              let(:file_name) { 'unsanitized.svg' }
+
+              it 'delivers the image' do
+                expect(response.headers['Content-Type']).to eq('image/svg+xml')
+                expect(response.headers['Content-Disposition']).to match(/^attachment/)
+                expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+              end
+            end
+          end
+
+          context 'disabled' do
+            let(:flag_value) { false }
+
+            it 'renders the content inline' do
+              expect(response.headers['Content-Type']).to eq('image/png')
+              expect(response.headers['Content-Disposition']).to match(/^inline/)
+              expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
+            end
+
+            context 'when file is a svg' do
+              let(:file_name) { 'unsanitized.svg' }
+
+              it 'renders the content as an attachment' do
+                expect(response.headers['Content-Type']).to eq('image/svg+xml')
+                expect(response.headers['Content-Disposition']).to match(/^attachment/)
+                expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
+              end
+            end
           end
         end
       end
@@ -77,8 +109,32 @@ describe Projects::WikisController do
       context 'when file is a pdf' do
         let(:file_name) { 'git-cheat-sheet.pdf' }
 
-        it 'sets the content type to application/octet-stream' do
-          expect(response.headers['Content-Type']).to eq 'application/octet-stream'
+        context 'when feature flag workhorse_set_content_type is' do
+          before do
+            stub_feature_flags(workhorse_set_content_type: flag_value)
+
+            subject
+          end
+
+          context 'enabled' do
+            let(:flag_value) { true }
+
+            it 'sets the content type to sets the content response headers' do
+              expect(response.headers['Content-Type']).to eq 'application/octet-stream'
+              expect(response.headers['Content-Disposition']).to match(/^inline/)
+              expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+            end
+          end
+
+          context 'disabled' do
+            let(:flag_value) { false }
+
+            it 'sets the content response headers' do
+              expect(response.headers['Content-Type']).to eq 'application/octet-stream'
+              expect(response.headers['Content-Disposition']).to match(/^inline/)
+              expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
+            end
+          end
         end
       end
     end

spec/controllers/snippets_controller_spec.rb

@@ -437,7 +437,10 @@ describe SnippetsController do
         end
 
         context 'when signed in user is the author' do
+          let(:flag_value) { false }
+
           before do
+            stub_feature_flags(workhorse_set_content_type: flag_value)
             get :raw, id: personal_snippet.to_param
           end
 
@@ -451,6 +454,24 @@ describe SnippetsController do
 
             expect(response.header['Content-Disposition']).to match(/inline/)
           end
+
+          context 'when feature flag workhorse_set_content_type is' do
+            context 'enabled' do
+              let(:flag_value) { true }
+
+              it "sets #{Gitlab::Workhorse::DETECT_HEADER} header" do
+                expect(response).to have_gitlab_http_status(200)
+                expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+              end
+            end
+
+            context 'disabled' do
+              it "does not set #{Gitlab::Workhorse::DETECT_HEADER} header" do
+                expect(response).to have_gitlab_http_status(200)
+                expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to be nil
+              end
+            end
+          end
         end
       end