User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/20080313 SeaMonkey/1.1.9 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/20080313 SeaMonkey/1.1.9 Bug 390025 is switching the password manager from Wallet to Satchel for SeaMonkey 2, but this will break support for autocompleteoverride. While the FireFox folks wontfixed bug 245333, SeaMonkey should continue to support this preference. Reproducible: Always

Reading bug 245333, bug 63961 comment #4 and other such comments, I get the impression that even having that pref is a bug in SeaMonkey 1.x and this should be a WONTFIX.

The setting has been a compromise between the 'never support autocomlete="off"' camp and the 'we must support autocomlete="off" in case the banks have a problem with it' camp. The compromise was to keep this as a hidden preference with the default to be off (i.e., recognize autocomplete="off"). One option was to only allow the preference wallet.crypto.autocompleteoverride when a master password was set but that wasn't implemented. There are a lot of users that use this preference (some even staying on SeaMonkey specifically for it), so that should be considered in the decision to implement it or not. kairo: Thanks for looking at the nomination. I would have nominated it for blocking 2.0 final but there's not a flag for that yet.

http://mxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/src/nsLoginManager.js#719 /* * _isAutoCompleteDisabled * * Returns true if the page requests autocomplete be disabled for the * specified form input. */ _isAutocompleteDisabled : function (element) { ==> IT WOULD BE TRIVIAL TO ADD A CHECK FOR wallet.crypto.autocompleteoverride HERE if (element && element.hasAttribute("autocomplete") && element.getAttribute("autocomplete").toLowerCase() == "off") return true; return false; },

After discussion in dev.apps.firefox, Justin Dokske pointed out that there are already observer hooks in nsLoginManager.js to which we can listen to and then fire off LoginManager._fillForm() ourselves with the |ignoreAutocomplete| parameter. It's up to us then whether we want to keep |wallet.crypto.autocompleteoverride| or change it to something else. I suggest putting our formfill observer in nsSuiteGlue.js for no reason other than it's already there and listening to observer events.

