Closed Bug 143575 Opened 23 years ago Closed 21 years ago

URL: http ignores username and password (http://user:passwd/domain) for auth cache

Categories

(Core :: Networking: HTTP, defect, P3)

x86
Windows 98
defect

Tracking

()

RESOLVED FIXED
mozilla1.5alpha

People

(Reporter: james_terrigal, Assigned: darin.moz)

References

Details

(Whiteboard: [ETA: June 12, 2003])

Attachments

(2 files, 4 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1) Gecko/20020417
BuildID:    2002041711

Upon trying to access a certain website with a specific username and password,
integrated into the URL (ie "http://username1:password1@www.securesite.com.au")
the access is perfect.  However, if at a later stage an attempt to access the
same site, with a different username and password (ie
"http://username2:password2@www.securesite.com.au") is made, Mozilla
authenticates with the original combination (username1:password1) instead of the
new combination (username2:password2).  This problem is only cleared by closing
all browser, mail and other Mozilla windows (but not necessarily the QuickLaunch
program) and reopening a new instance of the Mozilla browser.

For example: Fred has the username "fred" and the password "abcd1234" to access
his customer management system (cms.fredsisp.com.au).  Fred accesses this
website with the URL: "http://fred:abcd1234@cms.fredsisp.com.au".  Fred wanders
off to do other work, and Bob (with the username of "bob" and the password of
"wxyz7890") sits down at Fred's PC, and tries to access the cms system with the
URL: "http://bob:wxyz7890@cms.fredsisp.com.au".  Mozilla, however, authenticates
with Fred's username and password, ignoring Bob's username and password, despite
the fact they are explicitly stated in the URL.  The only way Bob can access the
site using his own username and password seems to be to close all Mozilla
windows (including browsers and mail, but not necessarily the QuickLauncher) and
open a new browser window.  Then Bob can log in, but once again he needs to
close all windows if Fred needs to log back in.

I hope that this bug report is helpful, and that I've filed it in the right
place.  If I can be of any more help, or there are any questions that I can
assist with, please email me (james_terrigal@hotkey.net.au).  Thanks!  :)

