Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Http token parser bug #7312

Merged
merged 1 commit into from

7 participants

@krainboltgreene

Fixing a problem detailed in #7311. Not ready to pull until that discussion is resolved.

@arunagw
Collaborator

Please also squash commits into one :-)

@krainboltgreene

Yup, will do, I still have tests to write and code might change when the discussion is over.

@krainboltgreene

Tests showing the bug:

Running tests:

....................FFFF

Finished tests in 0.057636s, 416.4064 tests/s, 815.4626 assertions/s.

1) Failure: test_0021_token_and_options returns correct token, equal bug(HttpTokenAuthenticationTest) [/Users/krainboltgreene/Code/Ruby/rails/rails/actionpack/test/controller/http_token_authentication_test.rb:120]:
Expected: "rcHu+HzSFw89Ypyhn/896A=="
Actual: "rcHu+HzSFw89Ypyhn/896A"

2) Failure: test_0022_token_and_options returns correct token, after equal bug(HttpTokenAuthenticationTest) [/Users/krainboltgreene/Code/Ruby/rails/rails/actionpack/test/controller/http_token_authentication_test.rb:127]:
Expected: "rcHu+=HzSFw89Ypyhn/896A==f34"
Actual: "rcHu+"

3) Failure: test_0023_token_and_options returns correct token, slash bug(HttpTokenAuthenticationTest) [/Users/krainboltgreene/Code/Ruby/rails/rails/actionpack/test/controller/http_token_authentication_test.rb:134]:
Expected: "rcHu+\\\"/896A"
Actual: "rcHu+\\"/896A"

4) Failure: test_0024_token_and_options returns correct token, slash quote bug(HttpTokenAuthenticationTest) [/Users/krainboltgreene/Code/Ruby/rails/rails/actionpack/test/controller/http_token_authentication_test.rb:141]:
Expected: "\\"quote\\" pretty"
Actual: "\"quote\" pretty"

24 tests, 47 assertions, 4 failures, 0 errors, 0 skips
rake aborted!
Command failed with status (1): [/Users/krainboltgreene/.rvm/rubies/ruby-1....]

Tasks: TOP => test => test_action_pack
(See full trace by running task with --trace)

@krainboltgreene

I've refactored this pull request to add tests showing the bugs in the initial code and re-added the fixes. Here are the tests after the fixes:

Running tests:

.......................

Finished tests in 0.053864s, 427.0013 tests/s, 816.8721 assertions/s.

23 tests, 44 assertions, 0 failures, 0 errors, 0 skips
Run options: --seed 4433

Running tests:

.......................

Finished tests in 0.048848s, 470.8483 tests/s, 900.7534 assertions/s.

23 tests, 44 assertions, 0 failures, 0 errors, 0 skips

...ck/lib/action_controller/metal/http_authentication.rb
((19 lines not shown))
end
end
+ def token_params_from(authorization)
+ raw = authorization.sub(%r/^Token /, '').split %r/"\s*(?:,|;|\t+)\s*/

4 times in this diff you're using %r/.../ do declare a regular expression. Why not just /.../?

In my code, I only use %r when I don't want to escape forward slashes in my regex, e.g.

