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))