Closed Bug 556873 Opened 14 years ago Closed 13 years ago

exiting a password field by pressing return fails to disable secure entry mode

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: bp, Assigned: jaas)

References

Details

Attachments

(2 files, 6 obsolete files)

testcase from the reporter
317 bytes, text/html
Details
fix v1.6
10.89 KB, patch
jst
: approval2.0+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

When editing a password field on the Mac, Firefox ultimately calls EnableSecureEventInput to prevent other input managers from sniffing the user's password.  If I exit the password field by clicking in another field, or by clicking the "submit" button, then Firefox properly calls DisableSecureEventInput.  However, if I exit the password field by hitting return (thereby submitting the form), then DisableSecureEventInput is not called, and other input managers on the system are permanently locked out of keyboard input.


Reproducible: Always

Steps to Reproduce:
1. create a form that has an input field of type "password" and an input of type "submit"
2. use Firefox, and type in the password form
3. set a breakpoint on DisableSecureEventInput
4. exit the form by hitting return
Actual Results:  
DisableSecureEventInput is not called, and other input managers are locked out of keyboard input



Expected Results:  
DisableSecureEventInput should be called.
Component: General → HTML: Form Submission
Product: Firefox → Core
QA Contact: general → form-submission
Testing/duplicating this one doesn't take much -- this will do it:
<html>
<head>
	<title>Secure Input Test</title>
</head>

<body>

<p>Test</p>
<form action="nothing" method="post">
<p>Name: <input name="name" type="text" size="50"/></p>
<p>Password: <input name="passwd" type="password" size="50"/></p>
<input type="submit" value="TestIt"/>
</form>

</body>
</html>
I used the testcase and set a breakpoint as described. I'm seeing it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Cocoa widgets is what calls DisableSecureEventInput() so, possibly the bug is there?
Component: HTML: Form Submission → Widget: Cocoa
QA Contact: form-submission → cocoa
Widget implements that as EndSecureKeyboardInput(), which is called only in layout/forms/nsTextControlFrame.cpp 

