Merge pull request #42960 from FestaLab/activestorage/unsafe-redirect

Fix open redirects in active storage

activestorage/app/controllers/active_storage/blobs/redirect_controller.rb

@@ -11,6 +11,6 @@ class ActiveStorage::Blobs::RedirectController < ActiveStorage::BaseController
 
   def show
     expires_in ActiveStorage.service_urls_expire_in
-    redirect_to @blob.url(disposition: params[:disposition])
+    redirect_to @blob.url(disposition: params[:disposition]), allow_other_host: true
   end
 end

activestorage/app/controllers/active_storage/representations/redirect_controller.rb

@@ -9,6 +9,6 @@
 class ActiveStorage::Representations::RedirectController < ActiveStorage::Representations::BaseController
   def show
     expires_in ActiveStorage.service_urls_expire_in
-    redirect_to @representation.url(disposition: params[:disposition])
+    redirect_to @representation.url(disposition: params[:disposition]), allow_other_host: true
   end
 end

activestorage/test/controllers/blobs/redirect_controller_test.rb

@@ -55,3 +55,38 @@ class ActiveStorage::Blobs::ExpiringRedirectControllerTest < ActionDispatch::Int
     assert_response :not_found
   end
 end
+
+class ActiveStorage::Blobs::RedirectControllerWithOpenRedirectTest < ActionDispatch::IntegrationTest
+  if SERVICE_CONFIGURATIONS[:s3]
+    test "showing existing blob stored in s3" do
+      with_raise_on_open_redirects(:s3) do
+        blob = create_file_blob filename: "racecar.jpg", service_name: :s3
+
+        get rails_storage_redirect_url(blob)
+        assert_redirected_to(/racecar\.jpg/)
+      end
+    end
+  end
+
+  if SERVICE_CONFIGURATIONS[:azure]
+    test "showing existing blob stored in azure" do
+      with_raise_on_open_redirects(:azure) do
+        blob = create_file_blob filename: "racecar.jpg", service_name: :azure
+
+        get rails_storage_redirect_url(blob)
+        assert_redirected_to(/racecar\.jpg/)
+      end
+    end
+  end
+
+  if SERVICE_CONFIGURATIONS[:gcs]
+    test "showing existing blob stored in gcs" do
+      with_raise_on_open_redirects(:gcs) do
+        blob = create_file_blob filename: "racecar.jpg", service_name: :gcs
+
+        get rails_storage_redirect_url(blob)
+        assert_redirected_to(/racecar\.jpg/)
+      end
+    end
+  end
+end

activestorage/test/controllers/representations/redirect_controller_test.rb

@@ -132,3 +132,50 @@ class ActiveStorage::Representations::RedirectControllerWithPreviewsWithStrictLo
     assert_equal 100, image.height
   end
 end
+
+class ActiveStorage::Representations::RedirectControllerWithOpenRedirectTest < ActionDispatch::IntegrationTest
+  if SERVICE_CONFIGURATIONS[:s3]
+    test "showing existing variant stored in s3" do
+      with_raise_on_open_redirects(:s3) do
+        blob = create_file_blob filename: "racecar.jpg", service_name: :s3
+
+        get rails_blob_representation_url(
+          filename: blob.filename,
+          signed_blob_id: blob.signed_id,
+          variation_key: ActiveStorage::Variation.encode(resize_to_limit: [100, 100]))
+
+        assert_redirected_to(/racecar\.jpg/)
+      end
+    end
+  end
+
+  if SERVICE_CONFIGURATIONS[:azure]
+    test "showing existing variant stored in azure" do
+      with_raise_on_open_redirects(:azure) do
+        blob = create_file_blob filename: "racecar.jpg", service_name: :azure
+
+        get rails_blob_representation_url(
+          filename: blob.filename,
+          signed_blob_id: blob.signed_id,
+          variation_key: ActiveStorage::Variation.encode(resize_to_limit: [100, 100]))
+
+        assert_redirected_to(/racecar\.jpg/)
+      end
+    end
+  end
+
+  if SERVICE_CONFIGURATIONS[:gcs]
+    test "showing existing variant stored in gcs" do
+      with_raise_on_open_redirects(:gcs) do
+        blob = create_file_blob filename: "racecar.jpg", service_name: :gcs
+
+        get rails_blob_representation_url(
+          filename: blob.filename,
+          signed_blob_id: blob.signed_id,
+          variation_key: ActiveStorage::Variation.encode(resize_to_limit: [100, 100]))
+
+        assert_redirected_to(/racecar\.jpg/)
+      end
+    end
+  end
+end

activestorage/test/test_helper.rb

@@ -139,6 +139,18 @@ class ActiveSupport::TestCase
       yield
       ActiveStorage.track_variants = variant_tracking_was
     end
+
+    def with_raise_on_open_redirects(service)
+      old_raise_on_open_redirects = ActionController::Base.raise_on_open_redirects
+      old_service = ActiveStorage::Blob.service
+
+      ActionController::Base.raise_on_open_redirects = true
+      ActiveStorage::Blob.service = ActiveStorage::Service.configure(service, SERVICE_CONFIGURATIONS)
+      yield
+    ensure
+      ActionController::Base.raise_on_open_redirects = old_raise_on_open_redirects
+      ActiveStorage::Blob.service = old_service
+    end
 end
 
 require "global_id"