157354 - URL bar should not display passwords in URL

Status: RESOLVED FIXED

(SeaMonkey :: Location Bar, defect)

Description

[Adrian Holzwarth]

If a url is typed by hand or by a bookmark the password will not become blocked
out. "http://charlie:[e-mail]/" should by changed (if typed by hand as
soon after being confirmed by hitting return) to http://charlie:*****@www.foor.bar/"

Some passwords are not important enough to trouble the password manager.

Checked on 1.1a, not checked on nightly build.

Comment 1

[Matthias Versen [:Matti]]

-> Url bar

we have also a bug about the history

Comment 2

[Daniel Bagenholm]

Same as #88771.

Comment 3

[Boris Zbarsky [:bzbarsky]]

*** This bug has been marked as a duplicate of 88771 ***

Comment 4

[benc]

REOPEN:
I think this is slightly different than the dupe, because that is about the
autocompelte URL menu.

In the actual field where you typed something, perhaps we should leave it in, in
case a user needs to cut and paste the literal URL. This is considered by some
to be unsafe behavior, but as a content tool, I don't think we would service the
user by forcing them to hack the password back into a URL string they are trying
to work with.

I do think that all our modules that handle URLs programatically should be
stripping the password via URL parser (autocompete, history, bookmarks, etc).

Comment 5

[R.K.Aa.]

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

Comment 6

[OstGote!]

IMHO this bug should block Bug 232560.

Comment 7

[Max Alekseyev]

Marking as blocker of 88771. Unless there is a browsing with hidden password
(this bugreport), autocomplete password hiding (bug 88771) makes no sense.

Comment 8

[Peter Kroll]

Attachment: removes username and password from display of http and https

Comment 9

[Daniel Veditz [:dveditz]]

Should this be a blocker for the stable branch 1.7? Taking to keep an eye on
since hewitt appears gone.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 143513 [details] [diff] [review]
removes username and password from display of http and https

This particular patch doesn't cut it... looks like it's against an old rev of
the file, isn't actually a patch, and should use nsURI to manipulate the URL
safely. The location should be fixed up near the top of the routine rather than
resetting the URL bar value twice -- in fact maybe this should be moved into
nsIURIFixup::createExposableURI

Comment 11

[Daniel Veditz [:dveditz]]

This patch was also put into bug 122445, where it has evolved somewhat. Should
we just close this bug?

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[Daniel Veditz [:dveditz]]

Attachment: patch nsIURIFixup::CreateExposableURI instead

By patching CreateExposableURI instead of browser chrome we fix the problem
pretty cleanly and it can be used by other docshell consumers. Other places
this ends up affecting are
  - default tab title if a page has no <title> tag (good)
  - user:pass disappears from the DOM0 location object.

I think the latter is OK, but that's the place to look for unintended
side-effects.

Comment 14

[Daniel Veditz [:dveditz]]

Added random bugzilla attachment without <title> as a test example. Can be used
to demonstrate the effect on the URLbar and tab title (if using tabs). To see
the effect on location you can then put javascript:alert(location) in the URL bar.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Changing CreateExposableURI is definitely the way to go if we've decided we want
to fix this.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 146329 [details] [diff] [review]
patch nsIURIFixup::CreateExposableURI instead

Try bz instead

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146329 [details] [diff] [review]
patch nsIURIFixup::CreateExposableURI instead

r=bzbarsky on the code changes.  But I recall there being a lot of discussion
about this approach, and I don't see it in this bug.  A number of people were
rather violently opposed to it, and I just want to make sure that we've read
their reasoning and decided we don't care as opposed to just not having read
it.

Comment 18

[Hixie (not reading bugmail)]

I need the username/passwords in the location bar. How do I disable this?

Comment 19

[Andrew Hagen]

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

Comment 20

[Daniel Veditz [:dveditz]]

bz: you're probably thinking of bug 122445, and re-reading it looks like Hixie
opposed the idea a number of times.

I don't suppose digging the URL out of the urlbar drop down is good enough, eh
Hixie? I suppose not, especially since people also consider it a bug to leave
user:pass in the drop down ("What about evil shoulder-surfers?").

*sigh*

new patch coming up

Comment 21

[Daniel Veditz [:dveditz]]

Attachment: similar, obeys pref setting

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146447 [details] [diff] [review]
similar, obeys pref setting

r=bzbarsky, assuming the actual indentation is fine

Comment 23

[Hixie (not reading bugmail)]

Thanks!

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146447 [details] [diff] [review]
similar, obeys pref setting

