Automatic merge of gitlab-org/gitlab-ce master

app/helpers/appearances_helper.rb

@@ -11,7 +11,7 @@ module AppearancesHelper
   end
 
   def brand_image
-    image_tag(current_appearance.logo) if current_appearance&.logo?
+    image_tag(current_appearance.logo_path) if current_appearance&.logo?
   end
 
   def brand_text
@@ -28,7 +28,7 @@ module AppearancesHelper
 
   def brand_header_logo
     if current_appearance&.header_logo?
-      image_tag current_appearance.header_logo
+      image_tag current_appearance.header_logo_path
     else
       render 'shared/logo.svg'
     end

app/helpers/blob_helper.rb

@@ -140,36 +140,6 @@ 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
-  # downloading the blob, and then the 'application/javascript' content
-  # type would tell the browser to execute the attacker's Javascript. By
-  # overriding the content type and setting it to 'text/plain' (in the
-  # example of Javascript) we tell the browser of the victim not to
-  # execute untrusted data.
-  def safe_content_type(blob)
-    if blob.extension == 'svg'
-      blob.mime_type
-    elsif blob.text?
-      'text/plain; charset=utf-8'
-    elsif blob.image?
-      blob.content_type
-    else
-      'application/octet-stream'
-    end
-  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'
-  end
-
   def ref_project
     @ref_project ||= @target_project || @project
   end

app/helpers/emails_helper.rb

@@ -58,7 +58,7 @@ module EmailsHelper
   def header_logo
     if current_appearance&.header_logo?
       image_tag(
-        current_appearance.header_logo,
+        current_appearance.header_logo_path,
         style: 'height: 50px'
       )
     else

app/helpers/workhorse_helper.rb

@@ -7,8 +7,7 @@ module WorkhorseHelper
   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)
+    headers['Content-Disposition'] = inline ? 'inline' : 'attachment'
 
     # If enabled, this will override the values set above
     workhorse_set_content_type!
@@ -47,6 +46,6 @@ module WorkhorseHelper
   end
 
   def workhorse_set_content_type!
-    headers[Gitlab::Workhorse::DETECT_HEADER] = "true" if Feature.enabled?(:workhorse_set_content_type)
+    headers[Gitlab::Workhorse::DETECT_HEADER] = "true"
   end
 end

app/models/appearance.rb

@@ -28,6 +28,34 @@ class Appearance < ActiveRecord::Base
       errors.add(:single_appearance_row, 'Only 1 appearances row can exist')
     end
   end
+
+  def logo_path
+    logo_system_path(logo, 'logo')
+  end
+
+  def header_logo_path
+    logo_system_path(header_logo, 'header_logo')
+  end
+
+  def favicon_path
+    logo_system_path(favicon, 'favicon')
+  end
+
+  private
+
+  def logo_system_path(logo, mount_type)
+    return unless logo&.upload
+
+    # If we're using a CDN, we need to use the full URL
+    asset_host = ActionController::Base.asset_host
+    local_path = Gitlab::Routing.url_helpers.appearance_upload_path(
+      filename: logo.filename,
+      id: logo.upload.model_id,
+      model: 'appearance',
+      mounted_as: mount_type)
+
+    Gitlab::Utils.append_path(asset_host, local_path)
+  end
 end
 
 Appearance.prepend(EE::Appearance)

app/views/admin/appearances/_form.html.haml

@@ -8,7 +8,7 @@
     = f.label :header_logo, 'Header logo', class: 'col-sm-2 col-form-label pt-0'
     .col-sm-10
       - if @appearance.header_logo?
-        = image_tag @appearance.header_logo_url, class: 'appearance-light-logo-preview'
+        = image_tag @appearance.header_logo_path, class: 'appearance-light-logo-preview'
         - if @appearance.persisted?
           %br
           = link_to 'Remove header logo', header_logos_admin_appearances_path, data: { confirm: "Header logo will be removed. Are you sure?"}, method: :delete, class: "btn btn-inverted btn-remove btn-sm remove-logo"
@@ -25,7 +25,7 @@
     = f.label :favicon, 'Favicon', class: 'col-sm-2 col-form-label pt-0'
     .col-sm-10
       - if @appearance.favicon?
-        = image_tag @appearance.favicon_url, class: 'appearance-light-logo-preview'
+        = image_tag @appearance.favicon_path, class: 'appearance-light-logo-preview'
         - if @appearance.persisted?
           %br
           = link_to 'Remove favicon', favicon_admin_appearances_path, data: { confirm: "Favicon will be removed. Are you sure?"}, method: :delete, class: "btn btn-inverted btn-remove btn-sm remove-logo"
@@ -56,7 +56,7 @@
     = f.label :logo, class: 'col-sm-2 col-form-label pt-0'
     .col-sm-10
       - if @appearance.logo?
-        = image_tag @appearance.logo_url, class: 'appearance-logo-preview'
+        = image_tag @appearance.logo_path, class: 'appearance-logo-preview'
         - if @appearance.persisted?
           %br
           = link_to 'Remove logo', logo_admin_appearances_path, data: { confirm: "Logo will be removed. Are you sure?"}, method: :delete, class: "btn btn-inverted btn-remove btn-sm remove-logo"

