[Password Manager] Fix password form reappear detection for empty act…

…ion forms. This CL chnages IsPasswordFormReappeared so it doesn't compare form actions if they are empty. R=cfroussios@chromium.org, kolos@chromium.org Bug: 699539 Change-Id: Ia57c6ec7245d972e5df5f88b64c5ddac9c2c5c43 Reviewed-on: https://chromium-review.googlesource.com/916565 Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org> Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org> Cr-Commit-Position: refs/heads/master@{#537643}
cbchan · Feb 19, 2018 · ada53fd · ada53fd
1 parent e1bb269
commit ada53fd
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 1 deletion.
diff --git a/AUTHORS b/AUTHORS
@@ -493,6 +493,7 @@ Lorenzo Stoakes <lstoakes@gmail.com>
 Lu Guanqun <guanqun.lu@gmail.com>
 Lucie Brozkova <lucinka.brozkova@gmail.com>
 Luiz Von Dentz <luiz.von.dentz@intel.com>
+Luka Dojcilovic <l.dojcilovic@gmail.com>
 Luke Inman-Semerau <luke.semerau@gmail.com>
 Luke Zarko <lukezarko@gmail.com>
 Luoxi Pan <l.panpax@gmail.com>

diff --git a/chrome/browser/password_manager/password_manager_browsertest.cc b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -510,6 +510,24 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
   EXPECT_FALSE(prompt_observer->IsSavePromptShownAutomatically());
 }
 
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
+                       PromptForDifferentFormWithEmptyAction) {
+  // Log in, see a form on the landing page. That form is not related to the
+  // signin form. The signin and the form on the landing page have empty
+  // actions, so we should offer saving the password.
+  NavigateToFile("/password/navigate_to_same_url_empty_actions.html");
+
+  NavigationObserver observer(WebContents());
+  auto prompt_observer = base::MakeUnique<BubbleObserver>(WebContents());
+  std::string fill_and_submit =
+      "document.getElementById('username').value = 'temp';"
+      "document.getElementById('password').value = 'random';"
+      "document.getElementById('submit-button').click()";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
+  observer.Wait();
+  EXPECT_TRUE(prompt_observer->IsSavePromptShownAutomatically());
+}
+
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
                        PromptAfterSubmitWithSubFrameNavigation) {
   NavigateToFile("/password/multi_frames.html");

diff --git a/chrome/test/data/password/navigate_to_same_url_empty_actions.html b/chrome/test/data/password/navigate_to_same_url_empty_actions.html
@@ -0,0 +1,35 @@
+<html>
+<head>
+  <script src="form_utils.js"></script>
+  <script>
+    function redirect() {
+      window.location.replace('navigate_to_same_url_empty_actions.html?done');
+      return false;
+    }
+  </script>
+  <script>
+    function onLoadHandler() {
+      // Signin form with an empty action is displayed first, once submitted,
+      // it navigates to same URL with '?done' appended to it so the if
+      // statement has the empty action landing page form displayed instead.
+      if (location.search == '?done') {
+        var landing_page_form = createSimplePasswordForm();
+        landing_page_form.action = '';
+        landing_page_form.children[1].id = 'not_username_field';
+        landing_page_form.children[1].name = 'not_username_field';
+        landing_page_form.children[3].id = 'not_password_field';
+        landing_page_form.children[3].name = 'not_password_field';
+        landing_page_form.children[4].id = 'not_login_button';
+        document.body.appendChild(landing_page_form);
+      } else {
+        var signin_form = createSimplePasswordForm();
+        signin_form.action = '';
+        signin_form.setAttribute('onsubmit', 'return redirect();');
+        document.body.appendChild(signin_form);
+      }
+    }
+  </script>
+</head>
+<body onload="onLoadHandler();">
+</body>
+</html>
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
@@ -71,11 +71,19 @@ bool URLsEqualUpToHttpHttpsSubstitution(const GURL& a, const GURL& b) {
   return false;
 }
 
+// Since empty or unspecified form's action is automatically set to the page
+// origin, this function checks if a form's action is empty by comparing it to
+// its origin.
+bool HasNonEmptyAction(const autofill::PasswordForm& form) {
+  return form.action != form.origin;
+}
+
 // Checks if the observed form looks like the submitted one to handle "Invalid
 // password entered" case so we don't offer a password save when we shouldn't.
 bool IsPasswordFormReappeared(const autofill::PasswordForm& observed_form,
                               const autofill::PasswordForm& submitted_form) {
-  if (observed_form.action.is_valid() &&
+  if (observed_form.action.is_valid() && HasNonEmptyAction(observed_form) &&
+      HasNonEmptyAction(submitted_form) &&
       URLsEqualUpToHttpHttpsSubstitution(submitted_form.action,
                                          observed_form.action)) {
     return true;