Don't redirect to another suburi (#16530).

git-svn-id: http://svn.redmine.org/redmine/trunk@13213 e93f8b46-1217-0410-a6f0-8f06a7374b81

2 files changed, 67 insertions, 12 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 8b7bff553..10e0ecbb2 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -376,18 +376,9 @@ class ApplicationController < ActionController::Base
 
   def redirect_back_or_default(default, options={})
     back_url = params[:back_url].to_s
-    if back_url.present?
-      begin
-        uri = URI.parse(back_url)
-        # do not redirect user to another host or to the login or register page
-        if ((uri.relative? && back_url.match(%r{\A/(\w.*)?\z})) || (uri.host == request.host)) && !uri.path.match(%r{/(login|account/register)})
-          redirect_to(back_url)
-          return
-        end
-      rescue URI::InvalidURIError
-        logger.warn("Could not redirect to invalid URL #{back_url}")
-        # redirect to default
-      end
+    if back_url.present? && valid_back_url?(back_url)
+      redirect_to(back_url)
+      return
     elsif options[:referer]
       redirect_to_referer_or default
       return
@@ -396,6 +387,34 @@ class ApplicationController < ActionController::Base
     false
   end
 
+  # Returns true if back_url is a valid url for redirection, otherwise false
+  def valid_back_url?(back_url)
+    if CGI.unescape(back_url).include?('..')
+      return false
+    end
+
+    begin
+      uri = URI.parse(back_url)
+    rescue URI::InvalidURIError
+      return false
+    end
+
+    if uri.host.present? && uri.host != request.host
+      return false
+    end
+
+    if uri.path.match(%r{/(login|account/register)})
+      return false
+    end
+
+    if relative_url_root.present? && !uri.path.starts_with?(relative_url_root)
+      return false
+    end
+
+    return true
+  end
+  private :valid_back_url?
+
   # Redirects to the request referer if present, redirects to args or call block otherwise.
   def redirect_to_referer_or(*args, &block)
     redirect_to :back
diff --git a/test/functional/account_controller_test.rb b/test/functional/account_controller_test.rb
index 991a8fb72..faf7356ea 100644
--- a/test/functional/account_controller_test.rb
+++ b/test/functional/account_controller_test.rb
@@ -71,6 +71,22 @@ class AccountControllerTest < ActionController::TestCase
     end
   end
 
+  def test_login_with_suburi_should_redirect_to_back_url_param
+    @relative_url_root = ApplicationController.relative_url_root
+    ApplicationController.relative_url_root = '/redmine'
+
+    back_urls = [
+      'http://test.host/redmine/issues/show/1',
+      '/redmine'
+    ]
+    back_urls.each do |back_url|
+      post :login, :username => 'jsmith', :password => 'jsmith', :back_url => back_url
+      assert_redirected_to back_url
+    end
+  ensure
+    ApplicationController.relative_url_root = @relative_url_root
+  end
+
   def test_login_should_not_redirect_to_another_host
     back_urls = [
       'http://test.foo/fake',
@@ -82,6 +98,26 @@ class AccountControllerTest < ActionController::TestCase
     end
   end
 
+  def test_login_with_suburi_should_not_redirect_to_another_suburi
+    @relative_url_root = ApplicationController.relative_url_root
+    ApplicationController.relative_url_root = '/redmine'
+
+    back_urls = [
+      'http://test.host/',
+      'http://test.host/fake',
+      'http://test.host/fake/issues',
+      'http://test.host/redmine/../fake',
+      'http://test.host/redmine/../fake/issues',
+      'http://test.host/redmine/%2e%2e/fake'
+    ]
+    back_urls.each do |back_url|
+      post :login, :username => 'jsmith', :password => 'jsmith', :back_url => back_url
+      assert_redirected_to '/my/page'
+    end
+  ensure
+    ApplicationController.relative_url_root = @relative_url_root
+  end
+
   def test_login_with_wrong_password
     post :login, :username => 'admin', :password => 'bad'
     assert_response :success