Closed Bug 362576 Opened 18 years ago Closed 16 years ago

autocomplete="off" should prevent filling passwords in addition to remembering passwords

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: BijuMailList, Assigned: Dolske)

References

()

Details

Attachments

(1 file, 2 obsolete files)

Per 360493 comment #187

If there is autocomplete="off" for a form,
it may not suppress saved password auto filled all the time.

Steps:-
* a site had login form at http://mysite.com/login.html without autocomplete="off" 

* While logging-in User_A saved his password when prompted by PM.

* this setting went for days, and User_A use to get his password auto filled in.

* one day developer at mysite.com learned about autocomplete attr.

* he modified login form to autocomplete="off"

* but User_A will still get password autofilled !!!!

PS: 
for future User_X, PM wont be prompted to save password.
as well as if User_A and User_B have save password early 
firefox wont Auto-fillin
This would break the "remember password" bookmarklet.
Summary: autocomplete behavior is not the same way as IE → autocomplete="off" should prevent filling passwords in addition to remembering passwords
(In reply to comment #1)
> This would break the "remember password" bookmarklet.

Sad... IMO bookmarklet should be always powerful than site authors code.

So this bug is fix should include an about:config option to revert to current way by advanced users.

Also we should decide on what to do with already saved password, remove it for PM?.
I dont like the Idea of keeping saved password in PM, without notifying user.

> 
> So this bug is fix should include an about:config option to revert to current
> way by advanced users.
> 

no, the option should address the issue directly and allow advanced users to ignore autocomplete=off altogether as opposed to restoring current behavior which still requires a js hack.  
Attached file First Test Case (obsolete) —
To make Mike happy :)

Biju, good work finding this bug, congrats.
Blocks: 368959
Blocks: 373140
No need for a Javascript hack. Look at this (disclaimer: my personal site):
http://dotancohen.com/howto/firefox_password_manager.php
Assignee: nobody → dolske
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 3 M8
Deferring to M9, this can be handled as a bugfix but Jesse had some concerns about it, so crash landing it in M8 doesn't seem fair. May also depend on bug 389185, which also had some objections. :-)
Depends on: 389185
Target Milestone: Firefox 3 M8 → Firefox 3 M9
One idea is to remember whether autocomplete=off was present and overridden when the password was saved.  That would help in the bug 389185 case but not in the bookmarklet case.
Why do we even want this? The only reason that Firefox supports autocomplete=off was so that the banks would not block the Firefox UA. No end users want autocomplete=off support. Now that we have the banks' support, why make life even harder for end users. I move that this bug be closed as it makes Firefox less usable for end users, with no benefit.
There are many very reasonable cases where autocomplete is a problem rather then help. Imagine a page to adjust personal data along with a password field. Firefox will automagically fill in the form fields, overriding stuff provided by the server (?), and breaking the password-change logic in a way the user as to CLEAR the password field manually to not update his password.

