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

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

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
+      # Only add a "content disposition: attachment" header for svgs
-      # filename for everything but safe images (not SVG). audio and video will
+      # see https://github.com/discourse/discourse/commit/31e31ef44973dc4daaee2f010d71588ea5873b53.
-      # still stream correctly in HTML players, and when a direct link is
+      # Adding this header for all files would break the ability to view attachments in the browser
-      # provided to any file but an image it will download correctly in the
+      if FileHelper.is_svg?(filename)
-      # browser.
-      if !FileHelper.is_inline_image?(filename)
         options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format(
           disposition: "attachment",
           filename: filename,

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",

spec/fixtures/svg/small.svg

@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg
+  width="12px"
+  height="12px"
+  viewBox="0 0 12 12"
+  version="1.0"
+  id="svg1849"
+  xmlns:xlink="http://www.w3.org/1999/xlink"
+  xmlns="http://www.w3.org/2000/svg">
+  <defs
+    id="defs1841">
+    <polygon
+      id="path-1"
+      points="19 13 13 13 13 19 11 19 11 13 5 13 5 11 11 11 11 5 13 5 13 11 19 11"/>
+  </defs>
+  <g
+    transform="translate(-6.000000, -6.000000)"
+    style="fill:#000;fill-opacity:1">
+    <use
+      xlink:href="#path-1"
+      style="fill:#000;fill-opacity:1"/>
+  </g>
+</svg>

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(:original_filename) { "small.svg" }
-        let(:uploaded_file) { file_from_fixtures("small.pdf", "pdf") }
+        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

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
+      context "when the file is a SVG" do
-        let(:original_filename) { "small.pdf" }
+        let(:original_filename) { "small.svg" }
-        let(:uploaded_file) { file_from_fixtures("small.pdf", "pdf") }
+        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
+        it "does not add content-disposition header" do
-          disp_opts = {
+          disp_opts = { content_type: "application/mp4" }
-            content_disposition:
-              "attachment; 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))
@@ -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
+        it "does not add content-disposition header" do
-          disp_opts = {
+          disp_opts = { content_type: "audio/mpeg" }
-            content_disposition:
-              "attachment; 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))