Merge pull request #10362 from derekprior/dp-fix-assert-redirect-to

Fix incorrect assert_redirected_to failure message
rails · Sep 19, 2013 · acd83a7 · acd83a7
2 parents fb2785e + a78c10d
commit acd83a7
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 34 deletions.
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
@@ -1,3 +1,13 @@
+*   Fix regex used to detect URI schemes in `redirect_to` to be consistent with
+    RFC 3986.
+
+    *Derek Prior*
+
+*   Fix incorrect `assert_redirected_to` failure message for protocol-relative
+    URLs.
+
+    *Derek Prior*
+
 *   Fix an issue where router can't recognize downcased url encoding path.
 
     Fixes #12269

diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -71,6 +71,26 @@ def redirect_to(options = {}, response_status = {}) #:doc:
       self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.h(location)}\">redirected</a>.</body></html>"
     end
 
+    def _compute_redirect_to_location(options) #:nodoc:
+      case options
+      # The scheme name consist of a letter followed by any combination of
+      # letters, digits, and the plus ("+"), period ("."), or hyphen ("-")
+      # characters; and is terminated by a colon (":").
+      # See http://tools.ietf.org/html/rfc3986#section-3.1
+      # The protocol relative scheme starts with a double slash "//".
+      when /\A([a-z][a-z\d\-+\.]*:|\/\/).*/i
+        options
+      when String
+        request.protocol + request.host_with_port + options
+      when :back
+        request.headers["Referer"] or raise RedirectBackError
+      when Proc
+        _compute_redirect_to_location options.call
+      else
+        url_for(options)
+      end.delete("\0\r\n")
+    end
+
     private
       def _extract_redirect_to_status(options, response_status)
         if options.is_a?(Hash) && options.key?(:status)
@@ -81,24 +101,5 @@ def _extract_redirect_to_status(options, response_status)
           302
         end
       end
-
-      def _compute_redirect_to_location(options)
-        case options
-        # The scheme name consist of a letter followed by any combination of
-        # letters, digits, and the plus ("+"), period ("."), or hyphen ("-")
-        # characters; and is terminated by a colon (":").
-        # The protocol relative scheme starts with a double slash "//"
-        when %r{\A(\w[\w+.-]*:|//).*}
-          options
-        when String
-          request.protocol + request.host_with_port + options
-        when :back
-          request.headers["Referer"] or raise RedirectBackError
-        when Proc
-          _compute_redirect_to_location options.call
-        else
-          url_for(options)
-        end.delete("\0\r\n")
-      end
   end
 end
diff --git a/actionpack/lib/action_dispatch/testing/assertions/response.rb b/actionpack/lib/action_dispatch/testing/assertions/response.rb
@@ -67,21 +67,11 @@ def parameterize(value)
         end
 
         def normalize_argument_to_redirection(fragment)
-          normalized = case fragment
-            when Regexp
-              fragment
-            when %r{^\w[A-Za-z\d+.-]*:.*}
-              fragment
-            when String
-              @request.protocol + @request.host_with_port + fragment
-            when :back
-              raise RedirectBackError unless refer = @request.headers["Referer"]
-              refer
-            else
-              @controller.url_for(fragment)
-            end
-
-          normalized.respond_to?(:delete) ? normalized.delete("\0\r\n") : normalized
+          if Regexp === fragment
+            fragment
+          else
+            @controller._compute_redirect_to_location(fragment)
+          end
         end
     end
   end

diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb
@@ -39,6 +39,8 @@ def redirect_to_named_route() redirect_to route_one_url end
 
   def redirect_external() redirect_to "http://www.rubyonrails.org"; end
 
+  def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org"; end
+
   def response404() head '404 AWOL' end
 
   def response500() head '500 Sorry' end
@@ -258,6 +260,19 @@ def test_assert_redirected_to_top_level_named_route_with_same_controller_name_in
     end
   end
 
+  def test_assert_redirect_failure_message_with_protocol_relative_url
+    begin
+      process :redirect_external_protocol_relative
+      assert_redirected_to "/foo"
+    rescue ActiveSupport::TestCase::Assertion => ex
+      assert_no_match(
+        /#{request.protocol}#{request.host}\/\/www.rubyonrails.org/,
+        ex.message,
+        'protocol relative url was incorrectly normalized'
+      )
+    end
+  end
+
   def test_template_objects_exist
     process :assign_this
     assert !@controller.instance_variable_defined?(:"@hi")
@@ -309,6 +324,9 @@ def test_redirection_location
 
     process :redirect_external
     assert_equal 'http://www.rubyonrails.org', @response.redirect_url
+
+    process :redirect_external_protocol_relative
+    assert_equal '//www.rubyonrails.org', @response.redirect_url
   end
 
   def test_no_redirect_url