From dd4b8c2afaa9ee8e6a176894cd427c6c3cf66315 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Mon, 6 Sep 2021 10:21:20 +1000
Subject: [PATCH] FIX: Use random file name for temporary uploads (#14250)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Other locale characters in file names (e.g. é, ä) as well
as special characters can cause issues on S3, notably the S3
copy object operation does not support these special characters.
Instead of storing the original file name in the key, which is
unnecessary, we now generate a random file name with the original
extension for the temporary file and use that for all external
upload stub operations.
---
 lib/file_store/base_store.rb    | 6 +++++-
 spec/multisite/s3_store_spec.rb | 6 +++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb
index 865929e3b63..788e6cd5936 100644
--- a/lib/file_store/base_store.rb
+++ b/lib/file_store/base_store.rb
@@ -42,12 +42,16 @@ module FileStore
     end
 
     def temporary_upload_path(file_name, folder_prefix: "")
+      # We don't want to use the original file name as it can contain special
+      # characters, which can interfere with external providers operations and
+      # introduce other unexpected behaviour.
+      file_name_random = "#{SecureRandom.hex}#{File.extname(file_name)}"
       File.join(
         TEMPORARY_UPLOAD_PREFIX,
         folder_prefix,
         upload_path,
         SecureRandom.hex,
-        file_name
+        file_name_random
       )
     end
 
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
index e86535f441a..17eb47f5557 100644
--- a/spec/multisite/s3_store_spec.rb
+++ b/spec/multisite/s3_store_spec.rb
@@ -313,7 +313,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         url = store.signed_url_for_temporary_upload("test.png")
         key = store.path_from_url(url)
         expect(url).to match(/Amz-Expires/)
-        expect(key).to match(/temp\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/test.png/)
+        expect(key).to match(/temp\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/[a-zA-z0-9]{0,32}.png/)
       end
 
       it "presigned url contans the metadata when provided" do
@@ -329,7 +329,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         url = store.signed_url_for_temporary_upload("test.png")
         key = store.path_from_url(url)
         expect(url).to match(/Amz-Expires/)
-        expect(key).to match(/temp\/site\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/test.png/)
+        expect(key).to match(/temp\/site\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/[a-zA-z0-9]{0,32}.png/)
       end
     end
 
@@ -341,7 +341,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
           url = store.signed_url_for_temporary_upload("test.png")
           key = store.path_from_url(url)
           expect(url).to match(/Amz-Expires/)
-          expect(key).to match(/temp\/standard99\/uploads\/second\/test_[0-9]\/[a-zA-z0-9]{0,32}\/test.png/)
+          expect(key).to match(/temp\/standard99\/uploads\/second\/test_[0-9]\/[a-zA-z0-9]{0,32}\/[a-zA-z0-9]{0,32}.png/)
         end
       end
     end