url =~ %r{^http://.*$}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ack/test/controller/http_token_authentication_test.rb
@@ -104,17 +104,40 @@ def authenticate_long_credentials
assert_equal 'Token realm="SuperSecret"', @response.headers['WWW-Authenticate']
end
- test "authentication request with valid credential" do
- @request.env['HTTP_AUTHORIZATION'] = encode_credentials('"quote" pretty', :algorithm => 'test')
- get :display
+ test "token_and_options returns correct token, equal bug" do
@steveklabnik Collaborator

We shouldn't be referencing the bug here. There's no more bug after the code is done. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ack/test/controller/http_token_authentication_test.rb
@@ -104,17 +104,40 @@ def authenticate_long_credentials
assert_equal 'Token realm="SuperSecret"', @response.headers['WWW-Authenticate']
end
- test "authentication request with valid credential" do
- @request.env['HTTP_AUTHORIZATION'] = encode_credentials('"quote" pretty', :algorithm => 'test')
- get :display
+ test "token_and_options returns correct token, equal bug" do
+ token = "rcHu+HzSFw89Ypyhn/896A=="
+ actual = ActionController::HttpAuthentication::Token.token_and_options(sample_request(token)).first
+ expected = token
+ assert_equal(expected, actual)
+ end
+
+ test "token_and_options returns correct token, after equal bug" do
@steveklabnik Collaborator

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@steveklabnik
Collaborator

@krainboltgreene , I'm interested in merging this. Things I need:

  1. rename those tests
  2. take off the %rs as discussed
  3. squash
  4. changelog

I'm also gonna double check with someone else on core, but this all seems good to me, and would solve #8208, #8196, and #7311.

@krainboltgreene

Sure, I'll get it in tonight.

Kurtis Rainbolt-Greene Refactoring the token_and_options method to fix bugs
Adding a test for the equal trun bug

Adding a test for the after equal trunc bug

Adding a test for the slash bug

Adding a test for the slash quote bug

Adding a helper method for creating a sample request object with token

Writing a method to create params array from raw params

Writing a method to rewrite param values in the params

Writing a method to get the token params from an authorization value

Refactoring the token_and_options method to fix bugs

Removing unnessecary test

A constant for this shared regex seemed appropriate

Wanting to split up this logic

Adding small documentation pieces
f71cca9
@krainboltgreene
Run options: --seed 29311

# Running tests:

.......................

Finished tests in 0.036201s, 635.3416 tests/s, 1215.4360 assertions/s.

23 tests, 44 assertions, 0 failures, 0 errors, 0 skips
Run options: --seed 49338

# Running tests:

.......................

Finished tests in 0.051028s, 450.7329 tests/s, 862.2717 assertions/s.

23 tests, 44 assertions, 0 failures, 0 errors, 0 skips
@steveklabnik
Collaborator

:metal:

If I can get a :+1: from one other person, then I'll put this in. @tenderlove @NZKoz, or someone else on core? It looks good to me.

@kalv

This work fixes my issues with handling the "=" character and other characters than what I did in my pull request #8208

@rafaelfranca

Seems good to me. :shipit:

@steveklabnik steveklabnik merged commit 625699a into from
@steveklabnik
Collaborator

Awesome. :shipit:

@krainboltgreene, thank you so much for your contribution. I know we took a long time to take care of this. :heart:

@krainboltgreene

Aww yeah

But seriously, was worth the wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 15, 2012
  1. @krainboltgreene

    Refactoring the token_and_options method to fix bugs

    Kurtis Rainbolt-Greene authored krainboltgreene committed
    Adding a test for the equal trun bug
    
    Adding a test for the after equal trunc bug
    
    Adding a test for the slash bug
    
    Adding a test for the slash quote bug
    
    Adding a helper method for creating a sample request object with token
    
    Writing a method to create params array from raw params
    
    Writing a method to rewrite param values in the params
    
    Writing a method to get the token params from an authorization value
    
    Refactoring the token_and_options method to fix bugs
    
    Removing unnessecary test
    
    A constant for this shared regex seemed appropriate
    
    Wanting to split up this logic
    
    Adding small documentation pieces
This page is out of date. Refresh to see the latest.
View
38 actionpack/lib/action_controller/metal/http_authentication.rb
@@ -384,6 +384,8 @@ def opaque(secret_key)
#
# RewriteRule ^(.*)$ dispatch.fcgi [E=X-HTTP_AUTHORIZATION:%{HTTP:Authorization},QSA,L]
module Token
+ TOKEN_REGEX = /^Token /
+ AUTHN_PAIR_DELIMITERS = /(?:,|;|\t+)/
extend self
module ControllerMethods
@@ -431,20 +433,34 @@ def authenticate(controller, &login_procedure)
# Returns an Array of [String, Hash] if a token is present.
# Returns nil if no token is found.
def token_and_options(request)
- if request.authorization.to_s[/^Token (.*)/]
- values = Hash[$1.split(',').map do |value|
- value.strip! # remove any spaces between commas and values
- key, value = value.split(/\=\"?/) # split key=value pairs
- if value
- value.chomp!('"') # chomp trailing " in value
- value.gsub!(/\\\"/, '"') # unescape remaining quotes
- [key, value]
- end
- end.compact]
- [values.delete("token"), values.with_indifferent_access]
+ authorization_request = request.authorization.to_s
+ if authorization_request[TOKEN_REGEX]
+ params = token_params_from authorization_request
+ [params.shift.last, Hash[params].with_indifferent_access]
end
end
+ def token_params_from(auth)
+ rewrite_param_values params_array_from raw_params auth
+ end
+
+ # Takes raw_params and turns it into an array of parameters
+ def params_array_from(raw_params)
+ raw_params.map { |param| param.split %r/=(.+)?/ }
+ end
+
+ # This removes the `"` characters wrapping the value.
+ def rewrite_param_values(array_params)
+ array_params.each { |param| param.last.gsub! %r/^"|"$/, '' }
+ end
+
+ # This method takes an authorization body and splits up the key-value
+ # pairs by the standardized `:`, `;`, or `\t` delimiters defined in
+ # `AUTHN_PAIR_DELIMITERS`.
+ def raw_params(auth)
+ auth.sub(TOKEN_REGEX, '').split /"\s*#{AUTHN_PAIR_DELIMITERS}\s*/
+ end
+
# Encodes the given token and options into an Authorization header value.
#
# token - String token.
View
35 actionpack/test/controller/http_token_authentication_test.rb
@@ -104,17 +104,40 @@ def authenticate_long_credentials
assert_equal 'Token realm="SuperSecret"', @response.headers['WWW-Authenticate']
end
- test "authentication request with valid credential" do
- @request.env['HTTP_AUTHORIZATION'] = encode_credentials('"quote" pretty', :algorithm => 'test')
- get :display
+ test "token_and_options returns correct token" do
+ token = "rcHu+HzSFw89Ypyhn/896A=="
+ actual = ActionController::HttpAuthentication::Token.token_and_options(sample_request(token)).first
+ expected = token
+ assert_equal(expected, actual)
+ end
+
+ test "token_and_options returns correct token" do
+ token = 'rcHu+=HzSFw89Ypyhn/896A==f34'
+ actual = ActionController::HttpAuthentication::Token.token_and_options(sample_request(token)).first
+ expected = token
+ assert_equal(expected, actual)
+ end
- assert_response :success
- assert assigns(:logged_in)
- assert_equal 'Definitely Maybe', @response.body
+ test "token_and_options returns correct token" do
+ token = 'rcHu+\\\\"/896A'
+ actual = ActionController::HttpAuthentication::Token.token_and_options(sample_request(token)).first
+ expected = token
+ assert_equal(expected, actual)
+ end
+
+ test "token_and_options returns correct token" do
+ token = '\"quote\" pretty'
+ actual = ActionController::HttpAuthentication::Token.token_and_options(sample_request(token)).first
+ expected = token
+ assert_equal(expected, actual)
end
private
+ def sample_request(token)
+ @sample_request ||= OpenStruct.new authorization: %{Token token="#{token}"}
+ end
+
def encode_credentials(token, options = {})
ActionController::HttpAuthentication::Token.encode_credentials(token, options)
end
Something went wrong with that request. Please try again.