diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb index c71e3ad015..a2dc50ce43 100644 --- a/app/controllers/admin/product_import_controller.rb +++ b/app/controllers/admin/product_import_controller.rb @@ -4,6 +4,8 @@ require 'roo' 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 @@ module Admin 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 @@ module Admin 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 @@ module Admin 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 @@ module Admin 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 diff --git a/app/models/product_import/product_importer.rb b/app/models/product_import/product_importer.rb index 1967245fb8..2cdfba52bd 100644 --- a/app/models/product_import/product_importer.rb +++ b/app/models/product_import/product_importer.rb @@ -287,8 +287,6 @@ module ProductImport end def delete_uploaded_file - return unless @file.path == Rails.root.join("tmp/product_import").to_s - File.delete(@file) end diff --git a/spec/controllers/admin/product_import_controller_spec.rb b/spec/controllers/admin/product_import_controller_spec.rb index 404e4ed763..ac6cf560f1 100644 --- a/spec/controllers/admin/product_import_controller_spec.rb +++ b/spec/controllers/admin/product_import_controller_spec.rb @@ -4,43 +4,48 @@ require 'spec_helper' 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