currently we are ignoring this.

Ok, I'll bite. What is 'realm'?

Moving to M17 which is now considered part of beta2.

a web site can have "realms" to control access to different areas. Its a mechanism to share access and is provided in the basic auth. We are ignoring it currrently.

*** Bug 33606 has been marked as a duplicate of this bug. ***

*** Bug 36376 has been marked as a duplicate of this bug. ***

*** Bug 25868 has been marked as a duplicate of this bug. ***

*** Bug 34661 has been marked as a duplicate of this bug. ***

Added Demain Turner to cc-list. This is a duplicate of their bug 35753, the double-signon bug.

removing nsbeta2 because it is annoying but not something we would stop the beta for

removing nsbeta2 because it is annoying but not something we would stop the beta for. Linda, please let us know if this is different than having to log twice

Putting on [nsbeta2-] radar. Not critical to beta2.

*** Bug 40123 has been marked as a duplicate of this bug. ***

*** Bug 38096 has been marked as a duplicate of this bug. ***

There are occasions when this is more than simply having to login twice. If the second directory is just used for <IMG>'s then mozilla doesn't prompt you for a username/password it just doesn't show the <IMG>'s. e.g. Put the HTML in /html/private/ and the images in /images/private/ and IMG tages in /html/private/ referencing /images/private don't work

*** Bug 47986 has been marked as a duplicate of this bug. ***

per triage team, nsbeta3+

*** Bug 49168 has been marked as a duplicate of this bug. ***

*** Bug 50251 has been marked as a duplicate of this bug. ***

Gagan, PDT is wondering if there are any security holes here? Can this give away the password for one realm to another? If so, it seems like a P1; if no there's no vulnerability here, it seems like a P2 or P3.

No security holes here. Since we just don't look at realms currently the problem is that we land up reauthenticating (or failing) for a common realm case. Its P1 becuz it completely makes basic auth unusable on several auth sites.

setting as P2 per PDT

*** Bug 52138 has been marked as a duplicate of this bug. ***

Not holding PR3 for this. Marking nsbeta3- but adding rtm nomination since this apparently hurts enough to fix after PR3.

The issue bnb@looking-glass.org reported does not seem to be necessarily related to realms. AFAICT, Mozilla *never* asks the user for a password for an embedded image, even if the main document is not password-protected at all. It just silently fails to display the image.

approving to work for rtm... ccing darin.

This patch does not fix the problem reported by bnb@looking-glass.org. That one is a separate issue... see bug 56031.

this looks good except for one minor issue. The GetAuthRealm function should create a copy of the authrealm in the out parameter. Other than that r=gagan.

ok all this looks great. r=gagan and moving to a rtm+

This is a big patch, so in order to take this on the branch, we need testing verification that the bug is fixed, and no regressions introduced. QA, please report back when this info is available. Marking [rtm need info]

Should basic-auth realm code really be in the nsHTTPChannel, rather than in the nsBasicAuth? What do digest and NTLM auth do for realms? Also, you seem to be prompting for username and password unconditionally: I'm not sure that's the right thing, since there exist some auth mechanisms that don't have to prompt the user.

I finally understand your patch. Thanks to gagan for explaining things in small words. r=shaver

I landed this fix on the trunk last night.

