TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Extract open redirect protection to separate method

Browse Source 
Let's us separate what's location generation and what's protection: fewer
arguments, avoids overloading safe, and polluting response_options.

The exception message has been clarified a bit too.
This commit is contained in:
Kasper Timm Hansen 2021-11-04 03:28:02 +01:00
parent 57fe7dfce9
commit 922729eb48
2 changed files with 12 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
actionpack/lib/action_controller/metal/redirecting.rb
View File

@ -63,14 +63,12 @@ module Redirecting
#
# Passing user input directly into +redirect_to+ is considered dangerous (e.g. `redirect_to(params[:location])`).
# Always use regular expressions or a permitted list when redirecting to a user specified location.
def redirect_to(options = {}, response_options = {})
response_options[:allow_other_host] ||= _allow_other_host unless response_options.key?(:allow_other_host)
def redirect_to(options = {}, response_options = {}, allow_other_host: _allow_other_host)
raise ActionControllerError.new("Cannot redirect to nil!") unless options
raise AbstractController::DoubleRenderError if response_body
self.status = _extract_redirect_to_status(options, response_options)
self.location = _compute_safe_redirect_to_location(request, options, response_options)
self.location = _enforce_open_redirect_protection(_compute_redirect_to_location(request, options), allow_other_host: allow_other_host)
self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>"
end
@ -110,19 +108,6 @@ def redirect_back_or_to(fallback_location, allow_other_host: _allow_other_host,
redirect_to location, allow_other_host: allow_other_host, **options
end
def _compute_safe_redirect_to_location(request, options, response_options)
location = _compute_redirect_to_location(request, options)
if response_options[:allow_other_host] || _url_host_allowed?(location)
location
else
raise(ArgumentError, <<~MSG.squish)
Unsafe redirect #{location.truncate(100).inspect},
use :allow_other_host to redirect anyway.
MSG
end
end
def _compute_redirect_to_location(request, options) # :nodoc:
case options
# The scheme name consist of a letter followed by any combination of
@ -158,6 +143,14 @@ def _extract_redirect_to_status(options, response_options)
end
end
def _enforce_open_redirect_protection(location, allow_other_host:)
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."
end
end
def _url_host_allowed?(url)
URI(url.to_s).host == request.host
rescue ArgumentError, URI::Error

10
actionpack/test/controller/redirect_test.rb
View File

@ -482,10 +482,7 @@ def test_unsafe_redirect
get :unsafe_redirect
end
assert_equal(<<~MSG.squish, error.message)
Unsafe redirect \"http://www.rubyonrails.org/\",
use :allow_other_host to redirect anyway.
MSG
assert_equal "Unsafe redirect to \"http://www.rubyonrails.org/\", pass allow_other_host: true to redirect anyway.", error.message
end
end
@ -495,10 +492,7 @@ def test_unsafe_redirect_back
get :unsafe_redirect_back
end
assert_equal(<<~MSG.squish, error.message)
Unsafe redirect \"http://www.rubyonrails.org/\",
use :allow_other_host to redirect anyway.
MSG
assert_equal "Unsafe redirect to \"http://www.rubyonrails.org/\", pass allow_other_host: true to redirect anyway.", error.message
end
end