11 files changed, 107 insertions, 33 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/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index fdf17ac67e..3b48ee72af 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -172,11 +172,11 @@ class ActiveStorage::Blob < ActiveRecord::Base
   end
 
 
-  # Returns an ActiveStorage::Preview instance for a previewable blob or an ActiveStorage::Variant instance for an image blob.
+  # Returns an ActiveStorage::Preview for a previewable blob or an ActiveStorage::Variant for a variable image blob.
   #
   #   blob.representation(resize: "100x100").processed.service_url
   #
-  # Raises ActiveStorage::Blob::UnrepresentableError if the receiving blob is neither an image nor previewable. Call
+  # Raises ActiveStorage::Blob::UnrepresentableError if the receiving blob is neither variable nor previewable. Call
   # ActiveStorage::Blob#representable? to determine whether a blob is representable.
   #
   # See ActiveStorage::Blob#preview and ActiveStorage::Blob#variant for more information.
@@ -191,7 +191,7 @@ class ActiveStorage::Blob < ActiveRecord::Base
     end
   end
 
-  # Returns true if the blob is variable or is previewable.
+  # Returns true if the blob is variable or previewable.
   def representable?
     variable? || previewable?
   end
diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb
index fa5aa69bd3..8909fd628a 100644
--- a/activestorage/app/models/active_storage/variant.rb
+++ b/activestorage/app/models/active_storage/variant.rb
@@ -35,6 +35,8 @@
 #
 #   avatar.variant(resize: "100x100", monochrome: true, flip: "-90")
 class ActiveStorage::Variant
+  WEB_IMAGE_CONTENT_TYPES = %w( image/png image/jpeg image/jpg image/gif )
+
   attr_reader :blob, :variation
   delegate :service, to: :blob
 
@@ -62,7 +64,7 @@ class ActiveStorage::Variant
   # for a variant that points to the ActiveStorage::VariantsController, which in turn will use this +service_call+ method
   # for its redirection.
   def service_url(expires_in: service.url_expires_in, disposition: :inline)
-    service.url key, expires_in: expires_in, disposition: disposition, filename: blob.filename, content_type: blob.content_type
+    service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type
   end
 
   # Returns the receiving variant. Allows ActiveStorage::Variant and ActiveStorage::Preview instances to be used interchangeably.
@@ -76,11 +78,40 @@ class ActiveStorage::Variant
     end
 
     def process
-      service.upload key, transform(service.download(blob.key))
+      service.upload key, transform(blob.download)
+    end
+
+
+    def filename
+      if WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
+        blob.filename
+      else
+        ActiveStorage::Filename.new("#{blob.filename.base}.png")
+      end
     end
 
+    def content_type
+      blob.content_type.presence_in(WEB_IMAGE_CONTENT_TYPES) || "image/png"
+    end
+
+
     def transform(io)
+      read_image_from(io) do |image|
+        mogrify image
+        format image
+      end
+    end
+
+    def read_image_from(io, &block)
       require "mini_magick"
-      File.open MiniMagick::Image.read(io).tap { |image| variation.transform(image) }.path
+      File.open MiniMagick::Image.read(io).tap(&block).path
+    end
+
+    def mogrify(image)
+      variation.transform(image)
+    end
+
+    def format(image)
+      image.format("PNG") unless WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
     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
diff --git a/activestorage/test/fixtures/files/icon.psd b/activestorage/test/fixtures/files/icon.psd
new file mode 100644
index 0000000000..631fceeaab
--- /dev/null
+++ b/activestorage/test/fixtures/files/icon.psd
diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb
index 83ebce4446..8eced41ee0 100644
--- a/activestorage/test/models/variant_test.rb
+++ b/activestorage/test/models/variant_test.rb
@@ -4,12 +4,9 @@ require "test_helper"
 require "database/setup"
 
 class ActiveStorage::VariantTest < ActiveSupport::TestCase
-  setup do
-    @blob = create_file_blob filename: "racecar.jpg"
-  end
-
-  test "resized variation" do
-    variant = @blob.variant(resize: "100x100").processed
+  test "resized variation of JPEG blob" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = blob.variant(resize: "100x100").processed
     assert_match(/racecar\.jpg/, variant.service_url)
 
     image = read_image(variant)
@@ -17,8 +14,9 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
     assert_equal 67, image.height
   end
 
-  test "resized and monochrome variation" do
-    variant = @blob.variant(resize: "100x100", monochrome: true).processed
+  test "resized and monochrome variation of JPEG blob" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = blob.variant(resize: "100x100", monochrome: true).processed
     assert_match(/racecar\.jpg/, variant.service_url)
 
     image = read_image(variant)
@@ -27,6 +25,17 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
     assert_match(/Gray/, image.colorspace)
   end
 
+  test "resized variation of PSD blob" do
+    blob = create_file_blob(filename: "icon.psd", content_type: "image/vnd.adobe.photoshop")
+    variant = blob.variant(resize: "20x20").processed
+    assert_match(/icon\.png/, variant.service_url)
+
+    image = read_image(variant)
+    assert_equal "PNG", image.type
+    assert_equal 20, image.width
+    assert_equal 20, image.height
+  end
+
   test "variation of invariable blob" do
     assert_raises ActiveStorage::Blob::InvariableError do
       create_file_blob(filename: "report.pdf", content_type: "application/pdf").variant(resize: "100x100")
@@ -34,7 +43,8 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   end
 
   test "service_url doesn't grow in length despite long variant options" do
-    variant = @blob.variant(font: "a" * 10_000).processed
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = blob.variant(font: "a" * 10_000).processed
     assert_operator variant.service_url.length, :<, 500
   end
 end