New issue
Advanced search Search tips

Issue 739792 link

Starred by 6 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Autofill dispatches keydown with an Event instance instead of a KeyboardEvent instance

Reported by lucco...@gmail.com, Jul 6 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.1 Safari/603.1.30

Steps to reproduce the problem:
I have created a sample app to demonstrate the problem:
https://powerful-badlands-23851.herokuapp.com/test

Steps to reproduce:
1. using chrome 59, login with "test@test.com" password "test"
2. When Chrome asks if you want to save the password say YES
3. now click logout... and you will should see the bug

What is the expected behavior?
Firstly I don't believe chrome should simply submit a form automatically without user input. But if this is the expected behavior then the form should use a password instead of simply letting it blank.

What went wrong?
From what I analyzed in Developer Tools, Chrome submits the form with empty password and when it fails, it keeps trying forever on an endless loop. 

Did this work before? Yes 58

Chrome version: 59.0.3071.115  Channel: stable
OS Version: OS X 10.12.4
Flash Version: 

This is frustrating and my clients are getting impacted real hard.
 
Components: UI>Browser>Passwords Blink>Forms
Status: Untriaged (was: Unconfirmed)
Cc: kolos@chromium.org
Components: UI>Browser>Autofill
Labels: -Pri-2 OS-Windows Pri-1
Owner: se...@chromium.org
Status: Assigned (was: Untriaged)
Summary: Autofill dispatches keydown with an Event instance instead of a KeyboardEvent instance (was: Update 59 issue with saved passwords - continuous login retries )
Summary (with spoilers) -
Chrome now fires keydown and more events when automatically filling usernames and passwords, but some code you load compares an undefined field on the keydown Event instance to an undefined field on $.ui.keyCode, which makes the code think Enter is pressed and thus a form submission is initiated.


Full details -

The root cause of the difference of behavior in Chrome is that it seems that Chrome has started firing the keydown events when it employs "autofill" for usernames and passwords. This change makes a lot of sense and it aligns usernames and passwords with the rest of the automatic filling mechanism, that already fired keydown (and others) when automatically filling other fields. r458459 implemented that.

It basically switched from setValue to setAutofillValue which adds more events and probably fixes a lot of case where forms could not handle the new value because it expected those events (they fire when a user actually types), perhaps for switching from non-native placeholders to the real value.

But (there is always a "but"), it seems that the event object is an instance of Event specifically and not of KeyboardEvent, so it lacks all of the standard-or-deprecated KeyboardEvent and UIEvent properties, like charCode, code, key, keyCode, and which.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/exported/WebFormControlElement.cpp?q=SetAutofillValue&sq=package:chromium&dr=CSs&l=103

Now, that seems wrong and can break code in general in interesting ways, even though it was apparently already used for about a year for automatically filling regular fields - r392842.

However - the real root cause of your issue, is that the code in components.js (that you load in your page) seems to have a bug and this chain of events simply triggers it -
Line 17 of https://powerful-badlands-23851.herokuapp.com/javax.faces.resource/components.js.jsf?ln=primefaces&v=6.1 include this snippet (beautified by the Developer Tools and commented by me) -
    // Listening to the keydown event.
    this.jqTarget.closest("form").off("keydown." + this.id).on("keydown." + this.id, function(d) {
      // Getting the jQuery UI keyCode mapping.
      var c = $.ui.keyCode;
      // Checking whether the user pressed Enter using the normal Enter key
      // or using the numpad Enter key.
      // But note the following (only 3 and 4 are crucial here) -
      // 1. Using the deprecated KeyboardEvent.prototype.which, but regardless.
      // 2. Comparing it to ENTER, which indeed exists in $.ui.keyCode.
      // 3. Comparing it to NUMPAD_ENTER, which does not(!) exist in
      // $.ui.keyCode, which means it returns undefined.
      // 4. Since d is an instance Event and not KeyboardEvent,
      // d.which is undefined. This makes the following expression -
      // d.which == c.NUMPAD_ENTER
      // Return true (undefined == undefined).
      if (d.which == c.ENTER || d.which == c.NUMPAD_ENTER) {
        // In case the submit button was clicked, do nothing, the
        // form will be submitted. This condition is false.
        if ((b.scope && b.scope.find(d.target).length == 0) || $(d.target).is('textarea,button,input[type="submit"],a')) {
          return true
        }
        // Dispatch a click event to the Enter button, which submits the form and automatically log in.
        b.jqTarget.click();


I bet components.js is not your code, but a library code, which is too bad, but that is life for you. :(

Comment 3 by lucco...@gmail.com, Jul 7 2017

Kolos, thanks a lot for your analysis. 
The file you found the bug on belongs to Primefaces, I will report it to them. 

I just didn't understand this part: "it seems that Chrome has started firing the keydown events when it employs "autofill" for usernames and passwords"

Why exactly does this make a lot of sense?

I'm a noob on JS, but shouldn't keydown event be triggered when a key has really been pressed down by a user? And if Chrome is firing this event, what is its related key?

Thanks

Comment 4 by phistuck@gmail.com, Jul 7 2017

#3 - this paragraph from my comment explains it -
"It basically switched from setValue to setAutofillValue which adds more events and probably fixes a lot of case where forms could not handle the new value because it expected those events (they fire when a user actually types), perhaps for switching from non-native placeholders to the real value."

To expand a bit, some websites have code that listens to various KeyboardEvent types for animating stuff, revealing certain form controls, enabling an initially disabled submit button, and for general processing before the form is submitted (or for re-implementing pressing Enter to submit for some reason, even though it has been natively supported in HTML without any JavaScript for decades, which is your case), so when they did not receive those events but the fields were automatically filled, the code of the website was not aware that something has been typed and thus, for example, did not enable the submit button. The user has to manually re-type one or more of the values, or remove and re-type a character for the code to detect that. That was user hostile and unhelpful.

Firing those events make sense, because then it would be as if the user has typed those values and thus normal processing might be needed and run.

I guess the right thing to do is to fire events per character (or similar) and not at the beginning of the entire input and at the end of the entire input, but perhaps using input method editors (IMEs) for entering and composing exotic language characters (Chinese, Japanese, Korean...) is already behaving the same way.

One more option you have is, if it is under your control, revert back to jQuery UI earlier than 1.11, because 1.11 removed NUMPAD_ENTER -
https://jqueryui.com/upgrade-guide/1.11/#removed-ui-keycode-numpad_
https://github.com/jquery/jquery-ui/commit/274ed73cd7da3025dc172b17f1c0820183f9a055

(That happened four years ago, wow. No one at PrimeFaces noticed?)

There is an issue filed already with a work around (add the relevant keyCode back) -
https://github.com/primefaces/primefaces/issues/2448
Which they have fixed if you get the latest version (or manually apply the fix).

Comment 5 by lucco...@gmail.com, Jul 7 2017

Thanks for the explanation Phistuck. 
Right now I don't need a workaround as the functionality that was using the bugged code is no longer necessary to me (<p:defaultCommand>)
Thanks!

Comment 6 by phistuck@gmail.com, Jul 7 2017

#5 - just be aware that several PrimeFaces widgets use this undefined property, so it might happen in other parts of your application. Hopefully not. :)

Comment 7 by tkent@chromium.org, Jul 11 2017

Components: -Blink>Forms

Comment 8 by ma...@chromium.org, May 1 2018

Status: Untriaged (was: Assigned)
Status: Assigned (was: Untriaged)

Sign in to add a comment