Bug 1543449 - use a minPasswordLength rather than skipEmptyFields property when collecting password fields. r=jaws
authorSam Foster <sfoster@mozilla.com>
Mon, 15 Apr 2019 16:34:15 +0000
changeset 469538 127dba490969122ab2c7523bc98db6f739d95f0a
parent 469537 1837a4550e101c93c6dcce65ef09c5a7b6f72bb2
child 469539 8ea3101cfbad67458ee23e560ce4c873e129d307
push id112801
push userccoroiu@mozilla.com
push dateMon, 15 Apr 2019 21:40:09 +0000
treeherdermozilla-inbound@afb20612c0e5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1543449
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1543449 - use a minPasswordLength rather than skipEmptyFields property when collecting password fields. r=jaws Differential Revision: https://phabricator.services.mozilla.com/D27202
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/test/browser/browser_notifications_2.js
toolkit/components/passwordmgr/test/mochitest/mochitest.ini
toolkit/components/passwordmgr/test/mochitest/test_password_length.html
toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -784,17 +784,17 @@ var LoginManagerContent = {
    *                                                 useful.
    * @param {Object} [options.fieldOverrideRecipe=null] - A relevant field override recipe to use.
    * @return {Array|null} Array of password field elements for the specified form.
    *                      If no pw fields are found, or if more than 3 are found, then null
    *                      is returned.
    */
   _getPasswordFields(form, {
     fieldOverrideRecipe = null,
-    skipEmptyFields = false,
+    minPasswordLength = 0,
   } = {}) {
     // Locate the password fields in the form.
     let pwFields = [];
     for (let i = 0; i < form.elements.length; i++) {
       let element = form.elements[i];
       if (ChromeUtils.getClassName(element) !== "HTMLInputElement" ||
           element.type != "password" ||
           !element.isConnected) {
@@ -804,18 +804,23 @@ var LoginManagerContent = {
       // Exclude ones matching a `notPasswordSelector`, if specified.
       if (fieldOverrideRecipe && fieldOverrideRecipe.notPasswordSelector &&
           element.matches(fieldOverrideRecipe.notPasswordSelector)) {
         log("skipping password field (id/name is", element.id, " / ",
             element.name + ") due to recipe:", fieldOverrideRecipe);
         continue;
       }
 
-      if (skipEmptyFields && !element.value.trim()) {
-        continue;
+      // XXX: Bug 780449 tracks our handling of emoji and multi-code-point characters in
+      // password fields. To avoid surprises, we should be consistent with the visual
+      // representation of the masked password
+      if (minPasswordLength && element.value.trim().length < minPasswordLength) {
+        log("skipping password field (id/name is", element.id, " / ",
+            element.name + ") as value is too short:", element.value.trim().length);
+        continue; // Ignore empty or too-short passwords fields
       }
 
       pwFields[pwFields.length] = {
         index: i,
         element,
       };
     }
 
@@ -877,19 +882,20 @@ var LoginManagerContent = {
       if (usernameOverrideField) {
         usernameField = usernameOverrideField;
       }
     }
 
     if (!pwFields) {
       // Locate the password field(s) in the form. Up to 3 supported.
       // If there's no password field, there's nothing for us to do.
+      const minSubmitPasswordLength = 2;
       pwFields = this._getPasswordFields(form, {
         fieldOverrideRecipe,
-        skipEmptyFields: isSubmission,
+        minPasswordLength: isSubmission ? minSubmitPasswordLength : 0,
       });
     }
 
     if (!pwFields) {
       return [null, null, null];
     }
 
     if (!usernameField) {
--- a/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js
+++ b/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js
@@ -19,28 +19,29 @@ add_task(async function test_empty_passw
     // case. This will cause the doorhanger notification to be displayed.
     let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
                                                      "popupshown",
                                                      (event) => event.target == PopupNotifications.panel);
     await ContentTask.spawn(browser, null,
                             async function() {
                               let doc = content.document;
                               doc.getElementById("form-basic-username").value = "username";
-                              doc.getElementById("form-basic-password").value = "p";
+                              doc.getElementById("form-basic-password").value = "pw";
                               doc.getElementById("form-basic").submit();
                             });
     await promiseShown;
 
     let notificationElement = PopupNotifications.panel.childNodes[0];
     let passwordTextbox = notificationElement.querySelector("#password-notification-password");
 
     // Synthesize input to empty the field
     passwordTextbox.focus();
     await EventUtils.synthesizeKey("KEY_ArrowRight");
     await EventUtils.synthesizeKey("KEY_Backspace");
+    await EventUtils.synthesizeKey("KEY_Backspace");
 
     let mainActionButton = notificationElement.button;
     Assert.ok(mainActionButton.disabled, "Main action button is disabled");
   });
 });
 
 /**
  * Test that the doorhanger password field shows plain or * text
@@ -55,17 +56,17 @@ add_task(async function test_toggle_pass
     // case. This will cause the doorhanger notification to be displayed.
     let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
                                                      "popupshown",
                                                      (event) => event.target == PopupNotifications.panel);
     await ContentTask.spawn(browser, null,
                             async function() {
                               let doc = content.document;
                               doc.getElementById("form-basic-username").value = "username";
-                              doc.getElementById("form-basic-password").value = "p";
+                              doc.getElementById("form-basic-password").value = "pw";
                               doc.getElementById("form-basic").submit();
                             });
     await promiseShown;
 
     let notificationElement = PopupNotifications.panel.childNodes[0];
     let passwordTextbox = notificationElement.querySelector("#password-notification-password");
     let toggleCheckbox = notificationElement.querySelector("#password-notification-visibilityToggle");
 
@@ -94,17 +95,17 @@ add_task(async function test_checkbox_di
                                                      "popupshown",
                                                      (event) => event.target == PopupNotifications.panel);
 
     LoginTestUtils.masterPassword.enable();
 
     await ContentTask.spawn(browser, null, async function() {
       let doc = content.document;
       doc.getElementById("form-basic-username").value = "username";
-      doc.getElementById("form-basic-password").value = "p";
+      doc.getElementById("form-basic-password").value = "pass";
       doc.getElementById("form-basic").submit();
     });
     await promiseShown;
 
     let notificationElement = PopupNotifications.panel.childNodes[0];
     let passwordTextbox = notificationElement.querySelector("#password-notification-password");
     let toggleCheckbox = notificationElement.querySelector("#password-notification-visibilityToggle");
 
--- a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
+++ b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
@@ -120,16 +120,19 @@ skip-if = toolkit == 'android' # bug 153
 skip-if = os == "linux" || toolkit == 'android' # Tests desktop prompts
 [test_prompt_async.html]
 skip-if = toolkit == 'android' # Tests desktop prompts
 support-files = subtst_prompt_async.html
 [test_prompt_http.html]
 skip-if = os == "linux" || toolkit == 'android' # Tests desktop prompts
 [test_prompt_noWindow.html]
 skip-if = e10s || toolkit == 'android' # Tests desktop prompts. e10s: bug 1217876
+[test_password_length.html]
+scheme = https
+skip-if = toolkit == 'android' # bug 1527403
 [test_prompt_promptAuth.html]
 skip-if = os == "linux" || toolkit == 'android' # Tests desktop prompts
 [test_prompt_promptAuth_proxy.html]
 skip-if = e10s || os == "linux" || toolkit == 'android' # Tests desktop prompts
 [test_recipe_login_fields.html]
 [test_username_focus.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_xhr.html]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/mochitest/test_password_length.html
@@ -0,0 +1,142 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test handling of different password length</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script src="/tests/SimpleTest/AddTask.js"></script>
+  <script src="pwmgr_common.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<script type="application/javascript">
+const LMCBackstagePass = SpecialPowers.Cu.import("resource://gre/modules/LoginManagerContent.jsm");
+const { LoginManagerContent } = LMCBackstagePass;
+
+let readyPromise = registerRunTests();
+
+let loadPromise = new Promise(resolve => {
+  document.addEventListener("DOMContentLoaded", () => {
+    document.getElementById("loginFrame").addEventListener("load", (evt) => {
+      resolve();
+    });
+  });
+});
+
+async function loadFormIntoIframe(origin, html) {
+  let loginFrame = document.getElementById("loginFrame");
+  let loadedPromise = new Promise((resolve) => {
+    loginFrame.addEventListener("load", function() {
+      resolve();
+    }, {once: true});
+  });
+  loginFrame.src = origin + "/tests/toolkit/components/passwordmgr/test/mochitest/blank.html";
+  await loadedPromise;
+  let frameDoc = SpecialPowers.wrap(loginFrame.contentWindow).document;
+  // eslint-disable-next-line no-unsanitized/property
+  frameDoc.documentElement.innerHTML = html;
+  // Wait for the form to be processed before trying to submit.
+  await promiseFormsProcessed();
+  return frameDoc;
+}
+
+add_task(async function setup() {
+  info("Waiting for setup and page and frame loads");
+  await readyPromise;
+  await loadPromise;
+});
+
+const DEFAULT_ORIGIN = "https://example.com";
+const TESTCASES = [
+  {
+    testName: "test_control2PasswordFields",
+    pword1: "pass1",
+    pword2: "pass2",
+    expectedNewPassword: { value: "pass2" },
+    expectedOldPassword: { value: "pass1" },
+  },
+  {
+    testName: "test_1characterPassword",
+    pword1: "x",
+    pword2: "pass2",
+    expectedNewPassword: { value: "pass2" },
+    expectedOldPassword: null,
+  },
+  {
+    testName: "test_2characterPassword",
+    pword1: "xy",
+    pword2: "pass2",
+    expectedNewPassword: { value: "pass2" },
+    expectedOldPassword: { value: "xy" },
+  },
+  {
+    testName: "test_1characterNewPassword",
+    pword1: "pass1",
+    pword2: "x",
+    expectedNewPassword: { value: "pass1" },
+    expectedOldPassword: null,
+  },
+];
+
+/**
+ * @return {Promise} resolving when form submission was processed.
+ */
+function getSubmitMessage() {
+  return new Promise((resolve, reject) => {
+    PWMGR_COMMON_PARENT.addMessageListener("formSubmissionProcessed", function processed(...args) {
+      info("got formSubmissionProcessed");
+      PWMGR_COMMON_PARENT.removeMessageListener("formSubmissionProcessed", processed);
+      resolve(...args);
+    });
+  });
+}
+
+add_task(async function test_password_lengths() {
+  for (let tc of TESTCASES) {
+    info("Starting testcase: " + tc.testName + ", " + JSON.stringify([tc.pword1, tc.pword2]));
+    let frameDoc = await loadFormIntoIframe(DEFAULT_ORIGIN, `<form id="form1" onsubmit="return false;">
+      <input  type="text"       name="uname" value="myname">
+      <input  type="password"   name="pword1" value="">
+      <input  type="password"   name="pword2" value="">
+      <button type="submit" id="submitBtn">Submit</button>
+    </form>`);
+
+    is(frameDoc.querySelector("[name='uname']").value, "myname", "Checking for filled username");
+    frameDoc.querySelector("[name='pword1']").setUserInput(tc.pword1);
+    frameDoc.querySelector("[name='pword2']").setUserInput(tc.pword2);
+
+    // Check data sent via PasswordManager:onFormSubmit
+    let processedPromise = getSubmitMessage();
+    frameDoc.getElementById("submitBtn").click();
+
+    let submittedResult = await processedPromise;
+    info("Got submittedResult: " + JSON.stringify(submittedResult));
+
+    if (tc.expectedNewPassword === null) {
+      is(submittedResult.newPasswordField,
+         tc.expectedNewPassword, "Check expectedNewPassword is null");
+    } else {
+      is(submittedResult.newPasswordField.value,
+         tc.expectedNewPassword.value,
+         "Check that newPasswordField.value matches the expectedNewPassword.value");
+    }
+    if (tc.expectedOldPassword === null) {
+      is(submittedResult.oldPasswordField,
+         tc.expectedOldPassword, "Check expectedOldPassword is null");
+    } else {
+      is(submittedResult.oldPasswordField.value,
+         tc.expectedOldPassword.value,
+         "Check that oldPasswordField.value matches expectedOldPassword.value");
+    }
+  }
+});
+</script>
+
+<p id="display"></p>
+
+<div id="content">
+  <iframe id="loginFrame" src="https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/blank.html"></iframe>
+</div>
+<pre id="test"></pre>
+</body>
+</html>
--- a/toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
+++ b/toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
@@ -1,129 +1,149 @@
 /*
  * Test for LoginManagerContent._getPasswordFields using LoginFormFactory.
  */
-
+/* globals todo_check_eq */
 "use strict";
 
 const {LoginFormFactory} = ChromeUtils.import("resource://gre/modules/LoginFormFactory.jsm");
 const LMCBackstagePass = ChromeUtils.import("resource://gre/modules/LoginManagerContent.jsm", null);
 const { LoginManagerContent } = LMCBackstagePass;
 const TESTCASES = [
   {
     description: "Empty document",
     document: ``,
     returnedFieldIDsByFormLike: [],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
     description: "Non-password input with no <form> present",
     document: `<input>`,
     // Only the IDs of password fields should be in this array
     returnedFieldIDsByFormLike: [[]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
     description: "1 password field outside of a <form>",
     document: `<input id="pw1" type=password>`,
     returnedFieldIDsByFormLike: [["pw1"]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
     description: "4 empty password fields outside of a <form>",
     document: `<input id="pw1" type=password>
       <input id="pw2" type=password>
       <input id="pw3" type=password>
       <input id="pw4" type=password>`,
     returnedFieldIDsByFormLike: [[]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
-    description: "4 password fields outside of a <form> (1 empty, 3 full) with skipEmpty",
+    description: "4 password fields outside of a <form> (1 empty, 3 full) with minPasswordLength=2",
     document: `<input id="pw1" type=password>
       <input id="pw2" type=password value="pass2">
       <input id="pw3" type=password value="pass3">
       <input id="pw4" type=password value="pass4">`,
     returnedFieldIDsByFormLike: [["pw2", "pw3", "pw4"]],
-    skipEmptyFields: true,
+    minPasswordLength: 2,
   },
   {
     description: "Form with 1 password field",
     document: `<form><input id="pw1" type=password></form>`,
     returnedFieldIDsByFormLike: [["pw1"]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
     description: "Form with 2 password fields",
     document: `<form><input id="pw1" type=password><input id='pw2' type=password></form>`,
     returnedFieldIDsByFormLike: [["pw1", "pw2"]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
     description: "1 password field in a form, 1 outside",
     document: `<form><input id="pw1" type=password></form><input id="pw2" type=password>`,
     returnedFieldIDsByFormLike: [["pw1"], ["pw2"]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
     description: "2 password fields outside of a <form> with 1 linked via @form",
     document: `<input id="pw1" type=password><input id="pw2" type=password form='form1'>
       <form id="form1"></form>`,
     returnedFieldIDsByFormLike: [["pw1"], ["pw2"]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
   {
-    description: "2 password fields outside of a <form> with 1 linked via @form + skipEmpty",
+    description: "2 password fields outside of a <form> with 1 linked via @form + minPasswordLength",
     document: `<input id="pw1" type=password><input id="pw2" type=password form="form1">
       <form id="form1"></form>`,
     returnedFieldIDsByFormLike: [[], []],
-    skipEmptyFields: true,
+    minPasswordLength: 2,
   },
   {
-    description: "skipEmptyFields should also skip white-space only fields",
+    description: "minPasswordLength should also skip white-space only fields",
     /* eslint-disable no-tabs */
     document: `<input id="pw-space" type=password value=" ">
                <input id="pw-tab" type=password value="	">
                <input id="pw-newline" type=password form="form1" value="
-">
+                                                                        ">
       <form id="form1"></form>`,
-    /* eslint-disable no-tabs */
+    /* eslint-enable no-tabs */
     returnedFieldIDsByFormLike: [[], []],
-    skipEmptyFields: true,
+    minPasswordLength: 2,
   },
   {
-    description: "2 password fields outside of a <form> with 1 linked via @form + skipEmpty with 1 empty",
+    description: "minPasswordLength should skip too-short field values",
+    document: `<form>
+               <input id="pw-empty" type=password>
+               <input id="pw-tooshort" type=password value="p">
+               <input id="pw" type=password value="pz">
+               </form>`,
+    returnedFieldIDsByFormLike: [["pw"]],
+    minPasswordLength: 2,
+  },
+  {
+    description: "minPasswordLength should allow matching-length field values",
+    document: `<form>
+               <input id="pw-empty" type=password>
+               <input id="pw-matchlen" type=password value="pz">
+               <input id="pw" type=password value="pazz">
+               </form>`,
+    returnedFieldIDsByFormLike: [["pw-matchlen", "pw"]],
+    minPasswordLength: 2,
+  },
+  {
+    description: "2 password fields outside of a <form> with 1 linked via @form + minPasswordLength with 1 empty",
     document: `<input id="pw1" type=password value=" pass1 "><input id="pw2" type=password form="form1">
       <form id="form1"></form>`,
     returnedFieldIDsByFormLike: [["pw1"], []],
-    skipEmptyFields: true,
+    minPasswordLength: 2,
     fieldOverrideRecipe: {
       // Ensure a recipe without `notPasswordSelector` doesn't cause a problem.
       hosts: ["localhost:8080"],
     },
   },
   {
-    description: "3 password fields outside of a <form> with 1 linked via @form + skipEmpty",
+    description: "3 password fields outside of a <form> with 1 linked via @form + minPasswordLength",
     document: `<input id="pw1" type=password value="pass1"><input id="pw2" type=password form="form1" value="pass2"><input id="pw3" type=password value="pass3">
       <form id="form1"><input id="pw4" type=password></form>`,
     returnedFieldIDsByFormLike: [["pw3"], ["pw2"]],
-    skipEmptyFields: true,
+    minPasswordLength: 2,
     fieldOverrideRecipe: {
       hosts: ["localhost:8080"],
       notPasswordSelector: "#pw1",
     },
   },
   {
     beforeGetFunction(doc) {
       doc.getElementById("pw1").remove();
     },
     description: "1 password field outside of a <form> which gets removed/disconnected",
     document: `<input id="pw1" type=password>`,
     returnedFieldIDsByFormLike: [[]],
-    skipEmptyFields: undefined,
+    minPasswordLength: undefined,
   },
 ];
 
 for (let tc of TESTCASES) {
   info("Sanity checking the testcase: " + tc.description);
 
   (function() {
     let testcase = tc;
@@ -153,17 +173,17 @@ for (let tc of TESTCASES) {
       Assert.strictEqual(mapRootElementToFormLike.size, testcase.returnedFieldIDsByFormLike.length,
                          "Check the correct number of different formLikes were returned");
 
       let formLikeIndex = -1;
       for (let formLikeFromInput of mapRootElementToFormLike.values()) {
         formLikeIndex++;
         let pwFields = LoginManagerContent._getPasswordFields(formLikeFromInput, {
           fieldOverrideRecipe: testcase.fieldOverrideRecipe,
-          skipEmptyFields: testcase.skipEmptyFields,
+          minPasswordLength: testcase.minPasswordLength,
         });
 
         if (ChromeUtils.getClassName(formLikeFromInput.rootElement) === "HTMLFormElement") {
           let formLikeFromForm = LoginFormFactory.createFromForm(formLikeFromInput.rootElement);
           info("Checking that the FormLike created for the <form> matches" +
                " the one from a password field");
           formLikeEqual(formLikeFromInput, formLikeFromForm);
         }
@@ -182,8 +202,52 @@ for (let tc of TESTCASES) {
           let expectedID = testcase.returnedFieldIDsByFormLike[formLikeIndex][i];
           Assert.strictEqual(pwFields[i].element.id, expectedID,
                              "Check password field " + i + " ID");
         }
       }
     });
   })();
 }
+
+const EMOJI_TESTCASES = [
+  {
+    description: "Single characters composed of 2 code units should ideally fail minPasswordLength of 2",
+    document: `<form>
+               <input id="pw" type=password value="💩">
+               </form>`,
+    returnedFieldIDsByFormLike: [["pw"]],
+    minPasswordLength: 2,
+  },
+  {
+    description: "Single characters composed of multiple code units should ideally fail minPasswordLength of 2",
+    document: `<form>
+               <input id="pw" type=password value="👪">
+               </form>`,
+    minPasswordLength: 2,
+  },
+];
+
+// Note: Bug 780449 tracks our handling of emoji and multi-code-point characters in password fields
+// and the .length we should expect when a password value includes them
+for (let tc of EMOJI_TESTCASES) {
+  info("Sanity checking the testcase: " + tc.description);
+
+  (function() {
+    let testcase = tc;
+    add_task(async function() {
+      info("Starting testcase: " + testcase.description);
+      let document = MockDocument.createTestDocument("http://localhost:8080/test/",
+                                                     testcase.document);
+      let input = document.querySelector("input[type='password']");
+      Assert.ok(input, "Found the password field");
+      let formLike = LoginFormFactory.createFromField(input);
+      let pwFields = LoginManagerContent._getPasswordFields(formLike, {
+        minPasswordLength: testcase.minPasswordLength,
+      });
+      info("Got password fields: " + pwFields.length);
+      todo_check_eq(
+        pwFields.length, 0,
+        "Check a single-character (emoji) password is excluded from the password fields collection"
+      );
+    });
+  })();
+}