FIX: Use ActionDispatch::Http::ContentDisposition for uploads content-disposition (#10108)

FIX: Use ActionDispatch::Http::ContentDisposition for uploads content-disposition (#10108)

See https://meta.discourse.org/t/broken-pipe-error-when-uploading-to-a-s3-clone-a-pdf-with-a-name-containing-e-i-etc/155414

When setting content-disposition for attachment, use the ContentDisposition class to format it. This handles filenames with weird characters and localization (accented characters) correctly.

diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index cc507ed..ef4717f 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -59,7 +59,9 @@ module FileStore
       # HTML players, and when a direct link is provided to any file but an image
       # it will download correctly in the browser.
       if !FileHelper.is_supported_image?(filename)
-        options[:content_disposition] = "attachment; filename=\"#{filename}\""
+        options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format(
+          disposition: "attachment", filename: filename
+        )
       end
 
       path.prepend(File.join(upload_path, "/")) if Rails.configuration.multisite
diff --git a/lib/file_store/to_s3_migration.rb b/lib/file_store/to_s3_migration.rb
index 6f19f2d..0235375 100644
--- a/lib/file_store/to_s3_migration.rb
+++ b/lib/file_store/to_s3_migration.rb
@@ -224,8 +224,9 @@ module FileStore
           upload = Upload.find_by(url: "/#{file}")
 
           if upload&.original_filename
-            options[:content_disposition] =
-              %Q{attachment; filename="#{upload.original_filename}"}
+            options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format(
+              disposition: "attachment", filename: upload.original_filename
+            )
           end
 
           if upload&.secure
diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb
index 15c36b0..d5bbceb 100644
--- a/spec/components/file_store/s3_store_spec.rb
+++ b/spec/components/file_store/s3_store_spec.rb
@@ -91,7 +91,7 @@ describe FileStore::S3Store do
             acl: "private",
             cache_control: "max-age=31556952, public, immutable",
             content_type: "application/pdf",
-            content_disposition: "attachment; filename=\"#{upload.original_filename}\"",
+            content_disposition: "attachment; filename=\"#{upload.original_filename}\"; filename*=UTF-8''#{upload.original_filename}",
             body: uploaded_file).returns(Aws::S3::Types::PutObjectOutput.new(etag: "\"#{etag}\""))
 
           expect(store.store_upload(uploaded_file, upload)).to eq(
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
index a4c0f3e..c857e3e 100644
--- a/spec/multisite/s3_store_spec.rb
+++ b/spec/multisite/s3_store_spec.rb
@@ -44,7 +44,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         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}\"", content_type: "application/pdf" }
+          disp_opts = { content_disposition: "attachment; filename=\"#{original_filename}\"; filename*=UTF-8''#{original_filename}", content_type: "application/pdf" }
           s3_helper.expects(:upload).with(uploaded_file, kind_of(String), upload_opts.merge(disp_opts)).returns(["path", "etag"])
           upload = build_upload
           store.store_upload(uploaded_file, upload)
@@ -56,7 +56,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         let(:uploaded_file) { file_from_fixtures("small.mp4", "media") }
 
         it "adds an attachment content-disposition with the original filename" do
-          disp_opts = { content_disposition: "attachment; filename=\"#{original_filename}\"", content_type: "application/mp4" }
+          disp_opts = { content_disposition: "attachment; filename=\"#{original_filename}\"; filename*=UTF-8''#{original_filename}", content_type: "application/mp4" }
           s3_helper.expects(:upload).with(uploaded_file, kind_of(String), upload_opts.merge(disp_opts)).returns(["path", "etag"])
           upload = build_upload
           store.store_upload(uploaded_file, upload)
@@ -68,7 +68,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         let(:uploaded_file) { file_from_fixtures("small.mp3", "media") }
 
         it "adds an attachment content-disposition with the original filename" do
-          disp_opts = { content_disposition: "attachment; filename=\"#{original_filename}\"", content_type: "audio/mpeg" }
+          disp_opts = { content_disposition: "attachment; filename=\"#{original_filename}\"; filename*=UTF-8''#{original_filename}", content_type: "audio/mpeg" }
           s3_helper.expects(:upload).with(uploaded_file, kind_of(String), upload_opts.merge(disp_opts)).returns(["path", "etag"])
           upload = build_upload
           store.store_upload(uploaded_file, upload)

GitHub sha: e92909aa

1 Like

This commit appears in #10108 which was merged by martin.