Skip to content
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

Basic authentication fails when specifying credentials for a realm that contains uppercase characters #458

Closed
chris-reeves opened this issue Jan 6, 2016 · 7 comments

Comments

@chris-reeves
Copy link
Contributor

Basic authentication works fine when credentials are added for a URI and the realm is not specified, or when the realm contains no uppercase characters. When credentials are added for a specific URI and realm combination, and the realm name contains uppercase characters, authentication will fail and throw an exception (Mechanize::UnauthorizedError).

Analysis

response_authenticate correctly determines that credentials exist in the AuthStore for the given URI and realm, but when the realm is extracted from the challenge in the following line:
realm = challenge.realm uri
the realm name in the AuthRealm object that is created has been converted to lowercase. The subsequent calls to request_auth and credentials_for fail to find credentials for the lowercased realm name.

As per RFC 1945 (section 11) and RFC 2617, the realm name is case sensitive.

The bug has been traced to Mechanize::HTTP::AuthRealm#initialize, which calls downcase on the realm parameter.

Test Case

  • ruby 2.1.3p242 (2014-09-19 revision 47630) [x86_64-linux]
  • mechanize-2.7.3

Sample code

agent = Mechanize.new
agent.add_auth('https://httpbin.org/basic-auth/user/pass', 'user', 'pass', 'Fake Realm')
agent.get('https://httpbin.org/basic-auth/user/pass')
puts "Successfully retrieved test page" if agent.page.code == '200'

Expected behaviour

This should output "Successfully retrieved test page".

Actual behaviour

Mechanize throws an exception:

/home/chris/.rvm/gems/ruby-2.1.3/gems/mechanize-2.7.4/lib/mechanize/http/agent.rb:789:in `response_authenticate': 401 => Net::HTTPUnauthorized for https://httpbin.org/basic-auth/user/pass -- Basic authentication failed -- available realms: Fake Realm (Mechanize::UnauthorizedError)
        from /home/chris/.rvm/gems/ruby-2.1.3/gems/mechanize-2.7.4/lib/mechanize/http/agent.rb:310:in `fetch'
        from /home/chris/.rvm/gems/ruby-2.1.3/gems/mechanize-2.7.4/lib/mechanize/http/agent.rb:798:in `response_authenticate'
        from /home/chris/.rvm/gems/ruby-2.1.3/gems/mechanize-2.7.4/lib/mechanize/http/agent.rb:310:in `fetch'
        from /home/chris/.rvm/gems/ruby-2.1.3/gems/mechanize-2.7.4/lib/mechanize.rb:464:in `get'
@knu
Copy link
Member

knu commented Jan 7, 2016

This should be fixed, but there would be a compatibility problem if we just stopped downcasing. I think it should first try exact match and if it fails then fall back to case insensitive match. I'll implement it if no one does.

@knu
Copy link
Member

knu commented Jan 7, 2016

Ah, I may have misread your analysis. Your fix is probably the right way to go.

@chris-reeves
Copy link
Contributor Author

When I initially looked at this I also thought there was the potential for compatibility issues, but I think we should be OK.

I did try adding credentials using a downcased equivalent of my realm name to try and work around this, but then the check towards the top of response_authenticate:
unless @auth_store.credentials? uri, challenges then
fails to find the credentials and aborts at that point.

Further analysis

AuthRealms are only created by instances of AuthChallenge, which are only created by the WWWAuthenticateParser, which is only called from within response_authenticate. Having said that, there must be something that I'm missing because I can't figure out how @authenticate_methods is populated after it is initialised.

So assuming that any code which creates AuthRealms must have passed through response_authenticate at some point, then I don't see how credentials tied to mixed-case realms could be working at this time: if the credentials are added for a realm that doesn't string-equal the realm that the server specifies (e.g. downcased) then the first check in response_authenticate (above) will fail as the check uses AuthChallenge#realm_name as the realm name (which has not been modified); if the credentials are added for the actual mixed-case realm then the code will make it past that check and then fail when trying to retrieve credentials in request_auth, which uses the downcased realm name from the AuthRealm object stored in @authenticate_methods (and which does not string-equal the realm name of the credentials in the AuthStore).

@chris-reeves
Copy link
Contributor Author

Is there anything further that you would like me to do in order to get this merged? My analysis suggests that this isn't a breaking change so should be safe.

The main area of uncertainty was around how @authenticate_methods was being populated, but looking more closely at the code I worked it out so I believe the analysis to be sound.

@knu
Copy link
Member

knu commented Feb 19, 2016

Sorry for keeping you waiting so long, it's only due to my having limited time to work on this project. Will look into #459.

@knu knu closed this as completed in bdf2bf3 Feb 19, 2016
knu added a commit that referenced this issue Feb 19, 2016
Fix issue #458: Basic authentication fails when specifying credentials for a realm that contains uppercase characters
@knu
Copy link
Member

knu commented Feb 19, 2016

The latet specification (RFC 7235 somehow does not explicitly say it's case-sensitive, but it's probably OK to take it so if it does not say it's case-insensitive.

knu added a commit that referenced this issue Feb 19, 2016
@chris-reeves
Copy link
Contributor Author

Thanks for merging. No apologies are necessary for the wait as I completely understand the lack of time, and I just want to say that I do really appreciate the time and effort that you and others have put into developing and supporting this incredibly tool, so thanks for that!

As far as RFC 7235 goes, although it doesn't explicitly state that authentication parameter values are case-sensitive, it is implicit, so I still believe this change is the right way to go.

Thanks again for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants