Skip to content

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