Fix: Fixes digest authentication with Cookies #235

dennissivia · 2018-06-20T12:41:40Z

Bug description

In issue #234 a bug regarding digest authentication has been reported.
In order to reproduce it there are two steps necessary:

Make sure the app responds with 401 and a digest challange
Make sure a Set-Cookie header is set.
If these two conditions are fulfilled, the reported error will
occur:

NoMethodError:
  undefined method `host' for "/digest":String

The stack trace already indicates that the issue is taking effect in
the cookie jar class.

 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:25:in `initialize'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:149:in `new'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:149:in `block in merge'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:148:in `each'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:148:in `merge'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/mock_session.rb:36:in `request'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:261:in `process_request'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:267:in `process_request'
 .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:119:in `request'
 ./spec/requests/api_spec.rb:16:in `block (3 levels) in <top (required)>'

Problem (root cause) description

When a cookie is set and digest authentication is requested, we
invalidly pass uri.path to process_request instead of just uri.

Detailled explanation

After some research I came to the following result.
When a digest request is issued the following if-condition is executed:

https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test.rb#L261
There process_request calls itself with uri.path instead of uri.

Next @rack_mock_session.request is called, which in turn calls
CookieJar#merge
with last_response.headers['Set-Cookie'].
Merge has a early return if nil is passed as the argument see code.
However if Set-Cookie is provided (a cookie is sent), then the cookie
is split and cookie objects are created per line. When this happends
the uri is passed (which in case of the .path invocation is just a string),
and then the initializer calls uri.host here.
This will then trigger the exception above.

This description also makes clear that this bugs only occurs, when the
user is testing auth-digest and also setting a header during the
authentication. However according to the sample rails app provided,
this might be a normal case for rails apps.
(depending for what they use cookies)

mjacobus

LGTM

mjacobus · 2018-06-20T12:50:59Z

spec/rack/test_spec.rb

+
+      session.request('/')
+      auth_headers = session.last_request.env['HTTP_AUTHORIZATION']
+      challange_data = 'realm="test-realm", qop="auth", nonce="nonsensenonce", opaque="morenonsense"'


mjacobus · 2018-06-20T16:04:40Z

spec/rack/test_spec.rb

@@ -356,6 +356,73 @@ def close
    end
  end

+  describe '#digest_authorize' do
+    let(:challange_data) do


typo, I guess: challenge_data ?

perlun

Thanks @scepticulous. As noted, I have no experience with Digest auth, so I'll take your word for the details there. The general style/"look and feel" of this PR is fine so feel to merge at your convenience.

junaruga · 2018-06-21T20:27:36Z

I will try to review it tomorrow.

junaruga · 2018-06-25T10:42:50Z

spec/rack/test_spec.rb

@@ -341,7 +341,7 @@ def close

  describe '#basic_authorize' do
    it 'sets the HTTP_AUTHORIZATION header' do
-      authorize 'bryan', 'secret'
+      basic_authorize 'bryan', 'secret'


This modification is not related to this fix directly, right?

Yes, exactly. I just noticed while I was working on the new tests.

As basic_authorize is the alias of authorize and it is not harmful without changing, I feel we can create another PR.

Yeah, this can go in another PR for tidiness.

junaruga · 2018-06-25T10:58:33Z

spec/rack/test_spec.rb

@@ -356,6 +356,69 @@ def close
    end
  end

+  describe '#digest_authorize' do


The unit tests for the digest auth has already been at spec/rack/test/digest_auth_spec.rb.
It might be good to add the new tests to spec/rack/test/digest_auth_spec.rb.
But in other words, in this time the code we want to test is at lib/rack/test.rb.
So, adding the unit tests at spec/rack/test_spec.rb might be good.

How do you think?

I vote for spec/rack/test_spec.rb, so it's easier to find the specs by looking at the file structure.

junaruga · 2018-06-25T11:14:22Z

spec/spec_helper.rb

@@ -18,6 +18,8 @@
  config.mock_with :rspec
  config.include Rack::Test::Methods

+  config.filter_run_when_matching :focus


This line for focus and to manage this kind of test it 'raises an error if no requests have been issued', focus: true is not related to the main issue directly isn't it?
We can create another PR for that.
How do you think?

I agree. I actually just left it in the PR to ask you and @perlun for your opinion of having it in our configuration. If it is fine with both of you, I will remove the line and add a new PR.

I'm fine with that change. For reference: https://relishapp.com/rspec/rspec-core/docs/filtering/filter-run-when-matching

