Bug 1543454 - Set single-character usernames to null when prompting to save password. r=MattN
authorSam Foster <sfoster@mozilla.com>
Mon, 22 Apr 2019 21:59:57 +0000
changeset 470504 0929ce86289280f106b3cd63d63290de507466a7
parent 470503 4f8806f0c9f0db51948b14556c24658de1753a9a
child 470505 6356a349f12f74871bcb3bc6bda8d1e0a43cba21
push id35906
push useraciure@mozilla.com
push dateTue, 23 Apr 2019 22:14:56 +0000
treeherdermozilla-central@0ce3633f8b80 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1543454
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 1543454 - Set single-character usernames to null when prompting to save password. r=MattN Differential Revision: https://phabricator.services.mozilla.com/D28003
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/test/mochitest/mochitest.ini
toolkit/components/passwordmgr/test/mochitest/pwmgr_common.js
toolkit/components/passwordmgr/test/mochitest/test_tooshort_username.html
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -11,16 +11,18 @@
 
 "use strict";
 
 var EXPORTED_SYMBOLS = ["LoginManagerContent"];
 
 const PASSWORD_INPUT_ADDED_COALESCING_THRESHOLD_MS = 1;
 const AUTOCOMPLETE_AFTER_RIGHT_CLICK_THRESHOLD_MS = 400;
 const AUTOFILL_STATE = "-moz-autofill";
+const MIN_SUBMIT_USERNAME_LENGTH = 2;
+const MIN_SUBMIT_PASSWORD_LENGTH = 2;
 
 const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const {PrivateBrowsingUtils} = ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
 const {PromiseUtils} = ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm");
 const {CreditCard} = ChromeUtils.import("resource://gre/modules/CreditCard.jsm");
 
 ChromeUtils.defineModuleGetter(this, "DeferredTask", "resource://gre/modules/DeferredTask.jsm");
@@ -897,20 +899,19 @@ 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,
-        minPasswordLength: isSubmission ? minSubmitPasswordLength : 0,
+        minPasswordLength: isSubmission ? MIN_SUBMIT_PASSWORD_LENGTH : 0,
       });
     }
 
     if (!pwFields) {
       return [null, null, null];
     }
 
     if (!usernameField) {
@@ -1105,16 +1106,21 @@ var LoginManagerContent = {
       return;
     }
 
     if (usernameField && usernameField.value.match(/[•\*]{3,}/)) {
       log(`usernameField.value "${usernameField.value}" looks munged, setting to null`);
       usernameField = null;
     }
 
+    if (usernameField && usernameField.value.length < MIN_SUBMIT_USERNAME_LENGTH) {
+      log(`usernameField.value "${usernameField.value}" too short, setting to null`);
+      usernameField = null;
+    }
+
     // Check for autocomplete=off attribute. We don't use it to prevent
     // autofilling (for existing logins), but won't save logins when it's
     // present and the storeWhenAutocompleteOff pref is false.
     // XXX spin out a bug that we don't update timeLastUsed in this case?
     if ((this._isAutocompleteDisabled(form) ||
          this._isAutocompleteDisabled(usernameField) ||
          this._isAutocompleteDisabled(newPasswordField) ||
          this._isAutocompleteDisabled(oldPasswordField)) &&
--- a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
+++ b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
@@ -131,16 +131,19 @@ skip-if = e10s || toolkit == 'android' #
 [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_tooshort_username.html]
+scheme = https
+skip-if = toolkit == 'android' # bug 1527403
 [test_username_focus.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_xhr.html]
 skip-if = toolkit == 'android' # Tests desktop prompts
 [test_xhr_2.html]
 [test_xml_load.html]
 skip-if = toolkit == 'android' # Tests desktop prompts
 
--- a/toolkit/components/passwordmgr/test/mochitest/pwmgr_common.js
+++ b/toolkit/components/passwordmgr/test/mochitest/pwmgr_common.js
@@ -268,16 +268,51 @@ function promisePromptShown(expectedTopi
       is(topic, expectedTopic, "Check expected prompt topic");
       PWMGR_COMMON_PARENT.removeMessageListener("promptShown", onPromptShown);
       resolve();
     }
     PWMGR_COMMON_PARENT.addMessageListener("promptShown", onPromptShown);
   });
 }
 
+async function promiseNoUnexpectedPromptShown() {
+  // listen for either -change or -save notifications
+  // and fail if we get either within a second
+  let timer;
+  let promptObserved;
+  let task = {};
+
+  function onPromptShown({ topic, data }) {
+    info("onPromptShown got topic: " + topic);
+    promptObserved = true;
+    task.reject(new Error("Unexpected prompt shown with topic: " + topic));
+  }
+
+  let promptShownPromise = new Promise((resolve, reject) => {
+    task.resolve = resolve;
+    task.reject = reject;
+    PWMGR_COMMON_PARENT.addMessageListener("promptShown", onPromptShown);
+  }).finally(() => {
+    clearTimeout(timer);
+    PWMGR_COMMON_PARENT.removeMessageListener("promptShown", onPromptShown);
+  });
+
+  let timeoutPromise = new Promise((resolve, reject) => {
+    SimpleTest.requestFlakyTimeout("Giving a chance for an unexpected passwordmgr-prompt-* to occur");
+    timer = setTimeout(resolve, 1000);
+  });
+
+  try {
+    await Promise.race([promptShownPromise, timeoutPromise]);
+    ok(!promptObserved, "Check no prompts observed before timer expired");
+  } catch (ex) {
+    ok(false, "Prompts observed before timer expired: " + ex.message);
+  }
+}
+
 /**
  * Run a function synchronously in the parent process and destroy it in the test cleanup function.
  * @param {Function|String} aFunctionOrURL - either a function that will be stringified and run
  *                                           or the URL to a JS file.
  * @return {Object} - the return value of loadChromeScript providing message-related methods.
  *                    @see loadChromeScript in specialpowersAPI.js
  */
 function runInParent(aFunctionOrURL) {
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/mochitest/test_tooshort_username.html
@@ -0,0 +1,217 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test handling of too-short username values</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script src="/tests/SimpleTest/AddTask.js"></script>
+  <script type="text/javascript" src="../../../satchel/test/satchel_common.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();
+
+function add2logins() {
+  runInParent(function initLogins() {
+    const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+    Services.logins.removeAllLogins();
+
+    let login1 = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+    login1.init("https://example.com", "https://example.com", null, "u", "pass1", "", "");
+
+    let login2 = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+    login2.init("https://example.com", "https://example.com", null, "user2", "pass2", "", "");
+
+    Services.logins.addLogin(login1);
+    Services.logins.addLogin(login2);
+  });
+}
+
+function addSingleLogin() {
+  runInParent(function initWithSingleLogin() {
+    const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+    Services.logins.removeAllLogins();
+
+    let login1 = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+    login1.init("https://example.com", "https://example.com", null, "u", "pass1", "", "");
+    Services.logins.addLogin(login1);
+  });
+}
+
+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";
+
+/**
+ * @return {Promise} resolving when form submission was processed.
+ */
+function getSubmitMessage() {
+  info("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_min_length() {
+// test that when a username <= minLength is submitted, we prompt w. an empty username field
+  let frameDoc = await loadFormIntoIframe(DEFAULT_ORIGIN, `<form id="form1" onsubmit="return false;">
+    <input  type="text"       name="uname" value="">
+    <input  type="password"   name="pword" value="thepassword">
+    <button type="submit" id="submitBtn">Submit</button>
+  </form>`);
+  is(frameDoc.querySelector("[name='uname']").value, "", "Checking username is initially empty");
+
+  frameDoc.querySelector("[name='uname']").setUserInput("u");
+  // Check data sent via PasswordManager:onFormSubmit
+  let processedPromise = getSubmitMessage();
+  let promisedPromptShown = promisePromptShown("passwordmgr-prompt-save");
+  frameDoc.getElementById("submitBtn").click();
+
+  let [submittedResult] = await Promise.all([processedPromise, promisedPromptShown]);
+  info("Got submittedResult: " + JSON.stringify(submittedResult));
+
+  is(submittedResult.usernameField, null, "Check usernameField was null");
+  is(submittedResult.newPasswordField.value, "thepassword",
+     "Check newPasswordField.value was expected password");
+});
+
+add_task(async function test_no_prompt_for_manual_match() {
+  // test that we do not prompt for a change if the value was manually given (not autofilled)
+  // run this test with 2 matching logins from this origin so we don't autofill
+  await add2logins();
+
+  let allLogins = await LoginManager.getAllLogins();
+  let matchingLogins = allLogins.filter(l => l.hostname == DEFAULT_ORIGIN);
+  is(matchingLogins.length, 2, "Expected number of matching logins");
+
+  let login = matchingLogins.find(l => l.username == "u");
+  ok(login, "Found the 'u' login");
+  let timesUsed = login.timesUsed;
+  let guid = login.guid;
+
+  let frameDoc = await loadFormIntoIframe(DEFAULT_ORIGIN, `<form id="form1" onsubmit="return false;">
+    <input  type="text"       name="uname" value="">
+    <input  type="password"   name="pword" value="">
+    <button type="submit" id="submitBtn">Submit</button>
+  </form>`);
+  is(frameDoc.querySelector("[name='uname']").value, "", "Check username didn't get autofilled");
+  // we manually fill username/password as the same as the saved values
+  frameDoc.querySelector("[name='uname']").setUserInput("u");
+  frameDoc.querySelector("[name='pword']").setUserInput("pass1");
+
+  // we shouldn't get the save password doorhanger as nothing changed...
+  let promisedPromptShown = promiseNoUnexpectedPromptShown();
+
+  // Check data sent via PasswordManager:onFormSubmit
+  let processedPromise = getSubmitMessage();
+  frameDoc.getElementById("submitBtn").click();
+
+  let submittedResult = await processedPromise;
+  info("Got submittedResult: " + JSON.stringify(submittedResult));
+
+  is(submittedResult.usernameField, null, "Check usernameField");
+
+  let updatedLogins = await LoginManager.getAllLogins();
+  is(allLogins.length, updatedLogins.length, "The number of saved logins didnt change");
+  let updatedLogin = updatedLogins.find(l => l.guid == guid);
+  ok(updatedLogin, "Got the login via guid");
+  is(updatedLogin.timesUsed, timesUsed + 1, "timesUsed was incremented");
+
+  await promisedPromptShown;
+});
+
+
+add_task(async function test_autofill_munged_username_matching_password() {
+  // test that when an auto-filled saved username <= minLength is used, we record its use
+
+  // only a single matching login so we autofill the username
+  await addSingleLogin();
+
+  let allLogins = await LoginManager.getAllLogins();
+  let matchingLogins = allLogins.filter(l => l.hostname == DEFAULT_ORIGIN);
+  is(matchingLogins.length, 1, "Expected number of matching logins");
+
+  info("matched login: " + matchingLogins[0].username);
+  let login = matchingLogins.find(l => l.username == "u");
+  ok(login, "Found the 'u' login");
+
+  let timesUsed = login.timesUsed;
+  let guid = login.guid;
+
+  let frameDoc = await loadFormIntoIframe(DEFAULT_ORIGIN, `<form id="form1" onsubmit="return false;">
+    <input  type="text"       name="uname" value="">
+    <input  type="password"   name="pword" value="">
+    <button type="submit" id="submitBtn">Submit</button>
+  </form>`);
+  is(frameDoc.querySelector("[name='uname']").value, "u", "Check username did get autofilled");
+  frameDoc.querySelector("[name='pword']").setUserInput("pass1");
+
+  // we shouldn't get the save/update password doorhanger as it didn't change
+  let promisedPromptShown = promiseNoUnexpectedPromptShown();
+
+  // Check data sent via PasswordManager:onFormSubmit
+  let processedPromise = getSubmitMessage();
+  frameDoc.getElementById("submitBtn").click();
+
+  let submittedResult = await processedPromise;
+  info("Got submittedResult: " + JSON.stringify(submittedResult));
+
+  is(submittedResult.usernameField, null, "Check usernameField");
+
+  let updatedLogins = await LoginManager.getAllLogins();
+  is(allLogins.length, updatedLogins.length, "The number of saved logins didnt change");
+  let updatedLogin = updatedLogins.find(l => l.guid == guid);
+  ok(updatedLogin, "Got the login via guid");
+  is(updatedLogin.timesUsed, timesUsed + 1, "timesUsed was incremented");
+
+  await promisedPromptShown;
+});
+
+</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>