extra security check - check db for saved file names

Author: tsubik

app/controllers/uploads_controller.rb

@@ -21,6 +21,7 @@ class UploadsController < ApplicationController
   def download
     sanitize_filepath
     parse_upload_path
+    ensure_valid_db_record
     track_download if trackable_request?
     send_file @sanitized_filepath, disposition: :inline
   end
@@ -47,9 +48,33 @@ def parse_upload_path
 
     model_key = path_parts[0].downcase
     @model_name = MODELS_OVERRIDES[model_key] || model_key
+    @uploader_name = path_parts[1].downcase
+    @record_id = path_parts[2].to_i
     @filename = path_parts[3..].join("/")
   end
 
+  # extra security step, do not let download antything for removed records or not saved in DB
+  # we already have sanitized file path that should be enough, but this also will ensure no protected file is downloaded
+  def ensure_valid_db_record
+    model_class = @model_name.classify.constantize
+    record = model_class.find(@record_id)
+    uploader = record.public_send(@uploader_name)
+    db_filenames = [uploader.file.file, *uploader.versions.values.map { |v| v.file.file }].map { |f| File.basename(f) }
+
+    unless db_filenames.include?(File.basename(@sanitized_filepath))
+      raise_not_found_exception
+    end
+  rescue NameError, ActiveRecord::RecordNotFound
+    raise_not_found_exception
+  end
+
+  def allowed_models
+    Dir.entries(Rails.root.join("uploads"))
+      .select { |entry| File.directory?(Rails.root.join("uploads", entry)) }
+      .reject { |entry| entry.start_with?(".") || entry == "tmp" }
+      .map { |entry| ALLOWED_MODELS_OVERRIDES[entry.downcase] || entry }
+  end
+
   def track_download
     TrackFileDownloadJob.perform_later(client_id, request.url, @filename, @model_name)
   end

spec/factories/contributors.rb

@@ -21,11 +21,11 @@
     priority { rand(0..10) }
     logo { Rack::Test::UploadedFile.new(File.join(Rails.root, "spec", "support", "files", "image.png")) }
 
-    factory :partner do
+    factory :partner, class: Partner do
       type { "Partner" }
     end
 
-    factory :donor do
+    factory :donor, class: Donor do
       type { "Donor" }
     end
   end

spec/integration/uploads_spec.rb

@@ -40,6 +40,15 @@
 
         expect(response).to have_http_status(:not_found)
       end
+
+      it "returns 404 for existing files but not in db" do
+        new_file = File.join(File.dirname(@document_file.attachment.file.file), "newfile.txt")
+        File.write(new_file, "test")
+
+        get "/uploads/operator_document_file/attachment/#{@document_file.id}/newfile.txt"
+
+        expect(response).to have_http_status(:not_found)
+      end
     end
 
     context "invalid path handling" do