From 0f0b3a21e691c02d74c701a5e8f078e2aba5b61e Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Tue, 7 Jan 2025 10:32:32 +0800 Subject: [PATCH] FIX: Allow attachments to be opened in a new tab instead of downloading them (#30535) Back then in 31e31ef, we added the Content-Disposition headers so that SVGs get downloaded instead of get run in the browser. Inadvertently, this also causes other attachments like pdfs and videos to be downloaded instead of heeding the "Open in new tab" option that users choose in the browser. When the header is removed, the default value is "inline", this allows the browser to perform as requested. This also applies to other file types like pdfs, allowing users to "Open in new tab" and view them in the browser instead of always downloading them. Existing tests (#10205) already do check that SVGs remain downloaded. Some existing tests written for PDFs have been modified to cater for SVGs instead, when there was a bug in defining the filenames per #10108 --- lib/file_helper.rb | 4 ++++ lib/file_store/s3_store.rb | 10 ++++------ lib/file_store/to_s3_migration.rb | 2 +- spec/fixtures/svg/small.svg | 23 +++++++++++++++++++++++ spec/lib/file_store/s3_store_spec.rb | 16 ++++++---------- spec/multisite/s3_store_spec.rb | 24 ++++++++---------------- 6 files changed, 46 insertions(+), 33 deletions(-) create mode 100644 spec/fixtures/svg/small.svg diff --git a/lib/file_helper.rb b/lib/file_helper.rb index 7bbd09ed27a..4bfb73921dc 100644 --- a/lib/file_helper.rb +++ b/lib/file_helper.rb @@ -28,6 +28,10 @@ class FileHelper filename.match?(inline_images_regexp) end + def self.is_svg?(filename) + filename.match?(/\.svg\z/i) + end + def self.is_supported_media?(filename) filename.match?(supported_media_regexp) end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index f332dc753c5..af37adb9ac8 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -94,12 +94,10 @@ module FileStore opts[:content_type].presence || MiniMime.lookup_by_filename(filename)&.content_type, } - # add a "content disposition: attachment" header with the original - # filename for everything but safe images (not SVG). 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_inline_image?(filename) + # 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, diff --git a/lib/file_store/to_s3_migration.rb b/lib/file_store/to_s3_migration.rb index dacfa20b785..a6966c389ba 100644 --- a/lib/file_store/to_s3_migration.rb +++ b/lib/file_store/to_s3_migration.rb @@ -255,7 +255,7 @@ module FileStore end options[:acl] = "private" if upload&.secure - elsif !FileHelper.is_inline_image?(name) + elsif !FileHelper.is_svg?(name) upload = Upload.find_by(url: "/#{file}") options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format( disposition: "attachment", diff --git a/spec/fixtures/svg/small.svg b/spec/fixtures/svg/small.svg new file mode 100644 index 00000000000..c2cd500b3eb --- /dev/null +++ b/spec/fixtures/svg/small.svg @@ -0,0 +1,23 @@ + + + + + + + + + diff --git a/spec/lib/file_store/s3_store_spec.rb b/spec/lib/file_store/s3_store_spec.rb index 908cfc5202e..71f8e2f7fbe 100644 --- a/spec/lib/file_store/s3_store_spec.rb +++ b/spec/lib/file_store/s3_store_spec.rb @@ -90,8 +90,6 @@ RSpec.describe FileStore::S3Store do acl: "private", cache_control: "max-age=31556952, public, immutable", content_type: "application/pdf", - content_disposition: - "attachment; filename=\"#{upload.original_filename}\"; filename*=UTF-8''#{upload.original_filename}", body: uploaded_file, }, ) @@ -150,8 +148,6 @@ RSpec.describe FileStore::S3Store do acl: nil, cache_control: "max-age=31556952, public, immutable", content_type: "application/pdf", - content_disposition: - "attachment; filename=\"#{upload.original_filename}\"; filename*=UTF-8''#{upload.original_filename}", body: uploaded_file, }, ) @@ -218,7 +214,7 @@ RSpec.describe FileStore::S3Store do let(:external_upload_stub) { Fabricate(:image_external_upload_stub) } let(:existing_external_upload_key) { external_upload_stub.key } - before { SiteSetting.authorized_extensions = "pdf|png" } + before { SiteSetting.authorized_extensions = "svg|png" } it "does not provide a content_disposition for images" do s3_helper @@ -240,18 +236,18 @@ RSpec.describe FileStore::S3Store do ) end - context "when the file is a PDF" do + context "when the file is a SVG" do let(:external_upload_stub) do Fabricate(:attachment_external_upload_stub, original_filename: original_filename) end - let(:original_filename) { "small.pdf" } - let(:uploaded_file) { file_from_fixtures("small.pdf", "pdf") } + let(:original_filename) { "small.svg" } + let(:uploaded_file) { file_from_fixtures("small.svg", "svg") } 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", + content_type: "image/svg+xml", } s3_helper .expects(:copy) @@ -267,7 +263,7 @@ RSpec.describe FileStore::S3Store do store.move_existing_stored_upload( existing_external_upload_key: external_upload_stub.key, upload: upload, - content_type: "application/pdf", + content_type: "image/svg+xml", ) end end diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb index 52444585476..d3ed44ff3af 100644 --- a/spec/multisite/s3_store_spec.rb +++ b/spec/multisite/s3_store_spec.rb @@ -42,15 +42,15 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do 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") } + context "when the file is a SVG" do + let(:original_filename) { "small.svg" } + let(:uploaded_file) { file_from_fixtures("small.svg", "svg") } 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", + content_type: "image/svg+xml", } s3_helper .expects(:upload) @@ -65,12 +65,8 @@ RSpec.describe "Multisite s3 uploads", type: :multisite 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}\"; filename*=UTF-8''#{original_filename}", - content_type: "application/mp4", - } + it "does not add content-disposition header" do + disp_opts = { content_type: "application/mp4" } s3_helper .expects(:upload) .with(uploaded_file, kind_of(String), upload_opts.merge(disp_opts)) @@ -84,12 +80,8 @@ RSpec.describe "Multisite s3 uploads", type: :multisite 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}\"; filename*=UTF-8''#{original_filename}", - content_type: "audio/mpeg", - } + it "does not add content-disposition header" do + disp_opts = { content_type: "audio/mpeg" } s3_helper .expects(:upload) .with(uploaded_file, kind_of(String), upload_opts.merge(disp_opts))