diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 5007b741e8e..4183e973cfa 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 00000000000..084a7d1f8d1 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 00000000000..ed139d6d50c 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 5681a1df8dc..a4c0f3e02a1 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