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

better HTTP DIGEST support #730

Merged
merged 3 commits into from
Dec 9, 2013
Merged

better HTTP DIGEST support #730

merged 3 commits into from
Dec 9, 2013

Conversation

dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Dec 8, 2013

This PR fixes followings:

  • proper handling of qop from server
  • proper nc format 8LHEX
  • no quoting for nc and qop

Ref: http://www.ietf.org/rfc/rfc2617.txt

@nylen
Copy link
Member

nylen commented Dec 8, 2013

Cool, thanks!

Is this comment still accurate? Is RFC 2617 fully implemented now? If not, which parts remain?

Can you add some tests for the additional functionality? (How about a test where the server sends qop=auth and one where it doesn't?)

@dai-shi
Copy link
Contributor Author

dai-shi commented Dec 9, 2013

Hi, I reviewed RFC2617 and did what I can which should be better than before. Listed remaining TODO items as far as I can tell, including less important ones. Added a test case (spent much time to find out RFC2069 has a errata.)

@nylen
Copy link
Member

nylen commented Dec 9, 2013

Looks good. I think we could use some more tests for digest auth (for example, test the actual values of the response hash in different situations, rather than just checking that it is in the right format) but this is an improvement.

nylen added a commit that referenced this pull request Dec 9, 2013
@nylen nylen merged commit 882b01b into request:master Dec 9, 2013
@dai-shi
Copy link
Contributor Author

dai-shi commented Dec 9, 2013

Thanks for merging. Yeah, that was also my original idea to use actual values until I found the errata, which made me exhausted...

@sberryman
Copy link

Any chance you want to push 2.29.1 with this pull request? I can't access an API with digest auth using 2.29.0 as the server sends qop="auth, auth-int"

@dai-shi
Copy link
Contributor Author

dai-shi commented Dec 10, 2013

By the way, this reminds me of something. We should make the regex more robust like the following.

diff --git a/request.js b/request.js
index b54a282..3c03c47 100644
--- a/request.js
+++ b/request.js
@@ -725,7 +725,7 @@ Request.prototype.onResponse = function (response) {

         var ha1 = md5(self._user + ':' + challenge.realm + ':' + self._pass)
         var ha2 = md5(self.method + ':' + self.uri.path)
-        var qop = /(^|,)auth($|,)/.test(challenge.qop) && 'auth'
+        var qop = /(^|,)\s*auth\s*($|,)/.test(challenge.qop) && 'auth'
         var nc = qop && '00000001'
         var cnonce = qop && uuid().replace(/-/g, '')
         var digestResponse = qop ? md5(ha1 + ':' + challenge.nonce + ':' + nc +

@nylen would you do this?

nylen added a commit that referenced this pull request Dec 12, 2013
@nylen
Copy link
Member

nylen commented Dec 12, 2013

@dai-shi done (f03be23)

nylen added a commit to nylen/request that referenced this pull request Oct 17, 2014
nylen added a commit to nylen/request that referenced this pull request Oct 17, 2014
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.

None yet

3 participants