From 0ef5f554d1a63362b87f00e10fa517d63c28c6a4 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 9 Jul 2019 20:51:42 +0200 Subject: Add direct upload support for personal snippets --- GITLAB_WORKHORSE_VERSION | 2 +- app/controllers/concerns/uploads_actions.rb | 4 ++++ app/controllers/uploads_controller.rb | 6 ++++-- app/uploaders/personal_file_uploader.rb | 4 ++++ changelogs/unreleased/security-personal-snippets.yml | 5 +++++ config/routes/uploads.rb | 4 ++++ doc/administration/raketasks/uploads/sanitize.md | 2 ++ lib/gitlab/sanitizers/exif.rb | 7 +++++-- lib/tasks/gitlab/uploads/sanitize.rake | 6 ++++-- spec/controllers/uploads_controller_spec.rb | 18 ++++++++++++++++++ spec/lib/gitlab/sanitizers/exif_spec.rb | 18 ++++++++++++++++-- .../controllers/uploads_actions_shared_examples.rb | 11 ++++++++++- 12 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/security-personal-snippets.yml diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index df5119ec64e..d139a75408e 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.7.0 +8.7.1 diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index f5d35379e10..60a68cec3c3 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -127,4 +127,8 @@ module UploadsActions def model strong_memoize(:model) { find_model } end + + def workhorse_authorize_request? + action_name == 'authorize' + end end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 94bd18f70d4..2adfeab182e 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -2,6 +2,7 @@ class UploadsController < ApplicationController include UploadsActions + include WorkhorseRequest UnknownUploadModelError = Class.new(StandardError) @@ -21,7 +22,8 @@ class UploadsController < ApplicationController before_action :upload_mount_satisfied? before_action :find_model before_action :authorize_access!, only: [:show] - before_action :authorize_create_access!, only: [:create] + before_action :authorize_create_access!, only: [:create, :authorize] + before_action :verify_workhorse_api!, only: [:authorize] def uploader_class PersonalFileUploader @@ -72,7 +74,7 @@ class UploadsController < ApplicationController end def render_unauthorized - if current_user + if current_user || workhorse_authorize_request? render_404 else authenticate_user! diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index 1ac69601d18..3efdd0aa1d9 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -6,6 +6,10 @@ class PersonalFileUploader < FileUploader options.storage_path end + def self.workhorse_local_upload_path + File.join(options.storage_path, 'uploads', TMP_UPLOAD_PATH) + end + def self.base_dir(model, _store = nil) # base_dir is the path seen by the user when rendering Markdown, so # it should be the same for both local and object storage. It is diff --git a/changelogs/unreleased/security-personal-snippets.yml b/changelogs/unreleased/security-personal-snippets.yml new file mode 100644 index 00000000000..95f61993b98 --- /dev/null +++ b/changelogs/unreleased/security-personal-snippets.yml @@ -0,0 +1,5 @@ +--- +title: Remove EXIF from users/personal snippet uploads. +merge_request: +author: +type: security diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index 920f8454ce2..096ef146e07 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -30,6 +30,10 @@ scope path: :uploads do to: 'uploads#create', constraints: { model: /personal_snippet|user/, id: /\d+/ }, as: 'upload' + + post ':model/authorize', + to: 'uploads#authorize', + constraints: { model: /personal_snippet|user/ } end # Redirect old note attachments path to new uploads path. diff --git a/doc/administration/raketasks/uploads/sanitize.md b/doc/administration/raketasks/uploads/sanitize.md index ae5ccfb9e37..7574660d848 100644 --- a/doc/administration/raketasks/uploads/sanitize.md +++ b/doc/administration/raketasks/uploads/sanitize.md @@ -37,6 +37,8 @@ Parameter | Type | Description `stop_id` | integer | Only uploads with equal or smaller ID will be processed `dry_run` | boolean | Do not remove EXIF data, only check if EXIF data are present or not, default: true `sleep_time` | float | Pause for number of seconds after processing each image, default: 0.3 seconds +`uploader` | string | Run sanitization only for uploads of the given uploader (`FileUploader`, `PersonalFileUploader`, `NamespaceFileUploader`) +`since` | date | Run sanitization only for uploads newer than given date (e.g. `2019-05-01`) If you have too many uploads, you can speed up sanitization by setting `sleep_time` to a lower value or by running multiple rake tasks in parallel, diff --git a/lib/gitlab/sanitizers/exif.rb b/lib/gitlab/sanitizers/exif.rb index bb4e4ce7bbc..2f3d14ecebd 100644 --- a/lib/gitlab/sanitizers/exif.rb +++ b/lib/gitlab/sanitizers/exif.rb @@ -53,15 +53,18 @@ module Gitlab end # rubocop: disable CodeReuse/ActiveRecord - def batch_clean(start_id: nil, stop_id: nil, dry_run: true, sleep_time: nil) + def batch_clean(start_id: nil, stop_id: nil, dry_run: true, sleep_time: nil, uploader: nil, since: nil) relation = Upload.where('lower(path) like ? or lower(path) like ? or lower(path) like ?', '%.jpg', '%.jpeg', '%.tiff') + relation = relation.where(uploader: uploader) if uploader + relation = relation.where('created_at > ?', since) if since logger.info "running in dry run mode, no images will be rewritten" if dry_run find_params = { start: start_id.present? ? start_id.to_i : nil, - finish: stop_id.present? ? stop_id.to_i : Upload.last&.id + finish: stop_id.present? ? stop_id.to_i : Upload.last&.id, + batch_size: 1000 } relation.find_each(find_params) do |upload| diff --git a/lib/tasks/gitlab/uploads/sanitize.rake b/lib/tasks/gitlab/uploads/sanitize.rake index 12cf5302555..4f23a0a5d82 100644 --- a/lib/tasks/gitlab/uploads/sanitize.rake +++ b/lib/tasks/gitlab/uploads/sanitize.rake @@ -2,7 +2,7 @@ namespace :gitlab do namespace :uploads do namespace :sanitize do desc 'GitLab | Uploads | Remove EXIF from images.' - task :remove_exif, [:start_id, :stop_id, :dry_run, :sleep_time] => :environment do |task, args| + task :remove_exif, [:start_id, :stop_id, :dry_run, :sleep_time, :uploader, :since] => :environment do |task, args| args.with_defaults(dry_run: 'true') args.with_defaults(sleep_time: 0.3) @@ -11,7 +11,9 @@ namespace :gitlab do sanitizer = Gitlab::Sanitizers::Exif.new(logger: logger) sanitizer.batch_clean(start_id: args.start_id, stop_id: args.stop_id, dry_run: args.dry_run != 'false', - sleep_time: args.sleep_time.to_f) + sleep_time: args.sleep_time.to_f, + uploader: args.uploader, + since: args.since) end end end diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 0876502a899..5f4a6bf8ee7 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -21,8 +21,20 @@ shared_examples 'content publicly cached' do end describe UploadsController do + include WorkhorseHelpers + let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } + describe 'POST #authorize' do + it_behaves_like 'handle uploads authorize' do + let(:uploader_class) { PersonalFileUploader } + let(:model) { create(:personal_snippet, :public) } + let(:params) do + { model: 'personal_snippet', id: model.id } + end + end + end + describe 'POST create' do let(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') } let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } @@ -636,4 +648,10 @@ describe UploadsController do end end end + + def post_authorize(verified: true) + request.headers.merge!(workhorse_internal_api_request_header) if verified + + post :authorize, params: { model: 'personal_snippet', id: model.id }, format: :json + end end diff --git a/spec/lib/gitlab/sanitizers/exif_spec.rb b/spec/lib/gitlab/sanitizers/exif_spec.rb index bd5f330c7a1..0a4bc1bbdbb 100644 --- a/spec/lib/gitlab/sanitizers/exif_spec.rb +++ b/spec/lib/gitlab/sanitizers/exif_spec.rb @@ -5,7 +5,9 @@ describe Gitlab::Sanitizers::Exif do describe '#batch_clean' do context 'with image uploads' do - let!(:uploads) { create_list(:upload, 3, :with_file, :issuable_upload) } + set(:upload1) { create(:upload, :with_file, :issuable_upload) } + set(:upload2) { create(:upload, :with_file, :personal_snippet_upload) } + set(:upload3) { create(:upload, :with_file, created_at: 3.days.ago) } it 'processes all uploads if range ID is not set' do expect(sanitizer).to receive(:clean).exactly(3).times @@ -16,7 +18,19 @@ describe Gitlab::Sanitizers::Exif do it 'processes only uploads in the selected range' do expect(sanitizer).to receive(:clean).once - sanitizer.batch_clean(start_id: uploads[1].id, stop_id: uploads[1].id) + sanitizer.batch_clean(start_id: upload1.id, stop_id: upload1.id) + end + + it 'processes only uploads for the selected uploader' do + expect(sanitizer).to receive(:clean).once + + sanitizer.batch_clean(uploader: 'PersonalFileUploader') + end + + it 'processes only uploads created since specified date' do + expect(sanitizer).to receive(:clean).exactly(2).times + + sanitizer.batch_clean(since: 2.days.ago) end it 'pauses if sleep_time is set' do diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 9036838e50a..d6bc1493254 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -5,6 +5,8 @@ shared_examples 'handle uploads' do let(:secret) { FileUploader.generate_secret } let(:uploader_class) { FileUploader } + it_behaves_like 'handle uploads authorize' + describe "POST #create" do context 'when a user is not authorized to upload a file' do it 'returns 404 status' do @@ -269,7 +271,9 @@ shared_examples 'handle uploads' do end end end +end +shared_examples 'handle uploads authorize' do describe "POST #authorize" do context 'when a user is not authorized to upload a file' do it 'returns 404 status' do @@ -282,7 +286,12 @@ shared_examples 'handle uploads' do context 'when a user can upload a file' do before do sign_in(user) - model.add_developer(user) + + if model.is_a?(PersonalSnippet) + model.update!(author: user) + else + model.add_developer(user) + end end context 'and the request bypassed workhorse' do -- cgit v1.2.3