diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 524cb55242b53f6a64cc646ea05db6acc7696d2d..26aaba0e86632e4d537006e45b0ec918d780b3b4 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -1.1.1 +1.2.0 diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index b6c52ddac7a6a5a16a9e7d327e1ffbcae8a1d35b..86f317dcd18f51d691c78e01e48e0614234c91fe 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -1,4 +1,4 @@ -class ArtifactUploader < CarrierWave::Uploader::Base +class ArtifactUploader < GitlabUploader storage :file attr_accessor :build, :field @@ -38,12 +38,4 @@ class ArtifactUploader < CarrierWave::Uploader::Base def exists? file.try(:exists?) end - - def move_to_cache - true - end - - def move_to_store - true - end end diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index fb3b5dfecd06a1d69613ea95a536d5c775dad9a7..cfcb877cc3e5b83df013184465d8e3959f0ca5f3 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -1,4 +1,4 @@ -class AttachmentUploader < CarrierWave::Uploader::Base +class AttachmentUploader < GitlabUploader include UploaderHelper storage :file diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb index 38683fdf6d7cb47a274e48a205770df7dd481dc1..a1ecb7bc00b799d2b7ae8ca6119790a0c38f083a 100644 --- a/app/uploaders/avatar_uploader.rb +++ b/app/uploaders/avatar_uploader.rb @@ -1,4 +1,4 @@ -class AvatarUploader < CarrierWave::Uploader::Base +class AvatarUploader < GitlabUploader include UploaderHelper storage :file diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 3ac6030c21c747a2b76b64bf292876bdfce1e820..47bef7cd1e48675b8a8cb7e7114182b569fbefc7 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -1,4 +1,4 @@ -class FileUploader < CarrierWave::Uploader::Base +class FileUploader < GitlabUploader include UploaderHelper MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?[0-9a-f]{32})/(?.*?)\)} diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb new file mode 100644 index 0000000000000000000000000000000000000000..02d7c601d6c08667b58d453738d477c194bf8a9f --- /dev/null +++ b/app/uploaders/gitlab_uploader.rb @@ -0,0 +1,11 @@ +class GitlabUploader < CarrierWave::Uploader::Base + # Reduce disk IO + def move_to_cache + true + end + + # Reduce disk IO + def move_to_store + true + end +end diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb index 4f356dd663ee303920027b11dce1b4707259918a..faab539b8e0bc5496018177cd7a949077722c2f5 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -1,4 +1,4 @@ -class LfsObjectUploader < CarrierWave::Uploader::Base +class LfsObjectUploader < GitlabUploader storage :file def store_dir @@ -9,14 +9,6 @@ class LfsObjectUploader < CarrierWave::Uploader::Base "#{Gitlab.config.lfs.storage_path}/tmp/cache" end - def move_to_cache - true - end - - def move_to_store - true - end - def exists? file.try(:exists?) end diff --git a/changelogs/unreleased/gitlab-workhorse-multipart.yml b/changelogs/unreleased/gitlab-workhorse-multipart.yml new file mode 100644 index 0000000000000000000000000000000000000000..23c2139cf9311f3634d3f2d82386dc0ffb09bd10 --- /dev/null +++ b/changelogs/unreleased/gitlab-workhorse-multipart.yml @@ -0,0 +1,4 @@ +--- +title: Replace Rack::Multipart with GitLab-Workhorse based solution +merge_request: 5867 +author: diff --git a/config/initializers/workhorse_multipart.rb b/config/initializers/workhorse_multipart.rb new file mode 100644 index 0000000000000000000000000000000000000000..3e2f25c354aa142d1ecda70683009fd93ac3f211 --- /dev/null +++ b/config/initializers/workhorse_multipart.rb @@ -0,0 +1,3 @@ +Rails.application.configure do |config| + config.middleware.use(Gitlab::Middleware::Multipart) +end diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index abc8c8c55e6f130952078f59231491318ef6ee4d..8fab54896160c69119f5cd646239df43a977916d 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/lib/gitlab/gfm/uploads_rewriter.rb @@ -1,3 +1,5 @@ +require 'fileutils' + module Gitlab module Gfm ## @@ -22,7 +24,9 @@ module Gitlab return markdown unless file.try(:exists?) new_uploader = FileUploader.new(target_project) - new_uploader.store!(file) + with_link_in_tmp_dir(file.file) do |open_tmp_file| + new_uploader.store!(open_tmp_file) + end new_uploader.to_markdown end end @@ -46,6 +50,19 @@ module Gitlab uploader.retrieve_from_store!(file) uploader.file end + + # Because the uploaders use 'move_to_store' we must have a temporary + # file that is allowed to be (re)moved. + def with_link_in_tmp_dir(file) + dir = Dir.mktmpdir('UploadsRewriter', File.dirname(file)) + # The filename matters to Carrierwave so we make sure to preserve it + tmp_file = File.join(dir, File.basename(file)) + File.link(file, tmp_file) + # Open the file to placate Carrierwave + File.open(tmp_file) { |open_file| yield open_file } + ensure + FileUtils.rm_rf(dir) + end end end end diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb new file mode 100644 index 0000000000000000000000000000000000000000..65713e73a59c050ca28721291951dac0287fdd02 --- /dev/null +++ b/lib/gitlab/middleware/multipart.rb @@ -0,0 +1,99 @@ +# Gitlab::Middleware::Multipart - a Rack::Multipart replacement +# +# Rack::Multipart leaves behind tempfiles in /tmp and uses valuable Ruby +# process time to copy files around. This alternative solution uses +# gitlab-workhorse to clean up the tempfiles and puts the tempfiles in a +# location where copying should not be needed. +# +# When gitlab-workhorse finds files in a multipart MIME body it sends +# a signed message via a request header. This message lists the names of +# the multipart entries that gitlab-workhorse filtered out of the +# multipart structure and saved to tempfiles. Workhorse adds new entries +# in the multipart structure with paths to the tempfiles. +# +# The job of this Rack middleware is to detect and decode the message +# from workhorse. If present, it walks the Rack 'params' hash for the +# current request, opens the respective tempfiles, and inserts the open +# Ruby File objects in the params hash where Rack::Multipart would have +# put them. The goal is that application code deeper down can keep +# working the way it did with Rack::Multipart without changes. +# +# CAVEAT: the code that modifies the params hash is a bit complex. It is +# conceivable that certain Rack params structures will not be modified +# correctly. We are not aware of such bugs at this time though. +# + +module Gitlab + module Middleware + class Multipart + RACK_ENV_KEY = 'HTTP_GITLAB_WORKHORSE_MULTIPART_FIELDS' + + class Handler + def initialize(env, message) + @request = Rack::Request.new(env) + @rewritten_fields = message['rewritten_fields'] + @open_files = [] + end + + def with_open_files + @rewritten_fields.each do |field, tmp_path| + parsed_field = Rack::Utils.parse_nested_query(field) + raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1 + + key, value = parsed_field.first + if value.nil? + value = File.open(tmp_path) + @open_files << value + else + value = decorate_params_value(value, @request.params[key], tmp_path) + end + @request.update_param(key, value) + end + + yield + ensure + @open_files.each(&:close) + end + + # This function calls itself recursively + def decorate_params_value(path_hash, value_hash, tmp_path) + unless path_hash.is_a?(Hash) && path_hash.count == 1 + raise "invalid path: #{path_hash.inspect}" + end + path_key, path_value = path_hash.first + + unless value_hash.is_a?(Hash) && value_hash[path_key] + raise "invalid value hash: #{value_hash.inspect}" + end + + case path_value + when nil + value_hash[path_key] = File.open(tmp_path) + @open_files << value_hash[path_key] + value_hash + when Hash + decorate_params_value(path_value, value_hash[path_key], tmp_path) + value_hash + else + raise "unexpected path value: #{path_value.inspect}" + end + end + end + + def initialize(app) + @app = app + end + + def call(env) + encoded_message = env.delete(RACK_ENV_KEY) + return @app.call(env) if encoded_message.blank? + + message = Gitlab::Workhorse.decode_jwt(encoded_message)[0] + + Handler.new(env, message).with_open_files do + @app.call(env) + end + end + end + end +end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 594439a5d4b3a6f979d362e58ebf39187faf651e..aeb1a26e1bae22bc2f16ad185ab1ac9975aff699 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -117,8 +117,12 @@ module Gitlab end def verify_api_request!(request_headers) + decode_jwt(request_headers[INTERNAL_API_REQUEST_HEADER]) + end + + def decode_jwt(encoded_message) JWT.decode( - request_headers[INTERNAL_API_REQUEST_HEADER], + encoded_message, secret, true, { iss: 'gitlab-workhorse', verify_iss: true, algorithm: 'HS256' }, diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 15863d444f8d5a5f6f6aa2d2758d71b298017e30..92053e5a7c64dcd4a5da7df935ec65c06802a566 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe ApplicationHelper do + include UploadHelpers + describe 'current_controller?' do it 'returns true when controller matches argument' do stub_controller_name('foo') @@ -52,10 +54,8 @@ describe ApplicationHelper do end describe 'project_icon' do - let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') } - it 'returns an url for the avatar' do - project = create(:project, avatar: File.open(avatar_file_path)) + project = create(:project, avatar: File.open(uploaded_image_temp_path)) avatar_url = "http://#{Gitlab.config.gitlab.host}/uploads/project/avatar/#{project.id}/banana_sample.gif" expect(helper.project_icon("#{project.namespace.to_param}/#{project.to_param}").to_s). @@ -74,10 +74,8 @@ describe ApplicationHelper do end describe 'avatar_icon' do - let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') } - it 'returns an url for the avatar' do - user = create(:user, avatar: File.open(avatar_file_path)) + user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user.email).to_s). to match("/uploads/user/avatar/#{user.id}/banana_sample.gif") @@ -88,7 +86,7 @@ describe ApplicationHelper do # Must be stubbed after the stub above, and separately stub_config_setting(url: Settings.send(:build_gitlab_url)) - user = create(:user, avatar: File.open(avatar_file_path)) + user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user.email).to_s). to match("/gitlab/uploads/user/avatar/#{user.id}/banana_sample.gif") @@ -102,7 +100,7 @@ describe ApplicationHelper do describe 'using a User' do it 'returns an URL for the avatar' do - user = create(:user, avatar: File.open(avatar_file_path)) + user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user).to_s). to match("/uploads/user/avatar/#{user.id}/banana_sample.gif") diff --git a/spec/lib/gitlab/import_export/avatar_restorer_spec.rb b/spec/lib/gitlab/import_export/avatar_restorer_spec.rb index 5ae178414cc240f529a55e96e557b4c64956f61d..08a42fd27a255bf1577adc61f75b5695734ad3ff 100644 --- a/spec/lib/gitlab/import_export/avatar_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/avatar_restorer_spec.rb @@ -1,12 +1,14 @@ require 'spec_helper' describe Gitlab::ImportExport::AvatarRestorer, lib: true do + include UploadHelpers + let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: 'test') } let(:project) { create(:empty_project) } before do allow_any_instance_of(described_class).to receive(:avatar_export_file) - .and_return(Rails.root + "spec/fixtures/dk.png") + .and_return(uploaded_image_temp_path) end after do diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c79c6494576d5e31b4299cabafd428a5ef13fbc7 --- /dev/null +++ b/spec/lib/gitlab/middleware/multipart_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +require 'tempfile' + +describe Gitlab::Middleware::Multipart do + let(:app) { double(:app) } + let(:middleware) { described_class.new(app) } + + it 'opens top-level files' do + Tempfile.open do |tempfile| + env = post_env({ 'file' => tempfile.path }, { 'file.name' => 'filename' }, Gitlab::Workhorse.secret, 'gitlab-workhorse') + + expect(app).to receive(:call) do |env| + file = Rack::Request.new(env).params['file'] + expect(file).to be_a(File) + expect(file.path).to eq(tempfile.path) + end + + middleware.call(env) + end + end + + it 'rejects headers signed with the wrong secret' do + env = post_env({ 'file' => '/var/empty/nonesuch' }, {}, 'x' * 32, 'gitlab-workhorse') + + expect { middleware.call(env) }.to raise_error(JWT::VerificationError) + end + + it 'rejects headers signed with the wrong issuer' do + env = post_env({ 'file' => '/var/empty/nonesuch' }, {}, Gitlab::Workhorse.secret, 'acme-inc') + + expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError) + end + + it 'opens files one level deep' do + Tempfile.open do |tempfile| + in_params = { 'user' => { 'avatar' => { '.name' => 'filename' } } } + env = post_env({ 'user[avatar]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') + + expect(app).to receive(:call) do |env| + file = Rack::Request.new(env).params['user']['avatar'] + expect(file).to be_a(File) + expect(file.path).to eq(tempfile.path) + end + + middleware.call(env) + end + end + + it 'opens files two levels deep' do + Tempfile.open do |tempfile| + in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => 'filename' } } } } + env = post_env({ 'project[milestone][themesong]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') + + expect(app).to receive(:call) do |env| + file = Rack::Request.new(env).params['project']['milestone']['themesong'] + expect(file).to be_a(File) + expect(file.path).to eq(tempfile.path) + end + + middleware.call(env) + end + end + + def post_env(rewritten_fields, params, secret, issuer) + token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256') + Rack::MockRequest.env_for( + '/', + method: 'post', + params: params, + described_class::RACK_ENV_KEY => token + ) + end +end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index a75ba824e85412bef0b39726108de4e5e3a62ade..cdeb965b413140d059f4ad3126df97f826dc8d60 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -2,13 +2,13 @@ require 'spec_helper' describe API::Groups, api: true do include ApiHelpers + include UploadHelpers let(:user1) { create(:user, can_create_group: false) } let(:user2) { create(:user) } let(:user3) { create(:user) } let(:admin) { create(:admin) } - let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') } - let!(:group1) { create(:group, avatar: File.open(avatar_file_path)) } + let!(:group1) { create(:group, avatar: File.open(uploaded_image_temp_path)) } let!(:group2) { create(:group, :private) } let!(:project1) { create(:project, namespace: group1) } let!(:project2) { create(:project, namespace: group2) } diff --git a/spec/support/upload_helpers.rb b/spec/support/upload_helpers.rb new file mode 100644 index 0000000000000000000000000000000000000000..5eead80c935d7121d1f3a8dd2df2190679d69624 --- /dev/null +++ b/spec/support/upload_helpers.rb @@ -0,0 +1,16 @@ +require 'fileutils' + +module UploadHelpers + extend self + + def uploaded_image_temp_path + basename = 'banana_sample.gif' + orig_path = File.join(Rails.root, 'spec', 'fixtures', basename) + tmp_path = File.join(Rails.root, 'tmp', 'tests', basename) + # Because we use 'move_to_store' on all uploaders, we create a new + # tempfile on each call: the file we return here will be renamed in most + # cases. + FileUtils.copy(orig_path, tmp_path) + tmp_path + end +end diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..6098be5cd45508848298b28c4016fd0ccb74b8aa --- /dev/null +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe AttachmentUploader do + let(:issue) { build(:issue) } + subject { described_class.new(issue) } + + describe '#move_to_cache' do + it 'is true' do + expect(subject.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(subject.move_to_store).to eq(true) + end + end +end diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1f0e8732587460ba58c229ab131dcd3f8b05d143 --- /dev/null +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe AvatarUploader do + let(:user) { build(:user) } + subject { described_class.new(user) } + + describe '#move_to_cache' do + it 'is true' do + expect(subject.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(subject.move_to_store).to eq(true) + end + end +end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index e8300abed5dc201ecf5cc4e01e2afe0d3a661a9d..bc86adfef6828c6927f1a7e8c8bbb664c5cbab14 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -42,4 +42,16 @@ describe FileUploader do expect(@uploader.image_or_video?).to be false end end + + describe '#move_to_cache' do + it 'is true' do + expect(@uploader.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(@uploader.move_to_store).to eq(true) + end + end end