search

jaredhanson / passport-http

HTTP Basic and Digest authentication strategies for Passport and Node.js.
https://www.passportjs.org/packages/passport-http/?utm_source=github&utm_medium=referral&utm_campaign=passport-http&utm_content=about
MIT License
268 stars 110 forks source link

Fixed issue where submitting empty string for username in Digest auth will return a 400, instead of a 401. Added empty user test case. #10

Closed davidpodolsky closed 11 years ago

davidpodolsky commented 11 years ago

https://github.com/jaredhanson/passport-http/issues/9 "Submitting empty string for username in Digest auth will return a 400, instead of a 401."

Added test case to validate empty username

jaredhanson commented 11 years ago

The test case was still proceed with authentication if the username was empty. I've modified it to immediately reply with a 401 and challenge. See here: https://github.com/jaredhanson/passport-http/commit/05433956b88a7c147a950e3ae3a42980170ea81f

Let me know if that looks correct to you. Thanks!

davidpodolsky commented 11 years ago

This isn't technically correct. The Digest Auth RFC states that the username can be an empty string (Who would in their right mind have an empty string as a username?). So empty strings should proceed to authentication.

This means you can't just check for a not(!) of the username string since it will catch the empty string case. The proper thing would be to check for undefined:

if (typeof creds.username === 'undefined')

Nobody will ever run into this so feel free to change at your own pace. Thanks for the changes.

jaredhanson commented 11 years ago

Yeah, I got that, however: the username is the only "key" to look up the corresponding secret/password. If the username is empty, how would you retrieve some unique secret from which to compute and verify the hash? What scenario would an empty username not fail?

davidpodolsky commented 11 years ago

Technically, in my database I could have a user with a empty string as a username, look up his password, and then authenticate. The real world example is that I turn on Digest Auth for testing and want to accept and authorize everything. Fatest way to test is to leave everything empty.

jaredhanson commented 11 years ago

OK. I'm not too compelled to make a change based on those cases :)

There's better auth schemes that don't need usernames (passport-http-bearer). And if "technically correct" means potentially insecure, I'm OK with not being correct. For testing, just use fixed, non-empty strings. If the current setup is actually preventing deployed use cases, I'll definitely change it.

davidpodolsky commented 11 years ago

I was only suggesting you make change to be compliant with the Digest Auth RFC (http://tools.ietf.org/html/rfc2617).

I agree the use case is minuscule.