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

VERIFIED FIXED in mozilla1.9.2a1



11 years ago
10 years ago


(Reporter: whimboo, Assigned: Dolske)


({privacy, regression, verified1.9.1})

1.9.1 Branch
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)




(1 attachment, 2 obsolete attachments)

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:


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

Justin, could it have been regressed by bug 446109?
Flags: blocking1.9.1?
Keywords: privacy
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.
Blocks: 400795
Flags: blocking1.9.1?
Keywords: privacy
Summary: Username and password auto-filled in even with signon.autofillForms set to false → Login manager's onblur handler shouldn't do anything when the username is blank
Target Milestone: --- → mozilla1.9.2a1
Posted patch Patch v.1 (WIP) (obsolete) — Splinter Review
Something like this... Also removes blank entries from the autocomplete dropdown, since they're confusing and wouldn't really work right anyway.
Assignee: nobody → dolske
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.
Severity: normal → major
Flags: wanted1.9.1?
Keywords: privacy
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.
Posted patch Patch v.2 (obsolete) — Splinter Review
Attachment #355161 - Attachment is obsolete: true
Attachment #366955 - Flags: review?(gavin.sharp)
Duplicate of this bug: 483349
Whiteboard: [needs review gavin]
Duplicate of this bug: 491824
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?
Attachment #366955 - Flags: review?(gavin.sharp) → review+
Posted patch Patch v.3Splinter Review
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).
Attachment #366955 - Attachment is obsolete: true
Whiteboard: [needs review gavin]
Pushed http://hg.mozilla.org/mozilla-central/rev/e1c90e99be30
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 376824 [details] [diff] [review]
Patch v.3

Attachment #376824 - Flags: approval1.9.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
You need to log in before you can comment on or make changes to this bug.