FIX: Add attachment content-disposition for all non-image files (#10058)

FIX: Add attachment content-disposition for all non-image files (#10058)

This will make it so the original filename is used when downloading all non-image files, bringing S3Store into line with the to_s3 migration and local storage. Video and audio files will still stream correctly in HTML players as well.

See Cannot download non-image media files, original filenames lost when uploaded to S3 - bug - Discourse Meta for a lot of extra context.

diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index 5007b74..4183e97 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -53,8 +53,14 @@ module FileStore
         cache_control: 'max-age=31556952, public, immutable',
         content_type: opts[:content_type].presence || MiniMime.lookup_by_filename(filename)&.content_type
       }
-      # add a "content disposition" header for "attachments"
-      options[:content_disposition] = "attachment; filename=\"#{filename}\"" unless FileHelper.is_supported_media?(filename)
+
+      # add a "content disposition: attachment" header with the original filename
+      # for everything but images. audio and video will still stream correctly in
+      # 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}\""
+      end
 
       path.prepend(File.join(upload_path, "/")) if Rails.configuration.multisite
 
diff --git a/spec/fixtures/media/small.mp3 b/spec/fixtures/media/small.mp3
new file mode 100644
index 0000000..084a7d1
Binary files /dev/null and b/spec/fixtures/media/small.mp3 differ
diff --git a/spec/fixtures/media/small.mp4 b/spec/fixtures/media/small.mp4
new file mode 100644
index 0000000..ed139d6
Binary files /dev/null and b/spec/fixtures/media/small.mp4 differ
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
index 5681a1d..a4c0f3e 100644
--- a/spec/multisite/s3_store_spec.rb
+++ b/spec/multisite/s3_store_spec.rb
@@ -4,12 +4,13 @@ require 'rails_helper'
 require 'file_store/s3_store'
 
 RSpec.describe 'Multisite s3 uploads', type: :multisite do
-  let(:uploaded_file) { file_from_fixtures("smallest.png") }
+  let(:original_filename) { "smallest.png" }
+  let(:uploaded_file) { file_from_fixtures(original_filename) }
   let(:upload_sha1) { Digest::SHA1.hexdigest(File.read(uploaded_file)) }
   let(:upload_path) { Discourse.store.upload_path }
 
   def build_upload
-    Fabricate.build(:upload, sha1: upload_sha1, id: 1)
+    Fabricate.build(:upload, sha1: upload_sha1, id: 1, original_filename: original_filename)
   end
 
   context 'uploading to s3' do
@@ -24,6 +25,55 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
       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"
+        }
+      end
+
+      it "does not provide a content_disposition for images" do
+        s3_helper.expects(:upload).with(uploaded_file, kind_of(String), upload_opts).returns(["path", "etag"])
+        upload = build_upload
+        store.store_upload(uploaded_file, upload)
+      end
+
+      context "when the file is a PDF" do
+        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}\"", 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)
+        end
+      end
+
+      context "when the file is a video" do
+        let(:original_filename) { "small.mp4" }
+        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" }
+          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)
+        end
+      end
+
+      context "when the file is audio" do
+        let(:original_filename) { "small.mp3" }
+        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" }
+          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)
+        end
+      end
 
       it "returns the correct url for default and second multisite db" do
         test_multisite_connection('default') do

GitHub sha: e5da2d24

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

This commit has been mentioned on Discourse Meta. There might be relevant details there:

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