diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 0ec8b5843e..d7426ba90e 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,10 @@ +* Raise error on unpermitted open redirects. + + Add `allow_other_host` options to `redirect_to`. + Opt in to this behaviour with `ActionController::Base.raise_on_open_redirects = true`. + + *Gannon McGibbon* + * Deprecate `poltergeist` and `webkit` (capybara-webkit) driver registration for system testing (they will be removed in Rails 7.1). Add `cuprite` instead. [Poltergeist](https://github.com/teampoltergeist/poltergeist) and [capybara-webkit](https://github.com/thoughtbot/capybara-webkit) are already not maintained. These usage in Rails are removed for avoiding confusing users. diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index 7670016d6f..04ea32d380 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -7,6 +7,10 @@ module ActionController include AbstractController::Logger include ActionController::UrlFor + included do + mattr_accessor :raise_on_open_redirects, default: false + end + # Redirects the browser to the target specified in +options+. This parameter can be any one of: # # * Hash - The URL will be generated by calling url_for with the +options+. @@ -60,17 +64,19 @@ module ActionController # 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) + 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_redirect_to_location(request, options) + self.location = _compute_safe_redirect_to_location(request, options, response_options) self.response_body = "You are being redirected." end # Soft deprecated alias for redirect_back_or_to where the fallback_location location is supplied as a keyword argument instead # of the first positional argument. - def redirect_back(fallback_location:, allow_other_host: true, **args) + def redirect_back(fallback_location:, allow_other_host: _allow_other_host, **args) redirect_back_or_to fallback_location, allow_other_host: allow_other_host, **args end @@ -96,10 +102,25 @@ module ActionController # # All other options that can be passed to #redirect_to are accepted as # options and the behavior is identical. - def redirect_back_or_to(fallback_location, allow_other_host: true, **args) - referer = request.headers["Referer"] - redirect_to_referer = referer && (allow_other_host || _url_host_allowed?(referer)) - redirect_to redirect_to_referer ? referer : fallback_location, **args + def redirect_back_or_to(fallback_location, allow_other_host: _allow_other_host, **options) + location = request.referer || fallback_location + location = fallback_location unless allow_other_host || _url_host_allowed?(request.referer) + allow_other_host = true if _allow_other_host && !allow_other_host # if the fallback is an open redirect + + 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: @@ -123,6 +144,10 @@ module ActionController public :_compute_redirect_to_location private + def _allow_other_host + !raise_on_open_redirects + end + def _extract_redirect_to_status(options, response_options) if options.is_a?(Hash) && options.key?(:status) Rack::Utils.status_code(options.delete(:status)) diff --git a/actionpack/lib/action_controller/railtie.rb b/actionpack/lib/action_controller/railtie.rb index 7d42f5d931..b382f5a385 100644 --- a/actionpack/lib/action_controller/railtie.rb +++ b/actionpack/lib/action_controller/railtie.rb @@ -10,6 +10,7 @@ require "action_view/railtie" module ActionController class Railtie < Rails::Railtie #:nodoc: config.action_controller = ActiveSupport::OrderedOptions.new + config.action_controller.raise_on_open_redirects = false config.eager_load_namespaces << ActionController diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index e1b31667a1..0f24f51042 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -80,6 +80,14 @@ class RedirectController < ActionController::Base redirect_back_or_to "http://www.rubyonrails.org/", status: 307, allow_other_host: false end + def unsafe_redirect + redirect_to "http://www.rubyonrails.org/" + end + + def unsafe_redirect_back + redirect_back_or_to "http://www.rubyonrails.org/" + end + def redirect_back_with_explicit_fallback_kwarg redirect_back(fallback_location: "/things/stuff", status: 307) end @@ -467,6 +475,41 @@ class RedirectTest < ActionController::TestCase assert_redirected_to "http://test.host/redirect/hello_world" end end + + def test_unsafe_redirect + with_raise_on_open_redirects do + error = assert_raise(ArgumentError) do + get :unsafe_redirect + end + + assert_equal(<<~MSG.squish, error.message) + Unsafe redirect \"http://www.rubyonrails.org/\", + use :allow_other_host to redirect anyway. + MSG + end + end + + def test_unsafe_redirect_back + with_raise_on_open_redirects do + error = assert_raise(ArgumentError) do + 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 + end + end + + private + def with_raise_on_open_redirects + old_raise_on_open_redirects = ActionController::Base.raise_on_open_redirects + ActionController::Base.raise_on_open_redirects = true + yield + ensure + ActionController::Base.raise_on_open_redirects = old_raise_on_open_redirects + end end module ModuleTest diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 26fb1538c6..8037b63112 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -571,6 +571,8 @@ The schema dumper adds two additional configuration options: Rendered recordings/threads/_thread.html.erb in 1.5 ms [cache miss] ``` +* `config.action_controller.raise_on_open_redirects` raises an `ArgumentError` when an unpermitted open redirect occurs. The default value is `false`. + ### Configuring Action Dispatch * `config.action_dispatch.session_store` sets the name of the store for session data. The default is `:cookie_store`; other valid options include `:active_record_store`, `:mem_cache_store` or the name of your own custom class. @@ -1095,6 +1097,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla #### For '7.0', defaults from previous versions below and: +- `config.action_controller.raise_on_open_redirects`: `true` - `config.action_view.button_to_generates_button_tag`: `true` - `config.action_view.apply_stylesheet_media_default`: `false` - `config.active_support.key_generator_hash_digest_class`: `OpenSSL::Digest::SHA256` @@ -1162,6 +1165,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla #### Baseline defaults: - `config.action_controller.default_protect_from_forgery`: `false` +- `config.action_controller.raise_on_open_redirects`: `false` - `config.action_controller.urlsafe_csrf_tokens`: `false` - `config.action_dispatch.cookies_same_site_protection`: `nil` - `config.action_mailer.delivery_job`: `ActionMailer::DeliveryJob` diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 463c511f0f..66dfdb6b6c 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -231,6 +231,10 @@ module Rails active_record.verify_foreign_keys_for_fixtures = true active_record.partial_inserts = false end + + if respond_to?(:action_controller) + action_controller.raise_on_open_redirects = true + end else raise "Unknown version #{target_version.to_s.inspect}" end diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt index 6ee3cf003f..c7be221b98 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt @@ -60,3 +60,6 @@ # This default means that all columns will be referenced in INSERT queries # regardless of whether they have a default or not. # Rails.application.config.active_record.partial_inserts = false +# +# Protect from open redirect attacks in `redirect_back_or_to` and `redirect_to`. +# Rails.application.config.action_controller.raise_on_open_redirects = true diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 21f6c214cd..ea4cdc383a 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -1330,6 +1330,32 @@ module ApplicationTests assert_equal false, ActionView::Resolver.caching? end + test "ActionController::Base.raise_on_open_redirects is true by default for new apps" do + app "development" + + assert_equal true, ActionController::Base.raise_on_open_redirects + end + + test "ActionController::Base.raise_on_open_redirects is false by default for upgraded apps" do + remove_from_config '.*config\.load_defaults.*\n' + add_to_config 'config.load_defaults "6.1"' + app "development" + + assert_equal false, ActionController::Base.raise_on_open_redirects + end + + test "ActionController::Base.raise_on_open_redirects can be configured in the new framework defaults" do + remove_from_config '.*config\.load_defaults.*\n' + + app_file "config/initializers/new_framework_defaults_6_2.rb", <<-RUBY + Rails.application.config.action_controller.raise_on_open_redirects = true + RUBY + + app "development" + + assert_equal true, ActionController::Base.raise_on_open_redirects + end + test "config.action_dispatch.show_exceptions is sent in env" do make_basic_app do |application| application.config.action_dispatch.show_exceptions = true