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.
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
Status: NEW → ASSIGNED
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 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+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 376824 [details] [diff] [review] Patch v.3 a191=beltzner
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
10 years ago
You need to log in before you can comment on or make changes to this bug.