Handle invalid signed blob IDs gracefully

7 files changed, 51 insertions, 18 deletions

diff --git a/activestorage/app/controllers/active_storage/blobs_controller.rb b/activestorage/app/controllers/active_storage/blobs_controller.rb
index a17e3852f9..fa44131048 100644
--- a/activestorage/app/controllers/active_storage/blobs_controller.rb
+++ b/activestorage/app/controllers/active_storage/blobs_controller.rb
@@ -5,12 +5,10 @@
 # security-through-obscurity factor of the signed blob references, you'll need to implement your own
 # authenticated redirection controller.
 class ActiveStorage::BlobsController < ActionController::Base
+  include ActiveStorage::SetBlob
+
   def show
-    if blob = ActiveStorage::Blob.find_signed(params[:signed_id])
-      expires_in ActiveStorage::Blob.service.url_expires_in
-      redirect_to blob.service_url(disposition: params[:disposition])
-    else
-      head :not_found
-    end
+    expires_in ActiveStorage::Blob.service.url_expires_in
+    redirect_to @blob.service_url(disposition: params[:disposition])
   end
 end
diff --git a/activestorage/app/controllers/active_storage/previews_controller.rb b/activestorage/app/controllers/active_storage/previews_controller.rb
index 9e8cf27b6e..aa7ef58ca4 100644
--- a/activestorage/app/controllers/active_storage/previews_controller.rb
+++ b/activestorage/app/controllers/active_storage/previews_controller.rb
@@ -1,12 +1,10 @@
 # frozen_string_literal: true
 
 class ActiveStorage::PreviewsController < ActionController::Base
+  include ActiveStorage::SetBlob
+
   def show
-    if blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id])
-      expires_in ActiveStorage::Blob.service.url_expires_in
-      redirect_to ActiveStorage::Preview.new(blob, params[:variation_key]).processed.service_url(disposition: params[:disposition])
-    else
-      head :not_found
-    end
+    expires_in ActiveStorage::Blob.service.url_expires_in
+    redirect_to ActiveStorage::Preview.new(@blob, params[:variation_key]).processed.service_url(disposition: params[:disposition])
   end
 end
diff --git a/activestorage/app/controllers/active_storage/variants_controller.rb b/activestorage/app/controllers/active_storage/variants_controller.rb
index dc5e78ecc0..e8f8dd592d 100644
--- a/activestorage/app/controllers/active_storage/variants_controller.rb
+++ b/activestorage/app/controllers/active_storage/variants_controller.rb
@@ -5,12 +5,10 @@
 # security-through-obscurity factor of the signed blob and variation reference, you'll need to implement your own
 # authenticated redirection controller.
 class ActiveStorage::VariantsController < ActionController::Base
+  include ActiveStorage::SetBlob
+
   def show
-    if blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id])
-      expires_in ActiveStorage::Blob.service.url_expires_in
-      redirect_to ActiveStorage::Variant.new(blob, params[:variation_key]).processed.service_url(disposition: params[:disposition])
-    else
-      head :not_found
-    end
+    expires_in ActiveStorage::Blob.service.url_expires_in
+    redirect_to ActiveStorage::Variant.new(@blob, params[:variation_key]).processed.service_url(disposition: params[:disposition])
   end
 end
diff --git a/activestorage/app/controllers/concerns/active_storage/set_blob.rb b/activestorage/app/controllers/concerns/active_storage/set_blob.rb
new file mode 100644
index 0000000000..b0f3d97a66
--- /dev/null
+++ b/activestorage/app/controllers/concerns/active_storage/set_blob.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module ActiveStorage::SetBlob
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :set_blob
+  end
+
+  private
+    def set_blob
+      @blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id] || params[:signed_id])
+    rescue ActiveSupport::MessageVerifier::InvalidSignature
+      head :not_found
+    end
+end
diff --git a/activestorage/test/controllers/blobs_controller_test.rb b/activestorage/test/controllers/blobs_controller_test.rb
index 97177e64c2..9c811df895 100644
--- a/activestorage/test/controllers/blobs_controller_test.rb
+++ b/activestorage/test/controllers/blobs_controller_test.rb
@@ -8,6 +8,11 @@ class ActiveStorage::BlobsControllerTest < ActionDispatch::IntegrationTest
     @blob = create_file_blob filename: "racecar.jpg"
   end
 
+  test "showing blob with invalid signed ID" do
+    get rails_service_blob_url("invalid", "racecar.jpg")
+    assert_response :not_found
+  end
+
   test "showing blob utilizes browser caching" do
     get rails_blob_url(@blob)
 
diff --git a/activestorage/test/controllers/previews_controller_test.rb b/activestorage/test/controllers/previews_controller_test.rb
index c3151a710e..704a466160 100644
--- a/activestorage/test/controllers/previews_controller_test.rb
+++ b/activestorage/test/controllers/previews_controller_test.rb
@@ -21,4 +21,13 @@ class ActiveStorage::PreviewsControllerTest < ActionDispatch::IntegrationTest
     assert_equal 77, image.width
     assert_equal 100, image.height
   end
+
+  test "showing preview with invalid signed blob ID" do
+    get rails_blob_preview_url(
+      filename: @blob.filename,
+      signed_blob_id: "invalid",
+      variation_key: ActiveStorage::Variation.encode(resize: "100x100"))
+
+    assert_response :not_found
+  end
 end
diff --git a/activestorage/test/controllers/variants_controller_test.rb b/activestorage/test/controllers/variants_controller_test.rb
index 6c70d73786..a0642f9bed 100644
--- a/activestorage/test/controllers/variants_controller_test.rb
+++ b/activestorage/test/controllers/variants_controller_test.rb
@@ -20,4 +20,13 @@ class ActiveStorage::VariantsControllerTest < ActionDispatch::IntegrationTest
     assert_equal 100, image.width
     assert_equal 67, image.height
   end
+
+  test "showing variant with invalid signed blob ID" do
+    get rails_blob_variation_url(
+      filename: @blob.filename,
+      signed_blob_id: "invalid",
+      variation_key: ActiveStorage::Variation.encode(resize: "100x100"))
+
+    assert_response :not_found
+  end
 end