Don't add the standard https port when using redirect in routes.rb an…

…d ensure that request.scheme returns https when using a reverse proxy. [#5408 state:resolved] Signed-off-by: José Valim <jose.valim@gmail.com>
rails · Aug 20, 2010 · 0d0fbf1 · 0d0fbf1
1 parent 771d2f9
commit 0d0fbf1
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 1 deletion.
diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb
@@ -6,6 +6,11 @@ def url
         protocol + host_with_port + fullpath
       end
 
+      # Returns 'https' if this is an SSL request and 'http' otherwise.
+      def scheme
+        ssl? ? 'https' : 'http'
+      end
+
       # Returns 'https://' if this is an SSL request and 'http://' otherwise.
       def protocol
         ssl? ? 'https://' : 'http://'
@@ -53,6 +58,11 @@ def standard_port
         end
       end
 
+      # Returns whether this request is using the standard port
+      def standard_port?
+        port == standard_port
+      end
+
       # Returns a \port suffix like ":8080" if the \port number of this request
       # is not the default HTTP \port 80 or HTTPS \port 443.
       def port_string

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -288,7 +288,7 @@ def redirect(*args, &block)
             uri = URI.parse(path_proc.call(*params))
             uri.scheme ||= req.scheme
             uri.host   ||= req.host
-            uri.port   ||= req.port unless req.port == 80
+            uri.port   ||= req.port unless req.standard_port?
 
             body = %(<html><body>You are being <a href="#{ERB::Util.h(uri.to_s)}">redirected</a>.</body></html>)
 

diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
@@ -132,6 +132,32 @@ class RequestTest < ActiveSupport::TestCase
     assert_equal [], request.subdomains
   end
 
+  test "standard_port" do
+    request = stub_request
+    assert_equal 80, request.standard_port
+
+    request = stub_request 'HTTPS' => 'on'
+    assert_equal 443, request.standard_port
+  end
+
+  test "standard_port?" do
+    request = stub_request
+    assert !request.ssl?
+    assert request.standard_port?
+
+    request = stub_request 'HTTPS' => 'on'
+    assert request.ssl?
+    assert request.standard_port?
+
+    request = stub_request 'HTTP_HOST' => 'www.example.org:8080'
+    assert !request.ssl?
+    assert !request.standard_port?
+
+    request = stub_request 'HTTP_HOST' => 'www.example.org:8443', 'HTTPS' => 'on'
+    assert request.ssl?
+    assert !request.standard_port?
+  end
+
   test "port string" do
     request = stub_request 'HTTP_HOST' => 'www.example.org:80'
     assert_equal "", request.port_string
@@ -223,6 +249,16 @@ class RequestTest < ActiveSupport::TestCase
     assert request.ssl?
   end
 
+  test "scheme returns https when proxied" do
+    request = stub_request 'rack.url_scheme' => 'http'
+    assert !request.ssl?
+    assert_equal 'http', request.scheme
+
+    request = stub_request 'rack.url_scheme' => 'http', 'HTTP_X_FORWARDED_PROTO' => 'https'
+    assert request.ssl?
+    assert_equal 'https', request.scheme
+  end
+
   test "String request methods" do
     [:get, :post, :put, :delete].each do |method|
       request = stub_request 'REQUEST_METHOD' => method.to_s.upcase

diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
@@ -45,6 +45,7 @@ def self.matches?(request)
 
       match 'account/logout' => redirect("/logout"), :as => :logout_redirect
       match 'account/login', :to => redirect("/login")
+      match 'secure', :to => redirect("/secure/login")
 
       constraints(lambda { |req| true }) do
         match 'account/overview'
@@ -2003,11 +2004,28 @@ def test_resources_path_can_be_a_symbol
     end
   end
 
+  def test_redirect_https
+    with_test_routes do
+      with_https do
+        get '/secure'
+        verify_redirect 'https://www.example.com/secure/login'
+      end
+    end
+  end
+
 private
   def with_test_routes
     yield
   end
 
+  def with_https
+    old_https = https?
+    https!
+    yield
+  ensure
+    https!(old_https)
+  end
+
   def verify_redirect(url, status=301)
     assert_equal status, @response.status
     assert_equal url, @response.headers['Location']