Merge pull request rails#55308 from Shopify/raise-on-relative-redirects-without-slash

Allow to log/notify/raise on path relative redirects

Author: tenderlove

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