New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

digest_authorize fails #234

Closed
ShikherVerma opened this Issue May 10, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@ShikherVerma

ShikherVerma commented May 10, 2018

Trying to run:

context 'testing digest auth' do
  it 'returns ok' do
    digest_authorize 'dhh', 'secret'
    request 'http://localhost:3000/digest'
    assert_equal 200, last_response.status
  end
end

fails with:

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

I have made an example repo which can be used to reproduce the bug. Just clone the project and run tests with rake spec.

I think this is the fix. Since the process_request function expects a Uri as the first parameter.

You can verify that this fixes the problem by adding:

# bug fixed version
gem 'rack-test', :git => 'git@github.com:ShikherVerma/rack-test.git'

to the group :test of the example repo gemfile and rerunning the tests in the example repo.

@ShikherVerma

This comment has been minimized.

ShikherVerma commented May 10, 2018

I'll be happy to send a PR to fix this 😄

@perlun

This comment has been minimized.

Contributor

perlun commented May 28, 2018

@ShikherVerma Thank you for this! Please submit a PR about this, preferably with a unit test included that demonstrates the fix.

(Confession: I've never ever used digest authorization myself.)

@scepticulous

This comment has been minimized.

Contributor

scepticulous commented Jun 20, 2018

@ShikherVerma I will take a look at your finding and will give you feedback asap. Thanks for pointing out the issue.
It would be amazing, If you could provide a PR that includes tests for this case.

scepticulous added a commit to scepticulous/rack-test that referenced this issue Jun 20, 2018

Fix: Fixes digest authentication with Cookies
Bug description
-----

In issue rack-test#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)
@ShikherVerma

This comment has been minimized.

ShikherVerma commented Jun 20, 2018

@scepticulous I tried but got stuck at these tests test/digest_auth_spec.rb. Couldn't understand why these were passing. Thanks for taking up this issue 👍

@scepticulous scepticulous added the bug label Jun 21, 2018

@stale

This comment has been minimized.

stale bot commented Aug 20, 2018

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.

@stale stale bot added the wontfix label Aug 20, 2018

@perlun

This comment has been minimized.

Contributor

perlun commented Aug 24, 2018

@scepticulous @ShikherVerma Either one of you interested in completing this?

@stale stale bot removed the wontfix label Aug 24, 2018

scepticulous added a commit to scepticulous/rack-test that referenced this issue Sep 24, 2018

Fix: Fixes digest authentication with Cookies
Bug description
-----

In issue rack-test#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)

scepticulous added a commit that referenced this issue Sep 24, 2018

Fix: Fixes digest authentication with Cookies (#235)
Fix: Fixes digest authentication with Cookies

Bug description
-----

In issue #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)
@stale

This comment has been minimized.

stale bot commented Oct 23, 2018

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.

@stale stale bot added the wontfix label Oct 23, 2018

@scepticulous

This comment has been minimized.

Contributor

scepticulous commented Oct 24, 2018

@ShikherVerma this issue should be solved by the merge of #235.
Thus I am closing this issue, please let us know if the solution is not completely fixing this issue for you.

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