diff --git a/app/models/attachment.rb b/app/models/attachment.rb index a4552f9ada1..4ba634a12bd 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -93,7 +93,9 @@ class Attachment < ApplicationRecord # specifically when using S3 for attachments. In the case of S3 the file name for the downloaded # file will still be correct as it's part of the URL before the query. def external_url_options(expires_in: nil) - { content_disposition: content_disposition(include_filename: false), expires_in: } + { content_disposition: content_disposition(include_filename: false), + content_type: served_content_type, + expires_in: } end def external_storage? @@ -119,11 +121,24 @@ class Attachment < ApplicationRecord end end + # Returns the Content-Type to use when serving this file inline in a browser. + # Text files are normalised to text/plain (prevents script execution) with an + # explicit charset. Non-inlineable files get application/octet-stream so the + # browser is forced to download them. + def served_content_type + if is_text? + "text/plain; charset=#{charset.presence || Setting.attachment_default_charset}" + elsif inlineable? + content_type + else + "application/octet-stream" + end + end + # Returns the content type to use when serving the file to a browser. # For text files, ensures a charset is always present so browsers don't - # fall back to ISO-8859-1. - # We use a configurable fallback (default utf-8) so that administrators - # can control content types for previously uploaded attachments + # fall back to ISO-8859-1. Preserves the real MIME subtype (e.g. text/x-ruby) + # unlike served_content_type which normalises to text/plain for security. def serving_content_type return content_type unless is_text? diff --git a/app/uploaders/fog_file_uploader.rb b/app/uploaders/fog_file_uploader.rb index 463073abcf6..05cb857bbb8 100644 --- a/app/uploaders/fog_file_uploader.rb +++ b/app/uploaders/fog_file_uploader.rb @@ -74,6 +74,7 @@ class FogFileUploader < CarrierWave::Uploader::Base # # @param options [Hash] Options hash. # @option options [String] :content_disposition Pass this content disposition to S3 so that it serves the file with it. + # @option options [String] :content_type Pass this content type to S3 so that it serves the file with it. # @option options [DateTime] :expires_at Date at which the link should expire (default: now + 5 minutes) # @option options [ActiveSupport::Duration] :expires_in Duration in which the link should expire. # @@ -82,6 +83,7 @@ class FogFileUploader < CarrierWave::Uploader::Base url_options = {} set_content_disposition!(url_options, options:) + set_content_type!(url_options, options:) set_expires_at!(url_options, options:) remote_file.url url_options @@ -103,15 +105,17 @@ class FogFileUploader < CarrierWave::Uploader::Base private def set_content_disposition!(url_options, options:) - if options[:content_disposition].present? - url_options[:query] = { - # Passing this option to S3 will make it serve the file with the - # respective content disposition. Without it no content disposition - # header is sent. This only works for S3 but we don't support - # anything else anyway (see carrierwave.rb). - "response-content-disposition" => options[:content_disposition] - } - end + return if options[:content_disposition].blank? + + (url_options[:query] ||= {})["response-content-disposition"] = options[:content_disposition] + end + + def set_content_type!(url_options, options:) + return if options[:content_type].blank? + + # Like the content disposition above, this makes S3 serve the file with the + # given Content-Type, overriding the stored object type. + (url_options[:query] ||= {})["response-content-type"] = options[:content_type] end def set_expires_at!(url_options, options:) diff --git a/lib/api/helpers/attachment_renderer.rb b/lib/api/helpers/attachment_renderer.rb index c086a6de814..3bc18c0fb01 100644 --- a/lib/api/helpers/attachment_renderer.rb +++ b/lib/api/helpers/attachment_renderer.rb @@ -100,17 +100,7 @@ module API end def attachment_content_type(attachment) - if attachment.is_text? - # Even if the text mime type might differ, always output plain text - # so this doesn't get interpreted as e.g., a script or html file - charset = attachment.charset.presence || Setting.attachment_default_charset - "text/plain; charset=#{charset}" - elsif attachment.inlineable? - attachment.content_type - else - # For security reasons, mark all non-inlinable files as an octet-stream first - "application/octet-stream" - end + attachment.served_content_type end def set_cache_headers diff --git a/spec/models/attachment_spec.rb b/spec/models/attachment_spec.rb index f1a7f81b1a3..be4ffcb0f27 100644 --- a/spec/models/attachment_spec.rb +++ b/spec/models/attachment_spec.rb @@ -320,6 +320,11 @@ RSpec.describe Attachment do it_behaves_like "it uses content disposition inline" do let(:attachment) { text_attachment } end + + it "includes response-content-type with text/plain and the detected charset in the S3 URL" do + url = text_attachment.external_url.to_s + expect(url).to include "response-content-type=text%2Fplain%3B%20charset%3D" + end end describe "for a video file" do @@ -351,6 +356,10 @@ RSpec.describe Attachment do expect(binary_attachment.content_disposition).to eq "attachment; filename=textfile.txt.gz" expect(binary_attachment.external_url.to_s).to include "response-content-disposition=attachment" end + + it "includes response-content-type application/octet-stream in the S3 URL" do + expect(binary_attachment.external_url.to_s).to include "response-content-type=application%2Foctet-stream" + end end end