Bug 1147563 - Provide autocomplete experience when formSubmitURL does not match. r=MattN
authorJared Wein <jwein@mozilla.com>
Tue, 02 Apr 2019 18:24:47 +0000
changeset 467650 b09ce6eebb4942cd2e570bf785c660ca6abb6258
parent 467649 cc2ac1b5534f166fbc0f225c5d66946fd8f410af
child 467651 49a9bb5d764ca3164dac1b0cb34285c561ecd6f9
push id35806
push userrgurzau@mozilla.com
push dateWed, 03 Apr 2019 04:07:39 +0000
treeherdermozilla-central@45808ab18609 [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
@@ -1174,19 +1174,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.
@@ -1224,18 +1225,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;
       }
 
@@ -1281,16 +1283,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
@@ -33,16 +33,19 @@ skip-if = toolkit == 'android' # autocom
 scheme = https
 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
@@ -56,16 +59,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>