I think it's probably layout (or, $deity forbid, editor) that's not handling the return/enter case properly.  (That said, it's still the Cocoa widget bug 394107 that added DisableSecureEventInput/EndSecureKeyboardInput and the usage in nsTextControlFrame, and the feature is Mac-only, so this may be as good a component as any.)
Blocks: 394107
Hardware: x86_64 → All
Indeed, the problem is exhibited in layout/forms/nsTextControlFrame.cpp:

When the user hits Return to submit the form, the password field does not lose focus and thus nsTextControlFrame::SetFocus is not called and thus nsTextControlFrame::MaybeEndSecureKeyboardInput is not called.

I'm not sure if the real problem is that the password field does not lose focus and that the real solution is that nsTextControlFrame::SetFocus should be called before the form is submitted via the Return key.

However, I did find a possible workaround. nsTextControlFrame::CheckFireOnChange() is called when switching fields. It's possible to add this after mFocusedValue = value; :

	// Disable secure input - fix https://bugzilla.mozilla.org/show_bug.cgi?id=556873
 	if (mInSecureKeyboardInputMode) {
	  MaybeEndSecureKeyboardInput();
	}

This may be a way to fix the problem without visiting why the field doesn't lose focus when the form is submitted using Return.
so, if the field loses focus the page would have to learn about it, and clicking back would result in a different experience.

you don't want to make the field lose [dom] focus.
Thanks @timeless for the info on focus. Can anyone see a problem with putting the proposed fix in nsTextControlFrame::CheckFireOnChange() as noted in comment 6 above? If not, who can I lobby to take this and run with it?
That seems like it wouldn't work right, since it would trigger the MaybeEndSecureKeyboardInput even if the form is not submitted and the user stays on the same page, no?
Assignee: nobody → sgreenlay
This problem is not only with submit. If I press back or forwards while in a secure field DisableSecureEventInput() is also not called. I think that we should look at a solution that calls DisableSecureEventInput() on page change if the currently selected field is secure.
In addition, we will need something that will call EnableSecureEventInput() when you return to the page.
Attached patch Secure Focus Fix (v1.0) (obsolete) — — Splinter Review 
Moved secure focus detection to nsFocusManager.
Attachment #481970 - Flags: review?(joshmoz)
Status: NEW → ASSIGNED
Comment on attachment 481970 [details] [diff] [review]
Secure Focus Fix (v1.0)

>+    // Use nsITextControlElement::IsPasswordTextControl() instead?

There is no reason we can't answer this question now. If it is better then use it, if it isn't then remove the comment.

>+  // if the focus currently secured, DisableSecureEventInput()
>+  if (inSecureKeyboardInputMode) {
>+    EndSecureKeyboardInput();
>+  }

The comment here doesn't make the code any more clear, remove it.

>+  // if the focus currently secured, DisableSecureEventInput()
>+  if (inSecureKeyboardInputMode) {
>+    EndSecureKeyboardInput();
>+  }

The comment here doesn't make the code any more clear, remove it.

>+  // if the focus is shifted to a secure input, EnableSecureEventInput()
>+  if (doesContentNeedSecureEntry(aContent)) {
>+    if (!inSecureKeyboardInputMode) {
>+      BeginSecureKeyboardInput();
>+    }
>+  }
>+  // if the focus is shifted away from a secure input, DisableSecureEventInput()
>+  else {
>+    if (inSecureKeyboardInputMode) {
>+      EndSecureKeyboardInput();
>+    }
>+  }
>+#endif

Same for both comments here.

>   if (aContent && (aContent == mFirstFocusEvent || aContent == mFirstBlurEvent))
>     return;
>-
>+  

Get rid of this whitespace modification.
Attachment #481970 - Flags: review?(joshmoz) → review-
Comment on attachment 481970 [details] [diff] [review]
Secure Focus Fix (v1.0)

>+static PRBool inSecureKeyboardInputMode = PR_FALSE;

Also, rename this to 'sInSecureKeyboardInputMode' per our convention for static variables. Otherwise it looks like a variable on the stack.
Attached patch Secure Focus Fix (v1.1) (obsolete) — — Splinter Review 

        Attachment #481970 -
        Attachment is obsolete: true

        Attachment #483077 -
        Flags: review?(joshmoz)

    
  

        Attachment #483077 -
        Flags: review?(joshmoz) → review+

    
  

        Attachment #483077 -
        Flags: review?(enndeakin)

    
    

    
  
Comment on attachment 483077 [details] [diff] [review]
Secure Focus Fix (v1.1)

Can you describe what the change being made here is and why it is necessary?


>+#ifdef XP_MACOSX
>+#include <Carbon/Carbon.h>
>+#endif
>+

This, for instance, immediately jumps out as being wrong.


>+#ifdef XP_MACOSX
>+static PRBool sInSecureKeyboardInputMode = PR_FALSE;
>+
>+static PRBool doesContentNeedSecureEntry(nsIContent* aContent) {
>+  nsCOMPtr<nsIDOMHTMLInputElement> inputElement(do_QueryInterface(aContent));
>+  if (inputElement) {
>+    PRBool isTextEntry = PR_FALSE;
>+    nsresult rv = inputElement->MozIsTextField(PR_FALSE, &isTextEntry);
>+    if (NS_SUCCEEDED(rv) && isTextEntry) {
>+      PRBool isNotPasswordEntry = PR_FALSE;
>+      rv = inputElement->MozIsTextField(PR_TRUE, &isNotPasswordEntry);
>+      if (NS_SUCCEEDED(rv) && !isNotPasswordEntry) {
>+        return PR_TRUE;

This would be much simpler if nsIFormControl->GetType() was used.


>@@ -1548,22 +1597,35 @@ nsFocusManager::Blur(nsPIDOMWindow* aWin
> void
> nsFocusManager::Focus(nsPIDOMWindow* aWindow,
>                       nsIContent* aContent,
>                       PRUint32 aFlags,
>                       PRBool aIsNewDocument,
>                       PRBool aFocusChanged,
>                       PRBool aWindowRaised)
> {
>+#ifdef XP_MACOSX
>+  if (doesContentNeedSecureEntry(aContent)) {
>+    if (!sInSecureKeyboardInputMode) {
>+      BeginSecureKeyboardInput();
>+    }
>+  }
>+  else {
>+    if (sInSecureKeyboardInputMode) {
>+      EndSecureKeyboardInput();
>+    }
>+  }
>+#endif

I'm not clear why this is being called right here. Nothing is focused yet, nor do we at this point know that aContent is going to be focused.

Same with the other places these methods are called.

I think a description of what was wrong with the old code would make it easier for me to help you and determine where these calls belong.

        Attachment #483077 -
        Flags: review?(enndeakin) → review-

    
    

    
  
The objective of this is to have 'SecureKeyboardInput' enabled when the user sets focus on a secure input (right now only PasswordEntry) and 'SecureKeyboardInput' disabled when the the secure input looses focus. This is done through calls to EnableSecureEventInput() and DisableSecureEventInput() (included through Carbon/Carbon.h)

BeginSecureKeyboardInput() should be called when the content that is going to be selected requires 'SecureKeyboardInput' and it is not currently enabled. I added this to nsFocusManager::Focus based on stepping through the focus process. If there is somewhere else where this should be put, let me know!

EndSecureKeyboardInput() should be called if 'SecureKeyboardInput' is enabled and it looses focus. I added this to nsFocusManager::Focus, nsFocusManager::WindowLowered (for the case where the window looses focus), and nsFocusManager::ClearFocus (for the case where the user selects nothing). If there is somewhere else where this should be put, let me know!

I actually tried to contact you earlier to get feedback on this, but never received a response.

    
    

    
  
(In reply to comment #17)
> BeginSecureKeyboardInput() should be called when the content that is going to
> be selected requires 'SecureKeyboardInput' and it is not currently enabled. I
> added this to nsFocusManager::Focus based on stepping through the focus
> process. If there is somewhere else where this should be put, let me know!
> 
> EndSecureKeyboardInput() should be called if 'SecureKeyboardInput' is enabled
> and it looses focus. I added this to nsFocusManager::Focus,
> nsFocusManager::WindowLowered (for the case where the window looses focus), and
> nsFocusManager::ClearFocus (for the case where the user selects nothing). If
> there is somewhere else where this should be put, let me know!

It sounds like you just want to add a focus and blur listener to nsTextControlFrame::HandleEvent, since this is specific to password elements.


> I actually tried to contact you earlier to get feedback on this, but never
> received a response.

Sorry, I didn't receive this message.

    
  
Blocks: 596901

    
    

    
  
See bug 596901 comment 3 for another why the way that we call Begin/EndSecureKeyboardInput is wrong, and why this patch is the right way to fix this, the way I see it.

    
    

    
  
Also, note that the assertion annotations added in bug 542116 should be removed once this lands.  And the patch no longer applies cleanly on trunk...

    
    

    
  
I don't want to block on this for 2.0 but I'd approve a patch.
blocking2.0: --- → -

          status2.0:
          --- → wanted

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Secure Focus Fix (v1.2) (obsolete)
        — 
          — Splinter Review
      

    


  
Updated patch so that it applies on trunk

        Attachment #483077 -
        Attachment is obsolete: true

    
    

    
  
I have tried alternate methods using nsTextControlFrame::HandleEvent and nsTextControlFrame::Focus/Blur, but the Focus events don't seem to make it to the nsTextControlFrame.

    
    

    
  
(In reply to comment #23)
> I have tried alternate methods using nsTextControlFrame::HandleEvent and
> nsTextControlFrame::Focus/Blur, but the Focus events don't seem to make it to
> the nsTextControlFrame.

Do we have a frame for the control when the event is dispatched?

    
    

    
  
The focus events sent to nsTextControlFrame::SetFocus (not HandleEvent), where the original secure entry code was. Dealing with focus changes there leads back to the original problem, since SetFocus is not called during page changes (either through pressing enter or back/forward buttons).

        Attachment #494228 -
        Flags: review?(joshmoz)

        Attachment #494228 -
        Flags: review?(joshmoz) → review?(enndeakin)

    
    

    
  
Comment on attachment 494228 [details] [diff] [review]
Secure Focus Fix (v1.2)

I'm pretty sure I already reviewed this with comments above.

Putting calls at the beginning of WindowLowered and Focus methods isn't correct, as nothing is guaranteed to be focused or blurred at all.

The actual place where a blur occurs is at:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1459

The actual place where a focus occurs is at:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1679

It sounds though like the 'secure password feature' (I'm not familiar with what this feature is) should just be treated like a variation of IME, and thus, should be enabled and disabled in the same places.

And as already indicated, I don't think you should be calling Cocoa methods directly in nsFocusManager.

        Attachment #494228 -
        Flags: review?(enndeakin) → review-

        Attachment #494228 -
        Attachment is obsolete: true

    
  
Assignee: sgreenlay → joshmoz

    
    

    
  
Here's another vote to fix this bug.

Thanks!

    
    

    
  
This breaks TextExpander, one of my favorite apps. :(

    
    

    
  
I'd love to see this fixed, it breaks Textexpander and 1Password. Thanks!

    
    

    
  
+1 on this. This bug is a real showstopper if you are using 1Password and TextExpander. I've been watching this thread patiently for many months and this issue is quite seriously driving me away from FireFox now and towards Chrome as it is seriously impacting my productivity. I hope it gets fixed soon.

    
    

    
  
(In reply to comment #30)
> ...driving me away from FireFox now and towards Chrome

I experienced the same (or similar) issue with Chrome over the last few days. No option then.

    
    

    
  
(In reply to comment #31)
> (In reply to comment #30)
> > ...driving me away from FireFox now and towards Chrome
> 
> I experienced the same (or similar) issue with Chrome over the last few days.
> No option then.

Argh... really? Well that's bad news. And one more reason to get it fixed in Firefox.

    
    

    
  
(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > ...driving me away from FireFox now and towards Chrome
> > 
> > I experienced the same (or similar) issue with Chrome over the last few days.
> > No option then.
> 
> Argh... really? Well that's bad news. And one more reason to get it fixed in
> Firefox.

According to TextExpander support it's a Mozilla issue (hence the bug in here) and therefore affects both Firefox and Chrome. It does not affect Safari.

The workaround is to disable auto submission <a href="http://screencast.com/t/hW7XyRsf6Xp6">[screenshot]</a> and to never hit [enter] but to click the [submit] buttons. To be honest I still find myself having to quit Chrome and relaunch it to reset the issue, sometimes even relaunch TextExpander.

Fortunately Chrome will restore your last windows automatically if you have that option enabled <a href="http://screencast.com/t/0w8hKfmtaNmy">[screenshot]</a> so it's quick to do… still sucks, of course. But at least there's a "workaround" for now.

-Joseph

    
    

    
  
It's because I use TextExpander that I noticed this bug. It keeps TextExpander from working.

Joseph, about the workaround;

Where do you find those Login Options, shown in your first screenshot? I don't see enough in the image to guide me to the same options in my browser. Are those options in Firefox?

    
    

    
  
(In reply to comment #34)
> Joseph, about the workaround;
> 
> Where do you find those Login Options, shown in your first screenshot? I don't
> see enough in the image to guide me to the same options in my browser. Are
> those options in Firefox?

Sorry, my bad. That screenshot is in 1Password. Here's a broader screenshot: http://screencast.com/t/BkVqZOm7S0Dv

The explanation came to me via Textexpander support. 

--

Most likely, you're being affected by the bug in Firefox and Chrome where they enable secure input but don't disable it. This bug is particularly infuriating because it doesn't seem clear to the end user what causes it. However, it does have a cause, a workaround, and it does seem to be getting some traction from the Mozilla folks. Switching product probably won't help, as the other text expansion applications observe input in the same way as TextExpander and thus are subject to deprivation by secure event input being left enabled.

When these browsers display a password field, they turn on secure event input so that no one, including TextExpander, can peek at your passwords. Problem is, if you use the Return key to submit a form from within its secure field, they won't turn secure event input off. This appears to be a bug they've inherited from some Firefox code they use.

The workaround is to use the submit button rather than using Return in the password field. 1Password is similarly affected by this bug. The workaround there is to turn off auto-submit and just use auto-fill then press the button to submit.

--

So, disable auto-submit in 1Password, and click the Submit button with the mouse (don't hit the return key) and that'll keep the bug from rearing it's ugly head.

I still do run into it occasionally, presumably because I'm careless and hit return in a field where I shouldn't, but it's a lot more rare now that I've disabled auto-submit.

-Joseph

    
    

    
  
+1 Same for me. I use Textexpander a lot for pre-written answers sent with Gmail Apps version.

Fixing this will help a lot of mac power-users.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix v1.3 (obsolete)
        — 
          — Splinter Review
      

    


  
This fixes the bug as reported here, generally seems to work better than our existing approach. I suspect this code could be better organized but I'm out of time for this at the moment.

    
  

        Attachment #510758 -
        Flags: feedback?(masayuki)

    
    

    
  
(In reply to comment #37)
> Created attachment 510758 [details] [diff] [review]
> fix v1.3
> 
> This fixes the bug as reported here, generally seems to work better than our
> existing approach. I suspect this code could be better organized but I'm out of
> time for this at the moment.

Josh, thanks for the effort to fix this problem. I have no clue what to do with the fixed code you attached to your mail. How do I make the code work my browser.

    
    

    
  
(In reply to comment #38)

> Josh, thanks for the effort to fix this problem. I have no clue what to do with
> the fixed code you attached to your mail

Probably just wait for it to get reviewed and released in a Firefox update.

    
    

    
  
> Probably just wait for it to get reviewed and released in a Firefox update.

"Constant improvement through innovation, eh?" Thanks!<G>

Regards,

Peter
_______________________
Peter Gold
KnowHow ProServices

    
    

    
  
(In reply to comment #38)
> (In reply to comment #37)
> > Created attachment 510758 [details] [diff] [review]
> > fix v1.3
> > 
> > This fixes the bug as reported here, generally seems to work better than our
> > existing approach. I suspect this code could be better organized but I'm out of
> > time for this at the moment.
> 
> Josh, thanks for the effort to fix this problem. I have no clue what to do with
> the fixed code you attached to your mail. How do I make the code work my
> browser.

Peter, this is a small piece of C code from a huge compilation, not a piece of PHP you can stitch easily. We, poor humans, will have to wait, but at least, a solution is coming ;-)

    
    

    
  
Comment on attachment 510758 [details] [diff] [review]
fix v1.3

nsIWidget::(Begin|End)SecureKeyboardInput() are only implemented on cocoa. So, I think that they don't need now in nsIWidget (but we cannot remove them now).

I think that should should call them from nsChildView::SetInputMode(), directly. However, we cannot do so by some reasons. So, I think that your patch is reasonable for now.

> bool needSecureInput = !!(aState & nsIContent::IME_STATUS_PASSWORD)

This is wrong, nsIContent::IME_STATUS_PASSWORD doesn't mean that the aContent is password field, it means "editor but IME should be disabled".

Furthermore, SetIMEState() is called only when the IME enabled state is changing. So, if you move between <input style="ime-mode: disabled;"> and <input type="password">, this isn't called probably.

I think you should move this block to OnChangeFocus() below the null check of |widget|. You should check the type attribute value of aContent if it's <input>. See here to see the logic to get the type attribute.
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#281

And please use PRBool and the static variable should be in nsIMEStateManager rather than global.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix v1.4 (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks for the feedback Masayuki, I am not very familiar with the IME code.

        Attachment #510758 -
        Attachment is obsolete: true

        Attachment #510936 -
        Flags: review?(masayuki)

        Attachment #510758 -
        Flags: feedback?(masayuki)

    
    

    
  
Comment on attachment 510936 [details] [diff] [review]
fix v1.4

> +  bool contentIsPassword = false;

PRBool

> +    if (aContent->Tag() == nsGkAtoms::input || aContent->Tag() == nsGkAtoms::textarea) {

You don't need to check <textarea>. Only <input type="password"> can be a password editor.

# Note that chrome script can make any editors password editor even if it's an HTML editor. However, we don't need to mind such case because it must not be and editor doesn't work as password editor (the text isn't invisible).

> +  }
> +  else {

} else {

Otherwise, looks good for me. r=masayuki with above changes.

        Attachment #510936 -
        Flags: review?(masayuki) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix v1.5 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #510936 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix v1.6
        — 
          — Splinter Review
      

    


  
This also fixes bug 596901, changing expected assertions for some reftests to 0.

        Attachment #511000 -
        Attachment is obsolete: true

    
  

        Attachment #511017 -
        Flags: approval2.0?

        Attachment #511017 -
        Flags: approval2.0? → approval2.0+

    
    

    
  
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/8632f4e089e0

This should be in tomorrow's nightly build and the next Firefox 4 beta.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
    

    
  
(In reply to comment #47)
> pushed to mozilla-central
> 
> http://hg.mozilla.org/mozilla-central/rev/8632f4e089e0
> 
> This should be in tomorrow's nightly build and the next Firefox 4 beta.

I just updated to Firefox 4.0b11 and the issue still occurs.

After I use the auto-submit feature of 1Password, Textexpander stops working.

    
    

    
  
(In reply to comment #48)
> (In reply to comment #47)
> > pushed to mozilla-central
> > 
> > http://hg.mozilla.org/mozilla-central/rev/8632f4e089e0
> > 
> > This should be in tomorrow's nightly build and the next Firefox 4 beta.
> 
> I just updated to Firefox 4.0b11

That would be the *current* beta, not the "next Firefox 4 beta".

You'll need to download a nightly build, or the *next* beta (when it's released), to test this.

    
    

    
  
(In reply to comment #49)
...
> > I just updated to Firefox 4.0b11
> 
> That would be the *current* beta, not the "next Firefox 4 beta".
> 
> You'll need to download a nightly build, or the *next* beta (when it's
> released), to test this.

Ah thanks! 

Really looking forward to see this fixed! ;)

Best

Christian

    
    

    
  
I can confirm this is 100% FIXED in the 2/9 nightly. THANK YOU JOSH AAS! I have been waiting for this fix for a year now.

    
    

    
  
I still seem to be getting this problem in Firefox for Mac version 15.0.1 (with a MacBook Pro 2008, Intel Core 2 Duo 2.5 GHz, 6 GB RAM). It shows up often with the utility, TextExpander (version 3.4).

    
    

    
  
BTW, this is with Mac OS 10.7.4 (Lion).

    
    

    
  
I've got the same problem running the latest version of firefox (16.0.1) for mac (Mountain Lion - 10.8.2)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: