Merge branch 'opt_into_open_redirect_protection'

2022-11-09 12:12:34 -05:00 · 2021-07-22 16:54:36 -04:00 · 2021-07-22 16:54:36 -04:00 · 77e5f91f42
commit 77e5f91f42
parent 25d8c1ce45 5e93cff835
8 changed files with 119 additions and 6 deletions
--- 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.
--- 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:
    #
    # * <tt>Hash</tt> - 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 = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>"
    end

    # Soft deprecated alias for <tt>redirect_back_or_to</tt> 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))
--- 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

--- 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
--- 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`
--- 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
--- 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
--- 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