Reproducible: Always
Steps to Reproduce:
1. Access a password protected site by including the username and password in
the URL (ie http://username1:password1@www.site.com).
2. Without closing the browser window, access the exact same site, with a
different username and password in the URL (ie
http://username2:password2@www.site.com).

Actual Results:  Mozilla will authenticate the second time around using
'username1' and 'password1', despite the fact that 'username2' and 'password2'
are explicitly requested.

Expected Results:  After attempting to log in the first time with 'username1'
and 'password1' in the URL (ie http://username1:password1@www.site.com), and
then attempting to log in the second time with 'username2' and 'password2' in
the URL (ie http://username2:password2@www.site.com), I think that Mozilla
should detect the fact that the URL has explicitly stated a new username and
password, and supply the new username and password instead.
hmm... yeah, makes sense that you are seeing this bug.  we do in fact give
preference to the username:password that worked before.

i need to think about how best to solve this... simply preferring the one in the
URL may not always be best.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla1.2alpha
Target Milestone: mozilla1.2alpha → ---
Darin--in what circumstances should the ones in the url not be used?
I expect that this could also affect publishing?
mass futuring of untargeted bugs
Target Milestone: --- → Future
bug 140645 ?
Depends on: 137852
The underyling behavior might be the same, but this example uses user|pass info
in the URl.

HTTP or some password manager-level problem?
QA Contact: tever → httpqa
This patch causes the cached http auth entry to be deleted when there is a
different username specified in the URI. This causes an additional 401 hit to
the server on changing users.
This patch takes out the printf's I left in, changes the bools to PRBools and
augments the check for asking the user for their name/password. The check now
works like this:  if(the uri doesn't have a user, or has a user but doesn't
have a password) then ask for a user and password.  It doesn't prefill the user
in the box like it should. There is a bug open about this [66177].

Also, there is no way that I can see to tell the difference between
http://user@host/path and http://user:@host/path. There is a bug open about
this as well [32761].

Darin -- go ahead and tear apart the patch and let me know what needs to be
fixed.
Attachment #118458 - Attachment is obsolete: true
Attachment #118574 - Flags: review?(darin)
Attached patch Patch against the trunk (obsolete) — Splinter Review
This patch is against the mozilla trunk. The previous patches were against
mozilla 1.3. Sorry for the spam.
Attachment #118776 - Flags: review?(darin)
Comment on attachment 118776 [details] [diff] [review]
Patch against the trunk

>Index: nsHttpChannel.cpp

>+        if (!entry && (ident->IsEmpty() || (nsCRT::strlen(ident->User()) > 0 && nsCRT::strlen(ident->Password()) == 0))) {

same as:

    if (!entry && (nsCRT::strlen(ident->Password()) == 0)) {

no??  keep in mind that it can be valid to have an empty password.  if the
URL says "http://foo@bar.com/" are you sure you don't want to try just sending
an empty password?


>+        if(URIContainsUserPass()) {
...
>+            mURI->GetUsername(buf);

you're copying the username out of mURI twice.	don't do that! ;-)


>+            NS_UnescapeURL(buf);
>+            iUser = ToNewUnicode(NS_ConvertASCIItoUCS2(buf));
>+            if(nsCRT::strcmp(aUser, iUser)) {
>+                authCache->ClearAuthEntry(mConnectionInfo->Host(), mConnectionInfo->Port(), entry->Realm());
>+                return;

you're leaking iUser!  also, what if ToNewUnicode fails?

instead of calling ToNewUnicode you could do this:

  NS_ConvertASCIItoUCS2 iUser(buf);
  if (iUser.Equals(aUser)) {
      // ...
  }
Attachment #118776 - Flags: review?(darin) → review-
This patch covers a two things:

1) the credentials specified on the uri take precedence over the credentials in
cache.

2) the authenication dialog is now prefilled.

I also update the URL bar with the username that they finally authenticate
with; I clear the password from the URL bar.
Attachment #118574 - Attachment is obsolete: true
Attachment #118776 - Attachment is obsolete: true
Attachment #120117 - Flags: review?(darin)
Trying for better summary.

nominated nsbeta, we have a couple auth bugs that are hard to separate, but I
think this might be the missing behavior we need...
Keywords: nsbeta1
Summary: Multiple username/password combinations for a single domain cannot be accessed when stated in URL → URL: http ignores username and password (http://user:passwd/domain) for auth cache
This cleanly applies to latest CVS. Ready for review.
Attachment #120117 - Attachment is obsolete: true
Attachment #123906 - Flags: review?(darin)
adt: nsbeta1-

CC'ing Mitch for a security evaluation of this defect.
Keywords: nsbeta1nsbeta1-
There's a moderately serious security problem here. Since we've got a patch,
let's get it in for 1.4 if possible.
Target Milestone: Future → mozilla1.4final
*** Bug 206707 has been marked as a duplicate of this bug. ***
Whiteboard: [ETA: June 12, 2003]
After talking to Darin, I no longer think this is moderately serious. It's a
"nice to have" for the next release, not a "must have."
Target Milestone: mozilla1.4final → mozilla1.5alpha
ok, the patch is actually addressing multiple problems.  i think we should break
it into separate parts (though i seem to recall okaying their combination).

the first part should just deal with this bug specifically.  we need to address
the problem that http://foo:foo@bar.com/ means try sending foo:foo.  while the
current patch takes a pretty good stab at solving this problem, i think i would
solve it in a slightly different way.  specifically, i don't agree with clearing
the auth cache when a non-matching username:password is provided in the URL
(maybe we only want to do this when the username differs).

at any rate, i'm planning to revise the patch shortly to solve this specific bug
only at first.  we should create another bug for the issue of prepopulating the
password prompt.
Attached patch v2 patchSplinter Review
ok, this patch is a wittled down version of the previous patch.  all it does is
guarantee that we try the username given in the URL before any username found
in the auth cache.  a couple things that this patch changes:

1) if the username given by the URL matches the username given in the auth
cache, then use the auth cache (including the password from the auth cache).

2) when challenged, if we have not yet tried the username from the URL, then
always try it.	do not use the auth cache entry unless the auth cache entry is
for the same username as that found in the URL.

3) when attempting to authenticate with the username given in the URL, do not
add the username:password from the URL into the auth cache.  this is an
important step since it preserves the original auth cache entry, which may
still be valid.  moreover, we may still want to fallover to the auth cache
entry if the username from the URL happens to be invalid.

NOTE:

a) the issue of falling over to the username found in the auth cache is bug
144256.  i don't want to deal with that bug at the moment, so i'm preserving
our current behavior w.r.t. that bug.  moreover, there are a number of tricky
issues to deal with regarding such a change.

b) i have removed the changes in the previous patch which attempted to prefill
the password prompt.  to do that correctly, we would need to modify
nsIAuthPrompt since the username and password parameters are |out| params only.
 the previous patch was using them like |inout| params which they are not.  the
previous patch would have not worked with JavaScript for example.  i think such
a change should be deferred to another bug since anyways there are many things
to consider with such a change, including the bugs having to do with why the
current behavior is the way it is.
Attachment #118574 - Flags: review?(darin)
Attachment #120117 - Flags: review?(darin)
Attachment #123906 - Flags: review?(darin) → review-
Comment on attachment 126403 [details] [diff] [review]
v2 patch

please see comment #18 for an explanation of this patch.
Attachment #126403 - Flags: review?(dougt)
Attachment #126403 - Flags: review?(dougt) → review+
Attachment #126403 - Flags: superreview?(alecf)
Comment on attachment 126403 [details] [diff] [review]
v2 patch

sr=alecf
Attachment #126403 - Flags: superreview?(alecf) → superreview+
fixed-on-trunk

we should use a different bug for the other changes covered by the original patch.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Blocks: 213282
*** Bug 220309 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: