From cc1fa7f563aa88a58317b76ab9cf487878e66e7f Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 9 Jul 2024 13:57:06 +1000 Subject: [PATCH 1/4] Always avoid error on redirect_to --- spec/controllers/admin/product_import_controller_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/controllers/admin/product_import_controller_spec.rb b/spec/controllers/admin/product_import_controller_spec.rb index 404e4ed7631..060c9519397 100644 --- a/spec/controllers/admin/product_import_controller_spec.rb +++ b/spec/controllers/admin/product_import_controller_spec.rb @@ -4,6 +4,11 @@ RSpec.describe Admin::ProductImportController, type: :controller do describe 'validate_file_path' do + 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' @@ -11,7 +16,6 @@ end it 'should reject other extensions' do - allow(controller).to receive(:raise_invalid_file_path).and_return(false) path = '/tmp/product_import123/import.pdf' expect(controller.__send__(:validate_file_path, path)).to be_falsey path1 = '/tmp/product_import123/import.xslx' @@ -30,7 +34,6 @@ 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' expect(controller.__send__(:validate_file_path, path)).to be_falsey From 823614c214d81fe0f8325a897be2bf7776cf0b7f Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 9 Jul 2024 15:06:00 +1000 Subject: [PATCH 2/4] Always delete uploaded file on error The file path was never going to be 'tmp/product_import', so I guess it never deleted the file. Hmm but shouldn't we clean up on success too? I suppose the tmp dir will be cleaned up eventually, and maybe we want to keep them for debugging purposes. --- app/models/product_import/product_importer.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/product_import/product_importer.rb b/app/models/product_import/product_importer.rb index 1967245fb83..2cdfba52bd0 100644 --- a/app/models/product_import/product_importer.rb +++ b/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 From ca612282f93bd3814dc6a0f4b8b3b2a3e3b3bb76 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 9 Jul 2024 15:16:44 +1000 Subject: [PATCH 3/4] Use Rails tmp dir for product imports again In https://github.com/openfoodfoundation/openfoodnetwork/pull/3435, it was switched to the system tmp dir because it conventiently provided a method to generate a unique filename. However it didn't handle the case where the system provided a symlink (macOS). I could have fixed that, but surely it's safer to use the Rails tmp directory. So I changed back to that, using a tip from https://stackoverflow.com/questions/13787746/creating-a-thread-safe-temporary-file-name to generate a unique name. Perhaps we could use a larger string (eg uuid) or append a timestamp too, but I don't know that it's necessary. Instead, we can just check that the dir didn't exist first (as mentioned in the PR). Let's do that.. --- .../admin/product_import_controller.rb | 19 ++++++++++++---- .../admin/product_import_controller_spec.rb | 22 ++++++++++--------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb index c71e3ad0153..657af946847 100644 --- a/app/controllers/admin/product_import_controller.rb +++ b/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,16 @@ def model_class ProductImport::ProductImporter end + def mktmpdir + tmpdir = tmpdir_base + SecureRandom.hex(6) + Dir.mkdir(tmpdir) + tmpdir + 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 +145,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 +157,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 diff --git a/spec/controllers/admin/product_import_controller_spec.rb b/spec/controllers/admin/product_import_controller_spec.rb index 060c9519397..ac6cf560f13 100644 --- a/spec/controllers/admin/product_import_controller_spec.rb +++ b/spec/controllers/admin/product_import_controller_spec.rb @@ -4,6 +4,8 @@ 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) @@ -11,39 +13,39 @@ 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 - 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 - 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 From 2ab6bcb2e4bedb9f100fff3c864ffbb5adecfb5a Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 9 Jul 2024 15:26:07 +1000 Subject: [PATCH 4/4] Use system method to generate temporary file path This method is not documented, but is used by Dir.mktmpdir. https://apidock.com/ruby/Dir/Tmpname/create --- app/controllers/admin/product_import_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb index 657af946847..a2dc50ce433 100644 --- a/app/controllers/admin/product_import_controller.rb +++ b/app/controllers/admin/product_import_controller.rb @@ -128,9 +128,7 @@ def model_class end def mktmpdir - tmpdir = tmpdir_base + SecureRandom.hex(6) - Dir.mkdir(tmpdir) - tmpdir + Dir::Tmpname.create(TMPDIR_PREFIX, Rails.root.join('tmp') ) { |tmpname| Dir.mkdir(tmpname) } end def tmpdir_base