Respect `SCRIPT_NAME` when using `redirect` with a relative path

Example: # application routes.rb mount BlogEngine => '/blog' # engine routes.rb get '/admin' => redirect('admin/dashboard') This now redirects to the path `/blog/admin/dashboard`, whereas before it would've generated an invalid url because there would be no slash between the host name and the path. It also allows redirects to work where the application is deployed to a subdirectory of a website. Fixes #7977
author: Andrew White <andyw@pixeltrix.co.uk> 2013-10-10 13:01:03 +0100
committer: Andrew White <andyw@pixeltrix.co.uk> 2013-10-10 13:03:26 +0100
commit: 9dbd208562ccd3d68009a72d37cbfe29b94f98c4 (patch)
tree: e30ecc10b5ada449c0d54a11b9586cbbb019bdff
parent: 0061c5e1ef0f7be8946602456a538c263fcafed2 (diff)
download: rails-9dbd208562ccd3d68009a72d37cbfe29b94f98c4.tar.gz
rails-9dbd208562ccd3d68009a72d37cbfe29b94f98c4.tar.bz2
rails-9dbd208562ccd3d68009a72d37cbfe29b94f98c4.zip
3 files changed, 136 insertions, 0 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 7af9165605..9fb914ac40 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,21 @@
+*   Respect `SCRIPT_NAME` when using `redirect` with a relative path
+
+    Example:
+        # application routes.rb
+        mount BlogEngine => '/blog'
+
+        # engine routes.rb
+        get '/admin' => redirect('admin/dashboard')
+
+    This now redirects to the path `/blog/admin/dashboard`, whereas before it would've
+    generated an invalid url because there would be no slash between the host name and
+    the path. It also allows redirects to work where the application is deployed to a
+    subdirectory of a website.
+
+    Fixes #7977
+
+    *Andrew White*
+
 *   Fixing repond_with working directly on the options hash
     This fixes an issue where the respond_with worked directly with the given
     options hash, so that if a user relied on it after calling respond_with,
diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb
index 68094f129f..3e54c7e71c 100644
--- a/actionpack/lib/action_dispatch/routing/redirection.rb
+++ b/actionpack/lib/action_dispatch/routing/redirection.rb
@@ -30,6 +30,10 @@ module ActionDispatch
         uri.host   ||= req.host
         uri.port   ||= req.port unless req.standard_port?
 
+        if relative_path?(uri.path)
+          uri.path = "#{req.script_name}/#{uri.path}"
+        end
+
         body = %(<html><body>You are being <a href="#{ERB::Util.h(uri.to_s)}">redirected</a>.</body></html>)
 
         headers = {
@@ -48,6 +52,11 @@ module ActionDispatch
       def inspect
         "redirect(#{status})"
       end
+
+      private
+        def relative_path?(path)
+          path && !path.empty? && path[0] != '/'
+        end
     end
 
     class PathRedirect < Redirect
@@ -81,6 +90,11 @@ module ActionDispatch
           url_options[:path] = (url_options[:path] % escape_path(params))
         end
 
+        if relative_path?(url_options[:path])
+          url_options[:path] = "/#{url_options[:path]}"
+          url_options[:script_name] = request.script_name
+        end
+
         ActionDispatch::Http::URL.url_for url_options
       end
 
@@ -104,6 +118,10 @@ module ActionDispatch
       #
       #   get 'docs/:article', to: redirect('/wiki/%{article}')
       #
+      # Note that if you return a path without a leading slash then the url is prefixed with the
+      # current SCRIPT_NAME environment variable. This is typically '/' but may be different in
+      # a mounted engine or where the application is deployed to a subdirectory of a website.
+      #
       # Alternatively you can use one of the other syntaxes:
       #
       # The block version of redirect allows for the easy encapsulation of any logic associated with
diff --git a/actionpack/test/dispatch/prefix_generation_test.rb b/actionpack/test/dispatch/prefix_generation_test.rb
index 113608ecf4..e519fff51e 100644
--- a/actionpack/test/dispatch/prefix_generation_test.rb
+++ b/actionpack/test/dispatch/prefix_generation_test.rb
@@ -31,6 +31,14 @@ module TestGenerationPrefix
             get "/polymorphic_path_for_engine", :to => "inside_engine_generating#polymorphic_path_for_engine"
             get "/conflicting_url", :to => "inside_engine_generating#conflicting"
             get "/foo", :to => "never#invoked", :as => :named_helper_that_should_be_invoked_only_in_respond_to_test
+
+            get "/relative_path_redirect", :to => redirect("foo")
+            get "/relative_option_redirect", :to => redirect(:path => "foo")
+            get "/relative_custom_redirect", :to => redirect { |params, request| "foo" }
+
+            get "/absolute_path_redirect", :to => redirect("/foo")
+            get "/absolute_option_redirect", :to => redirect(:path => "/foo")
+            get "/absolute_custom_redirect", :to => redirect { |params, request| "/foo" }
           end
 
           routes
@@ -182,6 +190,48 @@ module TestGenerationPrefix
       assert_equal "engine", last_response.body
     end
 
+    test "[ENGINE] relative path redirect uses SCRIPT_NAME from request" do
+      get "/awesome/blog/relative_path_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/awesome/blog/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/awesome/blog/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] relative option redirect uses SCRIPT_NAME from request" do
+      get "/awesome/blog/relative_option_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/awesome/blog/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/awesome/blog/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] relative custom redirect uses SCRIPT_NAME from request" do
+      get "/awesome/blog/relative_custom_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/awesome/blog/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/awesome/blog/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] absolute path redirect doesn't use SCRIPT_NAME from request" do
+      get "/awesome/blog/absolute_path_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] absolute option redirect doesn't use SCRIPT_NAME from request" do
+      get "/awesome/blog/absolute_option_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] absolute custom redirect doesn't use SCRIPT_NAME from request" do
+      get "/awesome/blog/absolute_custom_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
     # Inside Application
     test "[APP] generating engine's route includes prefix" do
       get "/generate"
@@ -281,6 +331,14 @@ module TestGenerationPrefix
           routes = ActionDispatch::Routing::RouteSet.new
           routes.draw do
             get "/posts/:id", :to => "posts#show", :as => :post
+
+            get "/relative_path_redirect", :to => redirect("foo")
+            get "/relative_option_redirect", :to => redirect(:path => "foo")
+            get "/relative_custom_redirect", :to => redirect { |params, request| "foo" }
+
+            get "/absolute_path_redirect", :to => redirect("/foo")
+            get "/absolute_option_redirect", :to => redirect(:path => "/foo")
+            get "/absolute_custom_redirect", :to => redirect { |params, request| "/foo" }
           end
 
           routes
@@ -331,5 +389,47 @@ module TestGenerationPrefix
       get "/posts/1"
       assert_equal "/posts/1", last_response.body
     end
+
+    test "[ENGINE] relative path redirect uses SCRIPT_NAME from request" do
+      get "/relative_path_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] relative option redirect uses SCRIPT_NAME from request" do
+      get "/relative_option_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] relative custom redirect uses SCRIPT_NAME from request" do
+      get "/relative_custom_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] absolute path redirect doesn't use SCRIPT_NAME from request" do
+      get "/absolute_path_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] absolute option redirect doesn't use SCRIPT_NAME from request" do
+      get "/absolute_option_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
+
+    test "[ENGINE] absolute custom redirect doesn't use SCRIPT_NAME from request" do
+      get "/absolute_custom_redirect"
+      assert_equal 301, last_response.status
+      assert_equal "http://example.org/foo", last_response.headers["Location"]
+      assert_equal %(<html><body>You are being <a href="http://example.org/foo">redirected</a>.</body></html>), last_response.body
+    end
   end
 end
author	Andrew White <andyw@pixeltrix.co.uk>	2013-10-10 13:01:03 +0100
committer	Andrew White <andyw@pixeltrix.co.uk>	2013-10-10 13:03:26 +0100
commit	9dbd208562ccd3d68009a72d37cbfe29b94f98c4 (patch)
tree	e30ecc10b5ada449c0d54a11b9586cbbb019bdff
parent	0061c5e1ef0f7be8946602456a538c263fcafed2 (diff)
download	rails-9dbd208562ccd3d68009a72d37cbfe29b94f98c4.tar.gz rails-9dbd208562ccd3d68009a72d37cbfe29b94f98c4.tar.bz2 rails-9dbd208562ccd3d68009a72d37cbfe29b94f98c4.zip