twisted.web._auth.digest mostly ignores the value of the uri field

[exarkun]

While this digest auth implementation does use the uri value to compute the correct challenge response (as it must), it does nothing to comply with the requirements in section 3.2.2.5 of RFC 2617, reproduced here:

The authenticating server must assure that the resource designated by the "uri" directive is the same as the resource specified in the Request-Line; if they are not, the server SHOULD return a 400 Bad Request error. (Since this may be a symptom of an attack, server implementers may want to consider logging such errors.) The purpose of duplicating information from the request URL in this field is to deal with the possibility that an intermediate proxy may alter the client's Request-Line. This altered (but presumably semantically equivalent) request would not result in the same digest as that calculated by the client.

[retenodus]

RFC 2617 says that headers of authorization request uri related are :

digest-uri = "uri" "=" digest-uri-value digest-uri-value = request-uri ; As specified by HTTP/1.1

[therve]

There are several problems with that patch. The biggest one being that it doesn't have tests, so it's hard to tell if it works or not. For the behavior, I believe that the change in getChallenge is unnecessary. For the other case, I don't think setting the response code on the request is enough: you should probably raise an exception and catch it in HTTPAuthSessionWrapper to render an ErrorPage. It would be nice to log something, too.

Thanks!

[retenodus]

In the last patch I've added unittests (if the header "digest-uri" matches "request.uri" it works. Else it doesn't) and I use the wrapper to render an error page and log.

[exarkun]

Thanks for working on this. The test coverage is still not quite what it needs to be. The failure case, where the Digest-URI does not match the Request-URI, is a different case - it should be tested in a different test method. The idea is what you did in test_decode, but it needs to be kept separate from the existing test_decode. The new method should have a name and docstring reflecting the new case.

Also:

1. Don't use try/except:pass - use TestCase.assertRaises.
2. There needs to be a test for the wrapper.py change
3. However, I don't think the wrapper needs to log the full traceback. It could log that there was a URI mismatch and maybe some other information about the request, but the stack doesn't provide any extra useful information.
4. The twisted.web.test.test_web change puts None into the fake request headers for most cases. This is bogus - it should leave headers empty if there is no digest-uri. Also, doing this in FakeRequest seems like a mistake. Instead, have digest auth tests populate the header - perhaps in DigestAuthTestCase.setUp.

Thanks again. Sorry about the review delay. I'll be working on Twisted all this week and next, so anything that goes up for review in that time will be looked at quickly. :)