Use `request.fullpath` to build redirect url in `force_ssl`

The `force_ssl` command now builds the redirect url from `request.fullpath`.
This ensures that the format is maintained and it doesn't redirect to a route
that has the same parameters but is defined earlier in `routes.rb`. Also any
optional segments are maintained.

Fixes #7528.
Fixes #9061.
Fixes #10305.

3 files changed, 79 insertions, 4 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index a545b067b3..817b3cab83 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,14 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   The `force_ssl` command now builds the redirect url from `request.fullpath`.
+    This ensures that the format is maintained and it doesn't redirect to a route
+    that has the same parameters but is defined earlier in `routes.rb`. Also any
+    optional segments are maintained.
+
+    Fixes #7528, #9061, #10305.
+
+    *Andrew White*
+
 *   Return a 405 Method Not Allowed response when a request contains an unknown
     HTTP method.
 
diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb
index f1e8714a86..fe61dd1f86 100644
--- a/actionpack/lib/action_controller/metal/force_ssl.rb
+++ b/actionpack/lib/action_controller/metal/force_ssl.rb
@@ -51,11 +51,14 @@ module ActionController
     # * <tt>host</tt> - Redirect to a different host name
     def force_ssl_redirect(host = nil)
       unless request.ssl?
-        redirect_options = {:protocol => 'https://', :status => :moved_permanently}
-        redirect_options.merge!(:host => host) if host
-        redirect_options.merge!(:params => request.query_parameters)
+        secure_url = ActionDispatch::Http::URL.url_for({
+          :protocol => 'https://',
+          :path     => request.fullpath,
+          :host     => host || request.host
+        })
+
         flash.keep if respond_to?(:flash)
-        redirect_to redirect_options
+        redirect_to secure_url, :status => :moved_permanently
       end
     end
   end
diff --git a/actionpack/test/controller/force_ssl_test.rb b/actionpack/test/controller/force_ssl_test.rb
index 6758668b7a..2e884cadd4 100644
--- a/actionpack/test/controller/force_ssl_test.rb
+++ b/actionpack/test/controller/force_ssl_test.rb
@@ -149,16 +149,79 @@ class ForceSSLFlashTest < ActionController::TestCase
     assert_response 302
     assert_equal "http://test.host/force_ssl_flash/cheeseburger", redirect_to_url
 
+    # FIXME: AC::TestCase#build_request_uri doesn't build a new uri if PATH_INFO exists
+    @request.env.delete('PATH_INFO')
+
     get :cheeseburger
     assert_response 301
     assert_equal "https://test.host/force_ssl_flash/cheeseburger", redirect_to_url
 
+    # FIXME: AC::TestCase#build_request_uri doesn't build a new uri if PATH_INFO exists
+    @request.env.delete('PATH_INFO')
+
     get :use_flash
     assert_equal "hello", assigns["flash_copy"]["that"]
     assert_equal "hello", assigns["flashy"]
   end
 end
 
+class ForceSSLDuplicateRoutesTest < ActionController::TestCase
+  tests ForceSSLCustomDomain
+
+  def test_force_ssl_redirects_to_same_path
+    with_routing do |set|
+      set.draw do
+        get '/foo', :to => 'force_ssl_custom_domain#banana'
+        get '/bar', :to => 'force_ssl_custom_domain#banana'
+      end
+
+      @request.env['PATH_INFO'] = '/bar'
+
+      get :banana
+      assert_response 301
+      assert_equal 'https://secure.test.host/bar', redirect_to_url
+    end
+  end
+end
+
+class ForceSSLFormatTest < ActionController::TestCase
+  tests ForceSSLControllerLevel
+
+  def test_force_ssl_redirects_to_same_format
+    with_routing do |set|
+      set.draw do
+        get '/foo', :to => 'force_ssl_controller_level#banana'
+      end
+
+      get :banana, :format => :json
+      assert_response 301
+      assert_equal 'https://test.host/foo.json', redirect_to_url
+    end
+  end
+end
+
+class ForceSSLOptionalSegmentsTest < ActionController::TestCase
+  tests ForceSSLControllerLevel
+
+  def test_force_ssl_redirects_to_same_format
+    with_routing do |set|
+      set.draw do
+        scope '(:locale)' do
+          defaults :locale => 'en' do
+            get '/foo', :to => 'force_ssl_controller_level#banana'
+          end
+        end
+      end
+
+      @request.env['PATH_INFO'] = '/en/foo'
+      get :banana, :locale => 'en'
+      assert_equal 'en',  @controller.params[:locale]
+      assert_response 301
+      assert_equal 'https://test.host/en/foo', redirect_to_url
+    end
+  end
+end
+
 class RedirectToSSLTest < ActionController::TestCase
   tests RedirectToSSL
   def test_banana_redirects_to_https_if_not_https