From e5f4162b6178489181e3d7e7163ac12b7e0efc9d Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 30 Dec 2018 17:56:22 +0100 Subject: [PATCH] Make Active Storage blob keys lowercase Accommodate case-insensitive filesystems and database collations. --- activestorage/CHANGELOG.md | 6 +++++ .../app/models/active_storage/blob.rb | 16 +++++++++--- activestorage/test/models/blob_test.rb | 4 +++ activestorage/test/models/variant_test.rb | 2 +- .../active_support/core_ext/securerandom.rb | 26 ++++++++++++++++--- .../test/core_ext/secure_random_test.rb | 20 ++++++++++++++ 6 files changed, 67 insertions(+), 7 deletions(-) diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 51890f308b..b643895893 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,9 @@ +* Use base36 (all lowercase) for all new Blob keys to prevent + collisions and undefined behavior with case-insensitive filesystems and + database indices. + + *Julik Tarkhanov* + * It doesn’t include an `X-CSRF-Token` header if a meta tag is not found on the page. It previously included one with a value of `undefined`. diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 04f9dbff9f..6ca7d49bc1 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -79,6 +79,15 @@ class ActiveStorage::Blob < ActiveRecord::Base def create_before_direct_upload!(filename:, byte_size:, checksum:, content_type: nil, metadata: nil) create! filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, metadata: metadata end + + # To prevent problems with case-insensitive filesystems, especially in combination + # with databases which treat indices as case-sensitive, all blob keys generated are going + # to only contain the base-36 character alphabet and will therefore be lowercase. To maintain + # the same or higher amount of entropy as in the base-58 encoding used by `has_secure_token` + # the number of bytes used is increased to 28 from the standard 24 + def generate_unique_secure_token + SecureRandom.base36(28) + end end # Returns a signed ID for this blob that's suitable for reference on the client-side without fear of tampering. @@ -87,9 +96,10 @@ class ActiveStorage::Blob < ActiveRecord::Base ActiveStorage.verifier.generate(id, purpose: :blob_id) end - # Returns the key pointing to the file on the service that's associated with this blob. The key is in the - # standard secure-token format from Rails. So it'll look like: XTAPjJCJiuDrLk3TmwyJGpUo. This key is not intended - # to be revealed directly to the user. Always refer to blobs using the signed_id or a verified form of the key. + # Returns the key pointing to the file on the service that's associated with this blob. The key is the + # secure-token format from Rails in lower case. So it'll look like: xtapjjcjiudrlk3tmwyjgpuobabd. + # This key is not intended to be revealed directly to the user. + # Always refer to blobs using the signed_id or a verified form of the key. def key # We can't wait until the record is first saved to have a key for it self[:key] ||= self.class.generate_unique_secure_token diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 1503f5fc50..54cf9e2b8a 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -47,6 +47,10 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase assert_equal "text/csv", blob.content_type end + test "create after upload generates a 28-character base36 key" do + assert_match(/^[a-z0-9]{28}$/, create_blob.key) + end + test "image?" do blob = create_file_blob filename: "racecar.jpg" assert_predicate blob, :image? diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb index 4f88440e54..012aee3cc9 100644 --- a/activestorage/test/models/variant_test.rb +++ b/activestorage/test/models/variant_test.rb @@ -150,7 +150,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, :<, 726 + assert_operator variant.service_url.length, :<, 730 end test "works for vips processor" do diff --git a/activesupport/lib/active_support/core_ext/securerandom.rb b/activesupport/lib/active_support/core_ext/securerandom.rb index b4a491f5fd..ef812f7e1a 100644 --- a/activesupport/lib/active_support/core_ext/securerandom.rb +++ b/activesupport/lib/active_support/core_ext/securerandom.rb @@ -4,17 +4,18 @@ require "securerandom" module SecureRandom BASE58_ALPHABET = ("0".."9").to_a + ("A".."Z").to_a + ("a".."z").to_a - ["0", "O", "I", "l"] + BASE36_ALPHABET = ("0".."9").to_a + ("a".."z").to_a + # SecureRandom.base58 generates a random base58 string. # - # The argument _n_ specifies the length, of the random string to be generated. + # The argument _n_ specifies the length of the random string to be generated. # # If _n_ is not specified or is +nil+, 16 is assumed. It may be larger in the future. # - # The result may contain alphanumeric characters except 0, O, I and l + # The result may contain alphanumeric characters except 0, O, I and l. # # p SecureRandom.base58 # => "4kUgL2pdQMSCQtjE" # p SecureRandom.base58(24) # => "77TMHrHJFvFDwodq8w7Ev2m7" - # def self.base58(n = 16) SecureRandom.random_bytes(n).unpack("C*").map do |byte| idx = byte % 64 @@ -22,4 +23,23 @@ module SecureRandom BASE58_ALPHABET[idx] end.join end + + # SecureRandom.base36 generates a random base36 string in lowercase. + # + # The argument _n_ specifies the length of the random string to be generated. + # + # If _n_ is not specified or is +nil+, 16 is assumed. It may be larger in the future. + # This method can be used over +base58+ if a deterministic case key is necessary. + # + # The result will contain alphanumeric characters in lowercase. + # + # p SecureRandom.base36 # => "4kugl2pdqmscqtje" + # p SecureRandom.base36(24) # => "77tmhrhjfvfdwodq8w7ev2m7" + def self.base36(n = 16) + SecureRandom.random_bytes(n).unpack("C*").map do |byte| + idx = byte % 64 + idx = SecureRandom.random_number(36) if idx >= 36 + BASE36_ALPHABET[idx] + end.join + end end diff --git a/activesupport/test/core_ext/secure_random_test.rb b/activesupport/test/core_ext/secure_random_test.rb index 7067fb524c..88c8a37c5d 100644 --- a/activesupport/test/core_ext/secure_random_test.rb +++ b/activesupport/test/core_ext/secure_random_test.rb @@ -19,4 +19,24 @@ class SecureRandomTest < ActiveSupport::TestCase assert_not_equal s1, s2 assert_equal 24, s1.length end + + def test_base36_lowercase + s1 = SecureRandom.base36 + s2 = SecureRandom.base36 + + assert_not_equal s1, s2 + assert_equal 16, s1.length + assert_match(/^[a-z0-9]+$/, s1) + assert_match(/^[a-z0-9]+$/, s2) + end + + def test_base36_with_length + s1 = SecureRandom.base36(24) + s2 = SecureRandom.base36(24) + + assert_not_equal s1, s2 + assert_equal 24, s1.length + assert_match(/^[a-z0-9]+$/, s1) + assert_match(/^[a-z0-9]+$/, s2) + end end