Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 4 commits into from

3 participants

@drogus
Collaborator

This is attempt to fix bugs #4038 and #3228. The problem is that PATH_INFO can be changed, so while checking for authentication it can differ from original value. That's why we need to save ORIGINAL_PATH, which acts similarly to REQUEST_URI, but since REQUEST_URI is unreliable, we can't use it.

jcarlson and others added some commits
@jcarlson jcarlson Added failing test to demonstrate digest authentication failure 53c1ae9
@drogus drogus 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
482ec2a
@drogus drogus Add original_fullpath and original_url methods to Request 80ab9e2
@drogus drogus Fix http digest authentication with trailing '/' or '?' (fixes #4038
…and #3228)
3131a93
@josevalim josevalim merged commit 618cb44 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 20, 2011
  1. @jcarlson @drogus
  2. @drogus

    Add ORIGINAL_FULLPATH to env

    drogus authored
    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
  3. @drogus
  4. @drogus
This page is out of date. Refresh to see the latest.
View
13 actionpack/lib/action_controller/metal/http_authentication.rb
@@ -192,12 +192,15 @@ def validate_digest_response(request, realm, &password_procedure)
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
View
8 actionpack/lib/action_dispatch/http/request.rb
@@ -122,10 +122,18 @@ def headers
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
View
45 actionpack/test/controller/http_digest_authentication_test.rb
@@ -139,7 +139,7 @@ def authenticate_with_request
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 @@ def authenticate_with_request
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 @@ def encode_credentials(options)
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
View
24 actionpack/test/dispatch/request_test.rb
@@ -618,6 +618,30 @@ def url_for(options = {})
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 = {})
View
17 railties/lib/rails/application.rb
@@ -215,6 +215,11 @@ def helpers_paths #:nodoc:
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 @@ def initialize_console #:nodoc:
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
View
27 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
View
11 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 @@ def app
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 @@ def index
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!
Something went wrong with that request. Please try again.