Bug 1147563 - Provide autocomplete experience when formSubmitURL does not match. r=MattN
☠☠ backed out by 5721f39a987d ☠ ☠
authorJared Wein <jwein@mozilla.com>
Sat, 23 Mar 2019 00:07:34 +0000
changeset 465802 b2f2db06d09441c51e9a921d32c8a49b9bdd1191
parent 465801 acaebd9d440a4bc85e9c85d6bcc4abfedf1f0617
child 465803 5e0ac9f08356c1f0c58cf49b2c79ef8f1f2c677c
push id35746
push usershindli@mozilla.com
push dateSat, 23 Mar 2019 09:46:24 +0000
treeherdermozilla-central@02b7484f316b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1147563
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 1147563 - Provide autocomplete experience when formSubmitURL does not match. r=MattN Differential Revision: https://phabricator.services.mozilla.com/D23442
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/LoginManagerParent.jsm
toolkit/components/passwordmgr/test/mochitest/mochitest.ini
toolkit/components/passwordmgr/test/mochitest/test_autofill_different_formSubmitURL.html
toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete_formSubmitURL.html
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -267,25 +267,30 @@ var LoginHelper = {
    *                                 from a form or page that we are considering.
    * @param {nsILoginFindOptions} aOptions - Options to affect whether the origin
    *                                         from the login (aLoginOrigin) is a
    *                                         match for the origin we're looking
    *                                         for (aSearchOrigin).
    */
   isOriginMatching(aLoginOrigin, aSearchOrigin, aOptions = {
     schemeUpgrades: false,
+    acceptWildcardMatch: false,
   }) {
     if (aLoginOrigin == aSearchOrigin) {
       return true;
     }
 
     if (!aOptions) {
       return false;
     }
 
+    if (aOptions.acceptWildcardMatch && aLoginOrigin == "") {
+      return true;
+    }
+
     if (aOptions.schemeUpgrades) {
       try {
         let loginURI = Services.io.newURI(aLoginOrigin);
         let searchURI = Services.io.newURI(aSearchOrigin);
         if (loginURI.scheme == "http" && searchURI.scheme == "https" &&
             loginURI.hostPort == searchURI.hostPort) {
           return true;
         }
@@ -475,22 +480,27 @@ var LoginHelper = {
    *        a scheme matching `preferredOrigin`'s if there are two logins with
    *        the same `uniqueKeys`. The default preference to distinguish two
    *        logins is `timeLastUsed`. If there is no preference between two
    *        logins, the first one found wins.
    * @param {string} [preferredOrigin = undefined]
    *        String representing the origin to use for preferring one login over
    *        another when they are dupes. This is used with "scheme" for
    *        `resolveBy` so the scheme from this origin will be preferred.
+   * @param {string} [preferredFormActionOrigin = undefined]
+   *        String representing the action origin to use for preferring one login over
+   *        another when they are dupes. This is used with "actionOrigin" for
+   *        `resolveBy` so the scheme from this action origin will be preferred.
    *
    * @returns {nsILoginInfo[]} list of unique logins.
    */
   dedupeLogins(logins, uniqueKeys = ["username", "password"],
                resolveBy = ["timeLastUsed"],
-               preferredOrigin = undefined) {
+               preferredOrigin = undefined,
+               preferredFormActionOrigin = undefined) {
     const KEY_DELIMITER = ":";
 
     if (!preferredOrigin && resolveBy.includes("scheme")) {
       throw new Error("dedupeLogins: `preferredOrigin` is required in order to " +
                       "prefer schemes which match it.");
     }
 
     let preferredOriginScheme;
@@ -525,16 +535,26 @@ var LoginHelper = {
     function isLoginPreferred(existingLogin, login) {
       if (!resolveBy || resolveBy.length == 0) {
         // If there is no preference, prefer the existing login.
         return false;
       }
 
       for (let preference of resolveBy) {
         switch (preference) {
+          case "actionOrigin": {
+            if (!preferredFormActionOrigin) {
+              break;
+            }
+            if (LoginHelper.isOriginMatching(existingLogin.formSubmitURL, preferredFormActionOrigin, {schemeUpgrades: LoginHelper.schemeUpgrades}) &&
+                !LoginHelper.isOriginMatching(login.formSubmitURL, preferredFormActionOrigin, {schemeUpgrades: LoginHelper.schemeUpgrades})) {
+              return false;
+            }
+            break;
+          }
           case "scheme": {
             if (!preferredOriginScheme) {
               break;
             }
 
             try {
               // Only `hostname` is currently considered
               let existingLoginURI = Services.io.newURI(existingLogin.hostname);
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -1169,19 +1169,20 @@ var LoginManagerContent = {
   },
 
   /**
    * Attempt to find the username and password fields in a form, and fill them
    * in using the provided logins and recipes.
    *
    * @param {LoginForm} form
    * @param {nsILoginInfo[]} foundLogins an array of nsILoginInfo that could be
-            used for the form
+   *        used for the form, including ones with a different form action origin
+   *        which are only used when the fill is userTriggered
    * @param {Set} recipes a set of recipes that could be used to affect how the
-            form is filled
+   *        form is filled
    * @param {Object} [options = {}] a list of options for this method
    * @param {HTMLInputElement} [options.inputElement = null] an optional target
    *        input element we want to fill
    * @param {bool} [options.autofillForm = false] denotes if we should fill the
    *        form in automatically
    * @param {bool} [options.clobberUsername = false] controls if an existing
    *        username can be overwritten. If this is false and an inputElement
    *        of type password is also passed, the username field will be ignored.
@@ -1219,18 +1220,19 @@ var LoginManagerContent = {
       MULTIPLE_LOGINS: 7,
       NO_AUTOFILL_FORMS: 8,
       AUTOCOMPLETE_OFF: 9,
       INSECURE: 10,
       PASSWORD_AUTOCOMPLETE_NEW_PASSWORD: 11,
     };
 
     try {
-      // Nothing to do if we have no matching logins available,
-      // and there isn't a need to show the insecure form warning.
+      // Nothing to do if we have no matching (excluding form action
+      // checks) 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.
         autofillResult = AUTOFILL_RESULT.NO_SAVED_LOGINS;
         return;
       }
 
@@ -1276,16 +1278,29 @@ var LoginManagerContent = {
       // 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);
         usernameField.addEventListener("keydown", observer);
       }
 
+      if (!userTriggered) {
+        // Only autofill logins that match the form's action. In the above code
+        // we have attached autocomplete for logins that don't match the form action.
+        foundLogins = foundLogins.filter(l => {
+          return LoginHelper.isOriginMatching(l.formSubmitURL,
+                                              LoginHelper.getFormActionOrigin(form),
+                                              {
+                                                schemeUpgrades: LoginHelper.schemeUpgrades,
+                                                acceptWildcardMatch: true,
+                                              });
+        });
+      }
+
       // 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.
         autofillResult = AUTOFILL_RESULT.NO_SAVED_LOGINS;
         return;
       }
--- a/toolkit/components/passwordmgr/LoginManagerParent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ -35,41 +35,45 @@ var LoginManagerParent = {
    * @deprecated
    */
   _recipeManager: null,
 
   // Tracks the last time the user cancelled the master password prompt,
   // to avoid spamming master password prompts on autocomplete searches.
   _lastMPLoginCancelled: Math.NEGATIVE_INFINITY,
 
-  _searchAndDedupeLogins(formOrigin, actionOrigin) {
+  _searchAndDedupeLogins(formOrigin, actionOrigin, {looseActionOriginMatch} = {}) {
     let logins;
+    let matchData = {
+      hostname: formOrigin,
+      schemeUpgrades: LoginHelper.schemeUpgrades,
+    };
+    if (!looseActionOriginMatch) {
+      matchData.formSubmitURL = actionOrigin;
+    }
     try {
-      logins = LoginHelper.searchLoginsWithObject({
-        hostname: formOrigin,
-        formSubmitURL: actionOrigin,
-        schemeUpgrades: LoginHelper.schemeUpgrades,
-      });
+      logins = LoginHelper.searchLoginsWithObject(matchData);
     } catch (e) {
       // Record the last time the user cancelled the MP prompt
       // to avoid spamming them with MP prompts for autocomplete.
       if (e.result == Cr.NS_ERROR_ABORT) {
         log("User cancelled master password prompt.");
         this._lastMPLoginCancelled = Date.now();
         return [];
       }
       throw e;
     }
 
     // Dedupe so the length checks below still make sense with scheme upgrades.
     let resolveBy = [
+      "actionOrigin",
       "scheme",
       "timePasswordChanged",
     ];
-    return LoginHelper.dedupeLogins(logins, ["username"], resolveBy, formOrigin);
+    return LoginHelper.dedupeLogins(logins, ["username"], resolveBy, formOrigin, actionOrigin);
   },
 
   // Listeners are added in BrowserGlue.jsm on desktop
   // and in BrowserCLH.js on mobile.
   receiveMessage(msg) {
     let data = msg.data;
     switch (msg.name) {
       case "PasswordManager:findLogins": {
@@ -217,17 +221,18 @@ var LoginManagerParent = {
       // never return). We should guarantee that at least one of these
       // will fire.
       // See bug XXX.
       Services.obs.addObserver(observer, "passwordmgr-crypto-login");
       Services.obs.addObserver(observer, "passwordmgr-crypto-loginCanceled");
       return;
     }
 
-    let logins = this._searchAndDedupeLogins(formOrigin, actionOrigin);
+    // Autocomplete results do not need to match actionOrigin.
+    let logins = this._searchAndDedupeLogins(formOrigin, actionOrigin, {looseActionOriginMatch: true});
 
     log("sendLoginDataToChild:", logins.length, "deduped logins");
     // Convert the array of nsILoginInfo to vanilla JS objects since nsILoginInfo
     // doesn't support structured cloning.
     var jsLogins = LoginHelper.loginsToVanillaObjects(logins);
     target.sendAsyncMessage("PasswordManager:loginsFound", {
       requestId,
       logins: jsLogins,
@@ -265,17 +270,18 @@ var LoginManagerParent = {
       log("Using previous autocomplete result");
 
       // We have a list of results for a shorter search string, so just
       // filter them further based on the new search string.
       logins = LoginHelper.vanillaObjectsToLogins(previousResult.logins);
     } else {
       log("Creating new autocomplete search result.");
 
-      logins = this._searchAndDedupeLogins(formOrigin, actionOrigin);
+      // Autocomplete results do not need to match actionOrigin.
+      logins = this._searchAndDedupeLogins(formOrigin, actionOrigin, {looseActionOriginMatch: true});
     }
 
     let matchingLogins = logins.filter(function(fullMatch) {
       let match = fullMatch.username;
 
       // Remove results that are too short, or have different prefix.
       // Also don't offer empty usernames as possible results except
       // for password field.
--- a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
+++ b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
@@ -30,16 +30,19 @@ skip-if = toolkit == 'android' # autocom
 [test_autocomplete_https_upgrade.html]
 skip-if = toolkit == 'android' # autocomplete
 [test_autocomplete_sandboxed.html]
 scheme = https
 skip-if = toolkit == 'android' # autocomplete
 [test_autofill_autocomplete_types.html]
 scheme = https
 skip-if = toolkit == 'android' # bug 1533965
+[test_autofill_different_formSubmitURL.html]
+scheme = https
+skip-if = toolkit == 'android' # Bug 1259768
 [test_autofill_from_bfcache.html]
 scheme = https
 skip-if = toolkit == 'android' # bug 1527403
 [test_autofill_highlight.html]
 scheme = https
 skip-if = toolkit == 'android' # Bug 1531185
 [test_autofill_https_upgrade.html]
 skip-if = toolkit == 'android' # Bug 1259768
@@ -53,16 +56,19 @@ skip-if = toolkit == 'android' # autocom
 [test_basic_form_0pw.html]
 [test_basic_form_1pw.html]
 [test_basic_form_1pw_2.html]
 [test_basic_form_2pw_1.html]
 [test_basic_form_2pw_2.html]
 [test_basic_form_3pw_1.html]
 [test_basic_form_autocomplete.html]
 skip-if = toolkit == 'android' # android:autocomplete.
+[test_basic_form_autocomplete_formSubmitURL.html]
+skip-if = toolkit == 'android' # android:autocomplete.
+scheme = https
 [test_basic_form_honor_autocomplete_off.html]
 scheme = https
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_insecure_form_field_autocomplete.html]
 skip-if = toolkit == 'android' || os == 'linux' # android:autocomplete., linux: bug 1325778
 [test_password_field_autocomplete.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_insecure_form_field_no_saved_login.html]
copy from toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
copy to toolkit/components/passwordmgr/test/mochitest/test_autofill_different_formSubmitURL.html
--- a/toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
+++ b/toolkit/components/passwordmgr/test/mochitest/test_autofill_different_formSubmitURL.html
@@ -1,23 +1,22 @@
 <!DOCTYPE HTML>
 <html>
 <head>
   <meta charset="utf-8">
-  <title>Test autocomplete on an HTTPS page using upgraded HTTP logins</title>
+  <title>Test autofill on an HTTPS page using upgraded HTTP logins wtih different formSubmitURL</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/AddTask.js"></script>
   <script type="text/javascript" src="pwmgr_common.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <script>
 const MISSING_ACTION_PATH = TESTS_DIR + "mochitest/form_basic.html";
-const CROSS_ORIGIN_SECURE_PATH = TESTS_DIR + "mochitest/form_cross_origin_secure_action.html";
 
 const chromeScript = runChecksAfterCommonInit(false);
 
 let nsLoginInfo = SpecialPowers.wrap(SpecialPowers.Components).Constructor("@mozilla.org/login-manager/loginInfo;1",
                                                                            SpecialPowers.Ci.nsILoginInfo,
                                                                            "init");
 </script>
 <p id="display"></p>
@@ -50,67 +49,30 @@ async function prepareLoginsAndProcessFo
     login.timeCreated = login.timePasswordChanged = login.timeLastUsed = dates;
     LoginManager.addLogin(login);
   }
 
   iframe.src = url;
   await promiseFormsProcessed();
 }
 
-add_task(async function setup() {
-  await SpecialPowers.pushPrefEnv({"set": [["signon.schemeUpgrades", true]]});
-});
-
-add_task(async function test_simpleNoDupesNoAction() {
+add_task(async function test_formSubmitURL_wildcard_should_autofill() {
   await prepareLoginsAndProcessForm("https://example.com" + MISSING_ACTION_PATH, [
-    new nsLoginInfo("http://example.com", "http://example.com", null,
-                    "name2", "pass2", "uname", "pword"),
-  ]);
-
-  checkACForm("name2", "pass2");
-});
-
-add_task(async function test_simpleNoDupesUpgradeOriginAndAction() {
-  await prepareLoginsAndProcessForm("https://example.com" + CROSS_ORIGIN_SECURE_PATH, [
-    new nsLoginInfo("http://example.com", "http://another.domain", null,
+    new nsLoginInfo("https://example.com", "", null,
                     "name2", "pass2", "uname", "pword"),
   ]);
 
   checkACForm("name2", "pass2");
 });
 
-add_task(async function test_simpleNoDupesUpgradeOriginOnly() {
-  await prepareLoginsAndProcessForm("https://example.com" + CROSS_ORIGIN_SECURE_PATH, [
-    new nsLoginInfo("http://example.com", "https://another.domain", null,
-                    "name2", "pass2", "uname", "pword"),
-  ]);
-
-  checkACForm("name2", "pass2");
-});
-
-add_task(async function test_simpleNoDupesUpgradeActionOnly() {
-  await prepareLoginsAndProcessForm("https://example.com" + CROSS_ORIGIN_SECURE_PATH, [
-    new nsLoginInfo("https://example.com", "http://another.domain", null,
+add_task(async function test_formSubmitURL_different_shouldnt_autofill() {
+  await prepareLoginsAndProcessForm("https://example.com" + MISSING_ACTION_PATH, [
+    new nsLoginInfo("https://example.com", "https://another.domain", null,
                     "name2", "pass2", "uname", "pword"),
   ]);
 
-  checkACForm("name2", "pass2");
-});
-
-add_task(async function test_dedupe() {
-  await prepareLoginsAndProcessForm("https://example.com" + MISSING_ACTION_PATH, [
-    new nsLoginInfo("https://example.com", "https://example.com", null,
-                    "name1", "passHTTPStoHTTPS", "uname", "pword"),
-    new nsLoginInfo("http://example.com", "http://example.com", null,
-                    "name1", "passHTTPtoHTTP", "uname", "pword"),
-    new nsLoginInfo("http://example.com", "https://example.com", null,
-                    "name1", "passHTTPtoHTTPS", "uname", "pword"),
-    new nsLoginInfo("https://example.com", "http://example.com", null,
-                    "name1", "passHTTPStoHTTP", "uname", "pword"),
-  ]);
-
-  checkACForm("name1", "passHTTPStoHTTPS");
+  checkACForm("", "");
 });
 
 </script>
 </pre>
 </body>
 </html>
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete_formSubmitURL.html
@@ -0,0 +1,112 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test that logins with non-matching formSubmitURL appear in autocomplete dropdown</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/AddTask.js"></script>
+  <script type="text/javascript" src="../../../satchel/test/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: logins with non-matching formSubmitURL appear in autocomplete dropdown
+
+<script>
+var chromeScript = runChecksAfterCommonInit();
+
+runInParent(function setup() {
+  const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+
+  // Create some logins just for this form, since we'll be deleting them.
+  var nsLoginInfo = Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
+                                           Ci.nsILoginInfo, "init");
+  assert.ok(nsLoginInfo != null, "nsLoginInfo constructor");
+
+  var login1 = new nsLoginInfo("https://example.com", "https://differentFormSubmitURL", null,
+                               "dfsu1", "dfsp1", "uname", "pword");
+
+  Services.logins.addLogin(login1);
+});
+</script>
+<p id="display"></p>
+
+<!-- we presumably can't hide the content for this test. -->
+<div id="content">
+
+  <!-- form1 tests multiple matching logins -->
+  <form id="form1" action="https://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: multiple login autocomplete. **/
+
+var uname = $_(1, "uname");
+var pword = $_(1, "pword");
+
+// 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) {
+  var 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(async function setup() {
+  listenForUnexpectedPopupShown();
+});
+
+add_task(async function test_form1_initial_empty() {
+  await SimpleTest.promiseFocus(window);
+
+  // Make sure initial form is empty.
+  checkACForm("", "");
+  let popupState = await getPopupState();
+  is(popupState.open, false, "Check popup is initially closed");
+});
+
+/* For this testcase, the only login that exists for this origin
+ * is one with a different formSubmitURL, so the login will appear
+ * in the autocomplete popup.
+ */
+add_task(async function test_form1_menu_shows_logins_for_different_formSubmitURL() {
+  await SimpleTest.promiseFocus(window);
+  // Trigger autocomplete popup
+  restoreForm();
+  let shownPromise = promiseACShown();
+  synthesizeKey("KEY_ArrowDown"); // open
+  let results = await shownPromise;
+
+  let popupState = await getPopupState();
+  is(popupState.selectedIndex, -1, "Check no entries are selected upon opening");
+
+  let expectedMenuItems = ["dfsu1"];
+  checkAutoCompleteResults(results, expectedMenuItems, "example.com", "Check all menuitems are displayed correctly.");
+
+  synthesizeKey("KEY_ArrowDown"); // first item
+  checkACForm("", ""); // value shouldn't update just by selecting
+
+  synthesizeKey("KEY_Enter");
+  await promiseFormsProcessed();
+  checkACForm("dfsu1", "dfsp1");
+});
+</script>
+</pre>
+</body>
+</html>