search

jshttp / basic-auth

Generic basic auth Authorization header field parser
MIT License
702 stars 86 forks source link

Support userid only fixes #9 #10

Closed tjconcept closed 10 years ago

jonathanong commented 10 years ago

@dougwilson wanna review?

dougwilson commented 10 years ago

@tjconcept is there a client that is sending the basic header like this, and if so, what is it?

tjconcept commented 10 years ago

@dougwilson I encountered this scenario using superagent. See the original issue #9 for an example.

dougwilson commented 10 years ago

Ok. I'll gave to try it out against apache to see what apache does. It could very well be a bug in superagent, we don't want to rule anything out.

tjconcept commented 10 years ago

Okay, I'll be looking forward to your results. That was also my concern, and the reason I opened an issue initially - to clarify whether I needed to do the fix here or at superagent.

tjconcept commented 10 years ago

In light of the discussion (and from reading rfc2617) my opinion is that the client is responsible for always adding a colon to avoid ambiguity. If you agree, I think this pull request should be closed along with the issue.

The test I removed was (I guess) supposed to test for malformed base64, but it actually only worked because it also (implicitly) tested for the colon. Therefor I think it should be replaced with the two tests from @ruimarinho and a third one testing with a missing colon.

dougwilson commented 10 years ago

Sorry I didn't respond here. So yes, I checked both Apache and nginx and both completely ignore the Basic authentication header if the : is not in there, which is what I expected from the spec.

dougwilson commented 10 years ago

Sorry, I think I was wrong about Apache. I think I had a configuration issue. I'll merge your PR with a change to the RegExp, as it's not correct :)

dougwilson commented 10 years ago

OK, so I have word from various sources that yes, having the : missing is invalid. There are some servers, though (for what reason, IDK) that just parse to the end of the header and if there was no colon, then just take the entire header as just the username. I don't really think that is desirable behavior, though.

dougwilson commented 10 years ago

The more servers I'm looking at, the more I see that the ones that actually accept : as optional, are also vulnerable to CVE-2014-6061 :)