basic auth must use realm

VERIFIED FIXED in M17

Status

()

P2
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: gagan, Assigned: darin.moz)

Tracking

Trunk
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2-][nsbeta3-][pdtp2][rtm++])

Attachments

(8 attachments)

(Reporter)

Description

19 years ago
currently we are ignoring this.
(Reporter)

Updated

19 years ago
Status: NEW → ASSIGNED
Keywords: beta2

Comment 1

19 years ago
Ok, I'll bite. What is 'realm'?
Target Milestone: --- → M16

Comment 2

19 years ago
Moving to M17 which is now considered part of beta2.
Target Milestone: M16 → M17
(Reporter)

Comment 3

19 years ago
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. 
(Reporter)

Comment 4

19 years ago
*** Bug 33606 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

19 years ago
Whiteboard: 2d

Comment 5

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

Updated

19 years ago
Keywords: nsbeta2
(Reporter)

Comment 6

19 years ago
*** Bug 25868 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

19 years ago
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

Updated

19 years ago
Whiteboard: 2d → [nsbeta2-]2d

Comment 11

19 years ago
Putting on [nsbeta2-] radar. Not critical to beta2.
(Reporter)

Comment 12

19 years ago
*** Bug 40123 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 13

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

Comment 14

19 years ago
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
(Reporter)

Updated

19 years ago
Keywords: nsbeta3
(Reporter)

Comment 15

19 years ago
*** Bug 47986 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

19 years ago
Whiteboard: [nsbeta2-]2d → [nsbeta2-]
(Reporter)

Comment 16

19 years ago
per triage team, nsbeta3+
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3+]
(Reporter)

Comment 17

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

Comment 18

19 years ago
*** Bug 50251 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

19 years ago
Priority: P3 → P1

Comment 19

19 years ago
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.
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta2-][nsbeta3+][need info]
(Reporter)

Comment 20

19 years ago
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.
Whiteboard: [nsbeta2-][nsbeta3+][need info] → [nsbeta2-][nsbeta3+]
setting as P2 per PDT
Priority: P1 → P2
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta2-][nsbeta3+][pdtp2]
(Reporter)

Comment 22

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

Comment 23

19 years ago
Not holding PR3 for this. Marking nsbeta3- but adding rtm nomination since this 
apparently hurts enough to fix after PR3.
Keywords: rtm
Whiteboard: [nsbeta2-][nsbeta3+][pdtp2] → [nsbeta2-][nsbeta3-][pdtp2]

Comment 24

19 years ago
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.
(Reporter)

Comment 25

18 years ago
approving to work for rtm... ccing darin. 
Whiteboard: [nsbeta2-][nsbeta3-][pdtp2] → [nsbeta2-][nsbeta3-][pdtp2][rtm need info]
(Assignee)

Comment 26

18 years ago
Created attachment 16818 [details] [diff] [review]
This patch adds support for HTTP realms.
(Assignee)

Comment 27

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

Comment 28

18 years ago
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. 
Assignee: gagan → darin
Status: ASSIGNED → NEW
(Assignee)

Comment 29

18 years ago
Created attachment 16825 [details] [diff] [review]
Revised patch with fix to nsHTTPChannel::GetAuthRealm()
(Assignee)

Comment 30

18 years ago
Created attachment 16834 [details] [diff] [review]
Revised patch to correctly check mPrompter _only if_ realm lookup fails.
(Reporter)

Comment 31

18 years ago
ok all this looks great. r=gagan and moving to a rtm+
Whiteboard: [nsbeta2-][nsbeta3-][pdtp2][rtm need info] → [nsbeta2-][nsbeta3-][pdtp2][rtm+]

Comment 32

18 years ago
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]
Whiteboard: [nsbeta2-][nsbeta3-][pdtp2][rtm+] → [nsbeta2-][nsbeta3-][pdtp2][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.
(Assignee)

Comment 34

18 years ago
Created attachment 16943 [details] [diff] [review]
Latest patch: minor changes over previous; tested on trunk.
I finally understand your patch.  Thanks to gagan for explaining things in small
words.

r=shaver
(Reporter)

Comment 36

18 years ago
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 :-)
(Assignee)

Comment 38

18 years ago
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.
(Assignee)

Comment 41

18 years ago
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.
(Reporter)

Comment 42

18 years ago
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.
(Assignee)

Comment 43

18 years ago
Created attachment 17303 [details] [diff] [review]
Revised patch to be applied to trunk.
(Assignee)

Comment 44

18 years ago
Created attachment 17304 [details] [diff] [review]
Revised patch to be applied to branch.
(Assignee)

Comment 45

18 years ago
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

Comment 47

18 years ago
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.

(Assignee)

Comment 48

18 years ago
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?

Comment 49

18 years ago
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.
(Assignee)

Comment 50

18 years ago
Created attachment 17463 [details] [diff] [review]
Revised patch to be applied to the Trunk, modified to use nsVoidArray.
(Assignee)

Comment 51

18 years ago
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.

Comment 52

18 years ago
PDT says rtm++, please land on branch ASAP.
Whiteboard: [nsbeta2-][nsbeta3-][pdtp2][rtm need info] → [nsbeta2-][nsbeta3-][pdtp2][rtm++]
Looks ok to me, mscott has dibs on sr=.

/be
(Assignee)

Comment 54

18 years ago
Created attachment 17496 [details] [diff] [review]
Revised patch to be applied to the branch.

Comment 55

18 years ago
Darin, your patch against the branch looks good to me. I like the nsVoidArray
change.

sr=mscott
(Reporter)

Comment 56

18 years ago
All fixes checked in all relevant places... and all auth sites could 
authenticate happily ever after. Thanks Darin. 
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 57

18 years ago
verified on branch
WinNT4 2000102008
Linux rh6 2000102009
Mac os9 2000102008

needs verified on trunk 
Keywords: vtrunk

Comment 58

18 years ago
verified on trunk
WinNT4 2000123120
Linux rh6 2000123106
Mac os9 2001010209

Status: RESOLVED → VERIFIED
Keywords: vtrunk
You need to log in before you can comment on or make changes to this bug.