Add a pref for bug 1217152 but leave autofill enabled for now.

Johann, have you made progress here?

Right, sorry, I've had this patch around for some time but didn't get to finishing the tests yet. I'll try to finish them soon. Matt, I'm not sure about the pref in my patch. Maybe we should put it under signon and not enable on pre-release. Let me know what you think.

Comment on attachment 8791874 [details] Bug 1302474 - Add a pref to disable login autofill on insecure forms. https://reviewboard.mozilla.org/r/79172/#review77864 ::: browser/app/profile/firefox.js:1226 (Diff revision 1) > > // Show degraded UI for http pages with password fields. > // Only for Nightly, Dev Edition and early beta, not for late beta or release. > #ifdef EARLY_BETA_OR_EARLIER > pref("security.insecure_password.ui.enabled", true); > +pref("security.insecure_password.autofill.disabled", true); Yeah, I think this should be "signon.autofillForms.http" and I'm fine with with setting it to false for a week or two for the sole purpose of gathering data on affected users but until we have autocomplete working well I don't think we should make pre-release users continue to suffer with it disabled. BTW. for the future use ".enabled" instead of ".disabled" as it's more common and avoids double negatives. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:44 (Diff revision 1) > +// Pref that disables form autofill on insecure pages > +var gInsecureAutofillDisabled; > + I'm transitioning away from these globals to having all pref obervers in LoginHelper. See my other comment. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:75 (Diff revision 1) > > onPrefChange() { > gEnabled = Services.prefs.getBoolPref("signon.rememberSignons"); > gAutofillForms = Services.prefs.getBoolPref("signon.autofillForms"); > gStoreWhenAutocompleteOff = Services.prefs.getBoolPref("signon.storeWhenAutocompleteOff"); > + gInsecureAutofillDisabled = Services.prefs.getBoolPref("security.insecure_password.autofill.disabled"); Please put this in LoginHelper instead. See https://dxr.mozilla.org/mozilla-central/rev/edfff7a9e4c43f6d516dfbf69a64e205e5cdb699/toolkit/components/passwordmgr/LoginHelper.jsm#37 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:987 (Diff revision 1) > > + if (gInsecureAutofillDisabled && !this.isDocumentSecure(form.ownerDocument)) { > + log("not filling form, the page is insecure"); This also doesn't take the form action into account (which is also true of the identity block warning: bug 1231914) I think ideally we should re-use the logic from InsecurePasswordsUtils (with some refactoring) instead of reimplementing it here. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:988 (Diff revision 1) > + if (gInsecureAutofillDisabled && !this.isDocumentSecure(form.ownerDocument)) { > + log("not filling form, the page is insecure"); This is incorrect as `_fillForm` is also used by fallback UI (autocomplete and the context menu) which should still be allowed to fill (since user interaction is involved). You need to take the `userTriggered` argument into account here (and please verify that it is sufficient for both autocomplete and the context menu).

Comment on attachment 8791874 [details] Bug 1302474 - Add a pref to disable login autofill on insecure forms. https://reviewboard.mozilla.org/r/79172/#review77892 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:987 (Diff revision 1) > > + if (gInsecureAutofillDisabled && !this.isDocumentSecure(form.ownerDocument)) { > + log("not filling form, the page is insecure"); Looks like Matt is right. The UI warning seems to come from _detectInsecureFormLikes https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#450 Using that is better than this.isDocumentSecure. And using InsecurePasswordUtils is even better than that. I'm okay either way; if you go with detectInsecureFormLkes, please file a followup bug to switch to InsecurePasswordUtils.

Ok, thanks! So we should probably factor out a common function in InsecurePasswordUtils that both detectInsecureFormLikes and _fillForm use.

Right

Comment on attachment 8791874 [details] Bug 1302474 - Add a pref to disable login autofill on insecure forms. https://reviewboard.mozilla.org/r/79172/#review78312 This is much closer now but the test needs changing and I think this will be less of a mess (e.g. the interdep. between LMC and IPU) after bug 1269568. ::: browser/app/profile/firefox.js:1521 (Diff revision 3) > // those reports. > #ifdef RELEASE_BUILD > pref("browser.crashReports.unsubmittedCheck.enabled", false); > #else > pref("browser.crashReports.unsubmittedCheck.enabled", true); > #endif Please revert the whitespace-only change to this file. ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:54 (Diff revision 3) > * and potentially stealing user data. Under such scenario, a user might not > * get a Mixed Content Blocker message, if the main document is served over HTTP > * and framing an HTTPS page as it would under the reverse scenario (http > * inside https). > */ > _checkForInsecureNestedDocuments(domDoc) { I know this isn't your fault but I think our password security checks are currently messy and therefore error-prone. This method would be much clearer if it was named `_hasInsecureAncestor` but really we should just use `Window.isSecureContext` instead as suggested by bug 1269568. I've cleaned this up now in bug 1269568 which will make these security functions simpler to understand. ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:102 (Diff revision 3) > + * @param {FormLike} aForm A form-like object. @See {FormLikeFactory} > + */ > + isFormSecure(aForm) { Nit: include `@return {boolean} …` please ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:988 (Diff revision 3) > } > > + // Prevent autofilling insecure forms. > + if (!userTriggered && !LoginHelper.insecureAutofill && > + !InsecurePasswordUtils.isFormSecure(form)) { > + log("not filling form, the page is insecure"); Nit: we're not just checking the "page". Maybe "not filling form since it's insecure". ::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js:1 (Diff revision 3) > +/* Any copyright is dedicated to the Public Domain. This shouldn't be a browser-chrome test as it's not specific to Desktop Firefox. ::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js:2 (Diff revision 3) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ Nit: Tests are PD by default so you don't need to add this ::: toolkit/components/passwordmgr/test/browser/form_insecure_action.html:1 (Diff revision 3) > +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body> Any reason you're not using form_cross_origin_insecure_action.html? ::: toolkit/components/passwordmgr/test/browser/form_secure_action.html:1 (Diff revision 3) > +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body> Likewise, any reason you're not using form_cross_origin_secure_action.html?

Matt, I updated the patch based on your suggestions, except for: > This shouldn't be a browser-chrome test as it's not specific to Desktop Firefox. I couldn't get the mochitest-plain to run in HTTPS, which is why I kept this test in browser for now. I'd appreciate suggestions on how to solve this. Also, if this bug really needs to land, I could make a follow-up bug for this issue and assign myself.

Marking as P1 because this needs to land in Firefox 52.

Comment on attachment 8791874 [details] Bug 1302474 - Add a pref to disable login autofill on insecure forms. https://reviewboard.mozilla.org/r/79172/#review86876 ::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js:1 (Diff revision 4) > +const TEST_URL_PATH = "://example.org/browser/toolkit/components/passwordmgr/test/browser/"; Please file a follow-up to convert this to mochittest-plain. I guess you would need to open HTTPS test pages in a new tab with window.open since the mochitest harness uses http.

Can you also update your commit message to make it clear that it's not just about the scheme of the form's page? Thanks

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/817eb00ba702 Add a pref to disable login autofill on insecure forms. r=MattN

verified as fixed on 52.0a2 20170122004006 on Ubuntu 16.04, Windows 10 and Mac OSX 10.10.