User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.5) Gecko/20050118 Firefox/1.0 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.5) Gecko/20050118 Firefox/1.0 In most cases I got directly a 301 after requesting a page secured by basic authentication. I have been relocated to something like www.example-domain.com/secure_area/ (from: www.example-domain.com/secure_area - the slash was missing). Firefox sent a new request which was followed by a 407 from my proxy server. My proxy server offered me ntlm authentication and basic authentication. Firefox chose ntlm and authenticated transparently. After that I got a 401 - the webserver only offered basic authentication. Therefore Firefox chose basic authentication. I entered the correct username and password and a popup occured telling me to authenticate against my proxy server. The first problem was that Firefox chose to use basic authentication to authenticate against the proxy server because it had used it before to authenticate against the webserver. I've made a change in nsHttpChannel::GetCredentials that avoids that this selection is performed for proxy authentication. I think this change does not break anything because we can only use one proxy (that requires authentication) anyways and it should not hurt anyone when we choose the first authentication scheme that we support to authenticate against a proxy server - just as in all the other requests. After I had changed this behaviour it worked when requesting www.example-domain.com/secure_area/ directly (not when requesting www.example-domain.com/secure_area). The second problem was that Firefox automatically sent the ntlm type 3 message when requesting the secured page with basic credentials. It seems as it simply takes it from the last request. Therefore I added a little check to nsHttpChannel::ProccessAuthentication to ensure that this message would not be sent before it has been requested. The ntlm authentication starts again and everything works fine now. I'm not very familiar with the mozilla code but I hope that this patch nevertheless fits into the design of the mozilla code. I know about https://bugzilla.mozilla.org/show_bug.cgi?id=211843 but because of the date of the last message and because I'm posting a potential patch I thought it would be no mistake to create a new bug report. In this bug report a similar - but not identical - situation is described. Reproducible: Sometimes Steps to Reproduce: 1. Setup proxy server (I use squid) offering ntlm and basic authentication. 2. Browse website that requires basic authentication. Actual Results: A popup will occur telling you to authenticate against your proxy server although you have authenticated already using the ntlm authentication method. Expected Results: After entering correct username and password for the basic authentication on the requested website the secured content should appear.

See also bug 211843. Is this a duplicate report?

Comment on attachment 172830 [details] [diff] [review] This one looks much nicer than the one posted last time. >+ if (!(precedingAuthFlags & nsIHttpAuthenticator::REUSABLE_CREDENTIALS)) { >+ mRequestHead.ClearHeader(nsHttp::Proxy_Authorization); >+ } I agree that we ultimately need to remove the Proxy_Authorization header in some cases, but all that should really require is testing some flag that is set back when we starting doing proxy auth with an authenticator that doesn't support reusable creds. >+ // Do not perform this selection when authenticating against a proxy. >+ // - jreich > // >- if (!mAuthType.IsEmpty() && authType != mAuthType) >+ if (!mAuthType.IsEmpty() && authType != mAuthType && !proxyAuth) > continue; this is definitely wrong. we want to do this for both proxies and non-proxies. what i think we want to do is keep separate state for each instead of lumping them together like this. or, we should clear mAuthType once we finish authenticating with the proxy.

I actually consider this a pretty major flaw. Thanks for the initial patch. If you don't have time to revise it as I suggested, please let me know, and I will take it from here.

+ rv = CallGetService(contractid.get(), (nsIHttpAuthenticator **) getter_AddRefs(precedingAuth)); this should just be: precedingAuth = do_GetService(contractid.get(), &rv);

This version fixes problems that even occur without authenticating against a webserver. In some special cases the proxy asks us to close the connection. In this cases we failed to authenticate against the proxy. I'll make some tests with ntlm authentication on webservers in the next few days.

Comment on attachment 182288 [details] [diff] [review] This one fixes more problems and is not as hakish as the version posted 3 months ago. Adding to my review queue. Many thanks for working on this patch!

This patch looks pretty good at first glance. I still need to spend some more time reviewing it. More in a bit...

The logic in this patch looks solid to me. Well done! I have some suggestions for cleaning up PrepareForAuthentication. I am going to attach a slightly revised version of this patch since it's just easier to do that then explain the changes I would like to see made :)

Actually, one thing I noticed: + if (NS_SUCCEEDED(rv) && responseStatus != 401 && responseStatus != 407) { + // reset the current continuation state because our last + // authentication attempt has been completed successfully + NS_IF_RELEASE(mAuthContinuationState); + + LOG((" continuation state has been reset")); + } This block, which is at the bottom of PrepareForAuthentication, will never be entered. PrepareForAuthentication is only called from ProcessAuthentication, which is only called if the response status is 401 or 407. Instead, I think that this code should move into ProcessResponse and apply to all responses other than 401 and 407. We already have a block of code in ProcessResponse for that case, and so I will add this NS_IF_RELEASE call into that block.

I also think that the check in PrepareForAuthentication that looks at REUSABLE_CREDENTIALS should look at REQUEST_BASED instead. Because, we should always send the Proxy-Authorization header along with every request if the authentication scheme is request based (i.e., for authentication schemes that conform to RFC 2617).

Darins' version makes me happy ;)

Comment on attachment 183421 [details] [diff] [review] Revised patch marking r= I'd really like to see this included in firefox 1.1a to maximize user testing.

Comment on attachment 183421 [details] [diff] [review] Revised patch a=asa

fixed-on-trunk