Digest Auth will auth regardless of status code

[Lukasa]

Discovered on IRC.

The digest auth handler in Requests' codebase doesn't ever actually check that it is responding to a 401: it just looks for an Authorization header. That's pretty dumb, so we should add a check to only actually do the execution for 401s.

This is a contributor friendly change.

[nateprewitt]

Are we sure that tightening this check is what we want to do? It looks like RFC 7235 suggests that it's acceptable to send a WWW-Authenticate challenge with a non-401 status code. I'm not sure handle_401 is the most apt name for the hook, but the functionality seems correct.

A server generating a 401 (Unauthorized) response MUST send a
WWW-Authenticate header field containing at least one challenge. A
server MAY generate a WWW-Authenticate header field in other response
messages to indicate that supplying credentials (or different
credentials) might affect the response.

[sigmavirus24]

@nateprewitt So we have two role models to look at here:

1. Browsers - If you read the logs for our IRC channel, you'll see that Browsers are doing what we do (although probably not as loosely as we do it)

2. Curl/Wget - They disregard the WWW-Authenticate headers on anything that's not a 401. In the case of the user on IRC they were receiving a 403 with WWW-Authenticate headers and we were authenticating while Curl and Wget were not.

Frankly, I'm not sold on restricting this to 401s only, but I do think we should restrict it to 4xx codes. Having these headers sent back on a 30x, for example, could lead to interesting behaviour. On the one hand, we have to answer the challenge, right? On the other we're being redirected, potentially to a totally different domain. I think the only right answer in non 4xx cases is to not authenticate. Unfortunately, we will answer any challenge we receive at the moment and that's problematic.

[sigmavirus24]

Also @nateprewitt, please let someone else pick this up.

[nateprewitt]

Yeah, I was planning on leaving it on the stack. I just did work on testing this functionality last week which is why I felt the need to comment.

I just implemented tests confirming that we won't send Authorization on a 3xx request unless challenged after the redirect, so I don't think that's an issue we need to worry about.

I don't think that restricting this to the 4XX range is unreasonable but I do think the RFC is pretty permissive. I'm just trying to provide information that I couldn't find in the IRC messages that seems relevant. Hopefully helping avoid similar problems that we just had with tightening basic auth.

The Authorization definition also seems to leave this pretty open.

The "Authorization" header field allows a user agent to authenticate
itself with an origin server -- usually, but not necessarily, after
receiving a 401 (Unauthorized) response.

[sigmavirus24]

Correct, which is why we send Basic Authentication headers on the first request. We can't do that with Digest-Auth because we need to respond to a Challenge.

[sigmavirus24]

You could also send Authorization eagerly if it's, for example, a bearer token, or some other kind of trivial authentication that doesn't rely on the challenge-response cycle put forth in the Digest Authentication specification.

[nateprewitt]

Sorry, I should have clarified further on Authorization. I agree we shouldn't be sending Digest Authorization without an appropriate challenge first. What I'm saying is that the section quoted above seems to also support that responding to an appropriate challenge in a non-401 response is permissible which is what we what we currently do.

The scoping here is obviously in the hands of you and @Lukasa, I just wanted to make sure we didn't completely constrict that to 401 hastily as the original post noted.

[sigmavirus24]

I agree we shouldn't be sending Digest Authorization without an appropriate challenge first.

We also literally can't.

I just wanted to make sure we didn't completely constrict that to 401 hastily as the original post noted.

I appreciate that :) I think @Lukasa is starting to feel more and more (as I do) that we should be very strict in our interpretation of specifications. That said, (as you point out) the spec is lenient, so my position is that we should be strict in the spirit of the specification. I can't see any reason why someone would challenge with a 200 response, so it should be safe to restrict it to 4xx responses.

[Lukasa]

So, let's consider this a 3.0.0 issue instead. Clearly there is no problem with Requests' current behaviour: we're well within specification, and we have nothing to worry about with that, but I do think we should restrict ourselves to 4xx responses in the future.

[iamrajhans]

Hey @Lukasa will work on this issue