From 4d5ba6a7e67e5468f63d6a5051eedf9ee87e8e57 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Tue, 9 May 2023 11:20:37 +0100 Subject: [PATCH] add file path validation --- .rubocop_todo.yml | 1 + .../admin/product_import_controller.rb | 26 +++++- .../admin/product_import_controller_spec.rb | 82 +++++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 spec/controllers/admin/product_import_controller_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index dc22595479..967fc56209 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -658,6 +658,7 @@ Metrics/ClassLength: - 'app/components/products_table_component.rb' - 'app/controllers/admin/enterprises_controller.rb' - 'app/controllers/admin/order_cycles_controller.rb' + - 'app/controllers/admin/product_import_controller.rb' - 'app/controllers/admin/resource_controller.rb' - 'app/controllers/admin/schedules_controller.rb' - 'app/controllers/admin/subscriptions_controller.rb' diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb index 6c3dce8334..3a288eb538 100644 --- a/app/controllers/admin/product_import_controller.rb +++ b/app/controllers/admin/product_import_controller.rb @@ -37,7 +37,7 @@ module Admin end def reset_absent_products - @importer = ProductImport::ProductImporter.new(File.new(params[:filepath]), + @importer = ProductImport::ProductImporter.new(File.new(file_path), spree_current_user, import_into: params[:import_into], enterprises_to_reset: params[:enterprises_to_reset], updated_ids: params[:updated_ids], settings: params[:settings]) if params.key?(:enterprises_to_reset) && params.key?(:updated_ids) @@ -56,7 +56,7 @@ module Admin end def process_data(method) - @importer = ProductImport::ProductImporter.new(File.new(params[:filepath]), + @importer = ProductImport::ProductImporter.new(File.new(file_path), spree_current_user, start: params[:start], end: params[:end], settings: params[:settings]) begin @@ -112,5 +112,27 @@ module Admin def model_class ProductImport::ProductImporter end + + def file_path + @file_path ||= validate_file_path(sanitize_file_path(params[:filepath])) + end + + def sanitize_file_path(file_path) + pathname = Pathname.new(file_path) + raise_invalid_file_path unless pathname.file? + pathname.realpath + end + + def validate_file_path(file_path) + return file_path if file_path.to_s.match?(TEMP_FILE_PATH_REGEX) + + raise_invalid_file_path + end + + def raise_invalid_file_path + redirect_to '/admin/product_import', 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 new file mode 100644 index 0000000000..431d8a2ba5 --- /dev/null +++ b/spec/controllers/admin/product_import_controller_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Admin::ProductImportController, type: :controller do + describe 'validate_file_path' do + context 'file extension' do + it 'should authorize csv extension' do + path = '/tmp/product_import123/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' + expect(controller.__send__(:validate_file_path, path)).to be_falsey + path1 = '/tmp/product_import123/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' + expect(controller.__send__(:validate_file_path, path)).to be_truthy + path1 = '/tmp/product_importabc/import.csv' + expect(controller.__send__(:validate_file_path, path1)).to be_truthy + path2 = '/tmp/product_importABC-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' + expect(controller.__send__(:validate_file_path, path)).to be_falsey + + path1 = '/tmp/product_import../etc/import.csv' + expect(controller.__send__(:validate_file_path, path1)).to be_falsey + + path2 = '/tmp/product_import132%2F..%2Fetc%2F/import.csv' + expect(controller.__send__(:validate_file_path, path2)).to be_falsey + + path3 = '/etc/tmp/product_import123/import.csv' + expect(controller.__send__(:validate_file_path, path3)).to be_falsey + end + end + end + + describe 'sanitize_file_path' do + let(:folder_path){ '/tmp/product_import123' } + let(:file_path) { "#{folder_path}/import.csv" } + + before do + FileUtils.mkdir_p(folder_path) + File.new(file_path, 'w') unless File.exist?(file_path) + end + + def subject(path) + # expose the private method sanitize_file_path + described_class.class_eval { public :sanitize_file_path } + described_class.new.__send__(:sanitize_file_path, path) + end + + it 'should covert relative path to absolute' do + path = "/tmp/product_import123/import.csv" + expect(subject(path).to_s).to eq file_path + + path1 = "/../../tmp/product_import123/import.csv" + expect(subject(path1).to_s).to eq file_path + + path2 = "/etc/../../../tmp/product_import123/import.csv" + expect(subject(path2).to_s).to eq file_path + end + + it "raise an exception if the file doesn't exist" do + path = '/tmp/product_import123/import1.csv' + allow_any_instance_of(described_class).to receive(:raise_invalid_file_path) + .and_raise('Invalid File Path') + expect{ subject(path) }.to raise_error('Invalid File Path') + end + end +end