I have posted a dupe of this bug (Bug 49168). I have Build 20000101320 from Friday. I have problems to access the page posted in the bug 49168. (http://informer2.comdirect.de/login.html?doauth=true&callerPage=/de/my/homepage/main.html) No Username/password dialog is coming up. Is this another problem or is your fix not correct or i´m stupid :-)

Ok, I am seeing this problem too under Linux build 2000101509. ->investigating...

I'm reviewing late, but wonder who gave a= (super-review)? No sign of mscott or a delegate giving approval. Cc'ing mscott and scc for backup. Looks like a leak here: + for (PRInt32 i = count-1; i>=0; --i) + { + nsAuth* auth = (nsAuth*) mAuthList->ElementAt(i); + + nsXPIDLCString authHost; + PRInt32 authPort; + + auth->uri->GetHost(getter_Copies(authHost)); + auth->uri->GetPort(&authPort); + + if ((0 == nsCRT::strcasecmp(authHost, host)) && + (0 == nsCRT::strcasecmp(auth->realm, i_Realm)) && + (port == authPort)) + { + *o_AuthString = nsCRT::strdup(auth->encodedString); + return (!*o_AuthString) ? NS_ERROR_OUT_OF_MEMORY : NS_OK; + } + } + return NS_ERROR_FAILURE; mAuthList is an (nsCOMPtr to an) nsISupportsArray, and nsSupportsArray::ElementAt NS_ADDREFs its result param if non-null, as usual for XPCOM out parameters. So fix by using an nsCOMPtr<nsAuth> here (it's ok to use a concrete type with nsCOMPtr if that type inherits singly from nsISupports, as nsAuth does), but take care to avoid an "extra" AddRef: + nsCOMPtr<nsAuth> auth = dont_AddRef(NS_STATIC_CAST(nsAuth*, mAuthList->ElementAt(i))); Please fix the other two places that leak in this way. Minor bloat here: + if (NS_SUCCEEDED(mURI->GetHost(getter_Copies(hostname)))) + { + // TODO localize + message.AppendWithConversion(" at "); + message.AppendWithConversion(hostname); + } Use message.Append(NS_LITERAL_STRINCT(" at ")) instead. Nit-picking from here on, these problems are obviously less important than leaks and bloat, but they're still worth fixing for code quality: +NS_IMETHODIMP +nsHTTPChannel::GetAuthRealm(char** aAuthRealm) +{ + if (aAuthRealm) + { + *aAuthRealm = nsCRT::strdup(mAuthRealm); + return (*aAuthRealm) ? NS_OK : NS_ERROR_OUT_OF_MEMORY; + } + else + return NS_ERROR_NULL_POINTER; +} else after return is a non-sequitur, and the error return might better be the overindented case in this function: +NS_IMETHODIMP +nsHTTPChannel::GetAuthRealm(char** aAuthRealm) +{ + if (!aAuthRealm) + return NS_ERROR_NULL_POINTER; + *aAuthRealm = nsCRT::strdup(mAuthRealm); + return (*aAuthRealm) ? NS_OK : NS_ERROR_OUT_OF_MEMORY; +} Then the only foolish consistency change would be to use ! and put the NS_OK at the end, as you did elsewhere: + return (!*o_AuthString) ? NS_ERROR_OUT_OF_MEMORY : NS_OK; Brace style in the modified parts of the patch is discordant with the file's prevailing style (which puts opening brace on same line as if, e.g.): + // Skip prompting if we already have an authentication string. + if (!authString || !*authString) + { + // Delay this check until we absolutely would need the prompter + if (!mPrompter) { + NS_WARNING("Failed to prompt for username/password: nsHTTPChannel::mPrompter == NULL"); + return NS_ERROR_FAILURE; + } Why not follow "When in Rome" and avoid mixing styles? It's recommended at http://www.mozilla.org/hacking/reviewers.html and it helps readability and ownership, no matter whose style is being upheld (if you are taking ownership, revise the whole file's style; if you have a share in ownership, get buy-in and make style changes; otherwise the prevailing style should be observed). Also, tabs in files are evil (because people and programs on various platforms cannot agree on what tabstops they advance to), and violate the Emacs modeline comment. It looks like some of these were already there before your patch, but could you fix 'em all anyway? @@ -2315,10 +2374,21 @@ { rv = GetRequestHeader(nsHTTPAtoms::Authorization, getter_Copies(authString)); - pEngine->SetAuthString(mURI, authString); +#ifdef DEBUG_darin + fprintf(stderr, "\n>>>>> Auth Accepted!! [realm=%s, auth=%s]\n\n", mAuthRealm, (const char*) authString); +#endif + if (mAuthRealm) + pEngine->SetAuthStringForRealm(mURI, mAuthRealm, authString); + else + pEngine->SetAuthString(mURI, authString); } Thanks, /be

I feel guilty about getting us to this point. My very first review stopped when I (thought I) saw problems with non-basic-auth, but when those problems were explained away by gagan, I didn't return to finish the job, and gave an r= inappropriately. Even if I didn't point out style woes -- which I should have -- there's no excuse for missing the leaks and string-conversion bloat. I promise to do a better job with your next patch, Darin, once you address brendan's concerns.

Using the latest CVS trunk (2000-10-16 12:19 PST), I've located the source of the problem described in bug 49168. The behavior is different than that originally reported because of this patch. The problem we are now seeing results when the site, mentioned in bug 49168, responds with a 301 redirection after being properly authenticated. The new URL does not "match" any previously authenticated URLs, so we do not send an authentication header with the request for the new URL. (We are expecting the server to respond with 401, if authentication is required). The servers response for the new URL is 301 (not 401) with a location back to the original URL. Thus, mozilla drops into an infinite loop. Netscape 4.X does not have this problem. Although, I haven't been able to confirm it from the source, it seems 4.X is sending the authentication acquired after the first 401 on successive 301 redirects, thus avoiding the loop. Despite this problem, I thought we were already checking for infinite redirects. Anybody know how this could slip through? Perhaps, it is due to the intermitent 401? I'll have to look at the code more carefully. Brendan: thanks for pointing out those improvements to the patch.

This is clearly a bad HTTP implementation that we have unintentionally encouraged with 4.*. A redirect should NOT carry original auth info. Possible security hazards are scary. Our best bet in our current scenario is to simply add the check for this goofy cyclic redirect. Ruslan has put in some code to detect that. Thanks for the great review brendan (since I had reviewed it, we "converted" shaver's review to a super-review) My bad! I will try and be more careful next time.

The patches I just posted fixes the leaks, bloat, and (some of the) style problems pointed out by Brendan.

+ nsAuth* auth = NS_STATIC_CAST(nsAuth*, mAuthList->ElementAt(i)); + Why not use an nsCOMPtr? Adding NS_RELEASEs is busy-work that someone will later break by adding a control flow transfer around the necessary release, without adding yet another release. I feel pretty strongly about this, and so do other reviewers. Mozilla leaks too much memory, and not because of any one leak. Manual reference counting tends to result in more leaks that you get with systematic use of smart pointers. If you are taking ownership (you're asserting your brace style, but maybe other netwerk hackers don't care about consistency there -- as a reader and potential writer of netwerk code, I do), I hope you get around to using nsCOMPtrs in other files, e.g. in nsHTTPHandler.cpp. nsHTTPHandler::NewChannel in particular needs a cleanup. Bogus indentation and gratuitous gotos are telltales; so is the NS_ADDREF you removed. When using nsCOMPtrs, you do need to ADDREF the out param as you set it (this is a Mozilla pattern, an ADDREF that people expect to see). Without it, the reviewer has to track the earlier ADDREF back to the ElementAt call, and check for necessary releases (which you added). Perhaps scc can say more, but his fine docs under http://www.mozilla.org/projects/xpcom/, linked to under http://www.mozilla.org/hacking/reviewers.html#rules-and-tips, are worth reading if you haven't. /be

Hey Darin, this patch looks pretty good to me. One suggestion I had is the same as Brendan's. You really want to leverage a nsCOMPtr with the nsAuth ptrs you are getting from that nsISupportsArray. I know it may look a little strange because you are dealing with an implementation class and not the interface but it's perfectly legal as long as your object inherits singly from nsISupports. This would clean up a lot of the NS_RELEASES you added and make the code more leak proof if someone else comes along and modifies the code introducing another return path. So there were a couple of spots were you should leverage that. I'm not sure you need to clean up the constructor for nsHTTPHandler before checking this patch in since you aren't introducing more gotos, or poor identation stuf. It would certainly be great if someone would clean it up but I think that's orthoganal to your changes. Or maybe I misunderstood Brendan's comment and he really was pointing out something in your patch and not existing code. Oh one more thing. As a convience to you, if you made mAuthRealm a nsCString instead of char * then you wouldn't have to worry about initializing it to null and freeing the memory in the dtor of nsHTTPChannel.

Gagan and I discussed using nsCOMPtr's in nsAuthEngine. However, the fact of the matter is that without an IID for the nsAuth class you can't do this. Instantiating a nsCOMPtr requires the GetIID method, which appears in interface header files through the NS_DEFINE_STATIC_IID_ACCESSOR(iid) macro. We decided that it would be less evasive to just do the NS_RELEASE's instead of defining a new IID for the nsAuth _class_. Ultimately, I don't think a nsISupportsArray is the correct container here. The only reason nsAuth derives from nsISupports is so that it can be used with nsISupportsArray. Perhaps, nsVoidArray would be more appropriate? Then, we would not have to mess around with reference counting at all. Overall, there are many things that need to be cleaned up in this code. But, the point right now (as I understand it) is to touch the code in the least evasive way in order to fix the problems at hand. Once we get past 6.0, then we will have the opportunity to do a clean sweep. Is (a) switching to nsVoidArray, (b) defining a new IID, or (c) littering the code with NS_RELEASE's the correct thing to do at this time?

The fact of the matter is, you are not allowed to get back to an implementation class from an |nsISupports|, so plainly you need to do one of two things: (a) don't use an |nsISupportsArray|, don't do reference counting (b) make a new interface that supports everything you want, and use that In this case, I agree with your assesment. Follow (a). Don't inherit from |nsISupports|, don't use |nsISupportsArray|. |nsVoidArray| may be the best solution we have for you at the moment, though really a templated class a la |std::vector<T>| would be best, and all casting could be avoided. If you still needed to manage lifetime (which you might avoid by storing objects instead of pointers and exploiting value and copy semantics) you might use |auto_ptr| or something home-grown. Clearly, this code is stretching to use |nsISupportsArray| and having to twist in unfortunate ways to do so. It's trying to walk a middle line between the two options above, and doing so very poorly. Ignore the change, for the moment, I wouldn't have let the original code in, for its wrong use of XPCOM.

After discussing this with Gagan, we decided that it would be best to switch from using nsISupportsArray to nsVoidArray in the implementation of nsAuthEngine. The required changes to the code are minimal, since both nsISupportsArray and nsVoidArray have similar interfaces. The only big difference is in nsAuthEngine::Logout, which must traverse the lists and individually call delete on each nsAuth pointer. I've tested this code on the trunk, and I believe that it is "safe" for inclusion. I'll shortly post a patch against the branch.

PDT says rtm++, please land on branch ASAP.

Looks ok to me, mscott has dibs on sr=. /be

Darin, your patch against the branch looks good to me. I like the nsVoidArray change. sr=mscott

All fixes checked in all relevant places... and all auth sites could authenticate happily ever after. Thanks Darin.

verified on branch WinNT4 2000102008 Linux rh6 2000102009 Mac os9 2000102008 needs verified on trunk

verified on trunk WinNT4 2000123120 Linux rh6 2000123106 Mac os9 2001010209