fwiw, r=caillon as well.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 146447 [details] [diff] [review]
similar, obeys pref setting

bz upgraded to sr= via mail, r=caillon, seeking a= for 1.7

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 146447 [details] [diff] [review]
similar, obeys pref setting

a=asa (on behalf of drivers) for checkin to 1.7

Comment 27

[benc]

Dan: per comment 13, does this hide both username and password? If so, can you
update the summary?

Can you describe how it will affect other areas? Does it hack the URL as entered
(so downstream modules to the URL like bookmarks and history would handle a
username:password stripped URL)>

(Also, I'd like to point out this bug is somewhat hijacked. The original point
was to keep passwords safe. The current patch is probably intended to address
URL spoofing concerns).

Comment 28

[Darin Fisher]

I spoke to dveditz over IRC about this patch.  I'm not very happy with it.  It
makes it very difficult to navigate an FTP site using the URL bar.  Each time
you want to change the directory, you also have to re-insert your username. 
Otherwise, you will get an error because Moz will try to log you in as the
anonymous user.  I think it would be much better to remove the password and
%-escape every char in the username.  or, maybe it is enough to only %-escape
the dots in the username, which is done by the patch for bug 240754.  From a
user's point of view the URL munging is very confusing since FTP directory
listings still show (in large bold font) the complete URL (minus any password)
in the generated HTML.  Can we please not accept this patch as is?

Comment 29

[Hixie (not reading bugmail)]

why not just leave both the username _and_ password in, and escape everything?

Comment 30

[Darin Fisher]

well, for FTP and HTTP the passwords are cached internally.  for FTP the
username is needed as the key to that cache, but for HTTP we can just need the
scheme://host:port/path

i think the idea here is to remove the password so someone cannot easily see
your password if they look over your shoulder.  but, in those cases it doesn't
make sense to put the password in the URL, so...

if we want to URL escape the entire username and password uniformly across the
board, then i think the right thing to do is to modify nsEscape.cpp as was done
for bug 240754.

Comment 31

[Hixie (not reading bugmail)]

People aren't going to remember escaped username/passwords (or if they are, they
 could just shoulder-surf the keystrokes anyway). This also gets around the need
for a pref and removes any chance of odd bugs whereby hitting enter on the
location bar doesn't do the expected thing, etc.

Comment 32

[Daniel Veditz [:dveditz]]

Ben's right: I've apparently misunderstood and hijacked this bug, misled by the
original patch which I see now more appropriately belongs in bug 122445.

My patch does not affect history or the urlbar drop down -- uri fixup happens
after navigation. The both user and password are removed to address phishing
concerns.

Darin's right that ftp navigation using the urlbar is made more difficult on
sites that require passwords; navigation using links on the site will work fine.
Since the user:pass was not removed from history you can also recover the data
from the dropdown, but that's a hack and there are people who'd like to hide it
there as well.

I don't think url escaping just the dots will be sufficient. A lot of people
will have seen enough escapes in paths that a likely response to seeing
"ebay%2Ecom" might well be "oh, that's just the web being wacky again". Escaping
the whole thing might be effective (bug 240754 on steroids).

Comment 33

[Daniel Veditz [:dveditz]]

Attachment: alternate escaping solution

Darin: how many people navigate FTP sites by manually changing directories in
the URL bar, on sites that require logins? I'm not convinced that population is
large enough to worry about, plus they're exactly the folks who understand how
to change preference settings (we should add UI to make it more obvious than
using about:config). And these are the same people who would not fall for a
phishing scam.

That said, escaping the user:pass works pretty well, I'm attaching a patch that
accomplishes that. The two solutions are not mutually exclusive.

Escaping does obscure phishing attempts to plant text suggestive of the spoofed
site, but at the same time it contributes to pushing the real site name further
out of sight.

Escaping has an additional advantage of obscuring the password (against casual
glances) in history and the urlbar dropdown, which the other solution does
nothing about.

Comment 34

[Darin Fisher]

> how many people navigate FTP sites by manually changing directories in
> the URL bar, on sites that require logins?

I'm sure the number is relatively small.

I believe we are in UI freeze, so adding new localized strings for a new
preference is not an option for 1.7, right?  Even if we have a preference to
enable browsing FTP sites using the URL bar, it doesn't mean that user's are
going to discover the preference.

How about a patch that combines escaping with hiding the password?  Preserving
the username is all that's important for making FTP URL bar navigation work
properly.  Hiding the password seems like a good thing in conjunction with
escaping since it helps minimize the problem of the hostname being pushed out
further to the right.

Comment 35

[Hixie (not reading bugmail)]

