From d427e1e00266b0432282b0e4f93068ac04a642cc Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 4 Nov 2024 10:58:12 +1000 Subject: [PATCH] Allow rescuing `ActionController::Redirecting::UnsafeRedirectError` in controllers Consider a controller that does this: ```ruby begin redirect_to "http://www.rubyonrails.org/", allow_other_host: false rescue ActionController::Redirecting::UnsafeRedirectError render plain: "caught error" end ``` The `redirect_to` will raise and the `rescue` will execute. But currently, the response status will still be changed (to 302). So even if you render something, we will return to the browser a 302 response code, with no response location. This is not a valid response. This PR fixes this, by only setting the status once the location has been verified. Note: I came across this issue while trying to work around https://github.com/rails/rails/issues/53464, but it's not dependent on that issue. --- .../lib/action_controller/metal/redirecting.rb | 3 ++- actionpack/test/controller/redirect_test.rb | 13 +++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index 952aca5bb0293..68241cdbc0ec8 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -106,13 +106,14 @@ def redirect_to(options = {}, response_options = {}) allow_other_host = response_options.delete(:allow_other_host) { _allow_other_host } - self.status = _extract_redirect_to_status(options, response_options) + proposed_status = _extract_redirect_to_status(options, response_options) redirect_to_location = _compute_redirect_to_location(request, options) _ensure_url_is_http_header_safe(redirect_to_location) self.location = _enforce_open_redirect_protection(redirect_to_location, allow_other_host: allow_other_host) self.response_body = "" + self.status = proposed_status end # Soft deprecated alias for #redirect_back_or_to where the `fallback_location` diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 9f4a49cd16058..8c3959e223ad8 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -203,6 +203,14 @@ def redirect_with_null_bytes redirect_to "\000/lol\r\nwat" end + def redirect_to_external_with_rescue + begin + redirect_to "http://www.rubyonrails.org/", allow_other_host: false + rescue ActionController::Redirecting::UnsafeRedirectError + render plain: "caught error" + end + end + def rescue_errors(e) raise e end private @@ -617,6 +625,11 @@ def test_redirect_to_instrumentation assert_equal "http://test.host/redirect/hello_world", payload[:location] end + def test_redirect_to_external_with_rescue + get :redirect_to_external_with_rescue + assert_response :ok + end + private def with_raise_on_open_redirects old_raise_on_open_redirects = ActionController::Base.raise_on_open_redirects