Fix attachments not displaying inline with Google Cloud Storage

There were several issues:

1. With Google Cloud Storage, we can't override the Content-Type with
Response-Content-Type once it is set.  Setting the value to
`application/octet-stream` doesn't buy us anything. GCS defaults to
`application/octet-stream`, and AWS uses `binary/octet-stream`. Just remove
this `Content-Type` when we upload new files.

2. CarrierWave and fog-google need to support query parameters:
https://github.com/fog/fog-google/pull/409/files, https://github.com/carrierwaveuploader/carrierwave/pull/2332/files.
CarrierWave has been monkey-patched until an official release.

3. Workhorse also needs to remove the Content-Type header in the request
(https://gitlab.com/gitlab-org/gitlab-workhorse/blob/ef80978ff89e628c8eeb66556720e30587d3deb6/internal/objectstore/object.go#L66),
or we'll get a 403 error when uploading due to signed URLs not matching the headers.
Upgrading to Workhorse 6.1.0 for https://gitlab.com/gitlab-org/gitlab-workhorse/merge_requests/297
will make Workhorse use the headers that are used by Rails.

Closes #49957

11 files changed, 87 insertions, 10 deletions

diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION
index 09b254e90c6..dfda3e0b4f0 100644
--- a/GITLAB_WORKHORSE_VERSION
+++ b/GITLAB_WORKHORSE_VERSION
@@ -1 +1 @@
-6.0.0
+6.1.0
diff --git a/Gemfile b/Gemfile
index 7b83c6d1178..746a152a59b 100644
--- a/Gemfile
+++ b/Gemfile
@@ -107,7 +107,9 @@ gem 'kaminari', '~> 1.0'
 gem 'hamlit', '~> 2.8.8'
 
 # Files attachments
-gem 'carrierwave', '~> 1.2'
+# Locked until https://github.com/carrierwaveuploader/carrierwave/pull/2332/files is merged.
+# config/initializers/carrierwave_patch.rb can be removed once that change is released.
+gem 'carrierwave', '= 1.2.3'
 gem 'mini_magick'
 
 # Drag and Drop UI
diff --git a/Gemfile.lock b/Gemfile.lock
index 91cd360e708..cf2634a26d3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -996,7 +996,7 @@ DEPENDENCIES
   bundler-audit (~> 0.5.0)
   capybara (~> 2.15)
   capybara-screenshot (~> 1.0.0)
-  carrierwave (~> 1.2)
+  carrierwave (= 1.2.3)
   charlock_holmes (~> 0.7.5)
   chronic (~> 0.10.2)
   chronic_duration (~> 0.10.6)
diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock
index ba9b06a08cb..9daf710ece6 100644
--- a/Gemfile.rails5.lock
+++ b/Gemfile.rails5.lock
@@ -1005,7 +1005,7 @@ DEPENDENCIES
   bundler-audit (~> 0.5.0)
   capybara (~> 2.15)
   capybara-screenshot (~> 1.0.0)
-  carrierwave (~> 1.2)
+  carrierwave (= 1.2.3)
   charlock_holmes (~> 0.7.5)
   chronic (~> 0.10.2)
   chronic_duration (~> 0.10.6)
diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb
index 237c93daee8..382ec91f771 100644
--- a/app/controllers/concerns/send_file_upload.rb
+++ b/app/controllers/concerns/send_file_upload.rb
@@ -1,7 +1,11 @@
 module SendFileUpload
   def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, disposition: 'attachment')
     if attachment
-      redirect_params[:query] = { "response-content-disposition" => "#{disposition};filename=#{attachment.inspect}" }
+      # Response-Content-Type will not override an existing Content-Type in
+      # Google Cloud Storage, so the metadata needs to be cleared on GCS for
+      # this to work. However, this override works with AWS.
+      redirect_params[:query] = { "response-content-disposition" => "#{disposition};filename=#{attachment.inspect}",
+                                  "response-content-type" => guess_content_type(attachment) }
       # By default, Rails will send uploads with an extension of .js with a
       # content-type of text/javascript, which will trigger Rails'
       # cross-origin JavaScript protection.
@@ -18,4 +22,14 @@ module SendFileUpload
       redirect_to file_upload.url(**redirect_params)
     end
   end
+
+  def guess_content_type(filename)
+    types = MIME::Types.type_for(filename)
+
+    if types.present?
+      types.first.content_type
+    else
+      "application/octet-stream"
+    end
+  end
 end
