719103 - Showing passwords in the Saved Passwords window should revert after x minutes, if somebody forgets to close it, to minimize privacy breaches

Status: RESOLVED INVALID

(Toolkit :: Password Manager, defect)

Description

[Irina Sandu]

When using the Password Manager to see my saved passwords, I forgot to close the window after having sen my passwords, and I discovered after a few days that I had my passwords open in Firefox, available for anybody that might be using my computer.

Suggestion: revert the showing of passwords after a few minutes (5?), so that anybody who forgets that window open and showing its passwords, does not incur a privacy risk.

Comment 1

[Manish Goregaokar [:manishearth]]

Mentoring and assigning (IIT KGP MozSetup).

Comment 2

[Affan]

Attachment: password.patch

I did the hide password

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8399086 [details] [diff] [review]
password.patch

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

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +26,5 @@
>  }
>  
>  function setFilter(aFilterString) {
>    document.getElementById("filter").value = aFilterString;
> +  _filterPasswords();ggle

You have some extra text lying around here.

@@ +148,3 @@
>    // Notify observers that the password visibility toggling is
>    // completed.  (Mostly useful for tests)
> +  hidePasswordInterval = setTimeout(TogglePasswordVisible, 30000);

We *might* want to put this value in a pref. Let's see what MattN has to say about it.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8399086 [details] [diff] [review]
password.patch

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

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +7,5 @@
>  /*** =================== SAVED SIGNONS CODE =================== ***/
>  
>  var kSignonBundle;
>  var showingPasswords = false;
> +var hidePasswordInterval = 0;

Nit: This isn't an interval, so perhaps hidePasswordTimer? Initially I thought this was defining the time interval before it hides.

@@ +12,1 @@
>  function SignonsStartup() {

Nit: leave the newline after the declarations.

@@ +148,3 @@
>    // Notify observers that the password visibility toggling is
>    // completed.  (Mostly useful for tests)
> +  hidePasswordInterval = setTimeout(TogglePasswordVisible, 30000);

I doubt we want a pref for something like this since most people won't know about it or change it unless we want it in the preferences UI. I would like this to be a const at the top of the file if we don't make it a preference.

I'm not sure how to make the decision on what the correct threshold is and I probably wouldn't have marked it as mentored until that was decided. I personally think 30s is too short (and it seems comment 0 agrees with me) but I'm not sure how to decide on a better time. Can you find research on or existing applications with this behaviour?

This line doesn't belong below the comment (which is for the lines below). I think you only want the timer to start when we actually made passwords visible whereas this function toggles both directions so this will make the passwords re-appear after 30s (or at least the prompt to do so).

@@ +371,5 @@
>    }
>  
>    return token.isLoggedIn();
>  }
> +

Nit: revert this addition

Comment 5

[Manish Goregaokar [:manishearth]]

Affan, any updates on this? I recall you having trouble with Mercurial, if you want more help with that don't hesitate to ask :)

Comment 6

[Manish Goregaokar [:manishearth]]

Affan, are you still working on this?

Comment 7

[Benny Mei]

Attachment: Fixed up Affan's patch according to its comments.

MattN suggested that some research be done on how long passwords should remain visible, but it doesn't seem like anybody else shows passwords the way Firefox does.

Many other programs which offer an option to show passwords only show passwords if the user's mouse is hovering over a specific area. 

Should Firefox create an addition like that, as opposed to a timeout when showing passwords?

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8431008 [details] [diff] [review]
Fixed up Affan's patch according to its comments.

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

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +13,5 @@
>  var dateAndTimeFormatter = new Intl.DateTimeFormat(undefined,
>                               { day: "numeric", month: "short", year: "numeric",
>                                 hour: "numeric", minute: "numeric" });
> +var hidePasswordTimer = 0;
> +const timeoutCountdown = 30000;

This is 30 seconds, not five minutes (the original proposal). I'm sort of okay with that, let's see what Matt says.

Comment 9

[Benny Mei]

