Exclude PSL-matched forms from update candidates

If Chrome sees a username-less form being submitted on an origin, and
has exactly one pair of credentials stored for that origin, it offers
the user to update that pair with the new password, as a convenience.

However, if the only credential is stored for a PSL-related (having the
same eTLD+1) but different origin, this update should not be offered.
There is much less chance that the old credential and the new password
actually represent the same account.

Also, saving PSL-related credentials has the side effect of skipping the
confirmation prompt, so such update happens without user confirmation.
That's bad, because it led to overwriting useful credentials (see the
attached bug).

Therefore, this CL removes PSL-matching credentials from the list of
candidates to be updated.

Bug: 856543
Change-Id: Ibb77eaf5bf22d5865b83edd0bbd1bc6e02c41107
Reviewed-on: https://chromium-review.googlesource.com/1114740
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570525}

chrome/browser/password_manager/password_manager_browsertest.cc

@@ -3646,6 +3646,57 @@ IN_PROC_BROWSER_TEST_P(PasswordManagerBrowserTestWithViewsFeature,
   EXPECT_TRUE(bubble_observer.IsSavePromptShownAutomatically());
 }
 
+// This test emulates what was observed in https://crbug.com/856543: Imagine the
+// user stores a single username/password pair on origin A, and later submits a
+// username-less password-reset form on origin B. In the bug, A and B were
+// PSL-matches (different, but with the same eTLD+1), and Chrome ended up
+// overwriting the old password with the new one. This test checks that it no
+// longer is the case.
+IN_PROC_BROWSER_TEST_P(PasswordManagerBrowserTestWithViewsFeature,
+                       NoSilentOverwriteOnPSLMatch) {
+  // Store a password at origin A.
+  const GURL url_A = embedded_test_server()->GetURL("abc.foo.com", "/");
+  scoped_refptr<password_manager::TestPasswordStore> password_store =
+      static_cast<password_manager::TestPasswordStore*>(
+          PasswordStoreFactory::GetForProfile(
+              browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS)
+              .get());
+  autofill::PasswordForm signin_form;
+  signin_form.signon_realm = url_A.GetOrigin().spec();
+  signin_form.origin = url_A;
+  signin_form.username_value = base::ASCIIToUTF16("user");
+  signin_form.password_value = base::ASCIIToUTF16("oldpassword");
+  password_store->AddLogin(signin_form);
+  WaitForPasswordStore();
+
+  // Visit origin B with a form only containing new- and confirmation-password
+  // fields.
+  GURL url_B = embedded_test_server()->GetURL(
+      "www.foo.com", "/password/new_password_form.html");
+  NavigationObserver observer_B(WebContents());
+  ui_test_utils::NavigateToURL(browser(), url_B);
+  observer_B.Wait();
+
+  // Fill in the new password and submit.
+  GURL url_done =
+      embedded_test_server()->GetURL("www.foo.com", "/password/done.html");
+  NavigationObserver observer_done(WebContents());
+  observer_done.SetPathToWaitFor("/password/done.html");
+  ASSERT_TRUE(content::ExecuteScriptWithoutUserGesture(
+      RenderFrameHost(),
+      "document.getElementById('new_p').value = 'new password';"
+      "document.getElementById('conf_p').value = 'new password';"
+      "document.getElementById('testform').submit();"));
+  observer_done.Wait();
+
+  // Check that the password for origin A was not updated automatically.
+  WaitForPasswordStore();  // Let the navigation take its effect on storing.
+  EXPECT_THAT(
+      password_store->stored_passwords().at(signin_form.signon_realm),
+      ElementsAre(testing::Field(&autofill::PasswordForm::password_value,
+                                 signin_form.password_value)));
+}
+
 INSTANTIATE_TEST_CASE_P(All,
                         PasswordManagerBrowserTestWithViewsFeature,
                         /*popup_views_enabled=*/::testing::Bool());

chrome/test/data/password/new_password_form.html

+<!--
+    A form with just a new password and confirmation password fields. No
+    username, no current password.
+ -->
+<html>
+<body>
+<form method="POST" action="done.html" id="testform">
+  <input type="password" id="new_p" name="new_p" autocomplete="new-password">
+  <input type="password" id="conf_p" name="conf_p" autocomplete="new-password">
+</form>
+
+</body>
+</html>

components/password_manager/core/browser/password_form_manager.cc

@@ -783,24 +783,43 @@ bool PasswordFormManager::IsMatch(const autofill::PasswordForm& form) const {
   return !form.blacklisted_by_user && form.scheme == observed_form_.scheme;
 }
 
+// Assuming that the user submitted a form with |current_password| and no
+// username, this function attempts to find a saved credential which likely
+// represents the same account, and should therefore be updated with the new
+// password from the submitted form (instead of creating a new username-less
+// stored credential with the new password). This can return null if there is no
+// such password. Note that |current_password| may be empty, e.g., for sign-up
+// forms. If it is not empty and there is a saved form with the same current
+// password, the latter is a likely candidate to be returned.
 const PasswordForm* PasswordFormManager::FindBestMatchForUpdatePassword(
-    const base::string16& password) const {
-  // This function is called for forms that do not contain a username field.
-  // This means that we cannot update credentials based on a matching username
-  // and that we may need to show an update prompt.
-  if (best_matches_.size() == 1 && !has_generated_password_) {
+    const base::string16& current_password) const {
+  // First, filter out PSL-matches from |best_matches_|. Only credentials saved
+  // for the same origin as the submitted form should be considered for
+  // updating. A PSL match which does not have a copy in the origin of the
+  // submitted form was not accepted by the user as representing the same
+  // account, so it is unlikely that the user would like it updated.
+  std::vector<const PasswordForm*> same_origin_matches;
+  for (const auto& key_value : best_matches_) {
+    if (!key_value.second->is_public_suffix_match)
+      same_origin_matches.push_back(key_value.second);
+  }
+
+  if (same_origin_matches.size() == 1 && !has_generated_password_) {
     // In case the submitted form contained no username but a password, and if
     // the user has only one credential stored, return it as the one that should
     // be updated.
-    return best_matches_.begin()->second;
+    return same_origin_matches[0];
   }
-  if (password.empty())
+
+  // Otherwise the old password provides guidance for selecting from multiple
+  // entries, so give up if there is none.
+  if (current_password.empty())
     return nullptr;
 
-  // Return any existing credential that has the same |password| saved already.
-  for (const auto& key_value : best_matches_) {
-    if (key_value.second->password_value == password)
-      return key_value.second;
+  // Return any existing credential that has the same password saved already.
+  for (const PasswordForm* saved_form : same_origin_matches) {
+    if (saved_form->password_value == current_password)
+      return saved_form;
   }
   return nullptr;
 }