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..

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,16 @@ module Admin
       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 @@ 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 +157,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

spec/controllers/admin/product_import_controller_spec.rb

@@ -4,6 +4,8 @@ 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)
@@ -11,39 +13,39 @@ RSpec.describe Admin::ProductImportController, type: :controller do
 
     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