Ensure external redirects are explicitly allowed

Add `fallback_location` and `allow_other_host` options to `redirect_to`.
author: Gannon McGibbon <gannon.mcgibbon@gmail.com> 2018-11-08 14:45:06 -0500
committer: Gannon McGibbon <gannon.mcgibbon@gmail.com> 2019-01-17 13:28:14 -0500
commit: 9dde7d8de047b55ec636c4c7cba89ec95324d492 (patch)
tree: 5afafc7ecef072ad779664827355cb0e91cc3768
parent: 9e34df00039d63b5672315419e76f06f80ef3dc4 (diff)
download: rails-9dde7d8de047b55ec636c4c7cba89ec95324d492.tar.gz
rails-9dde7d8de047b55ec636c4c7cba89ec95324d492.tar.bz2
rails-9dde7d8de047b55ec636c4c7cba89ec95324d492.zip
7 files changed, 87 insertions, 29 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 1457794354..2000be688f 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   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/flash.rb b/actionpack/lib/action_controller/metal/flash.rb
index 380f2e9591..a4861dc2c0 100644
--- a/actionpack/lib/action_controller/metal/flash.rb
+++ b/actionpack/lib/action_controller/metal/flash.rb
@@ -44,18 +44,18 @@ module ActionController #:nodoc:
     end
 
     private
-      def redirect_to(options = {}, response_status_and_flash = {}) #:doc:
+      def redirect_to(options = {}, response_options_and_flash = {}) #:doc:
         self.class._flash_types.each do |flash_type|
-          if type = response_status_and_flash.delete(flash_type)
+          if type = response_options_and_flash.delete(flash_type)
             flash[flash_type] = type
           end
         end
 
-        if other_flashes = response_status_and_flash.delete(:flash)
+        if other_flashes = response_options_and_flash.delete(:flash)
           flash.update(other_flashes)
         end
 
-        super(options, response_status_and_flash)
+        super(options, response_options_and_flash)
       end
   end
 end
diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb
index 26e6f72b66..205f84ae36 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]
+    REDIRECT_OPTIONS = [:status, :flash, :alert, :notice, :allow_other_host]
 
     module ClassMethods # :nodoc:
       def force_ssl(options = {})
@@ -40,7 +40,8 @@ module ActionController
           protocol: "https://",
           host: request.host,
           path: request.fullpath,
-          status: :moved_permanently
+          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 2804a06a58..8bd003f5ed 100644
--- a/actionpack/lib/action_controller/metal/redirecting.rb
+++ b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -55,12 +55,12 @@ module ActionController
     # Statements after +redirect_to+ in our controller get executed, so +redirect_to+ doesn't stop the execution of the function.
     # To terminate the execution of the function immediately after the +redirect_to+, use return.
     #   redirect_to post_url(@post) and return
-    def redirect_to(options = {}, response_status = {})
+    def redirect_to(options = {}, response_options = {})
       raise ActionControllerError.new("Cannot redirect to nil!") unless options
       raise AbstractController::DoubleRenderError if response_body
 
-      self.status        = _extract_redirect_to_status(options, response_status)
-      self.location      = _compute_redirect_to_location(request, options)
+      self.status        = _extract_redirect_to_status(options, response_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
 
@@ -88,9 +88,13 @@ 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["Referer"]
-      redirect_to_referer = referer && (allow_other_host || _url_host_allowed?(referer))
-      redirect_to redirect_to_referer ? referer : fallback_location, **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
     end
 
     def _compute_redirect_to_location(request, options) #:nodoc:
@@ -114,18 +118,35 @@ module ActionController
     public :_compute_redirect_to_location
 
     private
-      def _extract_redirect_to_status(options, response_status)
+      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))
-        elsif response_status.key?(:status)
-          Rack::Utils.status_code(response_status[:status])
+        elsif response_options.key?(:status)
+          Rack::Utils.status_code(response_options[:status])
         else
           302
         end
       end
 
-      def _url_host_allowed?(url)
-        URI(url.to_s).host == request.host
+      def _url_host_allowed?(url, options = {})
+        URI(url.to_s).host.in?([request.host, options[: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 ecb8c37e6b..c7aae034dd 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" end
+  def redirect_invalid_external_route() redirect_to "ht_tp://www.rubyonrails.org", allow_other_host: true end
 
   def redirect_to_named_route() redirect_to route_one_url end
 
-  def redirect_external() redirect_to "http://www.rubyonrails.org"; end
+  def redirect_external() redirect_to "http://www.rubyonrails.org", allow_other_host: true; end
 
-  def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org"; end
+  def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org", allow_other_host: true; 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 0562c16284..cbebc6b59c 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/"
+      redirect_to "http://foo.bar/", allow_other_host: true
     end
 
     def filterable_redirector
-      redirect_to "http://secret.foo.bar/"
+      redirect_to "http://secret.foo.bar/", allow_other_host: true
     end
 
     def data_sender
diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb
index 998498e1b2..945d2275c0 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)
+    redirect_to("http://www.example.com", status: :moved_permanently, allow_other_host: true)
   end
 
   def url_redirect_with_status_hash
-    redirect_to("http://www.example.com", status: 301)
+    redirect_to("http://www.example.com", status: 301, allow_other_host: true)
   end
 
   def relative_url_redirect_with_status
@@ -81,19 +81,27 @@ 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"
+    redirect_to "http://example.com/query?status=new", allow_other_host: true
   end
 
   def redirect_to_url_with_complex_scheme
-    redirect_to "x-test+scheme.complex:redirect"
+    redirect_to "x-test+scheme.complex:redirect", allow_other_host: true
   end
 
   def redirect_to_url_with_network_path_reference
-    redirect_to "//www.rubyonrails.org/"
+    redirect_to "//www.rubyonrails.org/", allow_other_host: true
   end
 
   def redirect_to_existing_record
@@ -113,12 +121,12 @@ class RedirectController < ActionController::Base
   end
 
   def redirect_to_with_block
-    redirect_to proc { "http://www.rubyonrails.org/" }
+    redirect_to proc { "http://www.rubyonrails.org/" }, allow_other_host: true
   end
 
   def redirect_to_with_block_and_assigns
     @url = "http://www.rubyonrails.org/"
-    redirect_to proc { @url }
+    redirect_to proc { @url }, allow_other_host: true
   end
 
   def redirect_to_with_block_and_options
@@ -245,6 +253,28 @@ 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
author	Gannon McGibbon <gannon.mcgibbon@gmail.com>	2018-11-08 14:45:06 -0500
committer	Gannon McGibbon <gannon.mcgibbon@gmail.com>	2019-01-17 13:28:14 -0500
commit	9dde7d8de047b55ec636c4c7cba89ec95324d492 (patch)
tree	5afafc7ecef072ad779664827355cb0e91cc3768
parent	9e34df00039d63b5672315419e76f06f80ef3dc4 (diff)
download	rails-9dde7d8de047b55ec636c4c7cba89ec95324d492.tar.gz rails-9dde7d8de047b55ec636c4c7cba89ec95324d492.tar.bz2 rails-9dde7d8de047b55ec636c4c7cba89ec95324d492.zip