From 77f2af34f91b04fa8e97b49d6f3a197c2a09c754 Mon Sep 17 00:00:00 2001 From: Breno Gazzola Date: Fri, 6 Aug 2021 19:14:19 -0300 Subject: [PATCH] Fix open redirects in active storage --- .../blobs/redirect_controller.rb | 2 +- .../representations/redirect_controller.rb | 2 +- .../blobs/redirect_controller_test.rb | 35 ++++++++++++++ .../redirect_controller_test.rb | 47 +++++++++++++++++++ activestorage/test/test_helper.rb | 12 +++++ 5 files changed, 96 insertions(+), 2 deletions(-) diff --git a/activestorage/app/controllers/active_storage/blobs/redirect_controller.rb b/activestorage/app/controllers/active_storage/blobs/redirect_controller.rb index 11b6418e27..8858249030 100644 --- a/activestorage/app/controllers/active_storage/blobs/redirect_controller.rb +++ b/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 diff --git a/activestorage/app/controllers/active_storage/representations/redirect_controller.rb b/activestorage/app/controllers/active_storage/representations/redirect_controller.rb index a07d52ab3b..1b70182a17 100644 --- a/activestorage/app/controllers/active_storage/representations/redirect_controller.rb +++ b/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 diff --git a/activestorage/test/controllers/blobs/redirect_controller_test.rb b/activestorage/test/controllers/blobs/redirect_controller_test.rb index f7709b1c2b..339da812b8 100644 --- a/activestorage/test/controllers/blobs/redirect_controller_test.rb +++ b/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 diff --git a/activestorage/test/controllers/representations/redirect_controller_test.rb b/activestorage/test/controllers/representations/redirect_controller_test.rb index f5bbf65393..61c91a5186 100644 --- a/activestorage/test/controllers/representations/redirect_controller_test.rb +++ b/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 diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 4b9651b4d4..0847d9becb 100644 --- a/activestorage/test/test_helper.rb +++ b/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"