Bug 1314478 - Always find and mark the usernameField on insecure pages, r=MattN
authorTimothy Guan-tin Chien <timdream@gmail.com>
Sun, 13 Nov 2016 12:30:30 +0800
changeset 322534 be95aa5249cf8d1cf33434273f7c789e332b886d
parent 322533 ef98320218dcaacf693b9b96d6f347c1432352c5
child 322535 6175d2094ebf7fab0c269471cbb2fe1838ff5423
push id21
push usermaklebus@msu.edu
push dateThu, 01 Dec 2016 06:22:08 +0000
reviewersMattN
bugs1314478
milestone52.0a1
Bug 1314478 - Always find and mark the usernameField on insecure pages, r=MattN 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
@@ -24,16 +24,18 @@ skip-if = toolkit == 'android' # Bug 125
 [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>