Bug 1314478 - Always find and mark the usernameField on insecure pages, r=MattN a=jcristau
authorTimothy Guan-tin Chien <timdream@gmail.com>
Sun, 13 Nov 2016 12:30:30 +0800
changeset 352898 40ddf243c06d044c732b0d46435edbe2be43ce7b
parent 352897 9dbc06789272d861f2a80be9513de1fe3f7047bf
child 352899 4ac86351a69e89de624f67cbee029e1673999df3
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN, jcristau
bugs1314478
milestone52.0a2
Bug 1314478 - Always find and mark the usernameField on insecure pages, r=MattN a=jcristau This patch allows us to locate a usernameField even if there isn't a found login, on insecure pages. Noted that we won't mark the field if the form has no password field. This also means we will be running our heuristics on insecure pages, even when there isn't any saved login found. MozReview-Commit-ID: Jt7jy7NFY3
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/test/mochitest/mochitest.ini
toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_no_saved_login.html
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -932,18 +932,21 @@ var LoginManagerContent = {
         // Ignore fills as a result of user action.
         return;
       }
       const autofillResultHist = Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT");
       autofillResultHist.add(result);
     }
 
     try {
-      // Nothing to do if we have no matching logins available.
-      if (foundLogins.length == 0) {
+      // Nothing to do if we have no matching logins available,
+      // and there isn't a need to show the insecure form warning.
+      if (foundLogins.length == 0 &&
+          (InsecurePasswordUtils.isFormSecure(form) ||
+          !LoginHelper.showInsecureFieldWarning)) {
         // We don't log() here since this is a very common case.
         recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
         return;
       }
 
       // Heuristically determine what the user/pass fields are
       // We do this before checking to see if logins are stored,
       // so that the user isn't prompted for a master password
@@ -978,16 +981,34 @@ var LoginManagerContent = {
 
       // If the password field is disabled or read-only, there's nothing to do.
       if (passwordField.disabled || passwordField.readOnly) {
         log("not filling form, password field disabled or read-only");
         recordAutofillResult(AUTOFILL_RESULT.PASSWORD_DISABLED_READONLY);
         return;
       }
 
+      // Attach autocomplete stuff to the username field, if we have
+      // one. This is normally used to select from multiple accounts,
+      // but even with one account we should refill if the user edits.
+      // We would also need this attached to show the insecure login
+      // warning, regardless of saved login.
+      if (usernameField) {
+        this._formFillService.markAsLoginManagerField(usernameField);
+      }
+
+      // Nothing to do if we have no matching logins available.
+      // Only insecure pages reach this block and logs the same
+      // telemetry flag.
+      if (foundLogins.length == 0) {
+        // We don't log() here since this is a very common case.
+        recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
+        return;
+      }
+
       // Prevent autofilling insecure forms.
       if (!userTriggered && !LoginHelper.insecureAutofill &&
           !InsecurePasswordUtils.isFormSecure(form)) {
         log("not filling form since it's insecure");
         recordAutofillResult(AUTOFILL_RESULT.INSECURE);
         return;
       }
 
@@ -1021,23 +1042,16 @@ var LoginManagerContent = {
       }, this);
 
       if (logins.length == 0) {
         log("form not filled, none of the logins fit in the field");
         recordAutofillResult(AUTOFILL_RESULT.NO_LOGINS_FIT);
         return;
       }
 
-      // Attach autocomplete stuff to the username field, if we have
-      // one. This is normally used to select from multiple accounts,
-      // but even with one account we should refill if the user edits.
-      if (usernameField) {
-        this._formFillService.markAsLoginManagerField(usernameField);
-      }
-
       // Don't clobber an existing password.
       if (passwordField.value && !clobberPassword) {
         log("form not filled, the password field was already filled");
         recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD);
         return;
       }
 
       // Select a login to use for filling in the form.
--- a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
+++ b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
@@ -27,16 +27,18 @@ skip-if = toolkit == 'android' # autocom
 [test_basic_form_2pw_2.html]
 [test_basic_form_3pw_1.html]
 [test_basic_form_autocomplete.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_insecure_form_field_autocomplete.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_password_field_autocomplete.html]
 skip-if = toolkit == 'android' # android:autocomplete.
+[test_insecure_form_field_no_saved_login.html]
+skip-if = toolkit == 'android' # android:autocomplete.
 [test_basic_form_html5.html]
 [test_basic_form_pwevent.html]
 [test_basic_form_pwonly.html]
 [test_bug_627616.html]
 skip-if = toolkit == 'android' # Tests desktop prompts
 [test_bug_776171.html]
 [test_case_differences.html]
 skip-if = toolkit == 'android' # autocomplete
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_no_saved_login.html
@@ -0,0 +1,103 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test basic login, contextual inscure password warning without saved logins</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>
+  <script type="text/javascript" src="satchel_common.js"></script>
+  <script type="text/javascript" src="pwmgr_common.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+Login Manager test: contextual inscure password warning without saved logins
+
+<script>
+let chromeScript = runChecksAfterCommonInit();
+</script>
+<p id="display"></p>
+
+<!-- we presumably can't hide the content for this test. -->
+<div id="content">
+
+  <form id="form1" action="http://autocomplete:8888/formtest.js" onsubmit="return false;">
+    <input  type="text"       name="uname">
+    <input  type="password"   name="pword">
+    <button type="submit">Submit</button>
+  </form>
+
+</div>
+
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+/** Test for Login Manager: contextual inscure password warning without saved logins. **/
+
+// Set to pref before the document loads.
+SpecialPowers.setBoolPref(
+  "security.insecure_field_warning.contextual.enabled", true);
+
+SimpleTest.registerCleanupFunction(() => {
+  SpecialPowers.clearUserPref(
+    "security.insecure_field_warning.contextual.enabled");
+});
+
+let uname = $_(1, "uname");
+let pword = $_(1, "pword");
+const shiftModifier = SpecialPowers.Ci.nsIDOMEvent.SHIFT_MASK;
+
+// Restore the form to the default state.
+function restoreForm() {
+  uname.value = "";
+  pword.value = "";
+  uname.focus();
+}
+
+// Check for expected username/password in form.
+function checkACForm(expectedUsername, expectedPassword) {
+  let formID = uname.parentNode.id;
+  is(uname.value, expectedUsername, "Checking " + formID + " username is: " + expectedUsername);
+  is(pword.value, expectedPassword, "Checking " + formID + " password is: " + expectedPassword);
+}
+
+function spinEventLoop() {
+  return Promise.resolve();
+}
+
+add_task(function* setup() {
+  listenForUnexpectedPopupShown();
+});
+
+add_task(function* test_form1_initial_empty() {
+  yield SimpleTest.promiseFocus(window);
+
+  // Make sure initial form is empty.
+  checkACForm("", "");
+  let popupState = yield getPopupState();
+  is(popupState.open, false, "Check popup is initially closed");
+});
+
+add_task(function* test_form1_warning_entry() {
+  yield SimpleTest.promiseFocus(window);
+  // Trigger autocomplete popup
+  restoreForm();
+  let shownPromise = promiseACShown();
+  doKey("down"); // open
+  yield shownPromise;
+
+  let popupState = yield getPopupState();
+  is(popupState.open, true, "Check popup is opened");
+  is(popupState.selectedIndex, -1, "Check no entries are selected upon opening");
+
+  doKey("down"); // select insecure warning
+  checkACForm("", ""); // value shouldn't update just by selecting
+  doKey("return"); // not "enter"!
+  yield spinEventLoop(); // let focus happen
+  checkACForm("", "");
+});
+
+</script>
+</pre>
+</body>
+</html>