It doesn't matter if the hostname is pushed out of site if all you can see is
%-escaped garbage. It's quite clear you are not at the site you think you are
at, which is what matters. (Users susceptible to these scams are just looking
for the tell-tale "citibank.com" signature, not parsing the URI in their head to
work out where they really are.)

And I don't care how many users navigate FTP uris that have passwords -- I'm one
of them, and believe me, Opera's solution (turning the password into ****s) is
unbelievably irritating. Please don't go down the route of removing data from
the URI, or adding new prefs or dialog boxes.

Let's at least try the escaping idea, since it appears to solve most of the
problems (shoulder surfing, phishing) without removing any functionality (you
can still copy URIs from the location bar, edit FTP ones in place, etc), and the
patch is so simple. :-)

Comment 36

[benc]

Darin:
The problem with FTP and this patch is that we don't have very robust
non-anonymous FTP support. We have plenty of bugs about that.

That being said, I'd like to ask that the current discussion find a new bug.

Removing the password has clear and simple security pros, esp if doing it here
would have prevented contaminating downstream data structures. Unfortunately as
Dan noted, the current patch does not.

I am completely unclear on how fixing the URL bar prevents confusing users. We
have perhaps a half dozen UI elements that display URLs, and if we are talking
about URL spoofing of usernames and hostnames, then we really need to discuss
that elsewhere.

Comment 37

[Darin Fisher]

> The problem with FTP and this patch is that we don't have very robust
> non-anonymous FTP support. We have plenty of bugs about that.

I'm not sure what you mean.  Which patch are you referring to?  The latest one
or the one before that?  The URL-escaping patch should not affect anything
negatively.


> That being said, I'd like to ask that the current discussion find a new bug.

Which discussion?  I think URL-escaping is similar to the "*****" solution
requested by the original bug reporter with the exceptions that URL-escaping is
non-destructive and can be decoded.


Hixie makes a good point about trying it first without removing the password
field.  I guess I'm ok with that.

Comment 38

[Darin Fisher]

Attachment: FTP auth prompt w/ URL-escaping patch

This screenshot shows that we should be unescaping the username portion of the
FTP URL before prompting the user for their password.

Comment 39

[Darin Fisher]

Comment on attachment 146572 [details] [diff] [review]
alternate escaping solution

sr=darin

this change looks correct, but i think we should fix the FTP prompt dialog (add
a call to UnEscapeURIForUI) along with this patch.

Comment 40

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146572 [details] [diff] [review]
alternate escaping solution

r=caillon for this.

Darin, can we just strip the username portion from the dialog altogether? 
"Enter password for darin on ftp://darin@" ... seems redundant.

Comment 41

[Darin Fisher]

> Darin, can we just strip the username portion from the dialog altogether? 
> "Enter password for darin on ftp://darin@" ... seems redundant.

Yes, I agree.  But, someone has to write that patch... and someone has to go and
test the rest of the product to see if there are other places where this would
have a similarly negative impact.

Comment 42

[Hixie (not reading bugmail)]

Seems like an ideal thing to do during an alpha milestone. :-)

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 146572 [details] [diff] [review]
alternate escaping solution

a=asa (on behalf of drivers) for checkin to 1.7

Comment 44

[benc]

Recommend re-assigning QA for 1.7, I'll be out for the rest of the week.

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 146447 [details] [diff] [review]
similar, obeys pref setting

Not for 1.7

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 146572 [details] [diff] [review]
alternate escaping solution

not for 1.7

Comment 47

[Asa Dotzler [:asa]]

Comment on attachment 146572 [details] [diff] [review]
alternate escaping solution

not such a good way to go (Asa & dveditz)

Comment 48

[Asa Dotzler [:asa]]

Comment on attachment 146447 [details] [diff] [review]
similar, obeys pref setting

a=asa (on behalf of drivers) for checkin to the branches.

Comment 49

[Daniel Veditz [:dveditz]]

Attachment: additional patch to make attachment 146447 work in FF

This is a browser.js addition to make attachment 146447 [details] [diff] [review] work in firefox since
they were munging wyciwyg locally instead of using URIFixup.createExposableURI

Comment 50

[chris hofmann]

may want to get this into PR..

Comment 51

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 158309 [details] [diff] [review]
additional patch to make attachment 146447 [details] [diff] [review] work in FF

r+a=[e-mail]

Comment 52

[Daniel Veditz [:dveditz]]

Fixed on trunk, aviary, and 1.7.x

Comment 53

[Ginn Chen]

Bug 260926 filed for 1.7.x branch