Merge pull request #12649 from dacook/product_import-tmpfile

Use application tmp dir for product import

app/controllers/admin/product_import_controller.rb

@@ -4,6 +4,8 @@
 
 module Admin
   class ProductImportController < Spree::Admin::BaseController
+    TMPDIR_PREFIX = "product_import-"
+
     before_action :validate_upload_presence, except: %i[index guide validate_data]
 
     def index
@@ -101,8 +103,7 @@ def check_spreadsheet_has_data(importer)
 
     def save_uploaded_file(upload)
       extension = File.extname(upload.original_filename)
-      directory = Dir.mktmpdir 'product_import'
-      File.open(File.join(directory, "import#{extension}"), 'wb') do |f|
+      File.open(File.join(mktmpdir, "import#{extension}"), 'wb') do |f|
         data = UploadSanitizer.new(upload.read).call
         f.write(data)
         f.path
@@ -126,6 +127,14 @@ def model_class
       ProductImport::ProductImporter
     end
 
+    def mktmpdir
+      Dir::Tmpname.create(TMPDIR_PREFIX, Rails.root.join('tmp') ) { |tmpname| Dir.mkdir(tmpname) }
+    end
+
+    def tmpdir_base
+      Rails.root.join('tmp', TMPDIR_PREFIX).to_s
+    end
+
     def file_path
       @file_path ||= validate_file_path(sanitize_file_path(params[:filepath]))
     end
@@ -134,8 +143,9 @@ def sanitize_file_path(file_path)
       FilePathSanitizer.new.sanitize(file_path, on_error: method(:raise_invalid_file_path))
     end
 
+    # Ensure file is under the safe tmp directory
     def validate_file_path(file_path)
-      return file_path if file_path.to_s.match?(TEMP_FILE_PATH_REGEX)
+      return file_path if file_path.to_s.match?(%r{^#{tmpdir_base}[A-Za-z0-9-]*/import\.csv$})
 
       raise_invalid_file_path
     end
@@ -145,6 +155,5 @@ def raise_invalid_file_path
                   notice: I18n.t(:product_import_no_data_in_spreadsheet_notice)
       raise 'Invalid File Path'
     end
-    TEMP_FILE_PATH_REGEX = %r{^/tmp/product_import[A-Za-z0-9-]*/import\.csv$}
   end
 end

app/models/product_import/product_importer.rb

@@ -287,8 +287,6 @@ def save_all_valid
     end
 
     def delete_uploaded_file
-      return unless @file.path == Rails.root.join("tmp/product_import").to_s
-
       File.delete(@file)
     end
 

spec/controllers/admin/product_import_controller_spec.rb

@@ -4,43 +4,48 @@
 
 RSpec.describe Admin::ProductImportController, type: :controller do
   describe 'validate_file_path' do
+    let(:tmp_directory_base) { Rails.root.join("tmp/product_import-") }
+
+    before do
+      # Avoid error on redirect_to
+      allow(controller).to receive(:raise_invalid_file_path).and_return(false)
+    end
+
     context 'file extension' do
       it 'should authorize csv extension' do
-        path = '/tmp/product_import123/import.csv'
+        path = "#{tmp_directory_base}123/import.csv"
         expect(controller.__send__(:validate_file_path, path)).to be_truthy
       end
 
       it 'should reject other extensions' do
-        allow(controller).to receive(:raise_invalid_file_path).and_return(false)
-        path = '/tmp/product_import123/import.pdf'
+        path = "#{tmp_directory_base}123/import.pdf"
         expect(controller.__send__(:validate_file_path, path)).to be_falsey
-        path1 = '/tmp/product_import123/import.xslx'
+        path1 = "#{tmp_directory_base}123/import.xslx"
         expect(controller.__send__(:validate_file_path, path1)).to be_falsey
       end
     end
 
     context 'folder path' do
       it 'should authorize valid paths' do
-        path = '/tmp/product_import123/import.csv'
+        path = "#{tmp_directory_base}123/import.csv"
         expect(controller.__send__(:validate_file_path, path)).to be_truthy
-        path1 = '/tmp/product_importabc/import.csv'
+        path1 = "#{tmp_directory_base}abc/import.csv"
         expect(controller.__send__(:validate_file_path, path1)).to be_truthy
-        path2 = '/tmp/product_importABC-abc-123/import.csv'
+        path2 = "#{tmp_directory_base}ABC-abc-123/import.csv"
         expect(controller.__send__(:validate_file_path, path2)).to be_truthy
       end
 
       it 'should reject invalid paths' do
-        allow(controller).to receive(:raise_invalid_file_path).and_return(false)
-        path = '/tmp/product_import123/../etc/import.csv'
+        path = "#{tmp_directory_base}123/../etc/import.csv"
         expect(controller.__send__(:validate_file_path, path)).to be_falsey
 
-        path1 = '/tmp/product_import../etc/import.csv'
+        path1 = "#{tmp_directory_base}../etc/import.csv"
         expect(controller.__send__(:validate_file_path, path1)).to be_falsey
 
-        path2 = '/tmp/product_import132%2F..%2Fetc%2F/import.csv'
+        path2 = "#{tmp_directory_base}132%2F..%2Fetc%2F/import.csv"
         expect(controller.__send__(:validate_file_path, path2)).to be_falsey
 
-        path3 = '/etc/tmp/product_import123/import.csv'
+        path3 = "/etc#{tmp_directory_base}123/import.csv"
         expect(controller.__send__(:validate_file_path, path3)).to be_falsey
       end
     end