From 47e563781cd625ce6fdcbb3994379bfcd168702c Mon Sep 17 00:00:00 2001
From: Eugene Kenny <elkenny@gmail.com>
Date: Tue, 1 Oct 2019 00:17:31 +0100
Subject: [PATCH] Always use 28 characters for Active Storage keys

Active Storage keys are generated in two ways: in the `before_create`
callback added by `has_secure_token`, or by calling `key` before the
blob is saved. a273da7619ac6a2b2f98532a5610238c68ad219b broke the
former code path, and the latter was previously untested.
---
 activestorage/app/models/active_storage/blob.rb | 4 ++--
 activestorage/test/models/blob_test.rb          | 4 ++++
 activestorage/test/test_helper.rb               | 4 ++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 57d45bcc04..8d526c2c87 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -29,7 +29,7 @@ class ActiveStorage::Blob < ActiveRecord::Base
 
   MINIMUM_TOKEN_LENGTH = 28
 
-  has_secure_token :key
+  has_secure_token :key, length: MINIMUM_TOKEN_LENGTH
   store :metadata, accessors: [ :analyzed, :identified ], coder: ActiveRecord::Coders::JSON
 
   class_attribute :service
@@ -115,7 +115,7 @@ class ActiveStorage::Blob < ActiveRecord::Base
   # 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
+    self[:key] ||= self.class.generate_unique_secure_token(length: MINIMUM_TOKEN_LENGTH)
   end
 
   # Returns an ActiveStorage::Filename instance of the filename that can be
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index 23bea3e147..b9dff6bbcd 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/activestorage/test/models/blob_test.rb
@@ -99,6 +99,10 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
+  test "build_after_unfurling generates a 28-character base36 key" do
+    assert_match(/^[a-z0-9]{28}$/, build_blob_after_unfurling.key)
+  end
+
   test "image?" do
     blob = create_file_blob filename: "racecar.jpg"
     assert_predicate blob, :image?
diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb
index d1596ae630..b0a39ae3af 100644
--- a/activestorage/test/test_helper.rb
+++ b/activestorage/test/test_helper.rb
@@ -63,6 +63,10 @@ class ActiveSupport::TestCase
       ActiveStorage::Blob.create_before_direct_upload! key: key, filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, record: record
     end
 
+    def build_blob_after_unfurling(key: nil, data: "Hello world!", filename: "hello.txt", content_type: "text/plain", identify: true, record: nil)
+      ActiveStorage::Blob.build_after_unfurling key: key, io: StringIO.new(data), filename: filename, content_type: content_type, identify: identify, record: record
+    end
+
     def directly_upload_file_blob(filename: "racecar.jpg", content_type: "image/jpeg", record: nil)
       file = file_fixture(filename)
       byte_size = file.size