diff options
Diffstat (limited to 'actionpack')
-rw-r--r-- | actionpack/CHANGELOG.md | 6 | ||||
-rw-r--r-- | actionpack/lib/action_controller/metal/force_ssl.rb | 3 | ||||
-rw-r--r-- | actionpack/lib/action_controller/metal/redirecting.rb | 33 | ||||
-rw-r--r-- | actionpack/test/controller/action_pack_assertions_test.rb | 6 | ||||
-rw-r--r-- | actionpack/test/controller/log_subscriber_test.rb | 4 | ||||
-rw-r--r-- | actionpack/test/controller/redirect_test.rb | 44 | ||||
-rw-r--r-- | actionpack/test/dispatch/debug_exceptions_test.rb | 42 |
7 files changed, 39 insertions, 99 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index c29257f83d..1d2f6b09c3 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -11,12 +11,6 @@ *Rafael Mendonça França* -* Ensure external redirects are explicitly allowed - - Add `fallback_location` and `allow_other_host` options to `redirect_to`. - - *Gannon McGibbon* - * Introduce ActionDispatch::HostAuthorization This is a new middleware that guards against DNS rebinding attacks by diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb index 205f84ae36..93fd57b640 100644 --- a/actionpack/lib/action_controller/metal/force_ssl.rb +++ b/actionpack/lib/action_controller/metal/force_ssl.rb @@ -13,7 +13,7 @@ module ActionController ACTION_OPTIONS = [:only, :except, :if, :unless] URL_OPTIONS = [:protocol, :host, :domain, :subdomain, :port, :path] - REDIRECT_OPTIONS = [:status, :flash, :alert, :notice, :allow_other_host] + REDIRECT_OPTIONS = [:status, :flash, :alert, :notice] module ClassMethods # :nodoc: def force_ssl(options = {}) @@ -41,7 +41,6 @@ module ActionController host: request.host, path: request.fullpath, status: :moved_permanently, - allow_other_host: true, } if host_or_options.is_a?(Hash) diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index 8bd003f5ed..67c198d150 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -60,7 +60,7 @@ module ActionController 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 = _compute_redirect_to_location(request, options) self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>" end @@ -88,13 +88,9 @@ module ActionController # All other options that can be passed to <tt>redirect_to</tt> are accepted as # options and the behavior is identical. def redirect_back(fallback_location:, allow_other_host: true, **args) - referer = request.headers.fetch("Referer", fallback_location) - response_options = { - fallback_location: fallback_location, - allow_other_host: allow_other_host, - **args, - } - redirect_to referer, response_options + referer = request.headers["Referer"] + redirect_to_referer = referer && (allow_other_host || _url_host_allowed?(referer)) + redirect_to redirect_to_referer ? referer : fallback_location, **args end def _compute_redirect_to_location(request, options) #:nodoc: @@ -118,23 +114,6 @@ module ActionController public :_compute_redirect_to_location private - def _compute_safe_redirect_to_location(request, options, response_options) - location = _compute_redirect_to_location(request, options) - location_options = options.is_a?(Hash) ? options : {} - if response_options[:allow_other_host] || _url_host_allowed?(location, location_options) - location - else - fallback_location = response_options.fetch(:fallback_location) do - raise ArgumentError, <<~MSG.squish - Unsafe redirect #{location.inspect}, - use :fallback_location to specify a fallback - or :allow_other_host to redirect anyway. - MSG - end - _compute_redirect_to_location(request, fallback_location) - end - end - def _extract_redirect_to_status(options, response_options) if options.is_a?(Hash) && options.key?(:status) Rack::Utils.status_code(options.delete(:status)) @@ -145,8 +124,8 @@ module ActionController end end - def _url_host_allowed?(url, options = {}) - URI(url.to_s).host.in?([request.host, options[:host]]) + def _url_host_allowed?(url) + URI(url.to_s).host == request.host rescue ArgumentError, URI::Error false end diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb index c7aae034dd..ecb8c37e6b 100644 --- a/actionpack/test/controller/action_pack_assertions_test.rb +++ b/actionpack/test/controller/action_pack_assertions_test.rb @@ -28,13 +28,13 @@ class ActionPackAssertionsController < ActionController::Base def redirect_to_path() redirect_to "/some/path" end - def redirect_invalid_external_route() redirect_to "ht_tp://www.rubyonrails.org", allow_other_host: true end + def redirect_invalid_external_route() redirect_to "ht_tp://www.rubyonrails.org" end def redirect_to_named_route() redirect_to route_one_url end - def redirect_external() redirect_to "http://www.rubyonrails.org", allow_other_host: true; end + def redirect_external() redirect_to "http://www.rubyonrails.org"; end - def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org", allow_other_host: true; end + def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org"; end def response404() head "404 AWOL" end diff --git a/actionpack/test/controller/log_subscriber_test.rb b/actionpack/test/controller/log_subscriber_test.rb index cbebc6b59c..0562c16284 100644 --- a/actionpack/test/controller/log_subscriber_test.rb +++ b/actionpack/test/controller/log_subscriber_test.rb @@ -25,11 +25,11 @@ module Another end def redirector - redirect_to "http://foo.bar/", allow_other_host: true + redirect_to "http://foo.bar/" end def filterable_redirector - redirect_to "http://secret.foo.bar/", allow_other_host: true + redirect_to "http://secret.foo.bar/" end def data_sender diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 945d2275c0..998498e1b2 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -49,11 +49,11 @@ class RedirectController < ActionController::Base end def url_redirect_with_status - redirect_to("http://www.example.com", status: :moved_permanently, allow_other_host: true) + redirect_to("http://www.example.com", status: :moved_permanently) end def url_redirect_with_status_hash - redirect_to("http://www.example.com", status: 301, allow_other_host: true) + redirect_to("http://www.example.com", status: 301) end def relative_url_redirect_with_status @@ -81,27 +81,19 @@ class RedirectController < ActionController::Base end def redirect_to_url - redirect_to "http://www.rubyonrails.org/", allow_other_host: true - end - - def redirect_to_unsafe_url redirect_to "http://www.rubyonrails.org/" end - def redirect_to_relative_unsafe_url - redirect_to ".br" - end - def redirect_to_url_with_unescaped_query_string - redirect_to "http://example.com/query?status=new", allow_other_host: true + redirect_to "http://example.com/query?status=new" end def redirect_to_url_with_complex_scheme - redirect_to "x-test+scheme.complex:redirect", allow_other_host: true + redirect_to "x-test+scheme.complex:redirect" end def redirect_to_url_with_network_path_reference - redirect_to "//www.rubyonrails.org/", allow_other_host: true + redirect_to "//www.rubyonrails.org/" end def redirect_to_existing_record @@ -121,12 +113,12 @@ class RedirectController < ActionController::Base end def redirect_to_with_block - redirect_to proc { "http://www.rubyonrails.org/" }, allow_other_host: true + redirect_to proc { "http://www.rubyonrails.org/" } end def redirect_to_with_block_and_assigns @url = "http://www.rubyonrails.org/" - redirect_to proc { @url }, allow_other_host: true + redirect_to proc { @url } end def redirect_to_with_block_and_options @@ -253,28 +245,6 @@ class RedirectTest < ActionController::TestCase assert_redirected_to "http://www.rubyonrails.org/" end - def test_redirect_to_unsafe_url - error = assert_raises(ArgumentError) do - get :redirect_to_unsafe_url - end - assert_equal <<~MSG.squish, error.message - Unsafe redirect \"http://www.rubyonrails.org/\", - use :fallback_location to specify a fallback or - :allow_other_host to redirect anyway. - MSG - end - - def test_redirect_to_relative_unsafe_url - error = assert_raises(ArgumentError) do - get :redirect_to_relative_unsafe_url - end - assert_equal <<~MSG.squish, error.message - Unsafe redirect \"http://test.host.br\", - use :fallback_location to specify a fallback or - :allow_other_host to redirect anyway. - MSG - end - def test_redirect_to_url_with_unescaped_query_string get :redirect_to_url_with_unescaped_query_string assert_response :redirect diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index aadc6be077..214e063276 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -39,52 +39,50 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest def call(env) env["action_dispatch.show_detailed_exceptions"] = @detailed req = ActionDispatch::Request.new(env) + template = ActionView::Template.new(File.read(__FILE__), __FILE__, ActionView::Template::Handlers::Raw.new, {}) + case req.path - when %r{/pass} + when "/pass" [404, { "X-Cascade" => "pass" }, self] - when %r{/not_found} + when "/not_found" raise AbstractController::ActionNotFound - when %r{/runtime_error} + when "/runtime_error" raise RuntimeError - when %r{/method_not_allowed} + when "/method_not_allowed" raise ActionController::MethodNotAllowed - when %r{/intercepted_error} + when "/intercepted_error" raise InterceptedErrorInstance - when %r{/unknown_http_method} + when "/unknown_http_method" raise ActionController::UnknownHttpMethod - when %r{/not_implemented} + when "/not_implemented" raise ActionController::NotImplemented - when %r{/unprocessable_entity} + when "/unprocessable_entity" raise ActionController::InvalidAuthenticityToken - when %r{/not_found_original_exception} + when "/not_found_original_exception" begin raise AbstractController::ActionNotFound.new rescue - raise ActionView::Template::Error.new("template") + raise ActionView::Template::Error.new(template) end - when %r{/missing_template} + when "/missing_template" raise ActionView::MissingTemplate.new(%w(foo), "foo/index", %w(foo), false, "mailer") - when %r{/bad_request} + when "/bad_request" raise ActionController::BadRequest - when %r{/missing_keys} + when "/missing_keys" raise ActionController::UrlGenerationError, "No route matches" - when %r{/parameter_missing} + when "/parameter_missing" raise ActionController::ParameterMissing, :missing_param_key - when %r{/original_syntax_error} + when "/original_syntax_error" eval "broke_syntax =" # `eval` need for raise native SyntaxError at runtime - when %r{/syntax_error_into_view} + when "/syntax_error_into_view" begin eval "broke_syntax =" rescue Exception - template = ActionView::Template.new(File.read(__FILE__), - __FILE__, - ActionView::Template::Handlers::Raw.new, - {}) raise ActionView::Template::Error.new(template) end - when %r{/framework_raises} + when "/framework_raises" method_that_raises - when %r{/nested_exceptions} + when "/nested_exceptions" raise_nested_exceptions else raise "puke!" |