514212 - Typed letters in password fields become asterisks immediately

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Madhava Enros [:madhava]]

This makes it very hard to know if you've typed correctly, which is _particularly_ troublesome on hard-to-type-on mobile devices.  Most other mobile software picks between:

(a) entered letters show up as themselves initially, and then turn into asterisks after a delay (time delay, or until next letter is typed)
(b) not bothering with asterisks at all, on the basis that it's harder to spy over someone's shoulder and see the screen on a small mobile device

I'd be happy with either, with a slight preference for (a) -- mostly for the case of returning to pages later with passwords remembered -- probably don't want things showing up in cleartext in those cases.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

A third approach could be turning the letters into asterisks when the textbox looses focus.  You'd get the readability and editability of option b and not be able to read your passwords when you return to the page. As an added bonus we wouldn't have to deal with really crappy timers.

Comment 2

[Doug Turner (:dougt)]

maybe we should do this for all passwords - not just in fennec?

Comment 3

[Brian Crowder]

I think you -want- the letters to be concealed relatively quickly, not after the textbox loses focus, since the whole point of the asterisks is to save the user from shoulder-surfers.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I guess that only the inputting character is in IME composition string. If so, I think that the composition string to be visible as the actual text is good. And it may be simple implementation.

Comment 5

[Madhava Enros [:madhava]]

(In reply to comment #3)
> I think you -want- the letters to be concealed relatively quickly, not after
> the textbox loses focus, since the whole point of the asterisks is to save the
> user from shoulder-surfers.

Part of the assertion here, for mobile devices, is that the shoulder-surfer risk is lower (because it's harder to do) on mobile devices than with a big monitor.

Anyway -- the _most_ important thing here, short term, is letting the user see actual letters rather than asterisks when typing.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)

> Anyway -- the _most_ important thing here, short term, is letting the user see
> actual letters rather than asterisks when typing.

We could easily stop turning letters into asterisks, if that's what we want. Anything more could be trickier.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 8

[Doug Turner (:dougt)]

lets not make this MOZ_GFX_OPTIMIZE_MOBILE, but instead lets use nsILookAndFeel so that other platforms can take advantage of it.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> lets not make this MOZ_GFX_OPTIMIZE_MOBILE, but instead lets use nsILookAndFeel
> so that other platforms can take advantage of it.

I'm hoping we can see this work in the simplest manner. We can file followup bugs to extend it to other platforms.

Comment 10

[Doug Turner (:dougt)]

mfinkle, leverage!  you want to improve the platform, not just the applications.  This functionality is bad-ass on all of the platforms, is it not?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #10)
> mfinkle, leverage!  you want to improve the platform, not just the
> applications.  This functionality is bad-ass on all of the platforms, is it
> not?

It sure is, but I'm willing to improve in steps. The extra reviewers, modules and gold-plating involved will slow down getting this feature into Fennec. Once the feature is working, then we can add the extra support.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch, uses nsILookAndFeel

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

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

I tested this patch on N900. It works pretty well. Currently the patch will
change the previously entered character into an asterisk and leave the newly
entered character visible.

I think at a minimum we need a few more edge cases working:
1. The previously entered character is visible until a new character is type.
We should use a timeout to convert the character.
2. The last character is never changed to an asterisk if you tab away. I think
#1 would fix this. We shouldn't need to watch for blur.
3. Moving the caret back into the "****" to type more characters leaves the
newly typed character visible unless another character is typed. Again, I think
#1 would fix this.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.3, with a timer

Comment 15

[[e-mail]]

Comment on attachment 409293 [details] [diff] [review]
patch v.3, with a timer

It looks as if you haven't understood the code (but then this is editor, which is almost as bad as mailnews...)

EchoInsertionToPWBuff has two tasks:
1. It updates the mPasswordText member, which contains the real value
2. It (inefficiently) replaces all the characters of aOutString with the password character, which is what the editor uses to update the anonymous DOM.

So the first part of the patch simply needs you to skip that part of the code in the case where you want to echo on a timer.

Later on, you want to update the anonymous DOM so that the password is hidden. Assuming the editor still exists at that point (try it with a timeout large enough for you to close the window before the timer fires, for instance), this should be just a case of updating the DOM with password characters.

Of course the fun seems to start because you'll lose your current selection. But as editor is linked into gklayout I feel sure there must be a way of overwriting the text node data with password characters without updating the selection.

If not, maybe bz can write you one ;-)

