Merge pull request #4079 from drogus/http_digest_issue

Fix http digest authentication when url ends with `/` or `?`

7 files changed, 137 insertions, 8 deletions

diff --git a/actionpack/lib/action_controller/metal/http_authentication.rb b/actionpack/lib/action_controller/metal/http_authentication.rb
index 264806cd36..56335a22cb 100644
--- a/actionpack/lib/action_controller/metal/http_authentication.rb
+++ b/actionpack/lib/action_controller/metal/http_authentication.rb
@@ -192,12 +192,15 @@ module ActionController
           return false unless password
 
           method = request.env['rack.methodoverride.original_method'] || request.env['REQUEST_METHOD']
-          uri    = credentials[:uri][0,1] == '/' ? request.fullpath : request.url
+          uri    = credentials[:uri][0,1] == '/' ? request.original_fullpath : request.original_url
 
-         [true, false].any? do |password_is_ha1|
-           expected = expected_response(method, uri, credentials, password, password_is_ha1)
-           expected == credentials[:response]
-         end
+          [true, false].any? do |trailing_question_mark|
+            [true, false].any? do |password_is_ha1|
+              _uri = trailing_question_mark ? uri + "?" : uri
+              expected = expected_response(method, _uri, credentials, password, password_is_ha1)
+              expected == credentials[:response]
+            end
+          end
         end
       end
 
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index c5c48ec489..820921252d 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -122,10 +122,18 @@ module ActionDispatch
       Http::Headers.new(@env)
     end
 
+    def original_fullpath
+      @original_fullpath ||= (env["ORIGINAL_FULLPATH"] || fullpath)
+    end
+
     def fullpath
       @fullpath ||= super
     end
 
+    def original_url
+      base_url + original_fullpath
+    end
+
     def media_type
       content_mime_type.to_s
     end
diff --git a/actionpack/test/controller/http_digest_authentication_test.rb b/actionpack/test/controller/http_digest_authentication_test.rb
index b011536717..a91e3cafa5 100644
--- a/actionpack/test/controller/http_digest_authentication_test.rb
+++ b/actionpack/test/controller/http_digest_authentication_test.rb
@@ -139,7 +139,7 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
 
   test "authentication request with request-uri that doesn't match credentials digest-uri" do
     @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please')
-    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest/altered/uri"
+    @request.env['ORIGINAL_FULLPATH'] = "/http_digest_authentication_test/dummy_digest/altered/uri"
     get :display
 
     assert_response :unauthorized
@@ -208,6 +208,44 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
     assert !ActionController::HttpAuthentication::Digest.validate_digest_response(@request, "SuperSecret"){nil}
   end
 
+  test "authentication request with request-uri ending in '/'" do
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest/"
+    @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please')
+
+    # simulate normalizing PATH_INFO
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest"
+    get :display
+
+    assert_response :success
+    assert_equal 'Definitely Maybe', @response.body
+  end
+
+  test "authentication request with request-uri ending in '?'" do
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest/?"
+    @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please')
+
+    # simulate normalizing PATH_INFO
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest"
+    get :display
+
+    assert_response :success
+    assert_equal 'Definitely Maybe', @response.body
+  end
+
+  test "authentication request with absolute uri in credentials (as in IE) ending with /" do
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest/"
+    @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:uri => "http://test.host/http_digest_authentication_test/dummy_digest/",
+                                                            :username => 'pretty', :password => 'please')
+
+    # simulate normalizing PATH_INFO
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest"
+    get :display
+
+    assert_response :success
+    assert assigns(:logged_in)
+    assert_equal 'Definitely Maybe', @response.body
+  end
+
   private
 
   def encode_credentials(options)
@@ -228,7 +266,10 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
 
     credentials = decode_credentials(@response.headers['WWW-Authenticate'])
     credentials.merge!(options)
-    credentials.merge!(:uri => @request.env['PATH_INFO'].to_s)
+    path_info = @request.env['PATH_INFO'].to_s
+    uri = options[:uri] || path_info
+    credentials.merge!(:uri => uri)
+    @request.env["ORIGINAL_FULLPATH"] = path_info
     ActionController::HttpAuthentication::Digest.encode_credentials(method, credentials, password, options[:password_is_ha1])
   end
 
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index 4d805464c2..5b3d38c48c 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -618,6 +618,30 @@ class RequestTest < ActiveSupport::TestCase
     assert_equal "/authenticate?secret", path
   end
 