Having a server-provided "autocomplete=off" will ease the situation.
(In reply to comment #9)
> There are many very reasonable cases where autocomplete is a problem rather
> then help. Imagine a page to adjust personal data along with a password field.
> Firefox will automagically fill in the form fields, overriding stuff provided
> by the server (?), and breaking the password-change logic in a way the user as
> to CLEAR the password field manually to not update his password.
> 
> Having a server-provided "autocomplete=off" will ease the situation.
> 

This should be client side, then, not server-side. When a user wants this functionality, do you think that he will write to the webmaster to request it?!? No, let him enable it from within his browser. A "Do not auto-complete this field" option in the right-click menu would be fine in this case.
(In reply to comment #10)
> (In reply to comment #9)
> > There are many very reasonable cases where autocomplete is a problem rather
> > then help. Imagine a page to adjust personal data along with a password field.
> > Firefox will automagically fill in the form fields, overriding stuff provided
> > by the server (?), and breaking the password-change logic in a way the user as
> > to CLEAR the password field manually to not update his password.
> > 
> > Having a server-provided "autocomplete=off" will ease the situation.
> > 
> 
> This should be client side, then, not server-side. When a user wants this
> functionality, do you think that he will write to the webmaster to request
> it?!? No, let him enable it from within his browser. A "Do not auto-complete
> this field" option in the right-click menu would be fine in this case.
> 

Well, that's where I disagree: The Webmaster should have the power to tell the browser how to behave - with the option of the user to OVERRIDE that decision. Not the other way around. Requiring a user to perform an action where, from a logical perspective of the website in question an autocomplete is not desired and thus disabled by the developer, is bad - imho. 

If the user wants to overrule the decision made by the webmaster, that's fine and firefox should support/offer a gui-way to do so. But the default behavior of the browser should be what the html-code suggests.

Actually, I don't really care what the default behaviour is as I'm not quite a defaut kind of guy. So long as the behaviour is configurable I'm game. That means, if "autocomplete=off" is specified then I want to be able to override it, even if I have to hack as described here:
http://dotancohen.com/howto/firefox_password_manager.php
Bumping to M10, I don't want to put this in M9.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
(In reply to comment #13)
> Bumping to M10, I don't want to put this in M9.
> 

What is the planned "fix" for this?
Assignee: dolske → nobody
Version: 2.0 Branch → Trunk
> What is the planned "fix" for this?

The fix should be to mark the bug WONTFIX. There is nothing broken to fix, quite the opposite, the bug is suggesting that we deliberately break something.
Target Milestone: Firefox 3 beta2 → ---
This is particularly obnoxious in combination with bug 364970 in FF3. I see that is WONTFIX so I will say this here: it is now impossible to use input type="password" for anything other than a login form, or to have more than one password field anywhere on the same hostname for different purposes.

The change password use case has already been mentioned. As another example, on some settings pages of my employer's main product, Zeus Extensible Traffic Manager, one user can set the password of another, or a password the device will use to authenticate to a hardware crypto device. This is done using input type="password" autocomplete="off" value="some value", but it is filled in with the (unrelated) password to log in to the UI. Worse, the previous text field, which is some way up the page and unrelated, is silently overwritten with the username.

There is currently nothing I can put in such pages to prevent that behaviour. We and other vendors require a way for the user to enter a password for a purpose other than logging in, and if Firefox 3 is released with its current autofill behaviour, what can we do except tell our customers that we are unable to support Firefox 3?
@chris: You should be using unique names for the individual password fields.

<form action="login.cgi">
<input type="password" name="loginPassword" />
</form>

<form action="changepassword.cgi">
<input type="password" name="oldPassword" />
<input type="password" name="newPassword" />
<input type="password" name="newPasswordVerify" />
</form>
@Dotan: We are. FF3 (beta 5) doesn't seem to care, see bug 364970 and possibly others.
(In reply to comment #19)
> @chris: You should be using unique names for the individual password fields.

No, this doesn't work in Firefox 3 anymore. Firefox 2 was looking at the name of the field to decide if it has to fill it or not. Firefox 3 doesn't care about the field name anymore and will fill it with the stored password in all cases. This also breaks Bugzilla, which is another example of an application concerned by this non-sense change in Firefox 3.

I'm honestly wondering if MoCo guys thought about this problem a bit or not when deciding this change.
(In reply to comment #21)
> I'm honestly wondering if MoCo guys thought about this problem a bit or not
> when deciding this change.

FWIW, I'm a MoCo guy and this bug is biting me too.
@Chris:
I see that the problem is that Firefox does not store the name of the fields. Autocomplete="off" would be a workaround, not a solution, to that problem. I will collect real-world use cases and file a descriptive bug report with a proposed solution. Please post the public URL of your application. Thanks.
In any case, this bug is not the correct place to discuss this. Please do not hijack this bug, and post all comments in the proper bug.
(In reply to comment #23)
> Please post the public URL of your application. Thanks.

The application in question is the web UI for a traffic manager, http://www.zeus.com/products/zxtm/ . There isn't a public instance available; you can download a VM if you really want at http://www.zeus.com/downloads/evaluation/desktop/your-details/id/zxtm

Problematic areas (areas where a password field is used other than for logging in) include System -> Users (one may edit a user other than oneself) and System -> Global Settings -> SSL Hardware.
(In reply to comment #24)
> In any case, this bug is not the correct place to discuss this. Please do not
> hijack this bug, and post all comments in the proper bug.

This bug *is* the correct place to discuss this. All other related bugs have been marked as dupes of this one. There is no need to file another bug about this topic.
@Dotan: bug 423966 got duped to this one.

The app in question is https://litmus.mozilla.org/

...and is available from cvs as mozilla/webtools/litmus.

You'll see the workaround here:

http://mxr.mozilla.org/mozilla/source/webtools/litmus/templates/en/default/auth/login.html.tmpl#179
Banks have legitimate, and legally required, security concerns, and Firefox should support that.

Users that have adequate physical and internet security for their machines, should also have the right to choose to have Firefox remember their passwords, and override the autocomplete=off issue.

Even if the users physical and internet security is not sufficient, they should have the ability to choose whether or not to take the risk of having Firefox remember their passwords.

Presently there is a bookmarklet that zaps the "autocomplete=off" stuff, it is annoying to have to use it, but at least it works, for most sites.  Some sites outsmart the bookmarklet through the use of complex javascript.  So far, this simply causes me to give my business to banks whose sites don't outsmart the bookmarklet.
Attached patch Patch v.1 (work in progress) (obsolete) — Splinter Review
I think this bug needs to be fixed, because a some recent bugs have shown that's there's a relatively common issue with this. The current problem is that the new password manager ignores form field names -- deliberately, because some sites were using different field names and accidentally breaking saving logins. However, this means sites don't have a way to prevent filling in logins where they really don't belong. The typical example seems to be:

-----
<h1>Welcome Admin! Adding a new user...</h1>
New Username:     <input name="user">
Initial password: <input name="pass" type="password">
-----

Filling in the admin's login here is unwanted, and sites should have the ability to prevent it from happening.

What this patch does:

When autocomplete=off is found when loading a page, the login is no longer automatically filled in. However, the autocomplete handler is still attached to the username field, so users can still manually select their login from the field's dropdown if they want to. This is what we already do when multiple logins are present -- we don't know which one to fill, so we fill nothing and attach the autocomplete handler to allow the user to choose from the dropdown.
Assignee: nobody → dolske
Attachment #253928 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3
Attached patch Patch v.2Splinter Review
Attachment #319447 - Attachment is obsolete: true
Attachment #319482 - Flags: ui-review?(beltzner)
Attachment #319482 - Flags: review?(gavin.sharp)
Comment on attachment 319482 [details] [diff] [review]
Patch v.2

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

>+    _isAutocompleteDisabled :  function (element) {
>+        if (element && element.hasAttribute("autocomplete") &&

nit: this was there before, but getAttribute already does a hasAttribute check, so it isn't necessary.

r=me code wise, but I'm a bit concerned about making this behavior change. It makes things more annoying for users who've used bookmarklets to force the password to be saved, or have passwords saved from before autocomplete=off was added, and the password manager dropdown isn't exactly easy to discover. Not sure how that compares to the annoyance of having fields filled in when they shouldn't be, considering that for this to help there the webmaster needs to be aware - is it common for pages to already have autocomplete=off for these kind of forms? Could we make this behavior conditional on the field names not matching, as well? I thought that's what you suggested when we discussed it on IRC.
Attachment #319482 - Flags: review?(gavin.sharp) → review+
It is easy to tab to an erroneously filled in field, and put in the right data... likely would have to do that anyway, if it hadn't been erroneously filled in.

It is easy to tab to or click on the blank field and fill in it, but if it had been filled in, that would have been unnecessary.

So my vote is to fill in the fields.
(In reply to comment #31)

> r=me code wise, but I'm a bit concerned about making this behavior change. It
> makes things more annoying

True, although it's no more annoying than we currently have with multiple logins (which people commonly hit on sites like Gmail). That's a known problem [c.f. bug 376668, bug 327044, bug 222653], but I'm optimistic that we can improve the UI for that case. And by reducing the autocomplete=off issue to that problem, such an improvement will kill 2 birds with one stone. (Although that's obviously not going to happen for Firefox 3.)

> the password manager dropdown isn't exactly easy to discover.

From a blank field it's not obvious that down-arrow/clicking will show the autocomplete-dropdown, but once you start typing in your username the dropdown is shown with matching logins. It's the same principle as form autocomplete and awesomebar, so I'm not *too* worried about discoverability.

> considering that for this to help there the webmaster needs to be
> aware - is it common for pages to already have autocomplete=off for these kind
> of forms?

Well, the reason for fixing this is that it provides a familiar workaround to sites that encounter this problem (which hopefully isn't often). I've been reluctant to fix this to date, because I didn't want to cripple login manager *or* stop users from using their bookmarklet-saved logins. This seemed like a reasonable compromise that still enables both cases.

> Could we make this behavior conditional on the field names not
> matching, as well? I thought that's what you suggested when we discussed it on
> IRC.

I wasn't entirely pleased with that idea (even though it was mine :), because it was kind of hackish. And it would have been confusing to explain that field names don't matter, except for when they do. :/
(In reply to comment #33)
> I wasn't entirely pleased with that idea (even though it was mine :), because
> it was kind of hackish. And it would have been confusing to explain that field
> names don't matter, except for when they do. :/

Well, the names currently matter in Firefox 2, right? We'd be making them matter slightly less, but I'm not really worried about confusion about the underlying implementation if we get all the cases that users will hit right, and I think using the field names this way is probably the closest we can get at this point.
The field names are important so the the password manager can work with some banks that require three fields to be filled in instead of two. Here is such an example:
https://login.bankhapoalim.co.il/cgi-bin/poalwwwc?dt=924&nls=EN
(In reply to comment #34)

> Well, the names currently matter in Firefox 2, right? We'd be making them
> matter slightly less, but I'm not really worried about confusion about the
> underlying implementation

I'd rather have a explicit mechanism for sites to use, instead of relying on details of the implementation. Using field names to identify what field to [use | not use] is underspecified and wishy-washy, but autocomplete=off isn't.

(In reply to comment #35)
> The field names are important so the the password manager can work with some
> banks that require three fields to be filled in instead of two.

Not really relevant, because a password manager login only consists of 1 username and 1 password. And there's no way to control what it remembered in the first place, other than field order.
(In reply to comment #36)
> (In reply to comment #35)
> > The field names are important so the the password manager can work with some
> > banks that require three fields to be filled in instead of two.
> 
> Not really relevant, because a password manager login only consists of 1
> username and 1 password. And there's no way to control what it remembered in
> the first place, other than field order.
> 

Then the password manager is inadequete for cases like this. I will file a new bug.
A commenter on this bug emailed me requesting that I send to him the bug number for the issue mentioned in Comment #35. I lost the email (1 year old daughter + keyboard). So here it is:
Bug# 432824 – Password Manager cannot remember three fields for login
Comment on attachment 319482 [details] [diff] [review]
Patch v.2

Ok, dolske convinced me over IRC that this is a problem we should fix before ship.

Short version is that by making the pw filling more tolerant of field changes, and fixing cases where forms wouldn't fill previously, we now fill in where we didn't before, and there's no easy way to prevent this.  Think we've hit this on bugzilla and litmus in our own tools, and there's been a growing number of reports.  We don't want to make the pwmgr less resilient, so we need to provide an explicit way for sites to prevent this behaviour.
Attachment #319482 - Flags: ui-review?(beltzner)
Attachment #319482 - Flags: ui-review+
Attachment #319482 - Flags: approval1.9+
Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
  new revision: 1.31; previous revision: 1.30
Checking in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html;
  new revision: 1.7; previous revision: 1.6
Checking in toolkit/components/passwordmgr/test/test_bug_227640.html;
  new revision: 1.6; previous revision: 1.5
Checking in toolkit/components/satchel/src/nsFormFillController.cpp;
  new revision: 1.101; previous revision: 1.100
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 433238
Verified fixed with Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9) Gecko/2008051202 Firefox/3.0. Fx3 no longer fills the name and password fields when editing a user account from Bugzilla (it was previously filling these fields with my own credentials, overwriting the other user's own password). Yay!
Status: RESOLVED → VERIFIED
(In reply to comment #39)

> so we need to provide
> an explicit way for sites to prevent this behaviour.
> 

1) This change broke password only forms where username and password are on different pages, this is used by several banks (Bug 434094)

<form><input type='password' autocomplete="off" name='password1'></form>

the autocomplete handler(for manually selecting saved credentials) is only attached to the username field, which is absent.

2) there needs to be an option to ignore aautocomplete="off" when saving credentials and now when filling them out

what is the objection to letting the user control his credentials?

Depends on: 434094
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: