130327 - Passwords in urls are saved in history

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Calvin]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.9+) Gecko/20020308
BuildID:    2002030803

When a URL is entered in the form ftp://username:password@sitename the 
complete string is saved into the history, including the password.     
Username and password is readable to anyone, which means that this is  
a security issue.                                                      


Reproducible: Always
Steps to Reproduce:
1.Go to any ftp site by typing in ftp://username:password@sitename

2.
3.

Actual Results:  The UserID and password are saved in History.

Expected Results:  The ftp site only should be saved in the History and not the
userid and password

Comment 1

[Mike Kaply [:mkaply]]

It appears that 4.x did remove the password (not the userid) when placing it in 
the history.

Note it is still in the URL bar dropdown.

Comment 2

[Alec Flett]

oy. This should probably be done in docshell, where the page gets added
docshell/base/nsDocShell.cpp.. though I kind of think that if someone puts a
password in their url, then they deserve to lose :)

what do 4.x and IE do?

Comment 3

[Mike Kaply [:mkaply]]

4.x removes it, IE does not.

> though I kind of think that if someone puts a
> password in their url, then they deserve to lose :)

I thought that as well, but this customer is kind of adamant about it.

Comment 4

[Matthias Versen [:Matti]]

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

Comment 5

[Mike Kaply [:mkaply]]

Attachment: Possible fix

This is a possible fix.

Query the password before adding to history and if it exists, set it to blank,
then set it back when we are done.

This keeps us from having to do ugly string manipulation on the URI.

The other thing I thought of was a "GetSpecWithoutPassword" API on URI, but
that seems like overkill.

Comment 6

[Alec Flett]

Comment on attachment 75274 [details] [diff] [review]
Possible fix

I'm personally reluctant to do this - I mean, lets say I have a url

ftp://[e-mail]/

and then I enter my password in the UI. will my password get inserted into
history as ftp://foo:[e-mail]/? 

if not, then this seems like much less of an issue.. hrm. I just don't know.

But, say I were to review this anyway, here's what I would say: :)

instead of "blankpw" you can just say NS_LITERAL_CSTRING("") - no need for
nsCAutoString here...
and "pw" should be "password" - there's no need to shorten this variable name.
Variable names are cheap :)

Comment 7

[Mike Kaply [:mkaply]]

I think the main reason to do this is because it worked this way in 4.x.

Do you think the removing and adding the password is OK or is it too kludgy?

Comment 8

[Blake Ross]

reassigning.

Comment 9

[HJ]

This is a major security issue, so please assign target milestone. The fact that
MSIE doe the same isn't an issue, or is it?

Comment 10

[HJ]

CC'ing and s/doe/does

Comment 11

[Ben Bucksch (:BenB)]

Narrowing summary.

Comment 12

[R.K.Aa.]

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

Comment 13

[Bradley Baetz (:bbaetz)]

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

Comment 14

[Tiger Feng]

The patch seems work well.
Hope it can be checked in soon with a little modification.
By the way, IE works the way just as the mozilla without the patch.
IE remembers passwd in its history too.
By the way, I still think it is the user's fault to type in the passwd into URL
field with user name.

Comment 15

[Richard Li]

With Michael Kaply's patch, there's still a problem. When the connection to the
website is fail, it generates an Error item in the History, and the user name
and password are still there.

Comment 16

[jhp (no longer active)]

Attachment: updated patch

Well, if anyone wants to add this to their build, here is an updated patch.

Comment 17

[Alon Altman]

Bug 186695 is related to this bug. Can the patch be updated to include "print"
and "print preview" as well?

Comment 18

[Brian 'netdragon' Bober]

Comment on attachment 98947 [details] [diff] [review]
updated patch

You have a better chance of being reviewed if you actually ask for it.

Comment 19

[Alec Flett]

Comment on attachment 98947 [details] [diff] [review]
updated patch

+	    uriToAdd.password = "";
+	    urlToAdd = uriToAdd.spec;

is this really what you intend? it looks like you're converting a url object to
a string...

+     // Now get the new url string (without the password)
+    uri->GetSpec(url);
+    newURLString = url.get();

woah, no! never save the char value from a .get()! there's no guarantee about
how long that pointer will be valid.

I'm also not psyched about creating a new url object each time a url is added
to global history.. I think at the very least that we've already created such
an object in nsDocShell - we should put the fix there..

Comment 20

[Jason Barnabe (np)]

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

Comment 21

[benc]

Michael:

What would it take to make a patch that fits your suggestions?

Also, before we checkin, I'd like to make sure that password manager records
passwords correctly in these situations, that would let the user session still
work smoothly as far as password entry, but not store passwords in plain text
throught the profile.

Comment 22

[(not currently active) Ted Mielczarek]

I left bug 250337 open since I couldn't find a Firefox bug for this.  Not sure
if a patch for this will fix Firefox.

Comment 23

[David Paenson]

Mozilla should not under any circumstances save any passwords!!

This is a serious security hole - especially in public places like universities
or schools. Anyone sitting at a computer after someone else sees his or her
password when logging in to gmx etc.

Mozilla has a reputation as a very secure browser - much better than msexplorer.
This password saving defeats the purpose!!!

Yours
David Paenson
Frankfurt University of Applied Sciences (Germany)

Comment 24

[M.Brent]

(In reply to comment #0)
I logged in to file this bug, looks like it's been sitting around for quite a while.

Microsoft fixed it in IE6. ;)

Comment 27

[Jason Duell]

I'm adding this to my list of likely network bugs to work on.  No guarantee when I'll actually get to it.

Comment 30

[Josh Matthews [:jdm]]

Attachment: Sanitize input URI when adding to session/global history

Hope I'm not stepping on anyone's toes here.  I've implemented Alec's suggestion of sanitizing inside nsDocShell and tested against my local server with authentication.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 432407 [details] [diff] [review]
Sanitize input URI when adding to session/global history

r- on this only because it doesn't apply any more, things changed around a bit in nsDocShell::AddToGlobalHistory(). Josh, can you provide an updated patch? I can review...

Comment 32

[Josh Matthews [:jdm]]

Attachment: Sanitize input URI when adding to session/global history

Unbitrotted.

Comment 33

[Johnny Stenback (:jst)]

Comment on attachment 444241 [details] [diff] [review]
Sanitize input URI when adding to session/global history

- In nsDocShell::AddToSessionHistory():