(In reply to Manish Goregaokar [:manishearth] from comment #8)
> Comment on attachment 8431008 [details] [diff] [review]
> Fixed up Affan's patch according to its comments.
> 
> Review of attachment 8431008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/passwordmgr/content/passwordManager.js
> @@ +13,5 @@
> >  var dateAndTimeFormatter = new Intl.DateTimeFormat(undefined,
> >                               { day: "numeric", month: "short", year: "numeric",
> >                                 hour: "numeric", minute: "numeric" });
> > +var hidePasswordTimer = 0;
> > +const timeoutCountdown = 30000;
> 
> This is 30 seconds, not five minutes (the original proposal). I'm sort of
> okay with that, let's see what Matt says.

Yes. I figure that this should be a step to the final solution. 

I was researching how other applications handle showing passwords, and many applications require that the user's mouse hover over a certain area in order to show passwords. I feel that shoud be the next step in the "Show Password" feature for Firefox, rather than simply leaving a timer to hide passwords.

Requiring a user to have their mouse in a certain place on the screen is much more secure than a timer. The password will show when the user wants to see it, and as soon as the need to see the password is gone, the password will be hidden again.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8431008 [details] [diff] [review]
Fixed up Affan's patch according to its comments.

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

(In reply to Benny Mei from comment #7)
> Created attachment 8431008 [details] [diff] [review]
> Fixed up Affan's patch according to its comments.
> 
> MattN suggested that some research be done on how long passwords should
> remain visible, but it doesn't seem like anybody else shows passwords the
> way Firefox does.

Chromium continues showing the password in plain text after clicking the show button and switching tabs back and forth.


(In reply to Benny Mei from comment #9)
> I was researching how other applications handle showing passwords, and many
> applications require that the user's mouse hover over a certain area in
> order to show passwords. I feel that shoud be the next step in the "Show
> Password" feature for Firefox, rather than simply leaving a timer to hide
> passwords.
> 
> Requiring a user to have their mouse in a certain place on the screen is
> much more secure than a timer. The password will show when the user wants to
> see it, and as soon as the need to see the password is gone, the password
> will be hidden again.

This proposal sounds reasonable but it seems like we should just implement that if UX thinks it's better. Flagging for UX review to see what the UX team thinks is necessary for protecting saved passwords visible on the screen in the password manager.

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +167,5 @@
>    }
>  
>    // Notify observers that the password visibility toggling is
>    // completed.  (Mostly useful for tests)
> +  hidePasswordTimer = setTimeout(TogglePasswordVisible, timeoutCountdown);

This has the same issue as Affan's patch:
"This line doesn't belong below the comment (which is for the lines below). I think you only want the timer to start when we actually made passwords visible whereas this function toggles both directions so this will make the passwords re-appear after 30s (or at least the prompt to do so)."

Comment 11

[Benny Mei]

(In reply to Matthew N. [:MattN] from comment #10)
> Comment on attachment 8431008 [details] [diff] [review]
> Fixed up Affan's patch according to its comments.
> 
> Review of attachment 8431008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Benny Mei from comment #7)
> > Created attachment 8431008 [details] [diff] [review]
> > Fixed up Affan's patch according to its comments.
> > 
> > MattN suggested that some research be done on how long passwords should
> > remain visible, but it doesn't seem like anybody else shows passwords the
> > way Firefox does.
> 
> Chromium continues showing the password in plain text after clicking the
> show button and switching tabs back and forth.
> 
> 
> (In reply to Benny Mei from comment #9)
> > I was researching how other applications handle showing passwords, and many
> > applications require that the user's mouse hover over a certain area in
> > order to show passwords. I feel that shoud be the next step in the "Show
> > Password" feature for Firefox, rather than simply leaving a timer to hide
> > passwords.
> > 
> > Requiring a user to have their mouse in a certain place on the screen is
> > much more secure than a timer. The password will show when the user wants to
> > see it, and as soon as the need to see the password is gone, the password
> > will be hidden again.
> 
> This proposal sounds reasonable but it seems like we should just implement
> that if UX thinks it's better. Flagging for UX review to see what the UX
> team thinks is necessary for protecting saved passwords visible on the
> screen in the password manager.
> 
> ::: toolkit/components/passwordmgr/content/passwordManager.js
> @@ +167,5 @@
> >    }
> >  
> >    // Notify observers that the password visibility toggling is
> >    // completed.  (Mostly useful for tests)
> > +  hidePasswordTimer = setTimeout(TogglePasswordVisible, timeoutCountdown);
> 
> This has the same issue as Affan's patch:
> "This line doesn't belong below the comment (which is for the lines below).
> I think you only want the timer to start when we actually made passwords
> visible whereas this function toggles both directions so this will make the
> passwords re-appear after 30s (or at least the prompt to do so)."

Should I keep working on this then? Or wait for the UX review?

Comment 12

[Matthew N. [:MattN]]

It shouldn't be hard to fix so if you don't mind, that would be good.

Comment 13

[Manish Goregaokar [:manishearth]]

Benny, any updates on this? Sorry about the delay.

Comment 14

[Benny Mei]

Manish, the last time I worked on this issue, it was flagged for UI Review and I haven't heard anything about it since. Do you know who to get in contact with to get more information about this?

Comment 15

[Manish Goregaokar [:manishearth]]

(In reply to Benny Mei from comment #14)
> Manish, the last time I worked on this issue, it was flagged for UI Review
> and I haven't heard anything about it since. Do you know who to get in
> contact with to get more information about this?

Philipp gave ui-review+, which means that the proposed UI is okay :)

Comment 16

[Benny Mei]

Attachment: Implement mouseover behavior to display passwords

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8546999 [details] [diff] [review]
Implement mouseover behavior to display passwords

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

::: toolkit/components/passwordmgr/content/passwordManager.xul
@@ +102,5 @@
>                label="&removeall.label;" accesskey="&removeall.accesskey;"
>                oncommand="DeleteAllSignons();"/>
>        <spacer flex="1"/>
>        <button id="togglePasswords"
> +              onmouseover="TogglePasswordVisible();"

We should probably use some other UI element for stuff that isn't clickable (so not a <button>).

Also, this prevents legitimate users from being able to copy the passwords since the moment they mouseout on the button, the passwords will hide again.

Matt, what do you think?

Comment 18

[Benny Mei]

(In reply to Manish Goregaokar [:manishearth] from comment #17)
> Comment on attachment 8546999 [details] [diff] [review]
> Implement mouseover behavior to display passwords
> 
> Review of attachment 8546999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/passwordmgr/content/passwordManager.xul
> @@ +102,5 @@
> >                label="&removeall.label;" accesskey="&removeall.accesskey;"
> >                oncommand="DeleteAllSignons();"/>
> >        <spacer flex="1"/>
> >        <button id="togglePasswords"
> > +              onmouseover="TogglePasswordVisible();"
> 
> We should probably use some other UI element for stuff that isn't clickable
> (so not a <button>).
> 
> Also, this prevents legitimate users from being able to copy the passwords
> since the moment they mouseout on the button, the passwords will hide again.
> 
> Matt, what do you think?

Which UI Element would be more appropriate for this? I'm sorry, I'm not so familiar with xul.

Comment 19

[Matthew N. [:MattN]]

There are plans to change this dialog (possibly removing Show All altogether) but I think it's reasonable to take a small patch to revert the visibility after some minutes in the meantime.

Unfortunately the latest patch changed to a different approach (other than what got ux-review) without explanation. Can you go back to using a timer?

Comment 20

[Manish Goregaokar [:manishearth]]

I guess you just need to fix up this patch[1] then


 [1]: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=719103&attachment=8431008

Comment 21

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Are you still working on this ticket, if so, do you have a plan for when you will submit it to a release train? Thank you!

Comment 22

[Ryan Feeley [:rfeeley]]

Great idea. I think it's safe to assume that it's highly unlikely a user would want their passwords shown indefinitely. One minute and then mask again? Related to checkbox bug too? https://bugzilla.mozilla.org/show_bug.cgi?id=1217134

Comment 23

[Benny Mei]

(In reply to Emma Humphries [:emceeaich] (GMT-8) from comment #21)
> Are you still working on this ticket, if so, do you have a plan for when you
> will submit it to a release train? Thank you!

I apologize for the lapse in communication. I'm swamped with work at the moment, so I won't have time to work on this ticket in the near future.

Comment 24

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Unassigning: could the mentor find a new assignee?

Comment 25

[Tony Mechelynck [:tonymec]]

@KaiRo: Does this bug affect the SeaMonkey Data Manager's Passwords tab?

If yes, the new behaviour introduced by the latest patch (attachment #8546999 [details] [diff] [review], passwords visible only onmouseover over the [Show Passwords] button) would IMHO be extremely annoying considering that our Data Manager opens in its own about:data browser tab.

If not, should or shouldn't we alter the behaviour of our Data Manager's Passwords tab (which would happen in a different bug, probably either in SeaMonkey::Passwords_&_Permissions or in SeaMonkey::UI_Design)?

Or in the latter case, maybe I should ask Neil?

Comment 26

[Manish Goregaokar [:manishearth]]

The UI for the password box recently changed; now you have to click directly on the password to see it (and it gets hidden if you click elsewhere). 

I don't think this bug is necessary anymore.

Comment 27

[Tony Mechelynck [:tonymec]]

If this bug is INVALID, my NEEDINFO question becomes a moot point.