FIX: Make sure S3 object headers are preserved on copy (#14302)

When copying an existing upload stub temporary object
on S3 to its final destination we were not copying across
its additional headers such as content-disposition and
cache-control, which led to issues like attachments not
downloading with their original filename when clicking
the download links in posts.

This is because the metadata_directive = REPLACE option
was not being passed to object.copy_from(), so only the
source object's headers were being used. Added an option
for apply_metadata_to_destination to apply this option
conditionally, because we may not always want to replace
this metadata, but we definitely do when copying a temporary
upload.

lib/file_store/s3_store.rb

@@ -98,6 +98,7 @@ module FileStore
       # if this fails, it will throw an exception
       if opts[:move_existing] && opts[:existing_external_upload_key]
         original_path = opts[:existing_external_upload_key]
+        options[:apply_metadata_to_destination] = true
         path, etag = s3_helper.copy(
           original_path,
           path,

lib/s3_helper.rb

@@ -86,6 +86,10 @@ class S3Helper
   end
 
   def copy(source, destination, options: {})
+    if options[:apply_metadata_to_destination]
+      options = options.except(:apply_metadata_to_destination).merge(metadata_directive: "REPLACE")
+    end
+
     destination = get_path_for_s3_upload(destination)
     if !Rails.configuration.multisite
       options[:copy_source] = File.join(@s3_bucket_name, source)
@@ -102,7 +106,21 @@ class S3Helper
       end
     end
 
-    response = s3_bucket.object(destination).copy_from(options)
+    destination_object = s3_bucket.object(destination)
+
+    # TODO: copy_source is a legacy option here and may become unsupported
+    # in later versions, we should change to use Aws::S3::Client#copy_object
+    # at some point.
+    #
+    # See https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-s3/lib/aws-sdk-s3/customizations/object.rb#L67-L74
+    #
+    # ----
+    #
+    # Also note, any options for metadata (e.g. content_disposition, content_type)
+    # will not be applied unless the metadata_directive = "REPLACE" option is passed
+    # in. If this is not passed in, the source object's metadata will be used.
+    response = destination_object.copy_from(options)
+
     [destination, response.copy_object_result.etag.gsub('"', '')]
   end
 

spec/components/file_store/s3_store_spec.rb

@@ -136,6 +136,49 @@ describe FileStore::S3Store do
         end
       end
     end
+
+    describe "#move_existing_stored_upload" do
+      let(:uploaded_file) { file_from_fixtures(original_filename) }
+      let(:upload_sha1) { Digest::SHA1.hexdigest(File.read(uploaded_file)) }
+      let(:original_filename) { "smallest.png" }
+      let(:s3_client) { Aws::S3::Client.new(stub_responses: true) }
+      let(:s3_helper) { S3Helper.new(SiteSetting.s3_upload_bucket, '', client: s3_client) }
+      let(:store) { FileStore::S3Store.new(s3_helper) }
+      let(:upload_opts) do
+        {
+          acl: "public-read",
+          cache_control: "max-age=31556952, public, immutable",
+          content_type: "image/png",
+          apply_metadata_to_destination: true
+        }
+      end
+      let(:external_upload_stub) { Fabricate(:image_external_upload_stub) }
+      let(:existing_external_upload_key) { external_upload_stub.key }
+
+      before do
+        SiteSetting.authorized_extensions = "pdf|png"
+      end
+
+      it "does not provide a content_disposition for images" do
+        s3_helper.expects(:copy).with(external_upload_stub.key, kind_of(String), options: upload_opts).returns(["path", "etag"])
+        s3_helper.expects(:delete_object).with(external_upload_stub.key)
+        upload = Fabricate(:upload, extension: "png", sha1: upload_sha1, original_filename: original_filename)
+        store.move_existing_stored_upload(external_upload_stub.key, upload, "image/png")
+      end
+
+      context "when the file is a PDF" do
+        let(:external_upload_stub) { Fabricate(:attachment_external_upload_stub, original_filename: original_filename) }
+        let(:original_filename) { "small.pdf" }
+        let(:uploaded_file) { file_from_fixtures("small.pdf", "pdf") }
+
+        it "adds an attachment content-disposition with the original filename" do
+          disp_opts = { content_disposition: "attachment; filename=\"#{original_filename}\"; filename*=UTF-8''#{original_filename}", content_type: "application/pdf" }
+          s3_helper.expects(:copy).with(external_upload_stub.key, kind_of(String), options: upload_opts.merge(disp_opts)).returns(["path", "etag"])
+          upload = Fabricate(:upload, extension: "png", sha1: upload_sha1, original_filename: original_filename)
+          store.move_existing_stored_upload(external_upload_stub.key, upload, "application/pdf")
+        end
+      end
+    end
   end
 
   context 'copying files in S3' do

spec/components/s3_helper_spec.rb

@@ -95,4 +95,38 @@ describe "S3Helper" do
     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
+
+  describe "#copy" do
+    let(:source_key) { "#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}uploads/default/blah/source.jpg" }
+    let(:destination_key) { "original/1X/destination.jpg" }
+    let(:s3_helper) { S3Helper.new("test-bucket", "", client: client) }
+
+    it "can copy an object from the source to the destination" do
+      destination_stub = Aws::S3::Object.new(bucket_name: "test-bucket", key: destination_key, client: client)
+      s3_helper.send(:s3_bucket).expects(:object).with(destination_key).returns(destination_stub)
+      destination_stub.expects(:copy_from).with(copy_source: "test-bucket/#{source_key}").returns(
+        stub(copy_object_result: stub(etag: "\"etag\""))
+      )
+      response = s3_helper.copy(source_key, destination_key)
+      expect(response.first).to eq(destination_key)
+      expect(response.second).to eq("etag")
+    end
+
+    it "puts the metadata from options onto the destination if apply_metadata_to_destination" do
+      content_disposition = "attachment; filename=\"source.jpg\"; filename*=UTF-8''source.jpg"
+      destination_stub = Aws::S3::Object.new(bucket_name: "test-bucket", key: destination_key, client: client)
+      s3_helper.send(:s3_bucket).expects(:object).with(destination_key).returns(destination_stub)
+      destination_stub.expects(:copy_from).with(
+        copy_source: "test-bucket/#{source_key}", content_disposition: content_disposition, metadata_directive: "REPLACE"
+      ).returns(
+        stub(copy_object_result: stub(etag: "\"etag\""))
+      )
+      response = s3_helper.copy(
+        source_key, destination_key,
+        options: { apply_metadata_to_destination: true, content_disposition: content_disposition }
+      )
+      expect(response.first).to eq(destination_key)
+      expect(response.second).to eq("etag")
+    end
+  end
 end