From cfac49fb1065cc08cc914b7aab02a91cd663e157 Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Wed, 15 Jan 2025 18:08:18 +0800 Subject: [PATCH] FIX: Include original filename in s3 uploads even if not attachment (#30789) Related: https://github.com/discourse/discourse/pull/30535 In the PR above, the [content-disposition header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition) was removed for all non-svg files due to the "attachment" keyword added to them, causing files to be downloaded instead of opening in a new tab when requested. When removing that, it also removed the filename attribute attached to s3 uploads. After some testing, it turns out that `filename` is also respected when next to `inline`, despite it not being obvious [in docs](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition#syntax). This commit adds inline+filename so that users can still download files and have filenames be respected instead of using the s3 hash. ```http Content-Disposition: inline Content-Disposition: attachment Content-Disposition: attachment; filename="file name.jpg" Content-Disposition: attachment; filename*=UTF-8''file%20name.jpg Content-Disposition: inline; filename="file name.jpg" ``` --- lib/file_store/s3_store.rb | 10 ++++------ spec/lib/file_store/s3_store_spec.rb | 24 ++++-------------------- spec/multisite/s3_store_spec.rb | 25 ++++++++++++------------- 3 files changed, 20 insertions(+), 39 deletions(-) diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index af37adb9ac8..4aef460efa8 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -97,12 +97,10 @@ module FileStore # Only add a "content disposition: attachment" header for svgs # see https://github.com/discourse/discourse/commit/31e31ef44973dc4daaee2f010d71588ea5873b53. # Adding this header for all files would break the ability to view attachments in the browser - if FileHelper.is_svg?(filename) - options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format( - disposition: "attachment", - filename: filename, - ) - end + options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format( + disposition: FileHelper.is_svg?(filename) ? "attachment" : "inline", + filename: filename, + ) path.prepend(File.join(upload_path, "/")) if Rails.configuration.multisite diff --git a/spec/lib/file_store/s3_store_spec.rb b/spec/lib/file_store/s3_store_spec.rb index 71f8e2f7fbe..c01766bfcbc 100644 --- a/spec/lib/file_store/s3_store_spec.rb +++ b/spec/lib/file_store/s3_store_spec.rb @@ -39,6 +39,7 @@ RSpec.describe FileStore::S3Store do acl: "public-read", cache_control: "max-age=31556952, public, immutable", content_type: "image/png", + content_disposition: "inline; filename=\"logo.png\"; filename*=UTF-8''logo.png", body: uploaded_file, }, ) @@ -90,6 +91,7 @@ RSpec.describe FileStore::S3Store do acl: "private", cache_control: "max-age=31556952, public, immutable", content_type: "application/pdf", + content_disposition: "inline; filename=\"small.pdf\"; filename*=UTF-8''small.pdf", body: uploaded_file, }, ) @@ -116,6 +118,7 @@ RSpec.describe FileStore::S3Store do acl: "public-read", cache_control: "max-age=31556952, public, immutable", content_type: "image/png", + content_disposition: "inline; filename=\"logo.png\"; filename*=UTF-8''logo.png", body: uploaded_file, }, ) @@ -148,6 +151,7 @@ RSpec.describe FileStore::S3Store do acl: nil, cache_control: "max-age=31556952, public, immutable", content_type: "application/pdf", + content_disposition: "inline; filename=\"small.pdf\"; filename*=UTF-8''small.pdf", body: uploaded_file, }, ) @@ -216,26 +220,6 @@ RSpec.describe FileStore::S3Store do before { SiteSetting.authorized_extensions = "svg|png" } - 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(%w[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( - existing_external_upload_key: external_upload_stub.key, - upload: upload, - content_type: "image/png", - ) - end - context "when the file is a SVG" do let(:external_upload_stub) do Fabricate(:attachment_external_upload_stub, original_filename: original_filename) diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb index d3ed44ff3af..d5268491360 100644 --- a/spec/multisite/s3_store_spec.rb +++ b/spec/multisite/s3_store_spec.rb @@ -33,15 +33,6 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do } end - it "does not provide a content_disposition for images" do - s3_helper - .expects(:upload) - .with(uploaded_file, kind_of(String), upload_opts) - .returns(%w[path etag]) - upload = build_upload - store.store_upload(uploaded_file, upload) - end - context "when the file is a SVG" do let(:original_filename) { "small.svg" } let(:uploaded_file) { file_from_fixtures("small.svg", "svg") } @@ -65,8 +56,12 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do let(:original_filename) { "small.mp4" } let(:uploaded_file) { file_from_fixtures("small.mp4", "media") } - it "does not add content-disposition header" do - disp_opts = { content_type: "application/mp4" } + it "adds inline content-disposition header with original filename" do + disp_opts = { + content_disposition: + "inline; 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)) @@ -80,8 +75,12 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do let(:original_filename) { "small.mp3" } let(:uploaded_file) { file_from_fixtures("small.mp3", "media") } - it "does not add content-disposition header" do - disp_opts = { content_type: "audio/mpeg" } + it "adds inline content-disposition header with filename" do + disp_opts = { + content_disposition: + "inline; 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))