471906 - Login manager's onblur handler shouldn't do anything when the username is blank

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090102 Shiretoko/3.1b3pre ID:20090102020450

The username and password fields are automatically filled in even with the hidden pref signon.autofillForms set to false.

Steps to reproduce:
1. Open about:config and set signon.autofillForms to false
2. Open https://litmus.mozilla.org/ and create a new account if not done yet
3. Open https://litmus.mozilla.org/login.cgi and login for the first time
4. Use the password manager to save the password 
5. Logout and reload https://litmus.mozilla.org/login.cgi
6. Click in the username text field (only if JavaScript is disabled)
7. Click anywhere on the page to blur the username text field

With step 7 the username and password fields are fill-in the the username and password given in step 4.

This is a regression on the 1.9.1 branch. I've identified the regression range and one of the following changesets has been caused this malfunction:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-06+00%3A00&enddate=2008-09-07+03%3A00

Related bugs: bug 399703, bug 447788, bug 446109, and bug 400795.

Justin, could it have been regressed by bug 446109?

Comment 1

[Justin Dolske [:Dolske]]

This is mostly working as expected... The autofill pref is for suppressing the automatic filling-in of logins when the page loads, and that's what happens. It's not supposed to prevent filling in logins when there's explicit user interaction.

Normally that interaction would be selecting a login from the autocomplete dropdown. There's also an optimization to allow typing a username and having that login filled when the focus shifts. This last case was changed by bug 400795 to share the normal fillForm() code instead of the special-purpose fillPassword() code.

The fillPassword() code would fill in the first login that exactly matches the username in the username field... The username in this case is "", and so the litmus login isn't filled in. (If you had a password-only litmus login, it would have been filled in.)

The fillForm() code basically does this too, except it handles a blank username specially, as meaning "no username restriction, fill in whatever works". [This is the same code that runs when a page is first loaded, where the username field would normally be blank.]

So, the fix here should be to just have the onblur handler bail out if the username field is blank. signon.autofillForms isn't really involved here (same thing happens with it set to true, when you clear out the fields and trigger a blur).

I don't think this is a blocker, but we should take it on branch.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

Something like this... Also removes blank entries from the autocomplete dropdown, since they're confusing and wouldn't really work right anyway.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the patch Justin.

Further this bug should be handled more sensitive IMO. If some bad js code is injected into a website and the described behavior happens, both the username and password can be submitted via Ajax to a server. Even most users like the auto-fill in feature it can be dangerous on sites like myspace where such scripts can by injected on user pages. And AFAIR we store the passwords for a domain and not a specific web page.

Justin, if you still disagree to have the privacy keyword here, feel free to remove it again.

Comment 4

[Justin Dolske [:Dolske]]

If a page is vulnerable to XSS and "bad JS" is present, there are an endless variety of ways for it to steal data; it's an old argument that has been rehashed endlessly, let's not do it again here.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 366955 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManager.js b/toolkit/components/passwordmgr/src/nsLoginManager.js

>+                if (username.length &&

nit: test just |username| rather than username.length?

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Fixed nits. Also fixed the test_privbrowsing test, which was accidentally relying on this bug (there needs to be a break in exectution, via setTimeout, to allow the autocomplete popup to appear before its entries can be selected, the other autocomplete tests already do this).

Comment 10

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e1c90e99be30

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 376824 [details] [diff] [review]
Patch v.3

a191=beltzner

Comment 12

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/55857924b947

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090519 Minefield/3.6a1pre ID:20090519032945

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090519 Shiretoko/3.5b5pre ID:20090519035556