Skip to content

Commit

Permalink
Allow rescuing ActionController::Redirecting::UnsafeRedirectError i…
Browse files Browse the repository at this point in the history
…n 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 rails#53464, but it's not dependent on that issue.
  • Loading branch information
ghiculescu committed Nov 4, 2024
1 parent 1ecc84a commit d427e1e
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 1 deletion.
3 changes: 2 additions & 1 deletion actionpack/lib/action_controller/metal/redirecting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
13 changes: 13 additions & 0 deletions actionpack/test/controller/redirect_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit d427e1e

Please sign in to comment.