-    entry->Create(aURI,              // uri
+    entry->Create(uri,              // uri
                   EmptyString(),     // Title
                   inputStream,       // Post data stream

Maybe add a space on the changed line to maintain the consistent comment indentation?

r=jst

Comment 34

[Josh Matthews [:jdm]]

Attachment: Patch

Nit addressed.

Comment 35

[Marco Bonardo [:mak]]

making a test for this should not be too hard I guess?

Comment 36

[Shawn Wilsher :sdwilsh]

This patch is not ready to land since it is missing tests.

Comment 37

[Matthew N. [:MattN]]

Attachment: Sanitize input URI when adding to session/global history (un-bitrot)

Fixed bitrot from jdm's patch

Comment 38

[Matthew N. [:MattN]]

Attachment: Places tests

Tests to make sure username/password are not stored in global or session history.

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 562229 [details] [diff] [review]
Places tests

Review of attachment 562229 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/browser/browser_bug130327.js
@@ +13,5 @@
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Corporation.

the Mozilla Foundation.

(if you wish in tests you can use the shorter pd license boilerplate)

@@ +38,5 @@
> +/**
> + * Test for Bug 130327.  Make sure that passwords typed in URLs are not saved in history
> + **/
> +
> +var testNum = 1;

nit: test is using half let half var, we usually prefer to avoid mixing up unless willing to be explicit, just s/var/let/ or be consistent

@@ +53,5 @@
> +  const dialogDelay = 10;
> +
> +  // Use a timer to invoke a callback to twiddle the authentication dialog
> +  timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  timer.init(observer, dialogDelay, Ci.nsITimer.TYPE_ONE_SHOT);

Is there any event we may listen for the auth dialog rather than guessing a timeout? Or a windows listener? see browser_bug511456.js for example, it seems more reliable.

@@ +63,5 @@
> +                        Ci.nsISupports, Ci.nsISupportsWeakReference];
> +
> +    if (!interfaces.some( function(v) { return iid.equals(v) } ))
> +      throw Components.results.NS_ERROR_NO_INTERFACE;
> +    return this;

Use XPCOMUtils.generateQI here please...

Well, actually nsITimer.init() may just get a function passed in and there is no need to have an observer object at all

@@ +68,5 @@
> +  },
> +
> +  observe : function (subject, topic, data) {
> +    netscape.security.PrivilegeManager
> +      .enablePrivilege('UniversalXPConnect');

I think this is not needed in a b-c test

@@ +84,5 @@
> +  // through all the open windows and all the <browsers> in each.
> +  var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
> +    getService(Ci.nsIWindowMediator);
> +  //var enumerator = wm.getEnumerator("navigator:browser");
> +  var enumerator = wm.getXULWindowEnumerator(null);

Use Services.wm.
the commented out code seems leftover?

@@ +101,5 @@
> +      if (childDocShell.busyFlags != Ci.nsIDocShell.BUSY_FLAGS_NONE)
> +        continue;
> +      var childDoc = childDocShell.QueryInterface(Ci.nsIDocShell)
> +        .contentViewer
> +        .DOMDocument;

nit: align dots

@@ +103,5 @@
> +      var childDoc = childDocShell.QueryInterface(Ci.nsIDocShell)
> +        .contentViewer
> +        .DOMDocument;
> +
> +      //ok(true, "Got window: " + childDoc.location.href);

either delete or uncomment (converting to a proper info())

@@ +122,5 @@
> + * timer is a one-shot).
> + */
> +function handleDialog(doc, testNum) {
> +  ok(true, "handleDialog running for test " + testNum);
> +  ok(true, "Time is " + (new Date()).toUTCString());

plse use info(msg) instead of ok(true, msg)

@@ +127,5 @@
> +
> +  var dialog = doc.getElementById("commonDialog");
> +  dialog.acceptDialog();
> +
> +  ok(true, "handleDialog done");

ditto

@@ +135,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  startCallbackTimer();
> +  let tab = gBrowser.addTab('http://user:[e-mail]');

let tab = gBrowser.selectedTab = gBrowser.addTab

@@ +140,5 @@
> +
> +  let observer = {
> +    onTitleChanged: function() {},
> +    onBeginUpdateBatch: function() { },
> +    onEndUpdateBatch: function() { },

nit: move these with other empty methods after onVisit

@@ +143,5 @@
> +    onBeginUpdateBatch: function() { },
> +    onEndUpdateBatch: function() { },
> +    onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID,
> +                       aTransitionType) {
> +      is(aURI.spec, 'http://example.com/', 'history URI should not contain the password');

I'd suggesto to add a TEST_URL and TEST_EXPOSABLE_URL consts to the top of the test and use those

@@ +152,5 @@
> +
> +      is(shEntry.URI.spec,  'http://example.com/', 'session history URI should not contain the password');
> +
> +
> +      gBrowser.removeCurrentTab();

lots of newlines in this method...

@@ +153,5 @@
> +      is(shEntry.URI.spec,  'http://example.com/', 'session history URI should not contain the password');
> +
> +
> +      gBrowser.removeCurrentTab();
> +      waitForClearHistory(finish);

is this guaranteed to run after the auth dialog has been dismissed? looks like recipe for random failures, maybe there should be a maybeFinish method that waits both the history entry has been added and the auth dialog dismissed.

Comment 40

[Marco Bonardo [:mak]]

Comment on attachment 562229 [details] [diff] [review]
Places tests

waiting next iteration, thanks for looking at the test!

Comment 41

[Matthew N. [:MattN]]

Attachment: Places tests v.2

The dialog test code was modified from toolkit/components/places/tests/mochitest/prompt_common.js and I didn't realize it wasn't up to current standards. I've switched to using dialog code from browser_bug511456.js as suggested.

All of the other review comments are also addressed.

Comment 42

[Marco Bonardo [:mak]]

Comment on attachment 562657 [details] [diff] [review]
Places tests v.2

Review of attachment 562657 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Matthew N. [:MattN] from comment #41)
> The dialog test code was modified from
> toolkit/components/places/tests/mochitest/prompt_common.js and I didn't
> realize it wasn't up to current standards.

yeah sorry, some tests are really old but rewriting them is time consuming

Looks much better, something may be improved but it looks good.

::: toolkit/components/places/tests/browser/browser_bug130327.js
@@ +37,5 @@
> +  },
> +
> +  onOpenWindow: function(win) {
> +    let domwindow = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                       .getInterface(Components.interfaces.nsIDOMWindow);

nit: You can use Ci

@@ +41,5 @@
> +                       .getInterface(Components.interfaces.nsIDOMWindow);
> +
> +    let self = this;
> +    domwindow.addEventListener("load", function() {
> +      domwindow.removeEventListener("load", arguments.callee, false);

arguments.callee is deprecated in ES5 strict mode, so rather give a label to the function and use the function name here

@@ +43,5 @@
> +    let self = this;
> +    domwindow.addEventListener("load", function() {
> +      domwindow.removeEventListener("load", arguments.callee, false);
> +      self.windowLoad(domwindow);
> +    }, false);

I think it's safe to use capture = true here since regardless we executeSoon later and it's harder something will cancel it (remember to also fix removeEventListener)

@@ +50,5 @@
> +  onCloseWindow: function(win) {
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIWindowMediatorListener,
> +                                         Components.interfaces.nsISupports])

nit: you can use Ci and generateQI adds nsISupports automatically, so no need to provide it in the list

@@ +73,5 @@
> +  let index = gBrowser.sessionHistory.index;
> +  let shEntry = gBrowser.sessionHistory.getEntryAtIndex(index, false);
> +
> +  is(shEntry.URI.spec, TEST_EXPOSABLE_URL, 'session history URI should not contain the password');
> +  cleanupTest();

you don't need to put cleanupTest both here and in registerCleanupFunction, also because registerCleanupFunction can only do synchronous stuff (no waitForClearHistory(finish);)

I suggest here you just invoke waitForClearHistory(finish);

At the end of test() you just directly do:

registerCleanupFunction(function () {
  gBrowser.removeCurrentTab();
  Services.wm.removeListener(WindowWatcher);
  if (!PlacesObserver.lastVisited)
    PlacesUtils.history.removeObserver(PlacesObserver, false);
});

And this should be enough, no need to track cleaning.

@@ +80,5 @@
> +let PlacesObserver = {
> +  lastVisited: null,
> +  onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID,
> +                     aTransitionType) {
> +    this.lastVisited = aURI;

nit: since you only care about the spec, you may store this.lastVisitedURL = aURI.spec

@@ +81,5 @@
> +  lastVisited: null,
> +  onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID,
> +                     aTransitionType) {
> +    this.lastVisited = aURI;
> +    maybeCheckHistory();

add PlacesUtils.history.removeObserver(this, false); before maybeCheckHistory() since at this point we are done and should stop listening.

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 562228 [details] [diff] [review]
Sanitize input URI when adding to session/global history (un-bitrot)

+    nsCOMPtr<nsIURI> uri = aURI;
+    if (sURIFixup) {
+        nsCOMPtr<nsIURI> exposableURI;
+        rv = sURIFixup->CreateExposableURI(uri, getter_AddRefs(exposableURI));

No need for exposableURI here, simply pass aURI as the input argument here, and use uri as the out param, save yourself some refcounting overhead. Same patter occurs in the second hunk in this patch too, and the same comment applies there too.

r=jst, but given what code this is in, I'd like another pair of eyes on this change.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 562228 [details] [diff] [review]
Sanitize input URI when adding to session/global history (un-bitrot)

Use -P when creating patches

But in general, I don't understand the first part. Why do we want to strip
password from *session* history.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Has any thought been given to putting the createExposableURI call in nsNavHistory::AddURI itself, rather than at this one specific call site? Are there valid use cases for adding URIs with passwords to global history?

Comment 46

[Matthew N. [:MattN]]

I didn't realize I wasn't CC'd.  Also, the try run[1] uncovered two test failures which I'm investigating:
75094 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug172261.html | Subframe should be able to call us - got false, expected true
75149 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug255820.html | Wrong color after reload - got rgb(0, 0, 0), expected rgb(0, 180, 0) 

(In reply to Olli Pettay [:smaug] from comment #44)
> But in general, I don't understand the first part. Why do we want to strip
> password from *session* history.

So that the username/password is not exposed through back/forward navigation, if I understand correctly.

(In reply to Gavin Sharp (use [e-mail] for email) from comment #45)
> Has any thought been given to putting the createExposableURI call in
> nsNavHistory::AddURI itself, rather than at this one specific call site? Are
> there valid use cases for adding URIs with passwords to global history?

I think this makes sense if jst/smaug agree.

[1] https://tbpl.mozilla.org/?tree=Try&rev=6d3dc1179c8a

Comment 47

[Marco Bonardo [:mak]]

AddURI is not the only entry point for URIs, it's the entry point for synchronous APIs. There are other 2 asynchronous entry points in History.cpp and AsyncFaviconHelpers.cpp that should be fixed. As a side question, may a user want to add a bookmark with credentials in it? Should we just consider that a bad habit?

Comment 48

[Marco Bonardo [:mak]]

Nevermind the bookmarks part, we use AddURIInternal, not AddURI there. Still which operations adding credentials to the database should be allowed?

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

> (In reply to Olli Pettay [:smaug] from comment #44)
> > But in general, I don't understand the first part. Why do we want to strip
> > password from *session* history.
> 
> So that the username/password is not exposed through back/forward
> navigation, if I understand correctly.

And do we want that? Are we sure that doesn't actually break some pages?
What do other browsers do? Is that kind of behavior specified in HTML spec?


> 
> (In reply to Gavin Sharp (use [e-mail] for email) from comment
> #45)
> > Has any thought been given to putting the createExposableURI call in
> > nsNavHistory::AddURI itself, rather than at this one specific call site? Are
> > there valid use cases for adding URIs with passwords to global history?
> 
> I think this makes sense if jst/smaug agree.

Sounds good to me.

Comment 50

[Josh Matthews [:jdm]]

MattN, feel free to keep running with this. It's off my radar for the moment.

Comment 51

[Justin Dolske [:Dolske]]

(In reply to Olli Pettay [:smaug] from comment #49)
> > (In reply to Olli Pettay [:smaug] from comment #44)
> > > But in general, I don't understand the first part. Why do we want to strip
> > > password from *session* history.
> > 
> > So that the username/password is not exposed through back/forward
> > navigation, if I understand correctly.
> 
> And do we want that? Are we sure that doesn't actually break some pages?
> What do other browsers do? Is that kind of behavior specified in HTML spec?

Once you're logged in, that's cached in netwerk, so it shouldn't really matter for history navigation purposes. Given the relative rarity of these kinds of URLs, I'd suspect any potential breakage is deep in edgecase-land, and could just be a WONTFIX.

Unless there's some obvious case we know this will break, I think we should try this as-is.

Comment 53

[Maciej Jaros]

Wow. 18 years. In Chrome the links like this don't even work:
https://login:[e-mail]/secured-path/

So I'm quite sure any concerns that removing auth data from history are simply invalid by now.

Do note that navigating to the url via JS does work in Chrome. Something like this does work and creates a Basic Auth session in Chrome.

location.href = 'https://login:[e-mail]/secured-path/';

After this, in Chrome history, you will see https://example.com/secured-path/. I believe this is valid, expected and desired behaviour.

Comment 54

[Ben Bucksch (:BenB)]

+1 on removing username/password from browser history, and on supporting the URLs in JS.

I do find entering username/password URLs on the location bar mildly useful (I've just used it yesterday, coincidentally), but Firefox shows a clear warning dialog in this case, so that's not a problem. I do not need and do not want the username/password in the browser history, even in such cases, and in fact consider it more a danger than a help.

Comment 56

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 57

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 130327: Use exposable URL when updating history

Comment 58

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 130327: Add hasUserPass attriubte to nsIURI

We will remove the user and pass from the URL for Histories and Favicons using
nsIOService::CreateExposableURI()[1], store them to the database. And, for
Favicons, we read them from the database as well without the user and pass.
Therefore, as the frequency calling CreateExposableURI() will be higher, we
optimize it. Currently, CreateExposableURI() gets/copies the String of UserPass
to check whether there is a user pass info in the URL, and its copying is waste.
So, we prevent from the copying, implement hasUserPass in nsIURI, and refer to
it instead.

[1] https://searchfox.org/mozilla-central/rev/90dce6b0223b4dc17bb10f1125b44f70951585f9/netwerk/base/nsIOService.cpp#1031
[2] https://searchfox.org/mozilla-central/rev/90dce6b0223b4dc17bb10f1125b44f70951585f9/netwerk/base/nsIOService.cpp#1017

Comment 59

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 130327: Use nsIURI::GetHasUserPass() instead of checking user pass string

Depends on D193623

Comment 60

[Marco Bonardo [:mak]]

Daisuke, I'd suggest to split the nsIURI patches to its own bug, or in general start splitting things that can land sooner into their own bugs, otherwise landing will be a nightmare if we end up with 10 parts or more.

Comment 61

[Daisuke Akatsuka (:daisuke)]

Okay, thanks!

Comment 62

[Phabricator Automation]

Comment on attachment 9363629 [details]
Bug 130327: Add hasUserPass attriubte to nsIURI

Revision D193623 was moved to bug 1864985. Setting attachment 9363629 [details] to obsolete.

Comment 63

[Phabricator Automation]

Comment on attachment 9363630 [details]
Bug 130327: Use nsIURI::GetHasUserPass() instead of checking user pass string

Revision D193624 was moved to bug 1864985. Setting attachment 9363630 [details] to obsolete.

Comment 64

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 130327: Use only exposable URL in favicon service

Depends on D193271

Comment 65

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 130327: Use exposable URL when bookmarking current tab(s)

Depends on D193763

Comment 66

[Marco Bonardo [:mak]]

Hi everyone, Desktop is tring to figure out how to handle this in history, and we'd like to coordinate with Mobile and other stakeholders before moving on, to have a coherent path forward.

The idea, so far, is to store history for the exposable URI (URI without userpass) even when the user visits a URI containing a userpass.
This is because not storing history at all would probably be confusing for the user.
This is coherent with Chrome, based on my research, the exposable URI is added to their History database.
Any concerns with that?

Then, there is still one problem with visited link coloring.
Let me start saying that Chrome is handling it properly, but only because they generate a Visited Links hashmap out of History, and apparently userpass sites are manually added into it.
We don't have such a thing, thus we're left with some options, all having downsides:

1. we never mark userpass uris as visited (as they won't be in history). This may be confusing for the user because they visit the link, but the link remains unvisited.
2. we use the exposable uri to mark visitedness coloring. This means urls with different userpass tuples will all be marked as visited when one is visited.
3. we handle them as embed visits, thus the link is marked as visited, but only until the next page reload. This is a volatile state that may be fine for the session.

If I should sort by preference I'd probably say 3, 2, 1.
Please forward to a more appropriate person in case.

Comment 67

[Emilio Cobos Álvarez (:emilio)]

I think given how uncommon these uris are nowadays, I agree with your assessment: 3 seems preferable and reasonably simple, 2 seems like an ok compromise, 1 would be unfortunate...

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f3eb4071a8c
Use exposable URL when updating history r=mak
https://hg.mozilla.org/integration/autoland/rev/7303defc69b0
Use only exposable URL in favicon service r=places-reviewers,mak
https://hg.mozilla.org/integration/autoland/rev/23d5336f3a16
Use exposable URL when bookmarking current tab(s) r=places-reviewers,mak

Comment 70

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages @ toolkit/components/places/nsFaviconService.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/e11b17390ffa5411434159e4a4a3c31e2c2304bf

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/toolkit/components/places/nsFaviconService.cpp:272:30: error: use of undeclared identifier 'GetExposableURI'

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49acd145a636
Use exposable URL when updating history r=mak
https://hg.mozilla.org/integration/autoland/rev/e1dc34d01733
Use only exposable URL in favicon service r=places-reviewers,mak
https://hg.mozilla.org/integration/autoland/rev/d9c6cd169312
Use exposable URL when bookmarking current tab(s) r=places-reviewers,mak

Comment 72

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/49acd145a636
https://hg.mozilla.org/mozilla-central/rev/e1dc34d01733
https://hg.mozilla.org/mozilla-central/rev/d9c6cd169312

Comment 77

[[:owlish] 🦉 PST]

[discussed over Slack. Also, I agree with Emilio on option 3 being preferable]