Merge branch 'security-personal-snippets' into 'master'

Add direct upload support for personal snippets

See merge request gitlab/gitlabhq!3226

GITLAB_WORKHORSE_VERSION

-8.8.0
+8.8.1

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

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!

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

changelogs/unreleased/security-personal-snippets.yml

+---
+title: Remove EXIF from users/personal snippet uploads.
+merge_request:
+author:
+type: security

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.

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,

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|

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

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

spec/lib/gitlab/sanitizers/exif_spec.rb

@@ -7,7 +7,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
@@ -18,7 +20,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

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -7,6 +7,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
@@ -271,7 +273,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
@@ -284,8 +288,13 @@ shared_examples 'handle uploads' do
     context 'when a user can upload a file' do
       before do
         sign_in(user)
+
+        if model.is_a?(PersonalSnippet)
+          model.update!(author: user)
+        else
           model.add_developer(user)
         end
+      end
 
       context 'and the request bypassed workhorse' do
         it 'raises an exception' do