Http token parser bug #7312

krainboltgreene · 2012-08-10T04:11:44Z

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

arunagw · 2012-08-10T04:12:52Z

Please also squash commits into one :-)

krainboltgreene · 2012-08-10T21:43:36Z

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

drogus · 2012-08-11T17:48:54Z

Also, don't forget about CHANGELOG entry and proper commit message as stated here: http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#updating-the-changelog and here: http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#commit-your-changes

krainboltgreene · 2012-10-07T22:41:03Z

Tests showing the bug:

Running tests:

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

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

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"
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+"
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"
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 · 2012-10-07T22:41:38Z

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

bquorning · 2012-11-07T21:35:25Z

actionpack/lib/action_controller/metal/http_authentication.rb

        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://.*$}

Originally there was a complex expression that had escapes. This was a quick way to make it easier to read. My refactor removed the need for he complex expression, but forgot to remove the change in syntax.

On Nov 7, 2012, at 1:35 PM, Benjamin Quorning notifications@github.com wrote:

In actionpack/lib/action_controller/metal/http_authentication.rb:

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://.*$}
—
Reply to this email directly or view it on GitHub.

steveklabnik · 2012-12-15T22:10:33Z

actionpack/test/controller/http_token_authentication_test.rb

-  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


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

steveklabnik · 2012-12-15T22:12:33Z

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

rename those tests
take off the %rs as discussed
squash
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 · 2012-12-15T22:15:37Z

Sure, I'll get it in tonight.

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

krainboltgreene · 2012-12-15T23:57:44Z

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 · 2012-12-16T01:15:02Z

🤘

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

kalv · 2012-12-19T16:29:23Z

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

rafaelfranca · 2012-12-21T22:00:53Z

Seems good to me.

Http token parser bug

steveklabnik · 2012-12-21T23:25:54Z

Awesome.

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

krainboltgreene · 2012-12-21T23:50:05Z

But seriously, was worth the wait.

bquorning reviewed Nov 7, 2012
View changes

al2o3cr mentioned this pull request Nov 26, 2012

authenticate_or_request_with_http_token breaks when token contains an equals sign #8196

Closed

steveklabnik reviewed Dec 15, 2012
View changes

steveklabnik added a commit that referenced this pull request Dec 21, 2012

Merge pull request #7312 from krainboltgreene/http-token-parser-bug

625699a

Http token parser bug

steveklabnik merged commit 625699a into rails:master Dec 21, 2012

This was referenced Dec 21, 2012

Fixing #8196, Http auth token breaks with equals sign #8208

Closed

Authentication With Token Problem #7311

Closed

Http token parser bug #7312

Http token parser bug #7312

krainboltgreene commented Aug 10, 2012

arunagw commented Aug 10, 2012

krainboltgreene commented Aug 10, 2012

drogus commented Aug 11, 2012

krainboltgreene commented Oct 7, 2012

krainboltgreene commented Oct 7, 2012

bquorning Nov 7, 2012

krainboltgreene Nov 7, 2012

steveklabnik Dec 15, 2012

steveklabnik commented Dec 15, 2012

krainboltgreene commented Dec 15, 2012

krainboltgreene commented Dec 15, 2012

steveklabnik commented Dec 16, 2012

kalv commented Dec 19, 2012

rafaelfranca commented Dec 21, 2012

steveklabnik commented Dec 21, 2012

krainboltgreene commented Dec 21, 2012

Http token parser bug #7312

Http token parser bug #7312

Conversation

krainboltgreene commented Aug 10, 2012

arunagw commented Aug 10, 2012

krainboltgreene commented Aug 10, 2012

drogus commented Aug 11, 2012

krainboltgreene commented Oct 7, 2012

Running tests:

krainboltgreene commented Oct 7, 2012

Running tests:

Running tests:

bquorning Nov 7, 2012

Choose a reason for hiding this comment

krainboltgreene Nov 7, 2012

Choose a reason for hiding this comment

steveklabnik Dec 15, 2012

Choose a reason for hiding this comment

steveklabnik commented Dec 15, 2012

krainboltgreene commented Dec 15, 2012

krainboltgreene commented Dec 15, 2012

steveklabnik commented Dec 16, 2012

kalv commented Dec 19, 2012

rafaelfranca commented Dec 21, 2012

steveklabnik commented Dec 21, 2012

krainboltgreene commented Dec 21, 2012