From 0d809197aaa567629ebbfe1201948e91f38cea3a Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 10 Sep 2021 12:59:51 +1000 Subject: [PATCH] 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 | 1 + lib/s3_helper.rb | 20 +++++++++- spec/components/file_store/s3_store_spec.rb | 43 +++++++++++++++++++++ spec/components/s3_helper_spec.rb | 34 ++++++++++++++++ 4 files changed, 97 insertions(+), 1 deletion(-) diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 82fadc4bc29..1c9f7e56abc 100644 --- a/lib/file_store/s3_store.rb +++ b/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, diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index e82e1ac1406..f5a57732953 100644 --- a/lib/s3_helper.rb +++ b/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 diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index cad9c05df41..472e61cd837 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/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 diff --git a/spec/components/s3_helper_spec.rb b/spec/components/s3_helper_spec.rb index f74c737bfa2..daf229fc6fe 100644 --- a/spec/components/s3_helper_spec.rb +++ b/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