+  test "original_fullpath returns ORIGINAL_FULLPATH" do
+    request = stub_request('ORIGINAL_FULLPATH' => "/foo?bar")
+
+    path = request.original_fullpath
+    assert_equal "/foo?bar", path
+  end
+
+  test "original_url returns url built using ORIGINAL_FULLPATH" do
+    request = stub_request('ORIGINAL_FULLPATH' => "/foo?bar",
+                           'HTTP_HOST'         => "example.org",
+                           'rack.url_scheme'   => "http")
+
+    url = request.original_url
+    assert_equal "http://example.org/foo?bar", url
+  end
+
+  test "original_fullpath returns fullpath if ORIGINAL_FULLPATH is not present" do
+    request = stub_request('PATH_INFO'    => "/foo",
+                           'QUERY_STRING' => "bar")
+
+    path = request.original_fullpath
+    assert_equal "/foo?bar", path
+  end
+
 protected
 
   def stub_request(env = {})
diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb
index b20634c5a9..19e8426e60 100644
--- a/railties/lib/rails/application.rb
+++ b/railties/lib/rails/application.rb
@@ -215,6 +215,11 @@ module Rails
       config.helpers_paths
     end
 
+    def call(env)
+      env["ORIGINAL_FULLPATH"] = build_original_fullpath(env)
+      super(env)
+    end
+
   protected
 
     alias :build_middleware_stack :app
@@ -291,5 +296,17 @@ module Rails
       require "rails/console/app"
       require "rails/console/helpers"
     end
+
+    def build_original_fullpath(env)
+      path_info    = env["PATH_INFO"]
+      query_string = env["QUERY_STRING"]
+      script_name  = env["SCRIPT_NAME"]
+
+      if query_string.present?
+        "#{script_name}#{path_info}?#{query_string}"
+      else
+        "#{script_name}#{path_info}"
+      end
+    end
   end
 end
diff --git a/railties/test/application/build_original_fullpath_test.rb b/railties/test/application/build_original_fullpath_test.rb
new file mode 100644
index 0000000000..7a679ea04e
--- /dev/null
+++ b/railties/test/application/build_original_fullpath_test.rb
@@ -0,0 +1,27 @@
+require "abstract_unit"
+
+module ApplicationTests
+  class BuildOriginalPathTest < Test::Unit::TestCase
+    def test_include_original_PATH_info_in_ORIGINAL_FULLPATH
+      env = { 'PATH_INFO' => '/foo/' }
+      assert_equal "/foo/", Rails.application.send(:build_original_fullpath, env)
+    end
+
+    def test_include_SCRIPT_NAME
+      env = {
+        'SCRIPT_NAME' => '/foo',
+        'PATH_INFO' => '/bar'
+      }
+
+      assert_equal "/foo/bar", Rails.application.send(:build_original_fullpath, env)
+    end
+
+    def test_include_QUERY_STRING
+      env = {
+        'PATH_INFO' => '/foo',
+        'QUERY_STRING' => 'bar',
+      }
+      assert_equal "/foo?bar", Rails.application.send(:build_original_fullpath, env)
+    end
+  end
+end
diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb
index 578370cfca..9e02ef9c66 100644
--- a/railties/test/application/middleware_test.rb
+++ b/railties/test/application/middleware_test.rb
@@ -1,5 +1,6 @@
 require 'isolation/abstract_unit'
 require 'stringio'
+require 'rack/test'
 
 module ApplicationTests
   class MiddlewareTest < Test::Unit::TestCase
@@ -75,7 +76,7 @@ module ApplicationTests
       add_to_config "config.force_ssl = true"
       add_to_config "config.ssl_options = { :host => 'example.com' }"
       boot!
-      
+
       assert_equal AppTemplate::Application.middleware.first.args, [{:host => 'example.com'}]
     end
 
@@ -193,6 +194,14 @@ module ApplicationTests
       assert_equal nil, last_response.headers["Etag"]
     end
 
+    test "ORIGINAL_FULLPATH is passed to env" do
+      boot!
+      env = ::Rack::MockRequest.env_for("/foo/?something")
+      Rails.application.call(env)
+
+      assert_equal "/foo/?something", env["ORIGINAL_FULLPATH"]
+    end
+
     private
 
       def boot!