diff options
Diffstat (limited to 'activestorage')
14 files changed, 172 insertions, 38 deletions
diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb index 99982202dd..df8d73cc91 100644 --- a/activestorage/app/controllers/active_storage/disk_controller.rb +++ b/activestorage/app/controllers/active_storage/disk_controller.rb @@ -9,7 +9,7 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController def show if key = decode_verified_key - serve_file disk_service.path_for(key), content_type: params[:content_type], disposition: params[:disposition] + serve_file disk_service.path_for(key[:key]), content_type: key[:content_type], disposition: key[:disposition] else head :not_found end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 53aa9f0237..d9bdb27b38 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -130,8 +130,8 @@ class ActiveStorage::Blob < ActiveRecord::Base def service_url(expires_in: ActiveStorage.service_urls_expire_in, disposition: :inline, filename: nil, **options) filename = ActiveStorage::Filename.wrap(filename || self.filename) - service.url key, expires_in: expires_in, filename: filename, content_type: content_type, - disposition: forcibly_serve_as_binary? ? :attachment : disposition, **options + service.url key, expires_in: expires_in, filename: filename, content_type: content_type_for_service_url, + disposition: forced_disposition_for_service_url || disposition, **options end # Returns a URL that can be used to directly upload a file for this blob on the service. This URL is intended to be @@ -170,7 +170,7 @@ class ActiveStorage::Blob < ActiveRecord::Base end def upload_without_unfurling(io) #:nodoc: - service.upload key, io, checksum: checksum + service.upload key, io, checksum: checksum, **service_metadata end # Downloads the file associated with this blob. If no block is given, the entire file is read into memory and returned. @@ -239,5 +239,29 @@ class ActiveStorage::Blob < ActiveRecord::Base ActiveStorage.content_types_to_serve_as_binary.include?(content_type) end + def allowed_inline? + ActiveStorage.content_types_allowed_inline.include?(content_type) + end + + def content_type_for_service_url + forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : content_type + end + + def forced_disposition_for_service_url + if forcibly_serve_as_binary? || !allowed_inline? + :attachment + end + end + + def service_metadata + if forcibly_serve_as_binary? + { content_type: ActiveStorage.binary_content_type, disposition: :attachment, filename: filename } + elsif !allowed_inline? + { content_type: content_type, disposition: :attachment, filename: filename } + else + { content_type: content_type } + end + end + ActiveSupport.run_load_hooks(:active_storage_blob, self) end diff --git a/activestorage/app/models/active_storage/blob/identifiable.rb b/activestorage/app/models/active_storage/blob/identifiable.rb index 2c17ddc25f..924bd06131 100644 --- a/activestorage/app/models/active_storage/blob/identifiable.rb +++ b/activestorage/app/models/active_storage/blob/identifiable.rb @@ -2,7 +2,10 @@ module ActiveStorage::Blob::Identifiable def identify - update! content_type: identify_content_type, identified: true unless identified? + unless identified? + update! content_type: identify_content_type, identified: true + update_service_metadata + end end def identified? @@ -21,4 +24,8 @@ module ActiveStorage::Blob::Identifiable "" end end + + def update_service_metadata + service.update_metadata key, service_metadata if service_metadata.any? + end end diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb index a94ef626f2..19c7f6be2a 100644 --- a/activestorage/lib/active_storage.rb +++ b/activestorage/lib/active_storage.rb @@ -49,6 +49,8 @@ module ActiveStorage mattr_accessor :paths, default: {} mattr_accessor :variable_content_types, default: [] mattr_accessor :content_types_to_serve_as_binary, default: [] + mattr_accessor :content_types_allowed_inline, default: [] + mattr_accessor :binary_content_type, default: "application/octet-stream" mattr_accessor :service_urls_expire_in, default: 5.minutes mattr_accessor :routes_prefix, default: "/rails/active_storage" diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index d51e806220..b3bcf48d6f 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -40,6 +40,18 @@ module ActiveStorage text/xml application/xml application/xhtml+xml + application/mathml+xml + text/cache-manifest + ) + + config.active_storage.content_types_allowed_inline = %w( + image/png + image/gif + image/jpg + image/jpeg + image/vnd.adobe.photoshop + image/vnd.microsoft.icon + application/pdf ) config.eager_load_namespaces << ActiveStorage @@ -57,6 +69,8 @@ module ActiveStorage ActiveStorage.variable_content_types = app.config.active_storage.variable_content_types || [] ActiveStorage.content_types_to_serve_as_binary = app.config.active_storage.content_types_to_serve_as_binary || [] ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes + ActiveStorage.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || [] + ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream" end end diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb index 54ba08fb87..c18fccbb1d 100644 --- a/activestorage/lib/active_storage/service.rb +++ b/activestorage/lib/active_storage/service.rb @@ -62,10 +62,16 @@ module ActiveStorage # Upload the +io+ to the +key+ specified. If a +checksum+ is provided, the service will # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **options) raise NotImplementedError end + # Update metadata for the file identified by +key+ in the service. + # Override in subclasses only if the service needs to store specific + # metadata that has to be updated upon identification. + def update_metadata(key, **metadata) + end + # Return the content of the file at the +key+. def download(key) raise NotImplementedError diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb index 8de3889cb5..a2c4b4d57c 100644 --- a/activestorage/lib/active_storage/service/azure_storage_service.rb +++ b/activestorage/lib/active_storage/service/azure_storage_service.rb @@ -17,7 +17,7 @@ module ActiveStorage @container = container end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **) instrument :upload, key: key, checksum: checksum do handle_errors do blobs.create_block_blob(container, key, IO.try_convert(io) || io, content_md5: checksum) diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb index 52f3a3df16..2588c41760 100644 --- a/activestorage/lib/active_storage/service/disk_service.rb +++ b/activestorage/lib/active_storage/service/disk_service.rb @@ -15,7 +15,7 @@ module ActiveStorage @root = root end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **) instrument :upload, key: key, checksum: checksum do IO.copy_stream(io, make_path_for(key)) ensure_integrity_of(key, checksum) if checksum @@ -79,17 +79,23 @@ module ActiveStorage def url(key, expires_in:, filename:, disposition:, content_type:) instrument :url, key: key do |payload| - verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key) - - generated_url = - url_helpers.rails_disk_service_url( - verified_key_with_expiration, - host: current_host, - filename: filename, - disposition: content_disposition_with(type: disposition, filename: filename), + content_disposition = content_disposition_with(type: disposition, filename: filename) + verified_key_with_expiration = ActiveStorage.verifier.generate( + { + key: key, + disposition: content_disposition, content_type: content_type - ) + }, + { expires_in: expires_in, + purpose: :blob_key } + ) + generated_url = url_helpers.rails_disk_service_url(verified_key_with_expiration, + host: current_host, + disposition: content_disposition, + content_type: content_type, + filename: filename + ) payload[:url] = generated_url generated_url diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb index 18c0f14cfc..e2590aa35d 100644 --- a/activestorage/lib/active_storage/service/gcs_service.rb +++ b/activestorage/lib/active_storage/service/gcs_service.rb @@ -11,16 +11,15 @@ module ActiveStorage @config = config end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, content_type: nil, disposition: nil, filename: nil) instrument :upload, key: key, checksum: checksum do begin - # The official GCS client library doesn't allow us to create a file with no Content-Type metadata. - # We need the file we create to have no Content-Type so we can control it via the response-content-type - # param in signed URLs. Workaround: let the GCS client create the file with an inferred - # Content-Type (usually "application/octet-stream") then clear it. - bucket.create_file(io, key, md5: checksum).update do |file| - file.content_type = nil - end + # GCS's signed URLs don't include params such as response-content-type response-content_disposition + # in the signature, which means an attacker can modify them and bypass our effort to force these to + # binary and attachment when the file's content type requires it. The only way to force them is to + # store them as object's metadata. + content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename + bucket.create_file(io, key, md5: checksum, content_type: content_type, content_disposition: content_disposition) rescue Google::Cloud::InvalidArgumentError raise ActiveStorage::IntegrityError end @@ -43,6 +42,15 @@ module ActiveStorage end end + def update_metadata(key, content_type:, disposition: nil, filename: nil) + instrument :update_metadata, key: key, content_type: content_type, disposition: disposition do + file_for(key).update do |file| + file.content_type = content_type + file.content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename + end + end + end + def download_chunk(key, range) instrument :download_chunk, key: key, range: range do begin diff --git a/activestorage/lib/active_storage/service/mirror_service.rb b/activestorage/lib/active_storage/service/mirror_service.rb index 6002ef5a00..75274f81b3 100644 --- a/activestorage/lib/active_storage/service/mirror_service.rb +++ b/activestorage/lib/active_storage/service/mirror_service.rb @@ -24,9 +24,9 @@ module ActiveStorage # Upload the +io+ to the +key+ specified to all services. If a +checksum+ is provided, all services will # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **options) each_service.collect do |service| - service.upload key, io.tap(&:rewind), checksum: checksum + service.upload key, io.tap(&:rewind), checksum: checksum, **options end end diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb index 7b5e989699..0aa8648db3 100644 --- a/activestorage/test/controllers/disk_controller_test.rb +++ b/activestorage/test/controllers/disk_controller_test.rb @@ -5,11 +5,12 @@ require "database/setup" class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest test "showing blob inline" do - blob = create_blob + blob = create_blob(filename: "hello.jpg", content_type: "image/jpg") + get blob.service_url assert_response :ok - assert_equal "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"] - assert_equal "text/plain", response.headers["Content-Type"] + assert_equal "inline; filename=\"hello.jpg\"; filename*=UTF-8''hello.jpg", response.headers["Content-Disposition"] + assert_equal "image/jpg", response.headers["Content-Type"] assert_equal "Hello world!", response.body end @@ -36,6 +37,10 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest blob.delete get blob.service_url + end + + test "showing blob with invalid key" do + get rails_disk_service_url(encoded_key: "Invalid key", filename: "hello.txt") assert_response :not_found end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 1a6a89de56..0eac622d72 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -121,12 +121,21 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end end - test "urls force attachment as content disposition for content types served as binary" do + test "urls force content_type to binary and attachment as content disposition for content types served as binary" do blob = create_blob(content_type: "text/html") freeze_time do - assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url - assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url(disposition: :inline) + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url(disposition: :inline) + end + end + + test "urls force attachment as content disposition when the content type is not allowed inline" do + blob = create_blob(content_type: "application/zip") + + freeze_time do + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url(disposition: :inline) end end @@ -148,7 +157,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase arguments = [ blob.key, expires_in: ActiveStorage.service_urls_expire_in, - disposition: :inline, + disposition: :attachment, content_type: blob.content_type, filename: blob.filename, thumb_size: "300x300", @@ -183,9 +192,13 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end private - def expected_url_for(blob, disposition: :inline, filename: nil) + def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil) filename ||= blob.filename - query_string = { content_type: blob.content_type, disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized) }.to_param - "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}" + content_type ||= blob.content_type + + query = { disposition: disposition.to_s + "; #{filename.parameters}", content_type: content_type } + key_params = { key: blob.key }.merge(query) + + "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(key_params, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query.to_param}" end end diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb index 6577f1cd9f..8552080e7b 100644 --- a/activestorage/test/models/variant_test.rb +++ b/activestorage/test/models/variant_test.rb @@ -156,7 +156,7 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase test "service_url doesn't grow in length despite long variant options" do blob = create_file_blob(filename: "racecar.jpg") variant = blob.variant(font: "a" * 10_000).processed - assert_operator variant.service_url.length, :<, 525 + assert_operator variant.service_url.length, :<, 726 end test "works for vips processor" do diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb index 2ba2f8b346..73337ef8e1 100644 --- a/activestorage/test/service/gcs_service_test.rb +++ b/activestorage/test/service/gcs_service_test.rb @@ -31,6 +31,55 @@ if SERVICE_CONFIGURATIONS[:gcs] end end + test "upload with content_type and content_disposition" do + begin + key = SecureRandom.base58(24) + data = "Something else entirely!" + + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") + + url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) + response = Net::HTTP.get_response(URI(url)) + assert_equal "text/plain", response.content_type + assert_match /attachment;.*test.txt/, response["Content-Disposition"] + ensure + @service.delete key + end + end + + test "upload with content_type" do + begin + key = SecureRandom.base58(24) + data = "Something else entirely!" + + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), content_type: "text/plain") + + url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) + response = Net::HTTP.get_response(URI(url)) + assert_equal "text/plain", response.content_type + assert_match /inline;.*test.html/, response["Content-Disposition"] + ensure + @service.delete key + end + end + + test "update metadata" do + begin + key = SecureRandom.base58(24) + data = "Something else entirely!" + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.html"), content_type: "text/html") + + @service.update_metadata(key, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") + url = @service.url(key, expires_in: 2.minutes, disposition: :attachment, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) + + response = Net::HTTP.get_response(URI(url)) + assert_equal "text/plain", response.content_type + assert_match /inline;.*test.txt/, response["Content-Disposition"] + ensure + @service.delete key + end + end + test "signed URL generation" do assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/, @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")) |