FIX: Do not prefix temp/ S3 keys with s3_bucket_folder_path in S3Helper (#14145)

This is unnecessary, as when the temporary key is created
in S3Store we already include the s3_bucket_folder_path, and
the key will always start with temp/ to assist with lifecycle
rules for multipart uploads.

This was affecting Discourse.store.object_from_path,
Discourse.store.signed_url_for_path, and possibly others.

See also: e0102a5

lib/s3_helper.rb

@@ -264,7 +264,12 @@ class S3Helper
   end
 
   def get_path_for_s3_upload(path)
-    path = File.join(@s3_bucket_folder_path, path) if @s3_bucket_folder_path && path !~ /^#{@s3_bucket_folder_path}\//
+    if @s3_bucket_folder_path &&
+        !path.starts_with?(@s3_bucket_folder_path) &&
+        !path.starts_with?(File.join(FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX, @s3_bucket_folder_path))
+      return File.join(@s3_bucket_folder_path, path)
+    end
+
     path
   end
 

spec/components/file_store/s3_store_spec.rb

@@ -420,6 +420,18 @@ describe FileStore::S3Store do
 
       expect(store.signed_url_for_path("special/optimized/file.png")).not_to eq(upload.url)
     end
+
+    it "does not prefix the s3_bucket_folder_path onto temporary upload prefixed keys" do
+      SiteSetting.s3_upload_bucket = "s3-upload-bucket/folder_path"
+      uri = URI.parse(store.signed_url_for_path("#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}folder_path/uploads/default/blah/def.xyz"))
+      expect(uri.path).to eq(
+        "/#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}folder_path/uploads/default/blah/def.xyz"
+      )
+      uri = URI.parse(store.signed_url_for_path("uploads/default/blah/def.xyz"))
+      expect(uri.path).to eq(
+        "/folder_path/uploads/default/blah/def.xyz"
+      )
+    end
   end
 
   def expect_copy_from(s3_object, source)

spec/components/s3_helper_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
-require "s3_helper"
 require "rails_helper"
+require "s3_helper"
 
 describe "S3Helper" do
   let(:client) { Aws::S3::Client.new(stub_responses: true) }
@@ -78,11 +78,21 @@ describe "S3Helper" do
   end
 
   it "should prefix bucket folder path only if not exists" do
-    s3_helper = S3Helper.new('bucket/folder_path', "", client: client)
+    s3_helper = S3Helper.new("bucket/folder_path", "", client: client)
 
     object1 = s3_helper.object("original/1X/def.xyz")
     object2 = s3_helper.object("folder_path/original/1X/def.xyz")
 
     expect(object1.key).to eq(object2.key)
   end
+
+  it "should not prefix the bucket folder path if the key begins with the temporary upload prefix" do
+    s3_helper = S3Helper.new("bucket/folder_path", "", client: client)
+
+    object1 = s3_helper.object("original/1X/def.xyz")
+    object2 = s3_helper.object("#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}folder_path/uploads/default/blah/def.xyz")
+
+    expect(object1.key).to eq("folder_path/original/1X/def.xyz")
+    expect(object2.key).to eq("#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}folder_path/uploads/default/blah/def.xyz")
+  end
 end