Skip to content

Commit

Permalink
Use request.fullpath to build redirect url in force_ssl
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pixeltrix committed Apr 25, 2013
1 parent 8aafb3d commit 8227bf7
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 4 deletions.
9 changes: 9 additions & 0 deletions 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.

Expand Down
11 changes: 7 additions & 4 deletions actionpack/lib/action_controller/metal/force_ssl.rb
Expand Up @@ -51,11 +51,14 @@ def force_ssl(options = {})
# * <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
Expand Down
63 changes: 63 additions & 0 deletions actionpack/test/controller/force_ssl_test.rb
Expand Up @@ -149,16 +149,79 @@ def test_cheeseburger_redirects_to_https
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
Expand Down

3 comments on commit 8227bf7

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice combo 😄

@guilleiguaran
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!! ❤️

@toreriklinnerud
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this issue has caused us serious confusion on numerous occasions.

Please sign in to comment.