If a url is typed by hand or by a bookmark the password will not become blocked out. "http://charlie:dTR4sl@www.foo.bar/" 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.

-> Url bar we have also a bug about the history

Same as #88771.

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

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).

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

IMHO this bug should block Bug 232560.

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

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

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

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

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

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.

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.

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

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

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.

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

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

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 on attachment 146447 [details] [diff] [review] similar, obeys pref setting r=bzbarsky, assuming the actual indentation is fine

Thanks!

Comment on attachment 146447 [details] [diff] [review] similar, obeys pref setting fwiw, r=caillon as well.

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 on attachment 146447 [details] [diff] [review] similar, obeys pref setting a=asa (on behalf of drivers) for checkin to 1.7

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).

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?

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

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.

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.

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).

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.

> 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.

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. :-)

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.

> 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.

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

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 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.

> 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.

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

Comment on attachment 146572 [details] [diff] [review] alternate escaping solution a=asa (on behalf of drivers) for checkin to 1.7

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

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

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

Comment on attachment 146572 [details] [diff] [review] alternate escaping solution not such a good way to go (Asa & dveditz)

Comment on attachment 146447 [details] [diff] [review] similar, obeys pref setting a=asa (on behalf of drivers) for checkin to the branches.

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

may want to get this into PR..

Comment on attachment 158309 [details] [diff] [review] additional patch to make attachment 146447 [details] [diff] [review] work in FF r+a=ben@mozilla.org

Fixed on trunk, aviary, and 1.7.x

Bug 260926 filed for 1.7.x branch