380222 - Rewrite fillDocument() after new pwmgr lands

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

After bug 374723 lands, fillPassword() in nsPasswordManager.js should be rewritten. The landed code is a straight port of the old C++ code... The logic is massy and hairy, so it was decided to untangle it in a second phase/step in order to help minimize landing regressions.

Bug 368959 covered issues with the existing C++ code, but it really needs to be hit with a bigger stick.

Comment 1

[Justin Dolske [:Dolske]]

Oops. It's fillDocument() that needs some lovin' -- not fillPassword().

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Draft patch

Checkpointing work... I think this is mostly in a final state. This throws out the bizarre logic in the old code, and shares more code between onFormSubmit() and fillDoc(). Form field names are no longer used, although they're still stored lest we need them later. It might be worth adding code to for generic (standard?) field names like "username" and "password" (and variations), although then you have to decide when to use which method.

I tested this out on a number of sites I had logins stored for, and it worked fine. But this should be targeted as a "high risk" change for A6, as there's a potential for breakage on sites finely tuned to quirks of the old code.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

This is unfortunately also going to require some rewrites of the Mochitests. :( The tests have a bunch of logins, but want to avoid triggering the autocomplete cases (which are hard to test, because they don't just automatically fill in a login)... I thought I was being clever by assigning different form field names to each, but this patch causes that cleverness to cease working.

I'd like to progress with a review before rewriting the tests, but will have them done before checking in.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's probably worth looking through checkin history to see if there were security bugs in this code in the past, to make sure you haven't regressed the fixes.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Updated and expanded test suite

The existing test suite was trying to be clever by using a bunch of logins with different form field names. I'm not really sure why I wrote them that way. :-) This match thwacks the tests so that it doesn't rely on that. I don't think this was a realistic use pattern for real web sites, anyway.

I've also expanded the tests to cover far more variations of login forms... Ordering of the input elements (and presence of an unused textfield), names of the elements, prefilled values.

test_basic_form_1pw.html covers every such variation for a form with 1 password field (288 forms).

test_basic_form_2pw_1.html covers the first batch of variations for a form with 2 password fields -- 1728 forms out of 8640. I'll follow up with the rest once some other small bugs are fixed, to avoid having to change the expected results for some of the other ~6000 tests. :-)

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 268040 [details] [diff] [review]
Patch for review, v.1

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js


>     /*
>+     * _getFormFields
>+     *
>+     * Returns the username and password fields found in the form.
>+     * Can handle complex forms by trying to figure out what the
>+     * relevant fields are.
>+     *
>+     * Returns: [usernameField, newPasswordField, oldPasswordField]
>+     *
>+     * usernameField may be null.
>+     * newPasswordField will always be non-null.
>+     * oldPasswordField may be null. If null, newPasswordField is just
>+     * "theLoginField". If not null, the form is apparently a
>+     * change-password field, with oldPasswordField containing the password
>+     * that is being changed.
>+     */
>+    _getFormFields : function (form, isSubmission) {
>+
>+        // local helper function
>+        function getUsernameField (form, passwordFieldIndex) {
>+            var usernameField = null;
>+
>+            // Search backwards to first text field (assume it's the username)
>+            for (var i = passwordFieldIndex - 1; i >= 0; i--) {
>+                if (form.elements[i].type == "text") {
>+                    usernameField = form.elements[i];
>+                    break;
>+                }
>+            }
>+
>+            return usernameField;
>+        };

why is this a helper? its called once, just inline it...


>+        // Locate the username field in the form. We might not find a
>+        // username field if the user is already logged in to the site. 
>+        var usernameField = getUsernameField(form, pwFields[0].index);
>+        if (!usernameField) {
>+            this.log("(form -- no username field found)");
>+        }

nit: brackets on single line (multiple times ;))

>+        if (pwFields.length == 3) {
>+            // Look for two identical passwords, that's the new password
>+
>+            if (pw1 == pw2 && pw2 == pw3) {
>+                // All 3 passwords the same? Weird! Treat as if 1 pw field.
>+                newPasswordField = pwFields[0].element;
>+                oldPasswordField = null;
>+            } else if (pw1 == pw2) {
>+                newPasswordField = pwFields[0].element;
>+                oldPasswordField = pwFields[2].element;
>+            } else if (pw2 == pw3) {
>+                oldPasswordField = pwFields[0].element;
>+                newPasswordField = pwFields[2].element;
>+            } else  if (pw1 == pw3) {
>+                // A bit odd, but could make sense with the right page layout.
>+                newPasswordField = pwFields[0].element;
>+                oldPasswordField = pwFields[1].element;
>+            } else {
>+                // We can't tell which of the 3 passwords should be saved.
>+                this.log("(form ignored -- all 3 pw fields differ)");
>+                return [null, null, null];
>+            }

I think its worth reordering these based on likely cases, probably pw2 == pw3, then pw1 == pw2, then the odd 1 == 2 == 3 and 1 != 2 != 3 cases last


>@@ -692,102 +786,57 @@ LoginManager.prototype = {

>+        // Get the appropriate fields from the form.
>+        var [usernameField, newPasswordField, oldPasswordField] =
>+                this._getFormFields(form, true);
>+
>+        // Need at least 1 valid password field to do anything.
>+        if (newPasswordField == null)
>+                return;

nit: double indent (why are we using 4-space anyway? :))

>+        // We need a username to work with, unless the form is a change
>+        // password form (which allows up to prompt to modify some existing
>+        // login).
>+        if (!usernameField && !oldPasswordField) {
>+            this.log("(form submission ignored -- couldn't find a " +
>+                     "username, and no oldPasswordField found.");
>+            return;
>         }

hmm, doesn't this preclude working with password-only sites (i.e. mailman admin pages).  IIRC, this used to work at one time...

>         // Check for autocomplete=off attribute. We don't use it to prevent
>         // autofilling (for existing logins), but won't save logins when it's
>         // present.
>         if (autocompleteDisabled(form) ||
>             autocompleteDisabled(usernameField) ||
>-            autocompleteDisabled(pwFields[0].element) ||
>-            (pwFields[1] && autocompleteDisabled(pwFields[1].element)) ||
>-            (pwFields[2] && autocompleteDisabled(pwFields[2].element))) {
>+            autocompleteDisabled(newPasswordField) ||
>+            autocompleteDisabled(oldPasswordField)) {
>                 this.log("(form submission ignored -- autocomplete=off found)");
>                 return;
>         }

maybe we should put this in getFormFields?  we should respect autocomplete="off" in the same way as http://msdn2.microsoft.com/en-us/library/ms533032.aspx

also, from IRC discussion, we should explore a hidden pref to ignore autocomplete=off if there's a master password, like wallet did in the beginning.

>         existingLogin = findExistingLogin(this);
>         if (existingLogin) {
>             // Change password if needed
>             this.log("Updating password for existing login.");
>             if (existingLogin.password != newPasswordField.value)
>                 this.modifyLogin(existingLogin, formLogin);

Hmm, shouldn't you log this only if we're calling modifyLogin? </driveby>

>             return;
>@@ -891,221 +940,107 @@ LoginManager.prototype = {
>         return this._getPasswordOrigin(uriString);
>     },
> 
> 
>     /*
>      * _fillDocument
>      *
>      * Called when a page has loaded. Checks the document for forms,
>-     * and for each form checks to see if it can be filled out with a
>+     * and for checks each form to see if it can be filled out with a

and checks each form?


>+            // If we have multiple logins, attach the autocomplete stuff
>+            // Note that we require a username field to do this!
>+            if (logins.length > 1) {
>+                // XXX should be able to pass in |logins| to init attachment
>+                if (usernameField)
>+                    this._attachToInput(usernameField);
>+                else
>+                    continue;

http://lxr.mozilla.org/mozilla1.8/source/toolkit/components/passwordmgr/base/nsPasswordManager.cpp#2025
shows that we always attached the listener if there was even one match, so I think we should just always do this

> 
> 
>     /*
>      * _attachToInput
>      *
>+     * Hooks up autocomplete support to a username field, when multiple
>+     * logins are available.

see previous comment about > 0 vs. > 1

close, but I want to see another patch rev

Comment 7

[Justin Dolske [:Dolske]]

(In reply to comment #6)

> I think its worth reordering these based on likely cases, probably pw2 == pw3,
> then pw1 == pw2, then the odd 1 == 2 == 3 and 1 != 2 != 3 cases last

The 1==2==3 case is first because it's simpler logic to catch that first, rather that having pw2 == pw3 succeed and then checking to see if pw1 was also the same value. I could flip the pw2==pw3/pw1==pw2 cases, but the sequence of checking isn't quite as clear... I think that maintainable code wins out over a micro-optimization here (especially since 3-password forms are not very common).

> hmm, doesn't this preclude working with password-only sites (i.e. mailman admin
> pages).  IIRC, this used to work at one time...

Hmm you're right. I'll have to see what other code/tests isn't expecting a blank username in stored logins, though. The only no-username case I had been thinking of was when you have a stored login (with username), but the site knows you via a Cookie so only displays a password field.

> maybe we should put this in getFormFields?  we should respect
> autocomplete="off" in the same way as
> http://msdn2.microsoft.com/en-us/library/ms533032.aspx
> 
> also, from IRC discussion, we should explore a hidden pref to ignore
> autocomplete=off if there's a master password, like wallet did in the
> beginning.

I want to spin this off to another bug, for landing after A6... That will make verifying "hey, A6 stopped filling in my password" bugs easier to verify as being a bug in fillDoc (or not).

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.2

This addresses the other review comments. Support for processing a submitted form without a username needs some more work, but since that's independent of fillDocument() I'll spin that off to a second bug. But I did fix a number of little bugs to get changing a password without a username working (eg, Google accounts).

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.2B (oops)

Oops, included a couple of lines from another patch in the last attachment.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Updated and expanded test suite (v2)

Adds some tests for forms without a username field.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 269651 [details] [diff] [review]
Patch for review, v.2B (oops)

looks good, let's get it in!

Comment 12

[Shawn Wilsher :sdwilsh]

File a bug on getting checkin access.

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/Makefile.in;
new revision: 1.6; previous revision: 1.5
Checking in toolkit/components/passwordmgr/test/pwmgr_common.js;
new revision: 1.4; previous revision: 1.3
Checking in toolkit/components/passwordmgr/test/test_0init.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_basic_form.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_basic_form_0pw.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_basic_form_1pw.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_basic_form_2pw_1.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_basic_form_2pw_2.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_basic_form_3pw_1.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_bug_221634.html;
new revision: 1.6; previous revision: 1.5
Checking in toolkit/components/passwordmgr/test/test_bug_227640.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_bug_242956.html;
new revision: 1.5; previous revision: 1.4
Removing toolkit/components/passwordmgr/test/test_bug_270558.html;
new revision: delete; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_bug_360493_1.html;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/passwordmgr/test/test_bug_360493_2.html;
new revision: 1.5; previous revision: 1.4

Comment 13

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-06-25+14%3A20&maxdate=2007-06-25+14%3A21&cvsroot=%2Fcvsroot