Add error handeling to UploadFinder

Handle upload errors by rescuing them
but sending them to sentry. Extract more functions.

app/finders/uploader_finder.rb

@@ -16,6 +16,11 @@ class UploaderFinder
     retrieve_file_state!
 
     uploader
+  rescue ::Gitlab::Utils::PathTraversalAttackError, StandardError => e
+    # Send any unexpected errors to sentry and return nil
+    Raven.capture_exception(e) unless e.is_a?(::Gitlab::Utils::PathTraversalAttackError)
+
+    nil
   end
 
   def prevent_path_traversal_attack!

ee/app/services/status_page/publication_service_helpers.rb

@@ -2,8 +2,6 @@
 
 module StatusPage
   module PublicationServiceHelpers
-    include Gitlab::Utils::StrongMemoize
-
     def error(message, payload = {})
       ServiceResponse.error(message: message, payload: payload)
     end

ee/app/services/status_page/publish_attachments_service.rb

@@ -4,6 +4,7 @@ module StatusPage
   # Publishes Attachments from incident comments and descriptions to s3
   # Should only be called from publish details or a service that inherits from the publish_base_service
   class PublishAttachmentsService
+    include Gitlab::Utils::StrongMemoize
     include StatusPage::PublicationServiceHelpers
 
     def initialize(project:, issue:, user_notes:, storage_client:)
@@ -27,29 +28,27 @@ module StatusPage
 
     def publish_description_attachments
       publish_markdown_uploads(
-        markdown_field: issue.description,
-        issue_iid: issue.iid
+        markdown_field: issue.description
       )
     end
 
     def publish_user_note_attachments
       user_notes.each do |user_note|
         publish_markdown_uploads(
-          markdown_field: user_note.note,
-          issue_iid: issue.iid
+          markdown_field: user_note.note
         )
       end
     end
 
-    def publish_markdown_uploads(markdown_field:, issue_iid:)
+    def publish_markdown_uploads(markdown_field:)
       markdown_field.scan(FileUploader::MARKDOWN_PATTERN).map do |secret, file_name|
         break if @total_uploads >= StatusPage::Storage::MAX_UPLOADS
 
-        key = StatusPage::Storage.upload_path(issue_iid, secret, file_name)
+        key = upload_path(secret, file_name)
         next if existing_keys.include? key
 
-        # uploader behaves like a file with an 'open' method
-        file = UploaderFinder.new(project, secret, file_name).execute
+        file = find_file(secret, file_name)
+        next if file.nil?
 
         upload_file(key, file)
       end
@@ -57,10 +56,14 @@ module StatusPage
 
     def upload_file(key, file)
       file.open do |open_file|
-        # Send files to s3 storage in parts (hanles large files)
+        # Send files to s3 storage in parts (handles large files)
         storage_client.multipart_upload(key, open_file)
         @total_uploads += 1
       end
+    rescue Error => e
+      # Continue uploading other files if one fails
+      # But report the failure to Sentry
+      Raven.capture_exception(e)
     end
 
     def existing_keys
@@ -69,8 +72,17 @@ module StatusPage
       end
     end
 
+    def upload_path(secret, file_name)
+      StatusPage::Storage.upload_path(issue.iid, secret, file_name)
+    end
+
     def uploads_path
       StatusPage::Storage.uploads_path(issue.iid)
     end
+
+    def find_file(secret, file_name)
+      # Uploader object behaves like a file with an 'open' method
+      UploaderFinder.new(project, secret, file_name).execute
+    end
   end
 end

ee/app/services/status_page/publish_base_service.rb

@@ -2,6 +2,7 @@
 
 module StatusPage
   class PublishBaseService
+    include Gitlab::Utils::StrongMemoize
     include StatusPage::PublicationServiceHelpers
 
     def initialize(project:)

ee/lib/status_page/storage/s3_multipart_upload.rb

@@ -30,7 +30,8 @@ module StatusPage
             complete_upload(upload_id, parts)
             # Rescue on Exception since even on keyboard inturrupt we want to abort the upload and re-raise
             # abort clears the already uploaded parts so that they do not cost the bucket owner
-            # The status page bucket lifecycle policy will clear out any parts if this fails without an exception (power failures etc.)
+            # The status page bucket lifecycle policy will clear out unaborted parts if
+            # this fails without an exception (power failures etc.)
           rescue Exception => e # rubocop:disable Lint/RescueException
             abort_upload(upload_id)
             raise e

lib/api/validations/validators/file_path.rb

@@ -8,7 +8,7 @@ module API
           path = params[attr_name]
 
           Gitlab::Utils.check_path_traversal!(path)
-        rescue StandardError
+        rescue Gitlab::Utils::PathTraversalAttackError
           raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
                                                message: "should be a valid file path"
         end

lib/gitlab/gfm/uploads_rewriter.rb

@@ -22,7 +22,9 @@ module Gitlab
         return @text unless needs_rewrite?
 
         @text.gsub(@pattern) do |markdown|
-          file = UploaderFinder.new(@source_project, $~[:secret], $~[:file]).execute
+          file = find_file($~[:secret], $~[:file])
+          # No file will be returned for a path traversal
+          next if file.nil?
 
           break markdown unless file.try(:exists?)
 
@@ -46,7 +48,7 @@ module Gitlab
 
       def files
         referenced_files = @text.scan(@pattern).map do
-          UploaderFinder.new(@source_project, $~[:secret], $~[:file]).execute
+          find_file($~[:secret], $~[:file])
         end
 
         referenced_files.compact.select(&:exists?)
@@ -55,6 +57,10 @@ module Gitlab
       def was_embedded?(markdown)
         markdown.starts_with?("!")
       end
+
+      def find_file(secret, file_name)
+        UploaderFinder.new(@source_project, secret, file_name).execute
+      end
     end
   end
 end

lib/gitlab/utils.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module Utils
     extend self
+    # disable Cop/CustomErrorClass because recommended style causes 'already initialized constant' warning
+    class PathTraversalAttackError < StandardError; end # rubocop:disable Cop/CustomErrorClass
 
     # Ensure that the relative path will not traverse outside the base directory
     # We url decode the path to avoid passing invalid paths forward in url encoded format.
@@ -17,7 +19,7 @@ module Gitlab
           path.end_with?("#{File::SEPARATOR}..") ||
           (!allowed_absolute && Pathname.new(path).absolute?)
 
-        raise StandardError.new("Invalid path")
+        raise PathTraversalAttackError.new('Invalid path')
       end
 
       path

spec/finders/uploader_finder_spec.rb

@@ -15,24 +15,40 @@ describe UploaderFinder do
       upload.save
     end
 
-    it 'gets the uploader' do
+    context 'when sucessful' do
+      before do
         allow_next_instance_of(FileUploader) do |uploader|
           allow(uploader).to receive(:retrieve_from_store!).with(upload.path).and_return(uploader)
         end
+      end
 
+      it 'gets the file-like uploader' do
         expect(subject).to be_an_instance_of(FileUploader)
         expect(subject.model).to eq(project)
         expect(subject.secret).to eq(secret)
       end
+    end
 
-    context 'path traversal in file name' do
+    context 'when path traversal in file name' do
       before do
         upload.path = '/uploads/11111111111111111111111111111111/../../../../../../../../../../../../../../etc/passwd)'
         upload.save
       end
 
-      it 'throws an error' do
-        expect { subject }.to raise_error(an_instance_of(StandardError).and(having_attributes(message: "Invalid path")))
+      it 'returns nil' do
+        expect(subject).to be(nil)
+      end
+    end
+
+    context 'when unexpected failure' do
+      before do
+        allow_next_instance_of(FileUploader) do |uploader|
+          allow(uploader).to receive(:retrieve_from_store!).and_raise(StandardError)
+        end
+      end
+
+      it 'returns nil when unexpected error is raised' do
+        expect(subject).to be(nil)
       end
     end
   end

spec/lib/gitlab/gfm/uploads_rewriter_spec.rb

@@ -54,6 +54,14 @@ describe Gitlab::Gfm::UploadsRewriter do
           expect(new_paths).not_to include image_uploader.secret
           expect(new_paths).not_to include zip_uploader.secret
         end
+
+        it 'skips nil files do' do
+          allow_next_instance_of(UploaderFinder) do |finder|
+            allow(finder).to receive(:execute).and_return(nil)
+          end
+
+          expect(new_files).to be_empty
+        end
       end
     end