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

Improve challenge parsing #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

paulvt
Copy link

@paulvt paulvt commented Feb 28, 2018

This branch improves and fixes the parsing of the challenge in the header. It fixes a crash if there are multiple authentication schemes in the header and it adds some robustness for HTTP servers that are not completely compliant with RFC 2617.

Parsing/regexps are inspired by RFC 2617: https://tools.ietf.org/html/rfc2617#section-3.2.1.

It is allowed to provide multiple authentication schemes with optional
challenges in the WWW-Authenticate headers.  This fixes that it always
took the first scheme, and thus failed on a header such as:
  WWW-Authenticate: NTLM, Digest ....

Also frameworks such as Faraday group multiple WWW-Authenticate headers in
a response, for example
  WWW-Authenticate: Digest ...
  WWW-Authenticate: NTLM
into:
  WWW-Authenticate: Digest ..., NTLM
so there is a single way of retrieving header data.

This commit changes the parser to pick out only the digest challenge part
and the authentication parameters until it encounters another scheme (if
any).
This allows for HTTP servers to provide headers where the challenge
parameter values are unquoted or when there is no spacing after the
parameter separator.
@paulvt
Copy link
Author

paulvt commented Feb 28, 2018

It might also be a good idea to throw Net::HTTP::DigestAuth::Error if the initial match fails on www_authenticate fails and challenge is nil as a result. Currently it will crash with undefined method ``gsub`` for nil:NilClass. What do you think?

@drbrain
Copy link
Owner

drbrain commented Mar 16, 2018

I think an exception that is understandable is better than NoMethodError for undiagnosable reasons

This also includes the case where there is no Digest authentication in the
auth header at all.

Adds a small remark about this to the documentation of
Net::HTTP::DigestAuth#auth_header.
@paulvt
Copy link
Author

paulvt commented Mar 21, 2018

I added a commit that raises a Net::HTTP::DigestAuth::Error if the www-authenticate header does not match (either because of a syntax error or a missing Digest authentication method).

@rgaufman
Copy link

Please accept this pull request, I believe it addresses
#18

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

Successfully merging this pull request may close these issues.

3 participants