github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-06-01 17:40:43 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: Include original filename in s3 uploads even if not attachment (#30789)

Browse Source 
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
<!-- mdn docs -->
Content-Disposition: inline
Content-Disposition: attachment
Content-Disposition: attachment; filename="file name.jpg"
Content-Disposition: attachment; filename*=UTF-8''file%20name.jpg
<!-- this actually works too -->
Content-Disposition: inline; filename="file name.jpg"
```
This commit is contained in:
Natalie Tay
2025-01-15 18:08:18 +08:00
committed by GitHub
parent 1a70d118a8
commit cfac49fb10
3 changed files with 20 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
lib/file_store/s3_store.rb
View File

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

24
spec/lib/file_store/s3_store_spec.rb
View File

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

25
spec/multisite/s3_store_spec.rb
View File

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