146289 - passwords in URLs are saved in URL dropdown

Status: NEW

(SeaMonkey :: Location Bar, defect)

Description

[Calvin]

When a URL is entered in the form ftp://uswername:password@sitename the complete
string is saved in the URL dropdown, including the password. Username and
password is readable and accessable to anyone, which means that this is a
security issue.
Someone could select this information in the URL dropdown and access the secure
site.

Comment 1

[R.K.Aa.]

Please always include build ID in bug-reports.

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

Comment 2

[Calvin]

Bug 130327 addresses the problem of the password in the history. This addresses
the problem of the password being save in the URL DROPDOWN. This is a different
issue. 

Comment 3

[R.K.Aa.]

Sorry. I always thought the urls in url dropdown depended on history.

Comment 4

[R.K.Aa.]

Looks like a duplicate of bug 26507

Comment 5

[Jesse Ruderman]

Also happens for http passwords (http://foo:bar@host/).

Comment 6

[Bradley Baetz (:bbaetz)]

Dupe of bug 130327

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

Comment 7

[Bradley Baetz (:bbaetz)]

I'll also add that the url dropdown list is obtained from history, so this is a
dupe. If its n0ot in history, it won't be autocompleted.

Comment 8

[Mike Kaply [:mkaply]]

But it will come up in the dropdown. We have code in 130327 that removes it from 
the global history, but it still shows up when you pop down the URL bar.

Comment 9

[Bradley Baetz (:bbaetz)]

Even with a clean profile? If its already in history, your patch won't help -
that ohnly stops new entries from being added.

Comment 10

[benc]

REOPEN:

I'm going to make this a depends because the two features should still be tested
separately.

Comment 11

[OstGote!]

see related bug 88771

Comment 12

[Andrew Schultz]

Attachment: patch

I don't know what the best way to handle this is.  We could do autocomplete, history drop down or both.  We could also attempt to snip the password out of what was typed, although we might occaisionally screw that up.

Comment 13

[Ian Neal]

Comment on attachment 206222 [details] [diff] [review]
patch

I think this is the safest option

Comment 14

[jag (Peter Annema)]

Comment on attachment 206222 [details] [diff] [review]
patch

While I'm not sure not adding the URL to global history is the right approach to begin with[0], don't do your own URL parsing. Instead, create a URL object from the string and ask it if it has a password, and reject based on that.

[0] I'd rather we add the url without the password part, or perhaps add it with password but filter it out when constructing the results for autocomplete (which would also help existing profiles), but I guess that's bug 88771.

Comment 15

[Andrew Schultz]

> [0] I'd rather we add the url without the password part, or perhaps add it with
> password but filter it out when constructing the results for autocomplete
> (which would also help existing profiles), but I guess that's bug 88771.

I thought about filtering after-the-fact too.  But it would mean that passwords would be stored as plaintext in your profile.  And assuming people have a sane limit on their history (a month?  default is 9 days), the passwords will get purged fairly quickly.  That also won't help the history dropdown, which is harder to filter.

Comment 16

[Andrew Schultz]

Attachment: snip out the password

This stores the passwordless URI in history but still doesn't add it to the history dropdown since it's a lot harder to remove it there reliably since we add what was typed to the dropdown instead of a URI.

Comment 17

[jag (Peter Annema)]

Hmm, I wonder if it's better to store the fixed up URI (without password) than store nothing.

Comment 18

[Andrew Schultz]

Well, the one advantage to storing it without the password is that the user would get "ftp://[e-mail]/" the next time from autocomplete.  They could select it and it would invoke the password dialog / password manager, which is better than if they get nothing and type it all out again (including password).

Comment 19

[Ian Neal]

(In reply to comment #16)
> Created an attachment (id=206282) [edit]
> snip out the password
> 
> This stores the passwordless URI in history but still doesn't add it to the
> history dropdown since it's a lot harder to remove it there reliably since we
> add what was typed to the dropdown instead of a URI.
> 
Wouldn't it be possible just to special case those with username:password in for the history dropdown? Somehow making use of the fixedupuri perhaps?

Comment 20

[Andrew Schultz]

Comment on attachment 206282 [details] [diff] [review]
snip out the password

Please assume this block is in a try/catch block:

>+      if (fixedUpURI.password) {
>+        // If the URI has a password, add to the typed history without the
>+        // password, and leave it out of the history dropdown completely.
>+        fixedUpURI.password = "";
>+        gGlobalHistory.markPageAsTyped(fixedUpURI);
>+        return;
>+      }

nsIURI.password might throw (nsSimpleURI does)

Comment 21

[Ian Neal]

Comment on attachment 206282 [details] [diff] [review]
snip out the password

r=me as long as you answer the question from comment #19

Comment 22

[[e-mail]]

Comment on attachment 206282 [details] [diff] [review]
snip out the password

>+    var fixedUpURI = gURIFixup.createFixupURI(url, 0);
I know this was a cut-n-paste job but this should really be FIXUP_FLAGS_NONE.

Comment 24

[Frank-Rainer Grahl (:frg)]

Comment on attachment 206282 [details] [diff] [review]
snip out the password

Clearing review. This patch would need to be redone.