Comment 16

[[e-mail]]

OK, let me describe things in a different way.

Current flow:
1. Figure out text to insert
2. Save text in internal password buffer
3. Insert replacement characters rather than actual text

Ideal flow:
1. Figure out text to insert
2. Save text in internal password buffer
3. If using delayed replacement, replace any unreplaced characters from last insertion and a set timeout to replace unreplaced characters automatically
4. If not using delayed replacement, replace the text to insert with replacement characters

Step 3 would ideally use a shared method to replace outstanding unreplaced characters when called directly from WillInsertText as well as on the timeout.
Step 3 and Step 4 would ideally use the a shared method to replace text in an nsString with replacement characters.
For extra bonus points, Step 3 could check for unreplaced characters before replacing them with replacement characters.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.4

This keeps track of the last range of text to be input and to update, deletes that range and replaces it with password characters. This work is also now in a single method that's called by both the timer and the WillInsertText method.

Comment 18

[[e-mail]]

Comment on attachment 409408 [details] [diff] [review]
patch v.4

Thanks for making those changes. The code already looks much simpler. These are just some style nits I've noticed, I haven't tried compiling this yet.

>+    nsCOMPtr<nsILookAndFeel> lookAndFeel = do_GetService(kLookAndFeelCID);
>+    if (lookAndFeel->GetEchoPassword()) {
>+      mPasswordText.Insert(*outString, start);
I'm wondering whether we should remove this line from EchoInsertionToPWBuff, and do the insertion in all cases before checking for GetEchoPassword...

>+      res = EchoInsertionToPWBuff(start, end, outString);
... or indeed completely remove EchoInsertionToPWBuff and replace it with a utility method that creates a string of mask characters of a specified length, which you could then also call from HideLastPWInput.

>+nsresult nsTextEditRules::Notify(class nsITimer *) {
This needs to be an NS_IMETHODIMP since it's on an interface.

>+  nsCOMPtr<nsIDOMCharacterData>  nodeAsText(do_QueryInterface(selNode));
Nit: double space crept in here  ^

>+  mEditor->DeleteText(nodeAsText,mLastStart, mLastEnd - mLastStart + 1);
Nit: missing a space after the first comma... although I do wonder whether calling nodeAsText->ReplaceData would be better.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.5, addresses comments

Comment 20

[[e-mail]]

Comment on attachment 409437 [details] [diff] [review]
patch v.5, addresses comments

>+      mLastStart = start;
>+      mLastEnd = end;
This is supposed to be the text you want to hide next time, right? In which case, I think the end needs to be the start plus the outString length (or if it was easier you could save mLastLength instead). Currently you only mask one more character than the number that were previously selected, so if you paste stuff in it's sometimes only partially masked, while if you had an existing selection then extra mask characters get added.

>+  res = mEditor->GetStartNodeAndOffset(selection, address_of(selNode), &selOffset);
It turns out that there's a problem here - if you hit Ctrl+A quickly after entering text, then selNode is not the text node, it's the root node. Perhaps there's a more efficient way of getting the text node.

>+  FillBufWithPWChars(&hiddenText, mLastEnd - mLastStart + 1);
[It was this unexpected + 1 that drew my attention to mLastEnd above.]

>+  res = mEditor->GetSelection(getter_AddRefs(selection));
It's the same selection, you don't need to get it twice!

>+  selection->Collapse(node, end);
I'm not convinced this is right; as far as I can tell from nsISelection.idl, you need to collapse to the start and extend to the end (if it's not the same), but you might find you can fix the selNode bug without changing this.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.6

I am not sure that collapsing and extending the selection is the correct behavior.  Is this what you meant though?

Comment 22

[[e-mail]]

(In reply to comment #21)
> I am not sure that collapsing and extending the selection is the correct
> behavior.  Is this what you meant though?
The code looks right; I'm not sure that I'll have time to review today though.

Comment 23

[[e-mail]]

It was going well, until somehow I hit

###!!! ASSERTION: invalid root in call to nsTraversal constructor: 'aRoot', file nsTraversal.cpp, line 59

followed by a crash :-(

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #23)
> It was going well, until somehow I hit
> 
> ###!!! ASSERTION: invalid root in call to nsTraversal constructor: 'aRoot',
> file nsTraversal.cpp, line 59
> 
> followed by a crash :-(

Looks like a null check of node before the nsNodeIterator constructor will avoid that.  Bailing early in that case may be appropriate since there doesn't seem to be any text to replace.

Comment 25

[[e-mail]]

Comment on attachment 409558 [details] [diff] [review]
patch v.6

>+      mTimer->InitWithCallback(this, 600, nsITimer::TYPE_ONE_SHOT);
>+      
>+    } else {
Nit: this blank line looks out of place here. Also, editor style is to have else on its own line.

>+      selNode->Release();
This can't be right, getter_AddRefs will release the old value.

>+      if (NS_OK != iter.NextNode(getter_AddRefs(selNode)) || selNode == nsnull) {
Normally we prefer to use NS_SUCCEEDED

>+        return res == NS_OK ? NS_ERROR_NULL_POINTER : res;
Which res did you mean to return? [You didn't save the return of NextNode.]

>+  if (NS_FAILED(res)) return res;
This should be somewhere nearer where you set res, shouldn't it?

>+   nsCOMPtr<nsIDOMNode> node;
>+  selection->GetAnchorNode(getter_AddRefs(node));
We don't want this node, we want the text node you so lovingly found!
[The nsCOMPtr has the wrong indentation anyway.]

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.7

Comment 27

[timeless]

Comment on attachment 409602 [details] [diff] [review]
patch v.7

>+      if (mTimer)
>+        mTimer->Cancel();

iirc creating timers is expensive, shouldn't you do this:
>+      mTimer = do_CreateInstance("@mozilla.org/timer;1", &res);
>+      if (NS_FAILED(res)) return res;

.. as an else block to the mTimer condition?

>+      mTimer->InitWithCallback(this, 600, nsITimer::TYPE_ONE_SHOT);

>-class nsTextEditRules : public nsIEditRules
>+class nsTextEditRules : public nsIEditRules,  public 

too many spaces before 'public'

Comment 28

[[e-mail]]

Comment on attachment 409602 [details] [diff] [review]
patch v.7

>+    if (nsnull == node) return NS_OK;
Apart from timeless's points, I noticed one other thing: we normally test for null using !node rather than a explicit comparison.

>+  selection->Collapse(selNode, start);
>+  if (start != end)
>+    selection->Extend(selNode, end);
Yes, this does what I want, thanks.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/23e6d66d7d65

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/40a7517cca9c

Comment 31

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (Windows; U; Window3sCE 5.2; en-US; rv:1.9.2b2pre) Gecko/20091103 Namoroka/3.6b2pre Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091103 Namoroka/3.6b2pre Fennec/1.0b5

Comment 32

[[e-mail]]

Sigh. I just noticed that you forgot to update the interface map :-(

Comment 33

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #32)
> Sigh. I just noticed that you forgot to update the interface map :-(

fixed with http://hg.mozilla.org/mozilla-central/rev/ad2e8d859c2e 

waiting for approval for 192 landing