(In reply to comment #8) > After discussion in dev.apps.firefox, Justin Dokske pointed out that there are > already observer hooks in nsLoginManager.js to which we can listen to and then Iiuc, |this._observerService.notifyObservers(form, "passwordmgr-found-form", "noAutofillForms");| and |this._observerService.notifyObservers(form, "passwordmgr-found-form", "autocompleteOff");| > fire off LoginManager._fillForm() ourselves with the |ignoreAutocomplete| Listening to the later, we would call |fillForm : function (form)|; but |foundLogins| would have to be (implicitly) rebuilded :-/ > parameter. Unless I'm missing something better than that, I still think your comment 5 is the way to look into.

> Listening to the later, we would call |fillForm : function (form)|; I said |_fillform| not |fillform| <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#981> /* * _fillform * * Fill the form with login information if we can find it. This will find * an array of logins if not given any, otherwise it will use the logins * passed in. The logins are returned so they can be reused for * optimization. Success of action is also returned in format * [success, foundLogins]. autofillForm denotes if we should fill the form * in automatically, ignoreAutocomplete denotes if we should ignore * autocomplete=off attributes, and foundLogins is an array of nsILoginInfo * for optimization */ _fillForm : function (form, autofillForm, ignoreAutocomplete, foundLogins) { > but |foundLogins| would have to be (implicitly) rebuilded :-/ Which it is if you bothered to actually read the source.

(In reply to comment #10) > > Listening to the later, we would call |fillForm : function (form)|; > > I said |_fillform| not |fillform| Yes, I read it correctly... > _fillForm : function (form, autofillForm, ignoreAutocomplete, foundLogins) What I'm not understanding is how it would be better than calling <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#1124> 1124 /* 1125 * fillForm 1126 * 1127 * Fill the form with login information if we can find it. 1128 */ 1129 fillForm : function (form) { 1130 this.log("fillForm processing form[id=" + form.id + "]"); 1131 return this._fillForm(form, true, true, null)[0]; 1132 }, Could you explain it to me ? > > but |foundLogins| would have to be (implicitly) rebuilded :-/ > Which it is if you bothered to actually read the source. I did ! That's why I wrote "implicitly". Then, my previous question stands: why isn't it better to do it your comment 5 way (which would not have to rebuild |foundLogins|) ?

> Could you explain it to me ? It skips the wrapper function and calls _fillForm directly? > Then, my previous question stands: why isn't it better to do it your comment 5 > way (which would not have to rebuild |foundLogins|) ? <http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/fecb7f47c4af2a89/3065a980372dcbba> You can fight city hall and perhaps win, or you could do it their way. You're welcome to write a patch and ask for r/a.

(In reply to comment #12) > It skips the wrapper function and calls _fillForm directly? Sure, it's only a choice of adding (3) const/useless parameter values then :-| > You can fight city hall and perhaps win, or you could do it their way. Thanks for the link. I was simply asking (more details) about what I didn't know. (The perf/optimization point wasn't explicitly discussed there, through.)

Since I made these changes, I thought I'd weigh in, with better than "It skips the wrapper function and calls _fillForm directly?" _fillForm will return the logins that it found for the form (based on the form action), which can then be passed in the next time around. So lookups are only done once in the ideal case. Look at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#976 and the loop it's in for the example. fillForm is just the public wrapper for _fillForm and does not do any memoization, as it's intended to be called as a one-off shot, ideally by somebody using the observer. Essentially what you said in comment #9 was right. So if you can call _fillForm, it's slightly better, but you'll have to be within nsLoginManager to make that call.

(In reply to comment #14) > [...] Essentially what you said in comment #9 was right. So if you can call Exactly. > _fillForm, it's slightly better, but you'll have to be within nsLoginManager to > make that call. Actually, if the "within nsLoginManager" (as in a (optimized) comment 5) solution would be accepted, there would be no more need to make any extra call: the override preference would be handled right away. Bug 439365 seems really fine for people without the pref; then, it's "good enough" to handle the pref :-> Justin, do you confirm that you refuse the "within nsLoginManager" solution ?

So, there are 2 separate issues with autocomplete=off... What happens when the page loads, and what happens when the form is submitted. The notification being discussed is used when the page loads. There are legitimate uses for disabling autocomplete on certain forms fields, and so the purpose of the notification let code (and, indirectly, the user) know that we didn't autofill a form. Firefox doesn't use this yet, but you can already fill in the form manually via the autocomplete drop-down widget. [Well, unless there's no username field... bug 433238] The other case -- what to do when a form is submitted -- seems to be what the wallet code actually uses this pref for. Currently, autocomplete=off causes login manager to ignore the form submission, and there's no pref or notification to stop that. There's a long history to that, and although it's unfortunate I don't think a global pref to disable that is the right solution. This can currently be worked around with Jesse's bookmarklet (https://www.squarefree.com/bookmarklets/forms.html#remember_password. I'd like to be able to give the user more control in some non-interrupting way, but some design work still needs done. This, again, touches on historical requirements, and there are legitimate uses of autocomplete=off for disabling saving logins (eg, on an admin's user-creation page). We probably need some UI that gives the user a non-intrusive notification that a password can be saved, without a full-fledged prompt. We've been talking about doing something like this with the Firefox UX folks, but more design work is needed. I don't think this is a huge problem, though. Saving logins on autocomplete=off forms is a relatively uncommon operation for a user. But once you've saved such a login, it can be used without a needing an awkward workaround.

Justin, thanks for the analysis. So basically without resorting to a bookmarklet (or greasemonkey script) there is no way the current LoginManager can be hooked to allow the saving of passwords with autocomplete=off. > (eg, on an admin's user-creation page). We probably need some UI that gives the > user a non-intrusive notification that a password can be saved, without a > full-fledged prompt. We've been talking about doing something like this with > the Firefox UX folks, but more design work is needed. A UI would be great but it would be great if in the interim if there is a backend hook for SeaMonkey (The suite never had a UI) to use to support the wallet.crypto.autocompleteoverride pref. Do you have any ideas what this API might look like? Perhaps a way for SeaMonkey or an extension to register a callback that would alter the behaviour of LoginManager._onFormSubmit. Would a #IFDEF MOZ_SUITE section in the LoginManager._isAutocompleteDisabled method be acceptable as an interim solution?

So, I think this bug is WONTFIX as is. A pref to globally ignore autocomplete=off when saving forms will break legit uses for the attribute. And I don't want to be #ifdefing code for something like this. However, as I mentioned in comment 16, I would be interested in investigating ways to give the user the ability to save such logins via some different UI/UX.

> However, as I mentioned in comment 16, I would be interested in investigating > ways to give the user the ability to save such logins via some different UI/UX. Justin, would a notification box asking the user if they want to save a password if "signon.autofillForms.autocompleteoverride" be acceptable?

Eh, I think it would still tend to be intrusive where it's not wanted. Firefox is talking about doing site-based notifications (basically, adding menu-like entries to the favicon/SSL dropdown panel), which could be used for exposing site-specific things in a non-obtrusive way. UI like that would be a good place for a "save this login anyway" entry (as well as an explicit command for filling a login when autocomplete=off is around), and that could presumably be enabled by default.

Justin, Sounds interesting. Bug number please?

Bug 398776.

Login manager will still attach the login autocomplete dropdown to autocomplete=off fields, it just doesn't fill them in automatically when the page loads. So, use a bookmarklet (eg comment 16) to save the login once, and then doubleclick/downarrow the username field to select the login next time you go to log in.

(In reply to comment #51) > Please do file a separate bug specifically for those issues. Bug 667233 was filed.

Updating my thinking on this since comments 16/21... The use of autocomplete=off is still correct and useful in some cases (as well as still being abused by some sites). So I don't want to break that. But we _should_ give the user the ability to override this. As alluded to in comment 21, Firefox now has support for doorhangers, and is using them today for asking to save passwords in normal cases. The doorhanger is anchored to an icon in the URL bar, and if you accidentally dismiss the doorhanger you can bring it back by clicking the icon. Doorhangers also support (or at least are intended to support) a mode whereby the icon is placed in the URL bar, but no prompt is shown until the user clicks the icon. So-called "silent" doorhangers are primarily intended as a mechanism to show the user when "always allow" permissions are in effect for a site (eg, for geolocation). That gives the user awareness of something being used, as well as the ability to revoke permission in a fairly obvious way (click icon, reset permissions). The same mechanism should work here -- show the icon only, which allows getting at the save-password dialog. Preserves the intended default action of autocomplete=off, while still allowing for user control. Patches welcome, I'd love for someone to take a shot at this. Not quite [good first bug] material, but shouldn't be very hard.

+1 for this bug. I was about to file the same. autocomplete=off has been abused a lot recently. Yahoo started using it for their login (including webmail and my.yahoo.com), which is why I stopped using Yahoo. Webmail apps - even some bigger providers - now use it, which was decidedly not the purpose. The admins are very self-righteous, and insist that the keep this "for security" because password saving "is unsafe". They are misguided, because a) keyboard loggers exist and are widespread, probably more widespread than malware that can read Firefox password store. b) even simple attacks by the little nephew exist: Just look over the shoulder c) possibly most importantly, forcing users to re-enter their password every time practically forces them to use a simple password - easy to remember, easy to type, probably even used on multiple websites. This obviously *lowers* overall security dramatically and thus poses a danger to security. So, autocomplete=off is actively harmful for security. And a massive pain for end users, without a recurse for them apart from severing entire customer relationships. Comment 16 is good and references Jesse's bookmarklet. That's good, but doesn't help the mass of users.

Copying patch over from Bug 951394

Comment on attachment 8349057 [details] [diff] [review] Patch 1 from #951394 Thanks for this patch! I hope it makes it into release. NIT: spaces after , and around = NIT: gOverrideAutocompleteAttribute

Comment on attachment 8349076 [details] [diff] [review] Patch 1 from #951394 +nitfix > +// Override autocomplete=false for password manager > +pref("signon.overrideAutocomplete",false); This is in /browser (firefox) > + gOverrideAutocompleteAttribute = Services.prefs.getBoolPref("signon.overrideAutocomplete"); This is in Toolkit. getBoolPref() will throw for anything not Firefox, e.g. SeaMonkey. Recommend that you move the pref to all.js

Moved to ./modules/libpref/src/init/all.js (I assume that that is the correct one)

Comment on attachment 8349278 [details] [diff] [review] Patch 1 from #951394 moved to all.js (In reply to Manish Goregaokar [:manishearth] from comment #70) > Moved to ./modules/libpref/src/init/all.js (I assume that that is the > correct one) Yes. Thank you very much.

Comment on attachment 8349278 [details] [diff] [review] Patch 1 from #951394 moved to all.js I think this would be slightly clearer if you put: if (!gOverrideAutocompleteAttribute) return false; in a separate block in _isAutocompleteDisabled (rather than combining it with the existing long condition). Also you should put at least one space after the comma in the line you're adding to all.js :) r=me with those changes. Can you file a followup on exposing this pref somehow?

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #72) > Can you file a followup on exposing this pref somehow? I don't quite get what you mean here. By followup do you mean that I should mention what the patch does? It adds an about:config option, `signon.overrideAutocomplete`, which when set to true allows the password manager to save passwords for pages where the form/form elements have `autocomplete=false`

(In reply to Manish Goregaokar [:manishearth] from comment #74) > I don't quite get what you mean here. > > > By followup do you mean that I should mention what the patch does? No, sorry, I was just using a bunch of jargon. :) I was requesting that as a next task, you file a new bug to expose this preference somehow (either in the preference pane or in some other way). It's best to do that in a separate bug since figuring out how to do that exactly shouldn't block this patch from landing. But I'm happy to file the bug if you would prefer. > > It adds an about:config option, `signon.overrideAutocomplete`, which when > set to true allows the password manager to save passwords for pages where > the form/form elements have `autocomplete=false`

Oh, I see, I can do that. I personally didn't think this was worth a full brown pref, but no harm filing a bug :)

Comment on attachment 8349839 [details] [diff] [review] Patch, cleaned up >diff --git a/toolkit/components/passwordmgr/LoginManagerContent.jsm b/toolkit/components/passwordmgr/LoginManagerContent.jsm > _isAutocompleteDisabled : function (element) { >+ if(element && element.hasAttribute("autocomplete") && >+ element.getAttribute("autocomplete").toLowerCase() == "off") Looks like the spacing is off here now. It should be: >+ if (element && element.hasAttribute("autocomplete") && >+ element.getAttribute("autocomplete").toLowerCase() == "off") No need to re-request review to address this comment, just upload the corrected patch and we can mark this checkin-needed.

Done.

Comment on attachment 8349851 [details] [diff] [review] Final patch r=Gavin In the future, just checkin-needed is needed.

I filed bug 956906 to investigate dropping support for autocomplete="off" entirely (or almost entirely).

Followup bug as per comment 53: bug 963913