(We shouldn't have any specs focused in the committed tree, so please remove the focus: true in your PR for that also.)

junaruga · 2018-06-25T11:14:46Z

spec/rack/test_spec.rb

@@ -446,7 +509,7 @@ def close
      expect(last_response['Content-Type']).to eq('text/html;charset=utf-8')
    end

-    it 'raises an error if no requests have been issued', focus: true do
+    it 'raises an error if no requests have been issued' do


Same with spec/spec_helper.rb the line config.filter_run_when_matching :focus.

I think removing it is quiet important, otherwise it might lead to unexpected and invalidly green tests by accident. So I would remove it asap, because I guess its a leftover and its not intended to be there.

@scepticulous yes that's true.

How about creating new 1 PR to modify the 2 lines at first before merging this PR?

config.filter_run_when_matching :focus

and

it 'raises an error if no requests have been issued', focus: true

junaruga · 2018-06-25T11:15:46Z

@scepticulous I commented some things. Thanks for the PR!

perlun · 2018-06-25T19:28:29Z

Yes thanks @scepticulous. Some feedback from me also.

perlun · 2018-07-22T10:53:11Z

@scepticulous Ping, any updates on this?

dennissivia · 2018-07-22T11:03:38Z

@perlun I did not have time since your initial review. I am going to implement the suggestions during July, as soon as I have time. 🤞

stale · 2018-09-20T11:11:03Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

dennissivia · 2018-09-23T11:30:19Z

I will have time to address the mentioned issues during the coming week. I hope we can get the PR merged then.

In order to focus on single tests during development rspec supports the option `filter_run_when_matching`. We agreed, that it is fine for us to support this in rack#235. Since it is important to not push :focus tags to the sourcetree, a leftover has been removed `focus: true`. Finally an inconsistency in the use of `basic_authorize` and `authorize` has been fixed. This has additional value, since this PR is the base for PR rack#235, which is supposed to add more tests for digest_authorization. Since we will have different ways to authorize, it is very helpful to make the method explicit within our tests.

In order to focus on single tests during development rspec supports the option `filter_run_when_matching`. We agreed, that it is fine for us to support this in #235. Since it is important to not push :focus tags to the sourcetree, a leftover has been removed `focus: true`. Finally an inconsistency in the use of `basic_authorize` and `authorize` has been fixed. This has additional value, since this PR is the base for PR #235, which is supposed to add more tests for digest_authorization. Since we will have different ways to authorize, it is very helpful to make the method explicit within our tests.

junaruga · 2018-09-24T10:25:06Z

@scepticulous Yeah, I can help for the review. As #237 was merged now, can you rebase this PR?

Bug description ----- In issue rack#234 a bug regarding digest authentication has been reported. In order to reproduce it there are two steps necessary: 1. Make sure the app responds with 401 and a digest challange 2. Make sure a Set-Cookie header is set. If these two conditions are fulfilled, the reported error will occur: ``` NoMethodError: undefined method `host' for "/digest":String ``` The stack trace already indicates that the issue is taking effect in the cookie jar class. ``` .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:25:in `initialize' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:149:in `new' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:149:in `block in merge' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:148:in `each' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:148:in `merge' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/mock_session.rb:36:in `request' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:261:in `process_request' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:267:in `process_request' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:119:in `request' ./spec/requests/api_spec.rb:16:in `block (3 levels) in <top (required)>' ``` Problem (root cause) description ----- When a cookie is set and digest authentication is requested, we invalidly pass `uri.path` to `process_request` instead of just `uri`. Detailled explanation ---- After some research I came to the following result. When a digest request is issued the following if-condition is executed: * https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test.rb#L261 There `process_request` calls itself with `uri.path` instead of `uri`. Next `@rack_mock_session.request` is called, which in turn calls [CookieJar#merge](https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/mock_session.rb#L34) with `last_response.headers['Set-Cookie']`. Merge has a early return if nil is passed as the argument [see code](https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test/cookie_jar.rb#L139). However if Set-Cookie is provided (a cookie is sent), then the cookie is split and cookie objects are created per line. When this happends the uri is passed (which in case of the .path invocation is just a string), and then the initializer calls uri.host [here](https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test/cookie_jar.rb#L23). This will then trigger the exception above. This description also makes clear that this bugs only occurs, when the user is testing auth-digest and also setting a header during the authentication. However according to the sample rails app provided, this might be a normal case for rails apps. (depending for what they use cookies)

Fixes rubocop complains and refactors challange_data so that it is reused in the request setup and the verification. This removes duplication and makes clear that we expect the same data in both parts.

dennissivia · 2018-09-24T11:28:56Z

Rebase is done :)

junaruga · 2018-09-24T12:02:55Z

@scepticulous It looks good to me!
Could you merge this PR by yourself?
I want to ask you to merge with "Squash and merge" button, adjusting the commit text.

junaruga · 2018-09-24T12:15:21Z

Thanks for your working!

In order to focus on single tests during development rspec supports the option `filter_run_when_matching`. We agreed, that it is fine for us to support this in rack#235. Since it is important to not push :focus tags to the sourcetree, a leftover has been removed `focus: true`. Finally an inconsistency in the use of `basic_authorize` and `authorize` has been fixed. This has additional value, since this PR is the base for PR rack#235, which is supposed to add more tests for digest_authorization. Since we will have different ways to authorize, it is very helpful to make the method explicit within our tests.

Fix: Fixes digest authentication with Cookies Bug description ----- In issue rack#234 a bug regarding digest authentication has been reported. In order to reproduce it there are two steps necessary: 1. Make sure the app responds with 401 and a digest challange 2. Make sure a Set-Cookie header is set. If these two conditions are fulfilled, the reported error will occur: ``` NoMethodError: undefined method `host' for "/digest":String ``` The stack trace already indicates that the issue is taking effect in the cookie jar class. ``` .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:25:in `initialize' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:149:in `new' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:149:in `block in merge' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:148:in `each' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test/cookie_jar.rb:148:in `merge' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/mock_session.rb:36:in `request' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:261:in `process_request' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:267:in `process_request' .../ruby-2.5.1/gems/rack-test-1.0.0/lib/rack/test.rb:119:in `request' ./spec/requests/api_spec.rb:16:in `block (3 levels) in <top (required)>' ``` Problem (root cause) description ----- When a cookie is set and digest authentication is requested, we invalidly pass `uri.path` to `process_request` instead of just `uri`. Detailled explanation ---- After some research I came to the following result. When a digest request is issued the following if-condition is executed: * https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test.rb#L261 There `process_request` calls itself with `uri.path` instead of `uri`. Next `@rack_mock_session.request` is called, which in turn calls [CookieJar#merge](https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/mock_session.rb#L34) with `last_response.headers['Set-Cookie']`. Merge has a early return if nil is passed as the argument [see code](https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test/cookie_jar.rb#L139). However if Set-Cookie is provided (a cookie is sent), then the cookie is split and cookie objects are created per line. When this happends the uri is passed (which in case of the .path invocation is just a string), and then the initializer calls uri.host [here](https://github.com/rack-test/rack-test/blob/v1.0.0/lib/rack/test/cookie_jar.rb#L23). This will then trigger the exception above. This description also makes clear that this bugs only occurs, when the user is testing auth-digest and also setting a header during the authentication. However according to the sample rails app provided, this might be a normal case for rails apps. (depending for what they use cookies)

dennissivia requested a review from perlun June 20, 2018 12:42

mjacobus approved these changes Jun 20, 2018

View reviewed changes

dennissivia requested a review from junaruga June 21, 2018 06:22

perlun mentioned this pull request Jun 21, 2018

The road towards 2.0 #225

Closed

perlun approved these changes Jun 21, 2018

View reviewed changes

junaruga reviewed Jun 25, 2018

View reviewed changes

stale bot added the wontfix label Sep 20, 2018

stale bot removed the wontfix label Sep 23, 2018

dennissivia mentioned this pull request Sep 24, 2018

Sets up focus filter for tests and fixes inconsistency #237

Merged

dennissivia added 3 commits September 24, 2018 12:39

Fix: Fixes style violations

a9eacc0

Fixes rubocop complains and refactors challange_data so that it is reused in the request setup and the verification. This removes duplication and makes clear that we expect the same data in both parts.

Fixes typo

12f3ba1

dennissivia force-pushed the bugfix/fix-digest-authentication-with-cookies branch from fc3f874 to 12f3ba1 Compare September 24, 2018 10:40

dennissivia merged commit 6c07bf5 into rack:master Sep 24, 2018

dennissivia mentioned this pull request Oct 24, 2018

digest_authorize fails #234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Fixes digest authentication with Cookies #235

Fix: Fixes digest authentication with Cookies #235

dennissivia commented Jun 20, 2018

mjacobus left a comment

mjacobus Jun 20, 2018

mjacobus Jun 20, 2018

perlun left a comment

junaruga commented Jun 21, 2018

junaruga Jun 25, 2018

dennissivia Jun 25, 2018

junaruga Jun 25, 2018

perlun Jun 25, 2018

junaruga Jun 25, 2018

perlun Jun 25, 2018

junaruga Jun 25, 2018

dennissivia Jun 25, 2018

perlun Jun 25, 2018

junaruga Jun 25, 2018

dennissivia Jun 25, 2018

junaruga Jun 25, 2018

junaruga commented Jun 25, 2018 •

edited

Loading

perlun commented Jun 25, 2018

perlun commented Jul 22, 2018

dennissivia commented Jul 22, 2018

stale bot commented Sep 20, 2018

dennissivia commented Sep 23, 2018

junaruga commented Sep 24, 2018

dennissivia commented Sep 24, 2018

junaruga commented Sep 24, 2018 •

edited

Loading

junaruga commented Sep 24, 2018

Fix: Fixes digest authentication with Cookies #235

Fix: Fixes digest authentication with Cookies #235

Conversation

dennissivia commented Jun 20, 2018

Bug description

Problem (root cause) description

Detailled explanation

mjacobus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlun left a comment

Choose a reason for hiding this comment

junaruga commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junaruga commented Jun 25, 2018 • edited Loading

perlun commented Jun 25, 2018

perlun commented Jul 22, 2018

dennissivia commented Jul 22, 2018

stale bot commented Sep 20, 2018

dennissivia commented Sep 23, 2018

junaruga commented Sep 24, 2018

dennissivia commented Sep 24, 2018

junaruga commented Sep 24, 2018 • edited Loading

junaruga commented Sep 24, 2018

junaruga commented Jun 25, 2018 •

edited

Loading

junaruga commented Sep 24, 2018 •

edited

Loading