aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJulik Tarkhanov <me@julik.nl>2018-12-30 17:56:22 +0100
committerGeorge Claghorn <george.claghorn@gmail.com>2018-12-30 11:56:22 -0500
commite5f4162b6178489181e3d7e7163ac12b7e0efc9d (patch)
tree75dea44e5e9bb7d87eeeeb2733666f5831a2344c
parenta796b993ad3717f9a0b3d5be070311ab7a0c95ba (diff)
downloadrails-e5f4162b6178489181e3d7e7163ac12b7e0efc9d.tar.gz
rails-e5f4162b6178489181e3d7e7163ac12b7e0efc9d.tar.bz2
rails-e5f4162b6178489181e3d7e7163ac12b7e0efc9d.zip
Make Active Storage blob keys lowercase
Accommodate case-insensitive filesystems and database collations.
-rw-r--r--activestorage/CHANGELOG.md6
-rw-r--r--activestorage/app/models/active_storage/blob.rb16
-rw-r--r--activestorage/test/models/blob_test.rb4
-rw-r--r--activestorage/test/models/variant_test.rb2
-rw-r--r--activesupport/lib/active_support/core_ext/securerandom.rb26
-rw-r--r--activesupport/test/core_ext/secure_random_test.rb20
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