From 53c1ae99f587d6d7f85a3910f97948906ebba877 Mon Sep 17 00:00:00 2001 From: Jarrod Carlson Date: Wed, 5 Oct 2011 00:18:37 -0400 Subject: Added failing test to demonstrate digest authentication failure --- actionpack/test/controller/http_digest_authentication_test.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/actionpack/test/controller/http_digest_authentication_test.rb b/actionpack/test/controller/http_digest_authentication_test.rb index b011536717..25fda58232 100644 --- a/actionpack/test/controller/http_digest_authentication_test.rb +++ b/actionpack/test/controller/http_digest_authentication_test.rb @@ -208,6 +208,16 @@ 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['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please', + :uri => '/http_digest_authentication_test/dummy_digest?') + @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest?" + get :display + + assert_response :success + assert_equal 'Definitely Maybe', @response.body + end + private def encode_credentials(options) -- cgit v1.2.3 From 482ec2ac3f518c91544a3b3e92765eef41b91419 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Tue, 20 Dec 2011 20:17:17 +0100 Subject: Add ORIGINAL_FULLPATH to env This behaves similarly to REQUEST_URI, but we need to implement it on our own because REQUEST_URI is not reliable. Note that since PATH_INFO does not contain information about trailing question mark, this is not 100% accurate, for example `/foo?` will result in `/foo` in ORIGINAL_FULLPATH --- railties/lib/rails/application.rb | 17 ++++++++++++++ .../application/build_original_fullpath_test.rb | 27 ++++++++++++++++++++++ railties/test/application/middleware_test.rb | 11 ++++++++- 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 railties/test/application/build_original_fullpath_test.rb 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! -- cgit v1.2.3 From 80ab9e2a7434a445eaacb097832fbad24ed01960 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Tue, 20 Dec 2011 20:34:04 +0100 Subject: Add original_fullpath and original_url methods to Request --- actionpack/lib/action_dispatch/http/request.rb | 8 ++++++++ actionpack/test/dispatch/request_test.rb | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+) 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/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 = {}) -- cgit v1.2.3 From 3131a9379766a735a80220fd2e94cb07791bed9c Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Tue, 20 Dec 2011 21:01:47 +0100 Subject: Fix http digest authentication with trailing '/' or '?' (fixes #4038 and #3228) --- .../action_controller/metal/http_authentication.rb | 13 ++++--- .../controller/http_digest_authentication_test.rb | 41 +++++++++++++++++++--- 2 files changed, 44 insertions(+), 10 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/test/controller/http_digest_authentication_test.rb b/actionpack/test/controller/http_digest_authentication_test.rb index 25fda58232..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,16 +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['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please', - :uri => '/http_digest_authentication_test/dummy_digest?') - @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest?" + @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) @@ -238,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 -- cgit v1.2.3