From c3758a71af949db849d5b7f176677653e4e4fae9 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Fri, 5 Nov 2021 03:13:15 +0100 Subject: [PATCH] Raise `ActionController::Redirecting::UnsafeRedirectError` for unsafe `redirect_to` redirects. This allows `rescue_from` to be used to add a default fallback route: ```ruby rescue_from ActionController::Redirecting::UnsafeRedirectError do redirect_to root_url end ``` Co-Authored-By: Chris Oliver --- actionpack/CHANGELOG.md | 12 ++++++++++++ .../lib/action_controller/metal/redirecting.rb | 6 +++++- actionpack/test/controller/redirect_test.rb | 4 ++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index cd3ebb3fca..6cce02a2a6 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,15 @@ +* Raise `ActionController::Redirecting::UnsafeRedirectError` for unsafe `redirect_to` redirects. + + This allows `rescue_from` to be used to add a default fallback route: + + ```ruby + rescue_from ActionController::Redirecting::UnsafeRedirectError do + redirect_to root_url + end + ``` + + *Kasper Timm Hansen*, *Chris Oliver* + * Add `url_from` to verify a redirect location is internal. Takes the open redirect protection from `redirect_to` so users can wrap a diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index a6490b6bb3..322c1d8a8b 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -7,6 +7,8 @@ module ActionController include AbstractController::Logger include ActionController::UrlFor + class UnsafeRedirectError < StandardError; end + included do mattr_accessor :raise_on_open_redirects, default: false end @@ -70,6 +72,8 @@ module ActionController # # redirect_to params[:redirect_url] # + # Raises ActionController::Redirecting::UnsafeRedirectError in the case of an unsafe redirect. + # # To allow any external redirects pass `allow_other_host: true`, though using a user-provided param in that case is unsafe. # # redirect_to "https://rubyonrails.org", allow_other_host: true @@ -186,7 +190,7 @@ module ActionController if allow_other_host || _url_host_allowed?(location) location else - raise ArgumentError, "Unsafe redirect to #{location.truncate(100).inspect}, pass allow_other_host: true to redirect anyway." + raise UnsafeRedirectError, "Unsafe redirect to #{location.truncate(100).inspect}, pass allow_other_host: true to redirect anyway." end end diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 96da10a8d6..600057bc7e 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -482,7 +482,7 @@ class RedirectTest < ActionController::TestCase def test_unsafe_redirect with_raise_on_open_redirects do - error = assert_raise(ArgumentError) do + error = assert_raise(ActionController::Redirecting::UnsafeRedirectError) do get :unsafe_redirect end @@ -492,7 +492,7 @@ class RedirectTest < ActionController::TestCase def test_unsafe_redirect_back with_raise_on_open_redirects do - error = assert_raise(ArgumentError) do + error = assert_raise(ActionController::Redirecting::UnsafeRedirectError) do get :unsafe_redirect_back end