config/routes/uploads.rb

@@ -17,7 +17,8 @@ scope path: :uploads do
   # Appearance
   get "-/system/:model/:mounted_as/:id/:filename",
       to:           "uploads#show",
-      constraints:  { model: /appearance/, mounted_as: /logo|header_logo|favicon/, filename: /.+/ }
+      constraints:  { model: /appearance/, mounted_as: /logo|header_logo|favicon/, filename: /.+/ },
+      as: 'appearance_upload'
 
   # Project markdown uploads
   get ":namespace_id/:project_id/:secret/:filename",

spec/controllers/projects/avatars_controller_spec.rb

@@ -26,37 +26,13 @@ describe Projects::AvatarsController do
       context 'when the avatar is stored in the repository' do
         let(:filepath) { 'files/images/logo-white.png' }
 
-        context 'when feature flag workhorse_set_content_type is' do
-          before do
-            stub_feature_flags(workhorse_set_content_type: flag_value)
-          end
+        it 'sends the avatar' do
+          subject
 
-          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
+          expect(response).to have_gitlab_http_status(200)
+          expect(response.header['Content-Disposition']).to eq('inline')
+          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
 

spec/controllers/projects/jobs_controller_spec.rb

@@ -892,36 +892,13 @@ 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) }
 
-      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 "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
+      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 be nil
-          end
-        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)
+        expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
       end
     end
 

spec/controllers/projects/raw_controller_spec.rb

@@ -16,74 +16,27 @@ describe Projects::RawController do
     context 'regular filename' do
       let(:filepath) { 'master/README.md' }
 
-      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
+      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::DETECT_HEADER]).to eq "true"
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
       end
     end
 
     context 'image header' do
       let(:filepath) { 'master/files/images/6049019_460s.jpg' }
 
-      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
+      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 nil
-            expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
-          end
-        end
+        expect(response).to have_gitlab_http_status(200)
+        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
 

spec/controllers/projects/wikis_controller_spec.rb

@@ -52,56 +52,24 @@ describe Projects::WikisController do
 
       let(:path) { upload_file_to_wiki(project, user, file_name) }
 
-      subject { get :show, params: { namespace_id: project.namespace, project_id: project, id: path } }
+      before do
+        get :show, params: { namespace_id: project.namespace, project_id: project, id: path }
+      end
 
       context 'when file is an image' do
         let(:file_name) { 'dk.png' }
 
-        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 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
+        it 'delivers the image' do
+          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' }
+        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
+          it 'delivers the image' do
+            expect(response.headers['Content-Disposition']).to match(/^inline/)
+            expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
           end
         end
       end
@@ -109,32 +77,9 @@ describe Projects::WikisController do
       context 'when file is a pdf' do
         let(:file_name) { 'git-cheat-sheet.pdf' }
 
-        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
+        it 'sets the content type to sets the content response headers' do
+          expect(response.headers['Content-Disposition']).to match(/^inline/)
+          expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
         end
       end
     end

spec/controllers/snippets_controller_spec.rb

@@ -437,10 +437,7 @@ 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, params: { id: personal_snippet.to_param }
           end
 
@@ -455,22 +452,9 @@ 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
+          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
       end

spec/factories/appearances.rb

@@ -15,6 +15,10 @@ FactoryBot.define do
     header_logo { fixture_file_upload('spec/fixtures/dk.png') }
   end
 
+  trait :with_favicon do
+    favicon { fixture_file_upload('spec/fixtures/dk.png') }
+  end
+
   trait :with_logos do
     with_logo
     with_header_logo

spec/models/appearance_spec.rb

@@ -26,4 +26,34 @@ describe Appearance do
       let(:uploader_class) { AttachmentUploader }
     end
   end
+
+  shared_examples 'logo paths' do |logo_type|
+    let(:appearance) { create(:appearance, "with_#{logo_type}".to_sym) }
+    let(:filename) { 'dk.png' }
+    let(:expected_path) { "/uploads/-/system/appearance/#{logo_type}/#{appearance.id}/#{filename}" }
+
+    it 'returns nil when there is no upload' do
+      expect(subject.send("#{logo_type}_path")).to be_nil
+    end
+
+    it 'returns a local path using the system route' do
+      expect(appearance.send("#{logo_type}_path")).to eq(expected_path)
+    end
+
+    describe 'with asset host configured' do
+      let(:asset_host) { 'https://gitlab-assets.example.com' }
+
+      before do
+        allow(ActionController::Base).to receive(:asset_host) { asset_host }
+      end
+
+      it 'returns a full URL with the system path' do
+        expect(appearance.send("#{logo_type}_path")).to eq("#{asset_host}#{expected_path}")
+      end
+    end
+  end
+
+  %i(logo header_logo favicon).each do |logo_type|
+    it_behaves_like 'logo paths', logo_type
+  end
 end