Allow to log/notify/raise on path relative redirects

This can lead to unexpected redirects, especially when the global
`raise_on_open_redirects` setting is disabled since it allows redirects
to different domains while calling `redirect_to` with a relative url.

E.g. with the app running on https://example.com, calling
`redirect_to("foo")`, will redirect to "https://example.comfoo" which
can potentially be exploited. `redirect_to("@dell.com") is even worse
since it redirects to "https://[email protected]" with
`example.com`being interpreted as the user and dell.com as the domain.

Allowing redirects to other hosts is very common in the authentication
real, especially when using OAuth and guarding more applications from
patterns like this makes sense (at least to me).

This adds the option to log, notify or raise with the default for
existing applications being log and the default for new applications
being raise. Notify is added to allow existing applications to gather
statistics about where this happens to analyze call sites individually,
working on them and then being able to confidently turn the setting to
raise.

Author: TobiasBales

actionpack/lib/action_controller/metal/redirecting.rb

@@ -15,6 +15,7 @@ class UnsafeRedirectError < StandardError; end
 
     included do
       mattr_accessor :raise_on_open_redirects, default: false
+      mattr_accessor :action_on_path_relative_redirect, default: :log
     end
 
     # Redirects the browser to the target specified in `options`. This parameter can
@@ -100,6 +101,26 @@ class UnsafeRedirectError < StandardError; end
     #
     # See #url_from for more information on what an internal and safe URL is, or how
     # to fall back to an alternate redirect URL in the unsafe case.
+    #
+    # ### Path Relative URL Redirect Protection
+    #
+    # Rails also protects against potentially unsafe path relative URL redirects that don't
+    # start with a leading slash. These can create security vulnerabilities:
+    #
+    #     redirect_to "example.com"     # Creates http://yourdomain.comexample.com
+    #     redirect_to "@attacker.com"   # Creates http://[email protected]
+    #                                   # which browsers interpret as user@host
+    #
+    # You can configure how Rails handles these cases using:
+    #
+    #     config.action_controller.action_on_path_relative_redirect = :log    # default
+    #     config.action_controller.action_on_path_relative_redirect = :notify
+    #     config.action_controller.action_on_path_relative_redirect = :raise
+    #
+    # * `:log` - Logs a warning but allows the redirect
+    # * `:notify` - Sends an ActiveSupport notification but allows the redirect
+    #   (includes stack trace to help identify the source)
+    # * `:raise` - Raises an UnsafeRedirectError
     def redirect_to(options = {}, response_options = {})
       raise ActionControllerError.new("Cannot redirect to nil!") unless options
       raise AbstractController::DoubleRenderError if response_body
@@ -166,6 +187,10 @@ def _compute_redirect_to_location(request, options) # :nodoc:
       when /\A([a-z][a-z\d\-+.]*:|\/\/).*/i
         options.to_str
       when String
+        if !options.start_with?("/") && !options.empty?
+          _handle_path_relative_redirect(options)
+        end
+
         request.protocol + request.host_with_port + options
       when Proc
         _compute_redirect_to_location request, instance_eval(&options)
@@ -248,5 +273,22 @@ def _ensure_url_is_http_header_safe(url)
           raise UnsafeRedirectError, msg
         end
       end
+
+      def _handle_path_relative_redirect(url)
+        message = "Path relative URL redirect detected: #{url.inspect}"
+
+        case action_on_path_relative_redirect
+        when :log
+          logger&.warn message
+        when :notify
+          ActiveSupport::Notifications.instrument("unsafe_redirect.action_controller",
+            url: url,
+            message: message,
+            stack_trace: caller
+          )
+        when :raise
+          raise UnsafeRedirectError, message
+        end
+      end
   end
 end

actionpack/lib/action_controller/railtie.rb

@@ -13,6 +13,7 @@ module ActionController
   class Railtie < Rails::Railtie # :nodoc:
     config.action_controller = ActiveSupport::OrderedOptions.new
     config.action_controller.raise_on_open_redirects = false
+    config.action_controller.action_on_path_relative_redirect = :log
     config.action_controller.log_query_tags_around_actions = true
     config.action_controller.wrap_parameters_by_default = false
 

actionpack/test/controller/redirect_test.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "abstract_unit"
+require "active_support/log_subscriber/test_helper"
 
 class Workshop
   extend ActiveModel::Naming
@@ -154,6 +155,14 @@ def redirect_to_url_with_network_path_reference
     redirect_to "//www.rubyonrails.org/"
   end
 
+  def redirect_to_path_relative_url
+    redirect_to "example.com"
+  end
+
+  def redirect_to_path_relative_url_starting_with_an_at
+    redirect_to "@example.com"
+  end
+
   def redirect_to_existing_record
     redirect_to Workshop.new(5)
   end
@@ -335,6 +344,18 @@ def test_redirect_to_url_with_complex_scheme
     assert_equal "x-test+scheme.complex:redirect", redirect_to_url
   end
 
+  def test_redirect_to_path_relative_url
+    get :redirect_to_path_relative_url
+    assert_response :redirect
+    assert_equal "http://test.hostexample.com", redirect_to_url
+  end
+
+  def test_redirect_to_url_with_path_relative_url_starting_with_an_at
+    get :redirect_to_path_relative_url_starting_with_an_at
+    assert_response :redirect
+    assert_equal "http://[email protected]", redirect_to_url
+  end
+
   def test_redirect_to_url_with_network_path_reference
     get :redirect_to_url_with_network_path_reference
     assert_response :redirect
@@ -620,6 +641,180 @@ def test_redirect_to_external_with_rescue
     assert_response :ok
   end
 
+  def test_redirect_to_path_relative_url_with_log
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :log
+
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger = ActionController::Base.logger
+    ActionController::Base.logger = logger
+
+    get :redirect_to_path_relative_url
+    assert_response :redirect
+    assert_equal "http://test.hostexample.com", redirect_to_url
+    assert_match(/Path relative URL redirect detected: "example.com"/, logger.logged(:warn).last)
+  ensure
+    ActionController::Base.logger = old_logger
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_path_relative_url_starting_with_an_at_with_log
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :log
+
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger = ActionController::Base.logger
+    ActionController::Base.logger = logger
+
+    get :redirect_to_path_relative_url_starting_with_an_at
+    assert_response :redirect
+    assert_equal "http://[email protected]", redirect_to_url
+    assert_match(/Path relative URL redirect detected: "@example.com"/, logger.logged(:warn).last)
+  ensure
+    ActionController::Base.logger = old_logger
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_path_relative_url_starting_with_an_at_with_notify
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :notify
+
+    events = []
+    ActiveSupport::Notifications.subscribe("unsafe_redirect.action_controller") do |*args|
+      events << ActiveSupport::Notifications::Event.new(*args)
+    end
+
+    get :redirect_to_path_relative_url_starting_with_an_at
+
+    assert_response :redirect
+    assert_equal "http://[email protected]", redirect_to_url
+
+    assert_equal 1, events.size
+    event = events.first
+    assert_equal "@example.com", event.payload[:url]
+    assert_equal 'Path relative URL redirect detected: "@example.com"', event.payload[:message]
+    assert_kind_of Array, event.payload[:stack_trace]
+    assert event.payload[:stack_trace].any? { |line| line.include?("redirect_to_path_relative_url_starting_with_an_at") }
+  ensure
+    ActiveSupport::Notifications.unsubscribe("unsafe_redirect.action_controller")
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_path_relative_url_with_notify
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :notify
+
+    events = []
+    ActiveSupport::Notifications.subscribe("unsafe_redirect.action_controller") do |*args|
+      events << ActiveSupport::Notifications::Event.new(*args)
+    end
+
+    get :redirect_to_path_relative_url
+
+    assert_response :redirect
+    assert_equal "http://test.hostexample.com", redirect_to_url
+
+    assert_equal 1, events.size
+    event = events.first
+    assert_equal "example.com", event.payload[:url]
+    assert_equal 'Path relative URL redirect detected: "example.com"', event.payload[:message]
+    assert_kind_of Array, event.payload[:stack_trace]
+    assert event.payload[:stack_trace].any? { |line| line.include?("redirect_to_path_relative_url") }
+  ensure
+    ActiveSupport::Notifications.unsubscribe("unsafe_redirect.action_controller")
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_path_relative_url_with_raise
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :raise
+
+    error = assert_raise(ActionController::Redirecting::UnsafeRedirectError) do
+      get :redirect_to_path_relative_url
+    end
+
+    assert_equal 'Path relative URL redirect detected: "example.com"', error.message
+  ensure
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_path_relative_url_starting_with_an_at_with_raise
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :raise
+
+    error = assert_raise(ActionController::Redirecting::UnsafeRedirectError) do
+      get :redirect_to_path_relative_url_starting_with_an_at
+    end
+
+    assert_equal 'Path relative URL redirect detected: "@example.com"', error.message
+  ensure
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_absolute_url_does_not_log
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :log
+
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger = ActionController::Base.logger
+    ActionController::Base.logger = logger
+
+    get :redirect_to_url
+    assert_response :redirect
+    assert_equal "http://www.rubyonrails.org/", redirect_to_url
+    assert_empty logger.logged(:warn)
+
+    get :relative_url_redirect_with_status
+    assert_response :redirect
+    assert_equal "http://test.host/things/stuff", redirect_to_url
+    assert_empty logger.logged(:warn)
+  ensure
+    ActionController::Base.logger = old_logger
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_absolute_url_does_not_notify
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :notify
+
+    events = []
+    ActiveSupport::Notifications.subscribe("unsafe_redirect.action_controller") do |*args|
+      events << ActiveSupport::Notifications::Event.new(*args)
+    end
+
+    get :redirect_to_url
+    assert_response :redirect
+    assert_equal "http://www.rubyonrails.org/", redirect_to_url
+    assert_empty events
+
+    get :relative_url_redirect_with_status
+    assert_response :redirect
+    assert_equal "http://test.host/things/stuff", redirect_to_url
+    assert_empty events
+  ensure
+    ActiveSupport::Notifications.unsubscribe("unsafe_redirect.action_controller")
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
+  def test_redirect_to_absolute_url_does_not_raise
+    old_config = ActionController::Base.action_on_path_relative_redirect
+    ActionController::Base.action_on_path_relative_redirect = :raise
+
+    get :redirect_to_url
+    assert_response :redirect
+    assert_equal "http://www.rubyonrails.org/", redirect_to_url
+
+    get :relative_url_redirect_with_status
+    assert_response :redirect
+    assert_equal "http://test.host/things/stuff", redirect_to_url
+
+    get :redirect_to_url_with_network_path_reference
+    assert_response :redirect
+    assert_equal "//www.rubyonrails.org/", redirect_to_url
+  ensure
+    ActionController::Base.action_on_path_relative_redirect = old_config
+  end
+
   private
     def with_raise_on_open_redirects
       old_raise_on_open_redirects = ActionController::Base.raise_on_open_redirects

guides/source/configuring.md

@@ -60,6 +60,7 @@ Below are the default values associated with each target version. In cases of co
 
 #### Default Values for Target Version 8.1
 
+- [`config.action_controller.action_on_path_relative_redirect`](#config-action-controller-action-on-path-relative-redirect): `:raise`
 - [`config.action_controller.escape_json_responses`](#config-action-controller-escape-json-responses): `false`
 - [`config.active_record.raise_on_missing_required_finder_order_columns`](#config-active-record-raise-on-missing-required-finder-order-columns): `true`
 - [`config.yjit`](#config-yjit): `!Rails.env.local?`
@@ -1970,6 +1971,26 @@ The default value depends on the `config.load_defaults` target version:
 
 [redirect_to]: https://api.rubyonrails.org/classes/ActionController/Redirecting.html#method-i-redirect_to
 
+#### `config.action_controller.action_on_path_relative_redirect`
+
+Controls how Rails handles paths relative URL redirects.
+
+When set to `:log` (default), Rails will log a warning when a path relative URL redirect
+is detected. When set to `:notify`, Rails will publish an
+`unsafe_redirect.action_controller` notification event. When set to `:raise`, Rails
+will raise an `ActionController::Redirecting::UnsafeRedirectError`.
+
+This helps detect potentially unsafe redirects that could be exploited for open
+redirect attacks.
+
+The default value depends on the `config.load_defaults` target version:
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `:log`               |
+| 8.1                   | `:raise`             |
+
+
 #### `config.action_controller.log_query_tags_around_actions`
 
 Determines whether controller context for query tags will be automatically

railties/lib/rails/application/configuration.rb

@@ -358,6 +358,7 @@ def load_defaults(target_version)
 
           if respond_to?(:action_controller)
             action_controller.escape_json_responses = false
+            action_controller.action_on_path_relative_redirect = :raise
           end
 
           if respond_to?(:active_record)

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_8_1.rb.tt

@@ -36,3 +36,18 @@
 # Rails 8.2.
 #++
 # Rails.configuration.active_record.raise_on_missing_required_finder_order_columns = true
+
+###
+# Controls how Rails handles path relative URL redirects.
+# When set to `:raise`, Rails will raise an `ActionController::Redirecting::UnsafeRedirectError`
+# for relative URLs without a leading slash, which can help prevent open redirect vulnerabilities.
+#
+# Example:
+#   redirect_to "example.com"     # Raises UnsafeRedirectError
+#   redirect_to "@attacker.com"   # Raises UnsafeRedirectError
+#   redirect_to "/safe/path"      # Works correctly
+#
+# Applications that want to allow these redirects can set the config to `:log` (previous default)
+# to only log warnings, or `:notify` to send ActiveSupport notifications.
+#++
+# Rails.configuration.action_controller.action_on_path_relative_redirect = :raise