diff --git a/changelogs/unreleased/sh-fix-attachments-inline.yml b/changelogs/unreleased/sh-fix-attachments-inline.yml
new file mode 100644
index 00000000000..2926edca97a
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-attachments-inline.yml
@@ -0,0 +1,5 @@
+---
+title: Fix attachments not displaying inline with Google Cloud Storage
+merge_request: 21265
+author:
+type: fixed
diff --git a/config/initializers/carrierwave_patch.rb b/config/initializers/carrierwave_patch.rb
new file mode 100644
index 00000000000..35ffff03abe
--- /dev/null
+++ b/config/initializers/carrierwave_patch.rb
@@ -0,0 +1,29 @@
+# This monkey patches CarrierWave 1.2.3 to make Google Cloud Storage work with
+# extra query parameters:
+# https://github.com/carrierwaveuploader/carrierwave/pull/2332/files
+module CarrierWave
+  module Storage
+    class Fog < Abstract
+      class File
+        def authenticated_url(options = {})
+          if %w(AWS Google Rackspace OpenStack).include?(@uploader.fog_credentials[:provider])
+            # avoid a get by using local references
+            local_directory = connection.directories.new(key: @uploader.fog_directory)
+            local_file = local_directory.files.new(key: path)
+            expire_at = ::Fog::Time.now + @uploader.fog_authenticated_url_expiration
+            case @uploader.fog_credentials[:provider]
+            when 'AWS', 'Google'
+              local_file.url(expire_at, options)
+            when 'Rackspace'
+              connection.get_object_https_url(@uploader.fog_directory, path, expire_at, options)
+            when 'OpenStack'
+              connection.get_object_https_url(@uploader.fog_directory, path, expire_at)
+            else
+              local_file.url(expire_at)
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/config/initializers/fog_google_https_private_urls.rb b/config/initializers/fog_google_https_private_urls.rb
index c65a534b536..682b1050c68 100644
--- a/config/initializers/fog_google_https_private_urls.rb
+++ b/config/initializers/fog_google_https_private_urls.rb
@@ -9,7 +9,7 @@ module Fog
         module MonkeyPatch
           def url(expires, options = {})
             requires :key
-            collection.get_https_url(key, expires)
+            collection.get_https_url(key, expires, options)
           end
         end
 
diff --git a/lib/object_storage/direct_upload.rb b/lib/object_storage/direct_upload.rb
index b372b4af090..ab43910c8bd 100644
--- a/lib/object_storage/direct_upload.rb
+++ b/lib/object_storage/direct_upload.rb
@@ -158,7 +158,7 @@ module ObjectStorage
     end
 
     def upload_options
-      { 'Content-Type' => 'application/octet-stream' }
+      {}
     end
 
     def connection
diff --git a/spec/controllers/concerns/send_file_upload_spec.rb b/spec/controllers/concerns/send_file_upload_spec.rb
index 58bb91a0c80..767fba7fd58 100644
--- a/spec/controllers/concerns/send_file_upload_spec.rb
+++ b/spec/controllers/concerns/send_file_upload_spec.rb
@@ -52,7 +52,7 @@ describe SendFileUpload do
     end
 
     context 'with attachment' do
-      subject { controller.send_upload(uploader, attachment: 'test.js') }
+      let(:send_attachment) { controller.send_upload(uploader, attachment: 'test.js') }
 
       it 'sends a file with content-type of text/plain' do
         expected_params = {
@@ -62,7 +62,29 @@ describe SendFileUpload do
         }
         expect(controller).to receive(:send_file).with(uploader.path, expected_params)
 
-        subject
+        send_attachment
+      end
+
+      context 'with a proxied file in object storage' do
+        before do
+          stub_uploads_object_storage(uploader: uploader_class)
+          uploader.object_store = ObjectStorage::Store::REMOTE
+          uploader.store!(temp_file)
+          allow(Gitlab.config.uploads.object_store).to receive(:proxy_download) { true }
+        end
+
+        it 'sends a file with a custom type' do
+          headers = double
+          expected_headers = %r(response-content-disposition=attachment%3Bfilename%3D%22test.js%22&response-content-type=application/javascript)
+          expect(Gitlab::Workhorse).to receive(:send_url).with(expected_headers).and_call_original
+          expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-url:/)
+
+          expect(controller).not_to receive(:send_file)
+          expect(controller).to receive(:headers) { headers }
+          expect(controller).to receive(:head).with(:ok)
+
+          send_attachment
+        end
       end
     end
 
@@ -80,7 +102,12 @@ describe SendFileUpload do
 
         it 'sends a file' do
           headers = double
+          expect(Gitlab::Workhorse).not_to receive(:send_url).with(/response-content-disposition/)
+          expect(Gitlab::Workhorse).not_to receive(:send_url).with(/response-content-type/)
+          expect(Gitlab::Workhorse).to receive(:send_url).and_call_original
+
           expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-url:/)
+          expect(controller).not_to receive(:send_file)
           expect(controller).to receive(:headers) { headers }
           expect(controller).to receive(:head).with(:ok)
 
diff --git a/spec/lib/object_storage/direct_upload_spec.rb b/spec/lib/object_storage/direct_upload_spec.rb
index 632acd6eb46..9c308cc1be9 100644
--- a/spec/lib/object_storage/direct_upload_spec.rb
+++ b/spec/lib/object_storage/direct_upload_spec.rb
@@ -62,7 +62,7 @@ describe ObjectStorage::DirectUpload do
         expect(subject[:StoreURL]).to start_with(storage_url)
         expect(subject[:DeleteURL]).to start_with(storage_url)
         expect(subject[:CustomPutHeaders]).to be_truthy
-        expect(subject[:PutHeaders]).to eq({ 'Content-Type' => 'application/octet-stream' })
+        expect(subject[:PutHeaders]).to eq({})
       end
     end