Commit 262b9741 authored by Stan Hu's avatar Stan Hu

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
parent 9dd34eac
...@@ -107,7 +107,9 @@ gem 'kaminari', '~> 1.0' ...@@ -107,7 +107,9 @@ gem 'kaminari', '~> 1.0'
gem 'hamlit', '~> 2.8.8' gem 'hamlit', '~> 2.8.8'
# Files attachments # 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' gem 'mini_magick'
# Drag and Drop UI # Drag and Drop UI
......
...@@ -996,7 +996,7 @@ DEPENDENCIES ...@@ -996,7 +996,7 @@ DEPENDENCIES
bundler-audit (~> 0.5.0) bundler-audit (~> 0.5.0)
capybara (~> 2.15) capybara (~> 2.15)
capybara-screenshot (~> 1.0.0) capybara-screenshot (~> 1.0.0)
carrierwave (~> 1.2) carrierwave (= 1.2.3)
charlock_holmes (~> 0.7.5) charlock_holmes (~> 0.7.5)
chronic (~> 0.10.2) chronic (~> 0.10.2)
chronic_duration (~> 0.10.6) chronic_duration (~> 0.10.6)
......
...@@ -1005,7 +1005,7 @@ DEPENDENCIES ...@@ -1005,7 +1005,7 @@ DEPENDENCIES
bundler-audit (~> 0.5.0) bundler-audit (~> 0.5.0)
capybara (~> 2.15) capybara (~> 2.15)
capybara-screenshot (~> 1.0.0) capybara-screenshot (~> 1.0.0)
carrierwave (~> 1.2) carrierwave (= 1.2.3)
charlock_holmes (~> 0.7.5) charlock_holmes (~> 0.7.5)
chronic (~> 0.10.2) chronic (~> 0.10.2)
chronic_duration (~> 0.10.6) chronic_duration (~> 0.10.6)
......
module SendFileUpload module SendFileUpload
def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, disposition: 'attachment') def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, disposition: 'attachment')
if 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 # By default, Rails will send uploads with an extension of .js with a
# content-type of text/javascript, which will trigger Rails' # content-type of text/javascript, which will trigger Rails'
# cross-origin JavaScript protection. # cross-origin JavaScript protection.
...@@ -18,4 +22,14 @@ module SendFileUpload ...@@ -18,4 +22,14 @@ module SendFileUpload
redirect_to file_upload.url(**redirect_params) redirect_to file_upload.url(**redirect_params)
end end
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 end
---
title: Fix attachments not displaying inline with Google Cloud Storage
merge_request: 21265
author:
type: fixed
# 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
...@@ -9,7 +9,7 @@ module Fog ...@@ -9,7 +9,7 @@ module Fog
module MonkeyPatch module MonkeyPatch
def url(expires, options = {}) def url(expires, options = {})
requires :key requires :key
collection.get_https_url(key, expires) collection.get_https_url(key, expires, options)
end end
end end
......
...@@ -158,7 +158,7 @@ module ObjectStorage ...@@ -158,7 +158,7 @@ module ObjectStorage
end end
def upload_options def upload_options
{ 'Content-Type' => 'application/octet-stream' } {}
end end
def connection def connection
......
...@@ -52,7 +52,7 @@ describe SendFileUpload do ...@@ -52,7 +52,7 @@ describe SendFileUpload do
end end
context 'with attachment' do 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 it 'sends a file with content-type of text/plain' do
expected_params = { expected_params = {
...@@ -62,7 +62,29 @@ describe SendFileUpload do ...@@ -62,7 +62,29 @@ describe SendFileUpload do
} }
expect(controller).to receive(:send_file).with(uploader.path, expected_params) 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
end end
...@@ -80,7 +102,12 @@ describe SendFileUpload do ...@@ -80,7 +102,12 @@ describe SendFileUpload do
it 'sends a file' do it 'sends a file' do
headers = double 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(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(:headers) { headers }
expect(controller).to receive(:head).with(:ok) expect(controller).to receive(:head).with(:ok)
......
...@@ -62,7 +62,7 @@ describe ObjectStorage::DirectUpload do ...@@ -62,7 +62,7 @@ describe ObjectStorage::DirectUpload do
expect(subject[:StoreURL]).to start_with(storage_url) expect(subject[:StoreURL]).to start_with(storage_url)
expect(subject[:DeleteURL]).to start_with(storage_url) expect(subject[:DeleteURL]).to start_with(storage_url)
expect(subject[:CustomPutHeaders]).to be_truthy expect(subject[:CustomPutHeaders]).to be_truthy
expect(subject[:PutHeaders]).to eq({ 'Content-Type' => 'application/octet-stream' }) expect(subject[:PutHeaders]).to eq({})
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment