From 262b974123d22b5d6b662b232ca4792d7998a166 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 13 Aug 2018 15:36:15 -0700 Subject: [PATCH] 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 --- GITLAB_WORKHORSE_VERSION | 2 +- Gemfile | 4 ++- Gemfile.lock | 2 +- Gemfile.rails5.lock | 2 +- app/controllers/concerns/send_file_upload.rb | 16 +++++++++- .../unreleased/sh-fix-attachments-inline.yml | 5 +++ config/initializers/carrierwave_patch.rb | 29 +++++++++++++++++ .../fog_google_https_private_urls.rb | 2 +- lib/object_storage/direct_upload.rb | 2 +- .../concerns/send_file_upload_spec.rb | 31 +++++++++++++++++-- spec/lib/object_storage/direct_upload_spec.rb | 2 +- 11 files changed, 87 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-attachments-inline.yml create mode 100644 config/initializers/carrierwave_patch.rb 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